Eric Blake <ebl...@redhat.com> writes: > 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.
I think so, too. >>> +++ 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. That's one purpose of implementing the method wrapped by this function. Perhaps simply: Prepare completing a struct visit. >>> 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. g_hash_table_iter_init() and g_hash_table_iter_next(). >>> +++ 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). Yes, please. Subset A is ready, and will be in my next QAPI pull request.