Am 11.04.2014 um 10:28 hat Markus Armbruster geschrieben: > Kevin Wolf <kw...@redhat.com> writes: > > > Am 09.04.2014 um 17:48 hat Markus Armbruster geschrieben: > >> 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. > >> > >> 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. > >> > >> Opinions? > > > > I agree, use the same style as everywhere else. > > > > The pattern in the generated visitor that I find more annoying, though, > > is that it has a lot of code like: > > > > if (!error_is_set(errp)) { > > /* long block of code here */ > > } > > > > And I believe there are even cases where this nests. > > I also find "if (error) bail_out" generally more readable than "if > (!error) do_more_work". More so when nested. > > I'll see what I can do about it in the generator scripts. > > > There are also > > error_propagate() calls that can (and do in the common case) propagate > > NULL, this way selecting the first error, if any, but not stopping on > > the first error. I always found it confusing to read that code. > > Can you point me to an instance in the generated code?
It's more or less everywhere. The pattern for structs is something like this: void visit_struct(..., Error *errp) { if (!error_is_set(errp)) { Error *err = NULL; visit_start_struct(..., &err); if (!err) { /* These functions set errors if none is set yet */ do_foo(&err); do_bar(&err); do_baz(&err); } /* err is NULL here for success */ error_propagate(errp, err); } } But you can find similar code for lists and unions. There's also code like this: /* err is NULL here for success */ error_propagate(errp, err); err = NULL; And then err can be assigned new errors, but the subsequent error_propagate() will simply free the Error objects again. Kevin