On 09/24/2015 08:58 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature