[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v9 07/37] qapi: Improve generated event use of qapi visitor
Date: Wed, 20 Jan 2016 10:10:19 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

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:

    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?


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

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]