Eric Blake <ebl...@redhat.com> writes: > 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) { > ...
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!