| Message ID | 1453219845-30939-9-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Inside the generated code between visit_start_struct() and > visit_end_struct(), we were blindly setting the error into > the caller's errp parameter. But a future patch to split > visit_end_struct() will require that we take action based > on whether an error has occurred, which requires us to track > all actions through a local err. Rewrite the visits to be > more in line with the other generated calls. > > Generated code changes look like: > > |@@ -42,12 +42,18 @@ void visit_type_GuestAgentCommandInfo(Vi I'd drop this line. > | Error *err = NULL; > | > | visit_start_struct(v, (void **)obj, "GuestAgentCommandInfo", name, sizeof(GuestAgentCommandInfo), &err); > |- if (!err) { > |- if (*obj) { > |- visit_type_GuestAgentCommandInfo_fields(v, obj, errp); > |- } > |- visit_end_struct(v, &err); > |+ if (err) { > |+ goto out; > | } > |+ if (!*obj) { err is clear here. > |+ goto out_obj; > |+ } > |+ visit_type_GuestAgentCommandInfo_fields(v, obj, &err); > |+out_obj: > |+ error_propagate(errp, err); > |+ err = NULL; If we come from goto out_obj, these two lines are no-ops. > |+ visit_end_struct(v, &err); > |+out: > | error_propagate(errp, err); > | } > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > v9: enhance commit message > v8: no change > v7: place earlier in series > v6: based loosely on v5 7/46, but mostly a rewrite to get the last > generated code in the same form as all the others, so that the > later conversion to split visit_check_struct() will be easier > --- > scripts/qapi-visit.py | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index b93690b..4a4f67d 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -123,12 +123,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error > Error *err = NULL; > > visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); > - if (!err) { > - if (*obj) { > - visit_type_%(c_name)s_fields(v, obj, errp); > - } > - visit_end_struct(v, &err); > + if (err) { > + goto out; > } > + if (!*obj) { > + goto out_obj; > + } > + visit_type_%(c_name)s_fields(v, obj, &err); > +out_obj: > + error_propagate(errp, err); > + err = NULL; > + visit_end_struct(v, &err); > +out: > error_propagate(errp, err); > } > ''', gen_visit_union(), the other generator of visit_start_struct(), already does it this way. It generates additional goto out_obj, so the placement of out_obj makes more sense there. I guess we want to place it in the same spot here to facilitate unifying the two.
On 01/20/2016 09:03 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Inside the generated code between visit_start_struct() and >> visit_end_struct(), we were blindly setting the error into >> the caller's errp parameter. But a future patch to split >> visit_end_struct() will require that we take action based >> on whether an error has occurred, which requires us to track >> all actions through a local err. Rewrite the visits to be >> more in line with the other generated calls. >> >> |+ if (!*obj) { > > err is clear here. > >> |+ goto out_obj; >> |+ } >> |+ visit_type_GuestAgentCommandInfo_fields(v, obj, &err); >> |+out_obj: >> |+ error_propagate(errp, err); >> |+ err = NULL; > > If we come from goto out_obj, these two lines are no-ops. I could move the label, if desired... > >> |+ visit_end_struct(v, &err); >> |+out: >> | error_propagate(errp, err); >> | } > > gen_visit_union(), the other generator of visit_start_struct(), already > does it this way. It generates additional goto out_obj, so the > placement of out_obj makes more sense there. I guess we want to place > it in the same spot here to facilitate unifying the two. ...but as you observed, the unification in 31 is a bit easier with it placed identically. Maybe a commit message comment is in order.
Eric Blake <eblake@redhat.com> writes: > On 01/20/2016 09:03 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Inside the generated code between visit_start_struct() and >>> visit_end_struct(), we were blindly setting the error into >>> the caller's errp parameter. But a future patch to split >>> visit_end_struct() will require that we take action based >>> on whether an error has occurred, which requires us to track >>> all actions through a local err. Rewrite the visits to be >>> more in line with the other generated calls. >>> > >>> |+ if (!*obj) { >> >> err is clear here. >> >>> |+ goto out_obj; >>> |+ } >>> |+ visit_type_GuestAgentCommandInfo_fields(v, obj, &err); >>> |+out_obj: >>> |+ error_propagate(errp, err); >>> |+ err = NULL; >> >> If we come from goto out_obj, these two lines are no-ops. > > I could move the label, if desired... > >> >>> |+ visit_end_struct(v, &err); >>> |+out: >>> | error_propagate(errp, err); >>> | } > >> >> gen_visit_union(), the other generator of visit_start_struct(), already >> does it this way. It generates additional goto out_obj, so the >> placement of out_obj makes more sense there. I guess we want to place >> it in the same spot here to facilitate unifying the two. > > ...but as you observed, the unification in 31 is a bit easier with it > placed identically. Maybe a commit message comment is in order. Or perhaps move it forward now, and move it back when you actually need it back. Reviewers appreciate miminal patch churn, but they also appreciate patches making sense on their own. Reviewer are looking at your patch with precious little context in general, and next to zero visibility into later patches in particular.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index b93690b..4a4f67d 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -123,12 +123,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error Error *err = NULL; visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); - if (!err) { - if (*obj) { - visit_type_%(c_name)s_fields(v, obj, errp); - } - visit_end_struct(v, &err); + if (err) { + goto out; } + if (!*obj) { + goto out_obj; + } + visit_type_%(c_name)s_fields(v, obj, &err); +out_obj: + error_propagate(errp, err); + err = NULL; + visit_end_struct(v, &err); +out: error_propagate(errp, err); } ''',
Inside the generated code between visit_start_struct() and visit_end_struct(), we were blindly setting the error into the caller's errp parameter. But a future patch to split visit_end_struct() will require that we take action based on whether an error has occurred, which requires us to track all actions through a local err. Rewrite the visits to be more in line with the other generated calls. Generated code changes look like: |@@ -42,12 +42,18 @@ void visit_type_GuestAgentCommandInfo(Vi | Error *err = NULL; | | visit_start_struct(v, (void **)obj, "GuestAgentCommandInfo", name, sizeof(GuestAgentCommandInfo), &err); |- if (!err) { |- if (*obj) { |- visit_type_GuestAgentCommandInfo_fields(v, obj, errp); |- } |- visit_end_struct(v, &err); |+ if (err) { |+ goto out; | } |+ if (!*obj) { |+ goto out_obj; |+ } |+ visit_type_GuestAgentCommandInfo_fields(v, obj, &err); |+out_obj: |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); |+out: | error_propagate(errp, err); | } Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- v9: enhance commit message v8: no change v7: place earlier in series v6: based loosely on v5 7/46, but mostly a rewrite to get the last generated code in the same form as all the others, so that the later conversion to split visit_check_struct() will be easier --- scripts/qapi-visit.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)