Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 09/24/2015 08:58 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Due to the existing semantics of the error_set() family,
>>> generated sequences in the qapi visitors such as:
>>>
>>>     visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
>>>         if (!err) {
>>>             visit_type_FOO_fields(m, obj, errp);
>>>             visit_end_implicit_struct(m, &err);
>>>         }
>>>     error_propagate(errp, err);
>> 
>> Indentation seems off.  Intentional?
>> 
>>>
>>> are risky: if visit_type_FOO_fields() sets errp, and then
>>> visit_end_implicit_struct() also encounters an error, the
>>> attempt to overwrite the first error will cause an abort().
>
> I didn't even read error_propagate()'s contract correctly. It
> specifically specifies that if errp is already set, then err is ignored.

Yes.  Differs from error_set() & friends, where the destination must not
contain an error.  The inconsistency is a bit ugly.  Perhaps it adds
enough convenience to make it worthwhile.  Anyway, changing it now would
be a huge bother.

Note that GLib's g_propagate_error() requires the destination not to
contain an error.

> So the above sequence is actually just fine, because only the following
> paths exist:
>
> visit_start_implicit_struct() fails into &err,
> error_propagate() passes err into caller's errp
>
> visit_start_implicit_struct() succeeds,
> visit_type_FOO_fields() fails into caller's errp,
> visit_end_implicit_struct() succeeds,
> error_propagate() does nothing
>
> visit_start_implicit_struct() succeeds,
> visit_type_FOO_fields() fails into caller's errp,
> visit_end_implicit_struct() fails int &err,
> error_propagate() does nothing (errp trumps err)

Yes, but visit_end_implicit_struct() gets called with an errp argument
that may already contain an error, and that's unusual.  Prominent notice
in the contract required.

> visit_start_implicit_struct() succeeds,
> visit_type_FOO_fields() succeeds,
> visit_end_implicit_struct() fails int &err,
> error_propagate() passes err into caller's errp
>
> visit_start_implicit_struct() succeeds,
> visit_type_FOO_fields() succeeds,
> visit_end_implicit_struct() succeeds,
> error_propagate() does nothing
>
>
> As such, I'm revisiting if anything is needed at all, other than making
> the various visit_start/visit_end patterns consistent with one another
> using existing idioms, and it may turn out we don't need the ternary
> after all.