Eric Blake <ebl...@redhat.com> writes:
> On 09/24/2015 08:58 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@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.
