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? :) > 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. > --- > scripts/qapi-event.py | 12 +++++------- > scripts/qapi.py | 5 +++-- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 720486f..761cda9 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -61,25 +61,23 @@ def gen_event_send(name, arg_type): > if arg_type and arg_type.members: > ret += mcgen(''' > qov = qmp_output_visitor_new(); > - g_assert(qov); > - Good riddance: qmp_output_visitor_new() can't fail. If that ever changes, qmp_output_get_visitor() will crash just fine. > v = qmp_output_get_visitor(qov); > - g_assert(v); Good riddance^2: qmp_output_get_visitor() can't fail, either. > > - /* Fake visit, as if all members are under a structure */ > - visit_start_struct(v, NULL, "", "%(name)s", 0, &err); > + visit_start_struct(v, NULL, NULL, "%(name)s", 0, &err); > ''', > name=name) > 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... I guess the idea is to go from gen_visit_fields() failure through visit_end_struct() here to out. Correct? > > obj = qmp_output_get_qobject(qov); > - g_assert(obj != NULL); > + g_assert(obj); > > qdict_put_obj(qmp, "data", obj); > ''') ret += mcgen(''' emit(%(c_enum)s, qmp, &err); ''', c_enum=c_enum_const(event_enum_name, name)) if arg_type and arg_type.members: ret += mcgen(''' out: qmp_output_visitor_cleanup(qov); ''') ret += mcgen(''' error_propagate(errp, err); QDECREF(qmp); } > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 7dec611..497eaba 100644 > --- a/scripts/qapi.py > +++ 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. > @@ -1664,7 +1665,7 @@ def gen_visit_fields(members, prefix='', > need_cast=False, skiperr=False): > c_type=memb.type.c_name(), prefix=prefix, cast=cast, > c_name=c_name(memb.name), name=memb.name, > errp=errparg) > - ret += gen_err_check(skiperr=skiperr) > + ret += gen_err_check(skiperr=skiperr, label=label) > > if memb.optional: > pop_indent()