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

[Eric Blake]

On 09/26/2015 03:41 PM, Eric Blake wrote:
> On 09/24/2015 08:58 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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.
> 
> So the above sequence is actually just fine, because only the following
> paths exist:
> 

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

Turns out patch 29/46 needs the ternary.  There, I'm changing the logic
of the various visit_type_FOO() to explicitly set *obj = NULL if
something fails in between visit_start_* and visit_end_* - but to do
that, I _have_ to track everything locally in &err (since errp might be
NULL in the caller).  That is, in the above example, if
visit_type_FOO_fields() fails, we need to track that locally in order to
clean up *obj.  Meanwhile, the call to visit_end_implicit_struct() must
be unconditional, whether or not we have detected earlier failure.

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

signature.asc
Description: OpenPGP digital signature