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

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) 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, 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.

Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK