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

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

[Markus Armbruster]

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?

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On 04/09/2014 09:48 AM, Markus Armbruster wrote:
> I stumbled over this while trying to purge error_is_set() from the code.
> 

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

Putting the tedium into the generated code is WHY we have generated
code; so that the rest of the code that is hand-written can be concise.
 I like this latter idea of letting the visitors assume that errp is
clean on entry with the caller responsible for checking err after each step.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Anthony Liguori]

On Wed, Apr 9, 2014 at 8:48 AM, 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.
>
> 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?

The original visiting code was loosely based on ASN1 marshaling code
from Samba which used the "if error, bail out at the top" style of
error handling.

As use of Error has evolved in QEMU, I agree that the paradigm of
"bail out as soon as you see an error and fail fast" is better so I'd
vote for changing the generated code to do that.

Regards,

Anthony Liguori

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

[Dr. David Alan Gilbert]

* 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, 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 / dgilbert@redhat.com / Manchester, UK

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

[Kevin Wolf]

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

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

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> The original visiting code was loosely based on ASN1 marshaling code
> from Samba which used the "if error, bail out at the top" style of
> error handling.
>
> As use of Error has evolved in QEMU, I agree that the paradigm of
> "bail out as soon as you see an error and fail fast" is better so I'd
> vote for changing the generated code to do that.

Okay, I'll give it a try.  Thanks!

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

[Markus Armbruster]

"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 :)

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

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

[Markus Armbruster]

Kevin Wolf <kwolf@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?

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

[Dr. David Alan Gilbert]

* 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

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 <kwolf@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

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

[Peter Crosthwaite]

On Thu, Apr 10, 2014 at 1:48 AM, 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.
>
> 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.

That should be the callers problem. If you pass a NULL errp then the
intended semantic is to ignore errors.

