| Message ID | 1453219845-30939-8-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@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 <eblake@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()
On 01/20/2016 08:19 AM, Markus Armbruster wrote: > Eric Blake <eblake@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 <eblake@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> writes: > On 01/20/2016 08:19 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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 <eblake@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!
On 01/20/2016 10:10 AM, Eric Blake wrote: > 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. Technically, visit_end_struct() for QMP output visitor never sets errp, but it's not nice to have to audit multiple files just to find that out. I'll stick to the ternary until patch 33 cleans it back up.
On 01/20/2016 08:19 AM, Markus Armbruster wrote: > Eric Blake <eblake@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. > >> +++ 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. Use of label=None resulted in literal "goto None;" in the generated file (Python stringized it, rather than treating it as a hint to behave as if the argument were not supplied). So yes, I had to duplicate the default value in both methods, to avoid having to do 'if label:' within the implementations.
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); - v = qmp_output_get_visitor(qov); - g_assert(v); - /* 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') ret += mcgen(''' +out_obj: visit_end_struct(v, &err); if (err) { goto out; } obj = qmp_output_get_qobject(qov); - g_assert(obj != NULL); + g_assert(obj); qdict_put_obj(qmp, "data", obj); ''') 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'): ret = '' if skiperr: errparg = 'NULL' @@ -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()
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. 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 <eblake@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(-)