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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@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 :)

I await the suggestion on how to do it in a nicer way - the
problem is I'd really like to be able to capture which element failed
to be read when reading in a stream, and that's quite difficult if you only
check the 'err' in a few places (yes you can do it by names passed into the
visitors etc but it gets equally messy).

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

It's in some way why I liked the error_is_set; you ended up with a type
check and it meant you just couldn't make that error.

I did wonder about a modified error_propagate - i.e.
  bool error_propagate(Error **dst_err, Error *local_err)

then you do:
  if (error_propagate(errp, local_err)) {
      goto out;
  }

where the error_propagate would do just what it does at the moment, but return
true if local_err had an error, or if errp was non-null and had an error.
error_propagate could be modified to return that bool without changing any
current caller.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK