Re: [Qemu-devel] [PATCH v15 02/23] qapi: Guarantee NULL obj on input visitor callback error

[Eric Blake]

On 04/28/2016 06:24 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Our existing input visitors were not very consistent on errors
>> in a function taking 'TYPE **obj' (that is, start_struct(),
>> start_alternate(), next_list(), type_str(), and type_any()).
>> While all of them set '*obj' to allocated storage on success,
>> it was not obvious whether '*obj' was guaranteed safe on failure,
>> or whether it was left uninitialized.  But a future patch wants
>> to guarantee that visit_type_FOO() does not leak a partially-
>> constructed obj back to the caller; it is easier to implement
>> this if we can reliably state that '*obj' is assigned on exit,
>> even on failures.  Add assertions to enforce it.
> 
> I had to read this several times, because by now I've forgotten that
> we're talking about input visitors only.  Easy enough to avoid: ... that
> input visitors assign to *obj regardless of success or failure.
> 
> Begs the question what is assigned to it on failure, though.

Easy enough to improve the commit message - the intent is that these all
set *obj to NULL on failure.


>> +    v->start_struct(v, name, obj, size, &err);
>> +    if (obj && v->type == VISITOR_INPUT) {
>> +        assert(err || *obj);
>> +    }
>> +    error_propagate(errp, err);
>>  }
> 
> The commit message claims you're adding assertions to enforce input
> visitors assign *obj even on failure.  This assertion doesn't do that.
> It enforces "on success, *obj is non-null".  Is that what you want?  Or
> do you actually want something like "either err or *obj are non-null"?
> I.e.
> 
>            assert(!err != !*obj);

Hmm - that's an even tighter assertion.  I'll run it through the
testsuite, and if it passes, I'll use it.


> 
> Hmm, you check the postcondition only when v implements
> start_alternate().  Shouldn't it hold regardless of v?  If yes, then
> let's check it regardless of v:
> 
>        if (v->start_alternate) {
>            v->start_alternate(v, name, obj, size, promote_int, &err);
>        }
>        if (v->type == VISITOR_INPUT) {
>            assert(err || *obj);
>        }
>        error_propagate(errp, err);
> 
> But that makes it pretty obvious that the postcondition won't hold when
> !v->start_alternate.  May v->start_alternate() be null for an input
> visitor?  According to visitor-impl.h, it may not.  Okay.

Doesn't hurt to make the check unconditional (to make sure no new input
visitors forget to implement start_alternate).  I'm also debating about
adding an assertion (more likely in the doc patch, since it is less
related to the theme of this one) that obj->type is sensible.


> 
> The commit message lists start_struct(), start_alternate(), next_list(),
> type_str(), and type_any().  You cover them except for next_list().  Why
> is that missing?

Because *obj can be NULL after next_list() if the list is empty.  But
there may still be a weaker assertion worth having: if err, then *obj
must be NULL; and if *obj, then err must not be set (weaker in that for
all the other functions touched, exactly one of the two conditions can
result, but here, !err and !*obj is valid as a third condition).

Depending on what else you find later in the series, I may just post a
fixup for this patch.

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

signature.asc
Description: OpenPGP digital signature