Re: [Qemu-devel] Error propagation in generated visitors and command marshallers

[Kevin Wolf]

Am 11.04.2014 um 10:28 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> 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