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

[Eric Blake]

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) {
...

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

Yes.

>> +++ 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature