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. 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.
Kevin
