On 01/20/2016 08:19 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
>>
>> ---
>> 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
