Re: [Qemu-devel] [PATCH v9 07/37] qapi: Improve generated event use of qapi visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 01/20/2016 08:19 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> All other successful clients of visit_start_struct() were paired
>>> with an unconditional visit_end_struct(); but the generated
>>> code for events was relying on qmp_output_visitor_cleanup() to
>>> work on an incomplete visit.
>> 
>> Disgusting, isn't it?  :)
>
> This, along with the fix in 5/37, were the two places that had to be
> fixed to avoid assertions in patch 24, when I turned on stricter
> enforcing of cleanup only on an evenly matched visit.
>
>> 
>>>                               Alter the code to guarantee that
>>> the struct is completed, which will make a future patch to
>>> split visit_end_struct() easier to reason about.  While at it,
>>> drop some assertions and comments that are not present in other
>>> uses of the qmp output visitor, and pass NULL rather than "" as
>>> the 'kind' parameter (matching most other uses where obj is NULL).
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>>> ---
>>> v9: save churn in declaration order for later series on boxed params,
>>> drop Marc-Andre's R-b
>>> v8: no change
>>> v7: place earlier in series, adjust handling of 'kind'
>>> v6: new patch
>>>
>>> If desired, I can defer the hunk re-ordering the declaration of
>>> obj to later in the series where it actually comes in handy.
>
> Dead sentence leftover from v8; as mentioned above, I DID sink the
> declaration reordering to a later series for v9.  But it's after the
> ---, so it gets trimmed automatically by 'git am'.
>
>
>>>          ret += gen_err_check()
>>> -        ret += gen_visit_fields(arg_type.members, need_cast=True)
>>> +        ret += gen_visit_fields(arg_type.members, need_cast=True,
>>> +                                label='out_obj')
>> 
>> On error, we now go to out_obj rather than out.  Some fields will be
>> unvisited then (possibly all), and err will be set.
>> 
>> Now I get to figure out what this change changes.
>> 
>>>          ret += mcgen('''
>>> +out_obj:
>>>      visit_end_struct(v, &err);
>>>      if (err) {
>>>          goto out;
>>>      }
>> 
>> Good: we actually call visit_end_struct() as we should.
>> 
>> Not so good: if we get here via the error exit of gen_visit_fields(),
>> err is set.  If visit_end_struct() tries to set another error...
>
> Oops. It all gets cleaned up in 33 when visit_end_struct() loses the
> errp argument, but in the meantime, I think the most robust way to write
> this would be:
>
> out_obj:
>     visit_end_struct(v, err ? NULL : &err);
>     if (err) {
> ...

Works.  I don't like it much, because it doesn't conform to the usual
error handling patterns, but since it's only temporary, I'm fine with
it.

>> I guess the idea is to go from gen_visit_fields() failure through
>> visit_end_struct() here to out.  Correct?
>
> Yes.

Rather complex control flow.  At the end of the series, it looks like
this:

        visit_start_struct(v, "%(name)s", NULL, 0, &err);
        if (err) {
            goto out;
        }
        [visit the fields, on error goto out_obj...]
        visit_check_struct(v, &err);
    out_obj:
        visit_end_struct(v);
        if (err) {
            goto out;
        }

The error check after visit_end_struct() sees either an error from a
field visit, or from the visit_end_struct().

Tolerable.

>>> +++ b/scripts/qapi.py
>>> @@ -1636,7 +1636,8 @@ def gen_err_check(label='out', skiperr=False):
>>>                   label=label)
>>>
>>>
>>> -def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
>>> +def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
>>> +                     label='out'):
>> 
>> Probably clearer than label=None, but duplicates gen_err_check()'s
>> default.  Fine with me.
>> 
>>>      ret = ''
>>>      if skiperr:
>>>          errparg = 'NULL'
>>        else:
>>            errparg = '&err'
>> 
>>        for memb in members:
>>            if memb.optional:
>>                ret += mcgen('''
>>        if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
>>    ''',
>>                             prefix=prefix, c_name=c_name(memb.name),
>>                             name=memb.name, errp=errparg)
>>                push_indent()
>> 
>> errp=errparg unused here.  Not this patch's job to clean up.
>
> Bah. Commit 5cdc8831 missed it, due to repeated refactoring. I'm a bit
> surprised that pep8 didn't complain.  Okay, I'm adding it as a separate
> cleanup.

Thanks!