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

[Eric Blake <eblake@redhat.com>]

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

On 09/24/2015 10:14 AM, Eric Blake wrote:

>>>
>>>     visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
>>>         if (!err) {
>>>             visit_type_FOO_fields(m, obj, &err);
>>>             visit_end_implicit_struct(m, err ? NULL : &err);
>>>         }
>>>     error_propagate(errp, err);
>>
>> Hmmmmm... not sure we do this anywhere else, yet.  The ternary isn't
>> exactly pretty, but the intent to ignore additional error is clear
>> enough, I think.
>>
>> If we elect to adopt this new error handling pattern, we should perhaps
>> document it in error.h.
>>
>> Third option: drop visit_end_implicit_struct()'s errp parameter.  If we
>> find a compelling use for it, we'll put it back and solve the problem.
>>
> 
> Ooh, interesting idea.  It changes the contract - but since the contract
> isn't (yet) documented, and happens to work with existing uses without a
> contract, it could indeed be nicer.  It would have knock-on effects to
> 24/46 where I first try documenting the contract.

Oh well.  We do have a compelling use: qmp-input-visitor.c can set errp
during visit_end_struct() when in strict mode (basically, the mode which
warns if the input QDict has leftover key:value pairs that were not
consumed by visit_type_FOO() between the start/end call).  I don't know
if visit_end_list() or visit_end_implicit_struct() care; but then we
have the argument that it is worth keeping them consistent with
visit_end_struct() which can indeed raise an error.

One other potential alternative:  What if we split visit_end_struct()
into two visitor functions, one that checks for success, and the other
that is called unconditionally to clean up resources.  That is, go from:

    visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err);
    if (!err) {
        if (*obj) {
            visit_type_FOO_fields(m, obj, errp);
        }
        visit_end_struct(m, &err);
    }
    error_propagate(errp, err);

to a form where the check for leftover key/value pairs is only done on
success with a new visit_check_struct():

    visit_start_struct(m, (void **)obj, "foo", name, sizeof(FOO), &err);
    if (!err) {
        if (*obj) {
            visit_type_FOO_fields(m, obj, &err);
        }
        if (!err) {
            visit_check_struct(m, &err);
        }
        visit_end_struct(m);
    }
    error_propagate(errp, err);

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