On 04/28/2016 06:24 AM, Markus Armbruster wrote: > Eric Blake <[email protected]> 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
