On 01/05/2016 07:05 AM, Marc-André Lureau wrote: > Hi > > On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <ebl...@redhat.com> wrote: >> Commit 6c2f9a15 ensured that we would not return NULL when the >> caller used an output visitor but had nothing to visit. But >> in doing so, it added a FIXME about a reference count leak >> that could abort qemu in the (unlikely) case of SIZE_MAX such >> visits (more plausible on 32-bit). (Although that commit >> suggested we might fix it in time for 2.5, we ran out of time; >> fortunately, it is unlikely enough to bite that it was not >> worth worrying about during the 2.5 release.) >> >> This fixes things by documenting the internal contracts, and >> explaining why the internal function can return NULL and only >> the public facing interface needs to worry about qnull(), >> thus avoiding over-referencing the qnull_ global object. >> >> It does not, however, fix the stupidity of the stack mixing >> up two separate pieces of information; add a FIXME to explain >> that issue. >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> Cc: qemu-sta...@nongnu.org >>
>> +++ b/qapi/qmp-output-visitor.c
>> @@ -29,6 +29,15 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
>> struct QmpOutputVisitor
>> {
>> Visitor visitor;
>> + /* FIXME: we are abusing stack to hold two separate pieces of
>> + * information: the current root object in slot 0, and the stack
>> + * of N objects still being built in slots 1 through N (for N+1
>> + * slots in use). Worse, our behavior is inconsistent:
>> + * qmp_output_add_obj() visiting two top-level scalars in a row
>> + * discards the first in favor of the second, but visiting two
>> + * top-level objects in a row tries to append the second object
>> + * into the first (since the first object was placed in the stack
>> + * in both slot 0 and 1, but only popped from slot 1). */
>
> I skipped checking thoroughly this comment, since it's a bit
> off-topic, although it looks ok.
>
> Later, oh well, it's fixed in next commit. Imho it's not strictly
> necessary in this commit.
I added the comment based on Markus' request that I document how the
stack is used; but yes, it does feel like a bit of churn since it
changes in the next commit.
If there's a reason to respin, I might change it to:
Visitor visitor;
/* Stack holds two pieces of information: the current root object in
* slot 0, then a stack of N objects still being built in slots 1
* through N (for N+1 slots in use).
* FIXME: The root object should be stored separately from the
* stack, particularly since qmp_output_add_obj() behaves
* differently when visiting two top-level scalars in a row than
* it does for two objects (the second object is appended to the
* first, since the first is placed in both slots 0 and 1 but only
* popped from slot 1). */
>
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
