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

[Eric Blake]

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