Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@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.