"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> I stumbled over this while trying to purge error_is_set() from the code. > >> Here's how we commonly use the Error API: >> >> Error *err = NULL; >> >> foo(arg, &err) >> if (err) { >> goto out; >> } >> bar(arg, &err) >> if (err) { >> goto out; >> } >> >> This ensures that err is null on entry, both for foo() and for bar(). >> Many functions rely on that, like this: >> >> void foo(ArgType arg, Error **errp) >> { >> if (frobnicate(arg) < 0) { >> error_setg(errp, "Can't frobnicate"); >> // This asserts errp != NULL >> } >> } >> >> >> Here's how some of our visitor code uses the Error API (for real code, >> check out generated qmp-marshal.c): >> >> Error *err = NULL; >> QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args)); >> Visitor *v = qmp_input_get_visitor(mi); >> char *foo = NULL; >> char *bar = NULL; >> >> visit_type_str(v, &foo, "foo", &err); >> visit_type_str(v, &bar, "bar", &err); >> if (err) { >> goto out; >> } >> >> Unlike above, this may pass a non-null errp to the second >> visit_type_str(), namely when the first one fails. > > Right, one of the problems is you just have long strings of visit_* calls > and adding a check to each one hides what you're actually doing in a sea > of checks. The downside is that if one of those visit's fails then you've > got no chance of figuring out which one it was. > > In my BER world I've got some macros along the lines of: > > #define LOCAL_ERR_REPORT(fallout) \ > if (local_err) { \ > fallout \ > } > > and at least then I can do things like: > visit_type_str(v, &foo, "foo", &err); > LOCAL_ERR_REPORT( goto out; ) > visit_type_str(v, &bar, "bar", &err); > LOCAL_ERR_REPORT( goto out; ) > > which while not nice,
Understatement :) > means that you can actually follow the code, and > I can also add a printf to the macro to record the function/line so > that when one of them fails I can see which visit was the cause of the problem > (something that's currently very difficult). > >> The visitor functions guard against that, like this: >> >> void visit_type_str(Visitor *v, char **obj, const char *name, Error >> **errp) >> { >> if (!error_is_set(errp)) { >> v->type_str(v, obj, name, errp); >> } >> } >> >> As discussed before, error_is_set() is almost almost wrong, fragile or >> unclean. What if errp is null? Then we fail to stop visiting after an >> error. >> >> The function could be improved like this: >> >> void visit_type_str(Visitor *v, char **obj, const char *name, Error >> **errp) >> { >> assert(errp); >> if (!*errp) { >> v->type_str(v, obj, name, errp); >> } >> } >> >> >> But: is it a good idea to have both patterns in the code? Should we >> perhaps use the common pattern for visiting, too? Like this: >> >> visit_type_str(v, &foo, "foo", &err); >> if (err) { >> goto out; >> } >> visit_type_str(v, &bar, "bar", &err); >> if (err) { >> goto out; >> } >> >> Then we can assume *errp is clear on function entry, like this: >> >> void visit_type_str(Visitor *v, char **obj, const char *name, Error >> **errp) >> { >> v->type_str(v, obj, name, errp); >> } >> >> Should execute roughly the same number of conditional branches. >> >> Tedious repetition of "if (err) goto out" in the caller, but that's what >> we do elsewhere, and unlike elsewhere, these one's are generated. > > The other problem is I had a tendency to typo some of the cases to > if (*err) and it's quite hard to spot and you wonder what's going on. The only help I can offer with that is naming conventions: use "errp" only for Error ** variables, and "err" only for Error *. I have patches in my queue to clean up current usage.