On 10/07/2015 06:00 AM, Markus Armbruster wrote: >>> Looks like we're getting drawn into visitor contract territory again. >>>
>> +++ b/hmp.c >> @@ -1658,8 +1658,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict) >> >> object_add(type, id, pdict, opts_get_visitor(ov), &err); >> >> + visit_check_struct(opts_get_visitor(ov), &err_end); >> out_end: >> - visit_end_struct(opts_get_visitor(ov), &err_end); >> + visit_end_struct(opts_get_visitor(ov)); >> if (!err && err_end) { >> qmp_object_del(id, NULL); >> } > > Preexisting: calling object_add() before visit_end_struct() is awkward. > Can we simplify things now we have separate visit_check_struct() and > visit_end_struct()? Call visit_check_struct() before object_add(), > bypass object_add() on error, avoiding the need to undo it with > qmp_object_del(). Okay, it sounds like I'm sitting on a pile of pre-patch cleanups, and that I'm on the right track for having done the split. >> +++ b/include/qapi/visitor.h >> @@ -56,11 +56,19 @@ typedef struct GenericList >> void visit_start_struct(Visitor *v, void **obj, const char *kind, >> const char *name, size_t size, Error **errp); >> /** >> + * Check whether completing a struct is safe. > > "Safe"? We need to complete the struct visit with visit_end_struct() > regardless of what this function returns... > >> + * Should be called prior to visit_end_struct() if all other intermediate >> + * visit steps were successful, to allow the caller one last chance to >> + * report errors such as remaining data that was not consumed by the visit. >> + */ >> +void visit_check_struct(Visitor *v, Error **errp); Maybe: Declare the current struct complete, and check for unvisited contents. >> static void >> -opts_end_struct(Visitor *v, Error **errp) >> +opts_check_struct(Visitor *v, Error **errp) >> { >> OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); >> GQueue *any; > > if (--ov->depth > 0) { > > Do we want to decrement ov->depth here? We'll decrement it again in > opts_end_struct()... Oh, good catch. This was an awkward split, and I got it off-by-one. >> +++ b/qapi/qmp-input-visitor.c >> @@ -88,14 +88,14 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject >> *obj, Error **errp) >> qiv->nb_stack++; >> } >> >> -/** Only for qmp_input_pop. */ >> +/** Only for qmp_input_check. */ > > Drop the comment instead? > > Aside: a loop would be more idiomatic C. Leave higher order functions > to languages that are actually equipped for the job. > >> static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey) >> { >> *(const char **)user_pkey = (const char *)key; >> return TRUE; >> } >> >> -static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) >> +static void qmp_input_check(QmpInputVisitor *qiv, Error **errp) >> { >> assert(qiv->nb_stack > 0); >> >> @@ -107,6 +107,17 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error >> **errp) >> g_hash_table_find(top_ht, always_true, &key); always_true() exists for g_hash_table_find() - unless you know of some other way to grab any arbitrary element of the hash table that doesn't require a higher-order function. >> +++ b/scripts/qapi-event.py >> @@ -73,13 +73,14 @@ def gen_event_send(name, arg_type): >> ret += gen_err_check() >> ret += gen_visit_fields(arg_type.members, need_cast=True) >> ret += mcgen(''' >> - visit_end_struct(v, &err); >> + visit_check_struct(v, &err); >> if (err) { >> goto out; >> } >> + visit_end_struct(v); >> >> obj = qmp_output_get_qobject(qov); >> - g_assert(obj != NULL); >> + g_assert(obj); > > I prefer the more laconic form myself, but it's an unrelated change. I can split that one-line change into a more appropriate patch. >> out_obj: >> error_propagate(errp, err); >> err = NULL; >> visit_end_union(v, !!(*obj)->data, &err); > > Should visit_end_union() be similarly split? Or should its Error ** > parameter be dropped? As far as I can tell, no visitor implements this > method... > visit_end_union() gets completely dropped in a different patch. See v5 28/46. >> +++ b/tests/test-qmp-output-visitor.c >> @@ -194,10 +194,11 @@ static void visit_type_TestStruct(Visitor *v, >> TestStruct **obj, >> } >> visit_type_str(v, &(*obj)->string, "string", &err); >> >> + if (!err) { >> + visit_check_struct(v, &err); >> + } > > ... but here, you guard it with an if. Either way works, but I'd like > us to pick just one for the generators. Sure. >> - for (*head = i = visit_next_list(v, head, errp); i; i = >> visit_next_list(v, &i, errp)) { >> + for (*head = i = visit_next_list(v, head, &err); >> + !err && i; >> + i = visit_next_list(v, &i, &err)) { >> TestStructList *native_i = (TestStructList *)i; >> - visit_type_TestStruct(v, &native_i->value, NULL, errp); >> + visit_type_TestStruct(v, &native_i->value, NULL, &err); >> } > > Is this a silent bug fix? Before your patch, the loop doesn't break on > error. > Yes, looks like it. And all the more reason for our test code to NOT hand-write this, but to rely on the generator (so that we are testing a single version of visit_* calls, rather than subtle differences in generated vs. hand-rolled). >> +++ b/vl.c >> @@ -2796,23 +2796,25 @@ static int object_create(void *opaque, QemuOpts >> *opts, Error **errp) >> qdict_del(pdict, "qom-type"); >> visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err); >> if (err) { >> - goto out; >> + goto obj_out; >> +obj_out: >> + visit_end_struct(opts_get_visitor(ov)); >> if (err) { >> qmp_object_del(id, NULL); >> } > > Silent bug fix: we now call visit_end_struct() even on error. Impact? > Separate patch? Yes, separate patch, and I'll evaluate impact there. So sounds like I should proceed with this RFC (which means more respin of my other patches, before I can post subset C - but that's okay, since we aren't even through reviewing subset B, nor is subset A in a pull request). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature