Message ID | 1442872682-6523-8-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > Due to the existing semantics of the error_set() family, > generated sequences in the qapi visitors such as: > > visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); > if (!err) { > visit_type_FOO_fields(m, obj, errp); > visit_end_implicit_struct(m, &err); > } > error_propagate(errp, err); Indentation seems off. Intentional? > > are risky: if visit_type_FOO_fields() sets errp, and then > visit_end_implicit_struct() also encounters an error, the > attempt to overwrite the first error will cause an abort(). > Obviously, we weren't triggering this situation (none of the > existing callbacks for visit_end_implicit_struct() currently > try to set an error), but it is better to not even cause the > problem in the first place. The code works, but it sets a problematic example. > Meanwhile, in spite of the poor documentation of the qapi > visitors, we want to guarantee that if a visit_start_*() > succeeds, then the matching visit_end_*() will be called. Agreed. > The options are to either propagate and clear a local err > multiple times: > > visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); > if (!err) { > visit_type_FOO_fields(m, obj, &err); > if (err) { > error_propagate(errp, err); > err = NULL; > } > visit_end_implicit_struct(m, &err); > } > error_propagate(errp, err); > > or, as this patch does, just pass in NULL to ignore further > errors once an error has occurred. > > visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); > if (!err) { > visit_type_FOO_fields(m, obj, &err); > visit_end_implicit_struct(m, err ? NULL : &err); > } > error_propagate(errp, err); Hmmmmm... not sure we do this anywhere else, yet. The ternary isn't exactly pretty, but the intent to ignore additional error is clear enough, I think. If we elect to adopt this new error handling pattern, we should perhaps document it in error.h. Third option: drop visit_end_implicit_struct()'s errp parameter. If we find a compelling use for it, we'll put it back and solve the problem. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > scripts/qapi-visit.py | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 97343cf..d911b20 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -51,8 +51,8 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error * > > visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err); > if (!err) { > - visit_type_%(c_type)s_fields(m, obj, errp); > - visit_end_implicit_struct(m, &err); > + visit_type_%(c_type)s_fields(m, obj, &err); > + visit_end_implicit_struct(m, err ? NULL : &err); > } > error_propagate(errp, err); > } > @@ -143,9 +143,9 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error > visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); > if (!err) { > if (*obj) { > - visit_type_%(c_name)s_fields(m, obj, errp); > + visit_type_%(c_name)s_fields(m, obj, &err); > } > - visit_end_struct(m, &err); > + visit_end_struct(m, err ? NULL : &err); > } > error_propagate(errp, err); > } Oh, it's about visit_end_struct(), too. Commit message only talks about visit_end_implicit_struct(). In particular, "none of the existing callbacks for visit_end_implicit_struct() currently try to set an error". Does that hold for visit_end_struct() callbacks, too? > @@ -175,9 +175,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error > visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err); > } > > - error_propagate(errp, err); > - err = NULL; > - visit_end_list(m, &err); > + visit_end_list(m, err ? NULL : &err); > out: > error_propagate(errp, err); > } Likewise. Does it hold for visit_end_list() callbacks, too? Looks like you switch from option 1 to option 2 here. Your slate isn't as clean as the commit message suggests :) > @@ -231,9 +229,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error > abort(); > } > out_end: > - error_propagate(errp, err); > - err = NULL; > - visit_end_implicit_struct(m, &err); > + visit_end_implicit_struct(m, err ? NULL : &err); > out: > error_propagate(errp, err); > } Looks like another switch from option 1 to option 2. > @@ -330,10 +326,8 @@ out_obj: > error_propagate(errp, err); > err = NULL; > visit_end_union(m, !!(*obj)->data, &err); > - error_propagate(errp, err); > - err = NULL; > } > - visit_end_struct(m, &err); > + visit_end_struct(m, err ? NULL : &err); > out: > error_propagate(errp, err); > } Again.
On 09/24/2015 08:58 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Due to the existing semantics of the error_set() family, >> generated sequences in the qapi visitors such as: >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, errp); >> visit_end_implicit_struct(m, &err); >> } >> error_propagate(errp, err); > > Indentation seems off. Intentional? No, probably due to rebase churn (I reindented generated code in 9/46, but the series as I worked on it wasn't always in the order presented here). Will fix. > >> >> are risky: if visit_type_FOO_fields() sets errp, and then >> visit_end_implicit_struct() also encounters an error, the >> attempt to overwrite the first error will cause an abort(). >> Obviously, we weren't triggering this situation (none of the >> existing callbacks for visit_end_implicit_struct() currently >> try to set an error), but it is better to not even cause the >> problem in the first place. > > The code works, but it sets a problematic example. > >> Meanwhile, in spite of the poor documentation of the qapi >> visitors, we want to guarantee that if a visit_start_*() >> succeeds, then the matching visit_end_*() will be called. > > Agreed. > >> The options are to either propagate and clear a local err >> multiple times: >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, &err); >> if (err) { >> error_propagate(errp, err); >> err = NULL; >> } >> visit_end_implicit_struct(m, &err); >> } >> error_propagate(errp, err); More poor indentation on my part. >> >> or, as this patch does, just pass in NULL to ignore further >> errors once an error has occurred. >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, &err); >> visit_end_implicit_struct(m, err ? NULL : &err); >> } >> error_propagate(errp, err); > > Hmmmmm... not sure we do this anywhere else, yet. The ternary isn't > exactly pretty, but the intent to ignore additional error is clear > enough, I think. > > If we elect to adopt this new error handling pattern, we should perhaps > document it in error.h. > > Third option: drop visit_end_implicit_struct()'s errp parameter. If we > find a compelling use for it, we'll put it back and solve the problem. > Ooh, interesting idea. It changes the contract - but since the contract isn't (yet) documented, and happens to work with existing uses without a contract, it could indeed be nicer. It would have knock-on effects to 24/46 where I first try documenting the contract. >> visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err); >> if (!err) { >> - visit_type_%(c_type)s_fields(m, obj, errp); >> - visit_end_implicit_struct(m, &err); >> + visit_type_%(c_type)s_fields(m, obj, &err); >> + visit_end_implicit_struct(m, err ? NULL : &err); >> visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); >> if (!err) { >> if (*obj) { >> - visit_type_%(c_name)s_fields(m, obj, errp); >> + visit_type_%(c_name)s_fields(m, obj, &err); >> } >> - visit_end_struct(m, &err); >> + visit_end_struct(m, err ? NULL : &err); >> } >> error_propagate(errp, err); >> } > > Oh, it's about visit_end_struct(), too. Commit message only talks about > visit_end_implicit_struct(). > > In particular, "none of the existing callbacks for > visit_end_implicit_struct() currently try to set an error". Does that > hold for visit_end_struct() callbacks, too? I'm fairly certain that ALL of the visit_end_* callbacks were similar in nature, but you've prompted me to re-audit things and update the commit message to be absolutely clear about it. > >> @@ -175,9 +175,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error >> visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err); >> } >> >> - error_propagate(errp, err); >> - err = NULL; >> - visit_end_list(m, &err); >> + visit_end_list(m, err ? NULL : &err); >> out: >> error_propagate(errp, err); >> } > > Likewise. Does it hold for visit_end_list() callbacks, too? > > Looks like you switch from option 1 to option 2 here. Your slate isn't > as clean as the commit message suggests :) Consistency is nice, but documenting where we started from to get to the consistent state would be even nicer. Point taken.
On 09/24/2015 10:14 AM, Eric Blake wrote: >>> >>> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >>> if (!err) { >>> visit_type_FOO_fields(m, obj, &err); >>> visit_end_implicit_struct(m, err ? NULL : &err); >>> } >>> error_propagate(errp, err); >> >> Hmmmmm... not sure we do this anywhere else, yet. The ternary isn't >> exactly pretty, but the intent to ignore additional error is clear >> enough, I think. >> >> If we elect to adopt this new error handling pattern, we should perhaps >> document it in error.h. >> >> Third option: drop visit_end_implicit_struct()'s errp parameter. If we >> find a compelling use for it, we'll put it back and solve the problem. >> > > Ooh, interesting idea. It changes the contract - but since the contract > isn't (yet) documented, and happens to work with existing uses without a > contract, it could indeed be nicer. It would have knock-on effects to > 24/46 where I first try documenting the contract. Oh well. We do have a compelling use: qmp-input-visitor.c can set errp during visit_end_struct() when in strict mode (basically, the mode which warns if the input QDict has leftover key:value pairs that were not consumed by visit_type_FOO() between the start/end call). I don't know if visit_end_list() or visit_end_implicit_struct() care; but then we have the argument that it is worth keeping them consistent with visit_end_struct() which can indeed raise an error. One other potential alternative: What if we split visit_end_struct() into two visitor functions, one that checks for success, and the other that is called unconditionally to clean up resources. That is, go from: visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err); if (!err) { if (*obj) { visit_type_FOO_fields(m, obj, errp); } visit_end_struct(m, &err); } error_propagate(errp, err); to a form where the check for leftover key/value pairs is only done on success with a new visit_check_struct(): visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err); if (!err) { if (*obj) { visit_type_FOO_fields(m, obj, &err); } if (!err) { visit_check_struct(m, &err); } visit_end_struct(m); } error_propagate(errp, err);
On 09/24/2015 08:58 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Due to the existing semantics of the error_set() family, >> generated sequences in the qapi visitors such as: >> >> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >> if (!err) { >> visit_type_FOO_fields(m, obj, errp); >> visit_end_implicit_struct(m, &err); >> } >> error_propagate(errp, err); > > Indentation seems off. Intentional? > >> >> are risky: if visit_type_FOO_fields() sets errp, and then >> visit_end_implicit_struct() also encounters an error, the >> attempt to overwrite the first error will cause an abort(). I didn't even read error_propagate()'s contract correctly. It specifically specifies that if errp is already set, then err is ignored. So the above sequence is actually just fine, because only the following paths exist: visit_start_implicit_struct() fails into &err, error_propagate() passes err into caller's errp visit_start_implicit_struct() succeeds, visit_type_FOO_fields() fails into caller's errp, visit_end_implicit_struct() succeeds, error_propagate() does nothing visit_start_implicit_struct() succeeds, visit_type_FOO_fields() fails into caller's errp, visit_end_implicit_struct() fails int &err, error_propagate() does nothing (errp trumps err) visit_start_implicit_struct() succeeds, visit_type_FOO_fields() succeeds, visit_end_implicit_struct() fails int &err, error_propagate() passes err into caller's errp visit_start_implicit_struct() succeeds, visit_type_FOO_fields() succeeds, visit_end_implicit_struct() succeeds, error_propagate() does nothing As such, I'm revisiting if anything is needed at all, other than making the various visit_start/visit_end patterns consistent with one another using existing idioms, and it may turn out we don't need the ternary after all.
On 09/26/2015 03:41 PM, Eric Blake wrote: > On 09/24/2015 08:58 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Due to the existing semantics of the error_set() family, >>> generated sequences in the qapi visitors such as: >>> >>> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >>> if (!err) { >>> visit_type_FOO_fields(m, obj, errp); >>> visit_end_implicit_struct(m, &err); >>> } >>> error_propagate(errp, err); >> >> Indentation seems off. Intentional? >> >>> >>> are risky: if visit_type_FOO_fields() sets errp, and then >>> visit_end_implicit_struct() also encounters an error, the >>> attempt to overwrite the first error will cause an abort(). > > I didn't even read error_propagate()'s contract correctly. It > specifically specifies that if errp is already set, then err is ignored. > > So the above sequence is actually just fine, because only the following > paths exist: > > > As such, I'm revisiting if anything is needed at all, other than making > the various visit_start/visit_end patterns consistent with one another > using existing idioms, and it may turn out we don't need the ternary > after all. Turns out patch 29/46 needs the ternary. There, I'm changing the logic of the various visit_type_FOO() to explicitly set *obj = NULL if something fails in between visit_start_* and visit_end_* - but to do that, I _have_ to track everything locally in &err (since errp might be NULL in the caller). That is, in the above example, if visit_type_FOO_fields() fails, we need to track that locally in order to clean up *obj. Meanwhile, the call to visit_end_implicit_struct() must be unconditional, whether or not we have detected earlier failure.
Eric Blake <eblake@redhat.com> writes: > On 09/24/2015 10:14 AM, Eric Blake wrote: > >>>> >>>> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >>>> if (!err) { >>>> visit_type_FOO_fields(m, obj, &err); >>>> visit_end_implicit_struct(m, err ? NULL : &err); >>>> } >>>> error_propagate(errp, err); >>> >>> Hmmmmm... not sure we do this anywhere else, yet. The ternary isn't >>> exactly pretty, but the intent to ignore additional error is clear >>> enough, I think. >>> >>> If we elect to adopt this new error handling pattern, we should perhaps >>> document it in error.h. >>> >>> Third option: drop visit_end_implicit_struct()'s errp parameter. If we >>> find a compelling use for it, we'll put it back and solve the problem. >>> >> >> Ooh, interesting idea. It changes the contract - but since the contract >> isn't (yet) documented, and happens to work with existing uses without a >> contract, it could indeed be nicer. It would have knock-on effects to >> 24/46 where I first try documenting the contract. > > Oh well. We do have a compelling use: qmp-input-visitor.c can set errp > during visit_end_struct() when in strict mode (basically, the mode which > warns if the input QDict has leftover key:value pairs that were not > consumed by visit_type_FOO() between the start/end call). I don't know > if visit_end_list() or visit_end_implicit_struct() care; but then we > have the argument that it is worth keeping them consistent with > visit_end_struct() which can indeed raise an error. > > One other potential alternative: What if we split visit_end_struct() > into two visitor functions, one that checks for success, and the other > that is called unconditionally to clean up resources. That is, go from: > > visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err); > if (!err) { > if (*obj) { > visit_type_FOO_fields(m, obj, errp); > } > visit_end_struct(m, &err); > } > error_propagate(errp, err); > > to a form where the check for leftover key/value pairs is only done on > success with a new visit_check_struct(): > > visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err); > if (!err) { > if (*obj) { > visit_type_FOO_fields(m, obj, &err); > } > if (!err) { > visit_check_struct(m, &err); > } > visit_end_struct(m); > } > error_propagate(errp, err); I think this split could help with writing safe code: in visit_check_struct() you can rely on "no error so far", as usual. In visit_end_struct(), you can't, but it should be a pure cleanup function, where that's quite normal. Looks like we're getting drawn into visitor contract territory again.
Eric Blake <eblake@redhat.com> writes: > On 09/24/2015 08:58 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Due to the existing semantics of the error_set() family, >>> generated sequences in the qapi visitors such as: >>> >>> visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); >>> if (!err) { >>> visit_type_FOO_fields(m, obj, errp); >>> visit_end_implicit_struct(m, &err); >>> } >>> error_propagate(errp, err); >> >> Indentation seems off. Intentional? >> >>> >>> are risky: if visit_type_FOO_fields() sets errp, and then >>> visit_end_implicit_struct() also encounters an error, the >>> attempt to overwrite the first error will cause an abort(). > > I didn't even read error_propagate()'s contract correctly. It > specifically specifies that if errp is already set, then err is ignored. Yes. Differs from error_set() & friends, where the destination must not contain an error. The inconsistency is a bit ugly. Perhaps it adds enough convenience to make it worthwhile. Anyway, changing it now would be a huge bother. Note that GLib's g_propagate_error() requires the destination not to contain an error. > So the above sequence is actually just fine, because only the following > paths exist: > > visit_start_implicit_struct() fails into &err, > error_propagate() passes err into caller's errp > > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() fails into caller's errp, > visit_end_implicit_struct() succeeds, > error_propagate() does nothing > > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() fails into caller's errp, > visit_end_implicit_struct() fails int &err, > error_propagate() does nothing (errp trumps err) Yes, but visit_end_implicit_struct() gets called with an errp argument that may already contain an error, and that's unusual. Prominent notice in the contract required. > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() succeeds, > visit_end_implicit_struct() fails int &err, > error_propagate() passes err into caller's errp > > visit_start_implicit_struct() succeeds, > visit_type_FOO_fields() succeeds, > visit_end_implicit_struct() succeeds, > error_propagate() does nothing > > > As such, I'm revisiting if anything is needed at all, other than making > the various visit_start/visit_end patterns consistent with one another > using existing idioms, and it may turn out we don't need the ternary > after all.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 97343cf..d911b20 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -51,8 +51,8 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error * visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err); if (!err) { - visit_type_%(c_type)s_fields(m, obj, errp); - visit_end_implicit_struct(m, &err); + visit_type_%(c_type)s_fields(m, obj, &err); + visit_end_implicit_struct(m, err ? NULL : &err); } error_propagate(errp, err); } @@ -143,9 +143,9 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err); if (!err) { if (*obj) { - visit_type_%(c_name)s_fields(m, obj, errp); + visit_type_%(c_name)s_fields(m, obj, &err); } - visit_end_struct(m, &err); + visit_end_struct(m, err ? NULL : &err); } error_propagate(errp, err); } @@ -175,9 +175,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err); } - error_propagate(errp, err); - err = NULL; - visit_end_list(m, &err); + visit_end_list(m, err ? NULL : &err); out: error_propagate(errp, err); } @@ -231,9 +229,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error abort(); } out_end: - error_propagate(errp, err); - err = NULL; - visit_end_implicit_struct(m, &err); + visit_end_implicit_struct(m, err ? NULL : &err); out: error_propagate(errp, err); } @@ -330,10 +326,8 @@ out_obj: error_propagate(errp, err); err = NULL; visit_end_union(m, !!(*obj)->data, &err); - error_propagate(errp, err); - err = NULL; } - visit_end_struct(m, &err); + visit_end_struct(m, err ? NULL : &err); out: error_propagate(errp, err); }
Due to the existing semantics of the error_set() family, generated sequences in the qapi visitors such as: visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); if (!err) { visit_type_FOO_fields(m, obj, errp); visit_end_implicit_struct(m, &err); } error_propagate(errp, err); are risky: if visit_type_FOO_fields() sets errp, and then visit_end_implicit_struct() also encounters an error, the attempt to overwrite the first error will cause an abort(). Obviously, we weren't triggering this situation (none of the existing callbacks for visit_end_implicit_struct() currently try to set an error), but it is better to not even cause the problem in the first place. Meanwhile, in spite of the poor documentation of the qapi visitors, we want to guarantee that if a visit_start_*() succeeds, then the matching visit_end_*() will be called. The options are to either propagate and clear a local err multiple times: visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); if (!err) { visit_type_FOO_fields(m, obj, &err); if (err) { error_propagate(errp, err); err = NULL; } visit_end_implicit_struct(m, &err); } error_propagate(errp, err); or, as this patch does, just pass in NULL to ignore further errors once an error has occurred. visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err); if (!err) { visit_type_FOO_fields(m, obj, &err); visit_end_implicit_struct(m, err ? NULL : &err); } error_propagate(errp, err); Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/qapi-visit.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)