>
> The function could be improved like this:
>
>     void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
>     {
>         assert(errp);

And this is irregular in that you are now mandating the Error ** and
thus removing the capability to ignore errors.

>         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 think this code as-is is a good example of what we should do
elsewhere. The code base has bloated with the if (error) { bail; } on
every Error ** accepting API call. I proposed a while back a semantic
that Error ** Accepting APIs perform no action when the error is
already set to allow for long sequences of calls to run without the
constant checks. You then report the first error in a catchall at the
end of the run.

I think this particular code is probably good, provided your case of
NULL errp is enforced against by the caller.

Regards,
Peter

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

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Thu, Apr 10, 2014 at 1:48 AM, 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.
>>
>> 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.
>
> That should be the callers problem. If you pass a NULL errp then the
> intended semantic is to ignore errors.

The *caller* isn't interested in an error.  But the callee's behavior
should *not* be affected by that at all other than not returning an
error.

In particular, the callee should never continue after an error just
because the caller isn't interested in detailed error information.

That's why "if (error_is_set(errp)) bail" and similar are always wrong
when errp is a parameter that may be null.

>
>>
>> The function could be improved like this:
>>
>>     void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
>>     {
>>         assert(errp);
>
> And this is irregular in that you are now mandating the Error ** and
> thus removing the capability to ignore errors.

It is irregular.  The irregularity is necessary as long as the function
isn't prepared for a null 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 think this code as-is is a good example of what we should do
> elsewhere. The code base has bloated with the if (error) { bail; } on
> every Error ** accepting API call. I proposed a while back a semantic
> that Error ** Accepting APIs perform no action when the error is
> already set to allow for long sequences of calls to run without the
> constant checks. You then report the first error in a catchall at the
> end of the run.
>
> I think this particular code is probably good, provided your case of
> NULL errp is enforced against by the caller.

My point isn't that this technique is bad, only that it's different from
what we do everywhere else, and the two techniques do not combine well.

Here's how we handle errors everywhere else:

    void frob([...], Error **errp)
    {
        Error *err = NULL;

        foo(&err)
        if (err) {
            goto out;
        }
        bar(&err) 
        if (err) {
            goto out;
        }
        [...]
    out:
        error_propagate(errp, err);
        [...]
    }

Both foo() and bar() are never entered with an error set.  Consequently,
they don't check for that case.

If you screw up and call them with an error set, they'll die on the
first error of their own, because error_set() asserts the error is
clear.

You might be tempted to elide err, like this:

        foo(errp)
        if (error_is_set(errp)) {
            goto out;
        }
        bar(errp) 
        if (error_is_set(errp)) {
            goto out;
        }
        [...]
   out:
        [...]

But that's *wrong*, because it executes bar() after foo() failed when
errp is null.  Ignoring errors from frob() also changes what frob()
does!  Not an acceptable interface.

You can elide err only in cases where all you ever do with it is pass it
on unexamined.

An alternative technique is to partly move error checks into the
callees, like this:

        err = NULL;
        foo(&err)
        bar(&err) 
        if (err) {
            goto out;
        }
        [...]
    out:
        error_propagate(errp, err);
        [...]

Now bar() must not do anything when called with an error set.  It needs
to begin with code like this:

        if (error_is_set(errp) {
            return;
        }

Trades bloating fewer call sites against bloating all the functions.
Tradeoff.

Note that adding the check above to bar() makes bar() less suited to the
technique I described first.  Remember, with that technique, bar() must
not be called with an error set, and we rely on an assertion to protect
us against mistakes.  The check above completely bypasses the assertion.

Moreover, having both techniques in the tree is bound to lead to
confusion.  Do I have to check after my function call?  Do I have to
check before doing anything in my function?  Sounds like a surefire
recipe for error handling botches to me.

This is what I mean by "the two techniques do not combine well".  Let's
pick one and stick to it.

The first technique is overwhelmingly prevalent now.  The only holdout
of the second technique is some QAPI-related code.

I'm going to try converting that to the first technique.  If you can
come up with patches converting everything else to the second technique,
we can discuss which one is better :)

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

[Peter Crosthwaite]

On Fri, Apr 11, 2014 at 11:41 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> On Thu, Apr 10, 2014 at 1:48 AM, 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.
>>>
>>> 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.
>>
>> That should be the callers problem. If you pass a NULL errp then the
>> intended semantic is to ignore errors.
>
> The *caller* isn't interested in an error.  But the callee's behavior
> should *not* be affected by that at all other than not returning an
> error.
>
> In particular, the callee should never continue after an error just
> because the caller isn't interested in detailed error information.
>

But the error is from a previous call not the current call. Callers
job to inform second call that first one failed (or in current status
quo - not call the second call at all). But its caller job to know the
dependancy otherwise calls are not self contained.

> That's why "if (error_is_set(errp)) bail" and similar are always wrong
> when errp is a parameter that may be null.
>

Agreed. Don't see the problem here though - it's bad caller code too.

>>
>>>
>>> The function could be improved like this:
>>>
>>>     void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
>>>     {
>>>         assert(errp);
>>
>> And this is irregular in that you are now mandating the Error ** and
>> thus removing the capability to ignore errors.
>
> It is irregular.  The irregularity is necessary as long as the function
> isn't prepared for a null errp.
>

My understanding is all functions should be prepared for NULL 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 think this code as-is is a good example of what we should do
>> elsewhere. The code base has bloated with the if (error) { bail; } on
>> every Error ** accepting API call. I proposed a while back a semantic
>> that Error ** Accepting APIs perform no action when the error is
>> already set to allow for long sequences of calls to run without the
>> constant checks. You then report the first error in a catchall at the
>> end of the run.
>>
>> I think this particular code is probably good, provided your case of
>> NULL errp is enforced against by the caller.
>
> My point isn't that this technique is bad, only that it's different from
> what we do everywhere else, and the two techniques do not combine well.
>
> Here's how we handle errors everywhere else:
>
>     void frob([...], Error **errp)
>     {
>         Error *err = NULL;
>
>         foo(&err)
>         if (err) {
>             goto out;
>         }
>         bar(&err)
>         if (err) {
>             goto out;
>         }
>         [...]
>     out:
>         error_propagate(errp, err);
>         [...]
>     }
>
> Both foo() and bar() are never entered with an error set.  Consequently,
> they don't check for that case.
>
> If you screw up and call them with an error set, they'll die on the
> first error of their own, because error_set() asserts the error is
> clear.
>
> You might be tempted to elide err, like this:
>
>         foo(errp)
>         if (error_is_set(errp)) {
>             goto out;
>         }
>         bar(errp)
>         if (error_is_set(errp)) {
>             goto out;
>         }
>         [...]
>    out:
>         [...]
>
> But that's *wrong*, because it executes bar() after foo() failed when
> errp is null.  Ignoring errors from frob() also changes what frob()
> does!  Not an acceptable interface.
>
> You can elide err only in cases where all you ever do with it is pass it
> on unexamined.
>

Yeh so you must define an Error * locally in cases where you want the
first call failure to inhibit the second. I think this is reasonable
when its the callers wish (frob()) to behave so.

> An alternative technique is to partly move error checks into the
> callees, like this:
>
>         err = NULL;
>         foo(&err)
>         bar(&err)
>         if (err) {
>             goto out;
>         }
>         [...]
>     out:
>         error_propagate(errp, err);
>         [...]
>
> Now bar() must not do anything when called with an error set.  It needs
> to begin with code like this:
>
>         if (error_is_set(errp) {
>             return;
>         }
>

The other compormise is to only bail the second function in its own
error case. Let it run as normal and when it encounters an already set
error, ignore it. This is useful when you have lots of independant
calls one after the other . For an extreme example, check the
ppc/e500.c device tree API calls which I want to convert to Error API
but having to "if (err) { bail };" every one of them is not going to
fly.

> Trades bloating fewer call sites against bloating all the functions.
> Tradeoff.
>

In many API cases, number of callers greatly outnumbers number of functions.

> Note that adding the check above to bar() makes bar() less suited to the
> technique I described first.  Remember, with that technique, bar() must
> not be called with an error set, and we rely on an assertion to protect
> us against mistakes.  The check above completely bypasses the assertion.
>
> Moreover, having both techniques in the tree is bound to lead to
> confusion.  Do I have to check after my function call?  Do I have to
> check before doing anything in my function?  Sounds like a surefire
> recipe for error handling botches to me.
>
> This is what I mean by "the two techniques do not combine well".  Let's
> pick one and stick to it.
>
> The first technique is overwhelmingly prevalent now.  The only holdout
> of the second technique is some QAPI-related code.
>
> I'm going to try converting that to the first technique.  If you can
> come up with patches converting everything else to the second technique,
> we can discuss which one is better :)
>

Yeh one day. Will be an awesome -ve diffstat if it happens though.

Regards,
Peter