Re: [Qemu-devel] [PATCH] qapi: change QmpInputVisitor to QSLIST

[Eric Blake]

On 07/07/2016 02:42 AM, Markus Armbruster wrote:
> Needs a rebase.  The other one, too.
> 
> Paolo Bonzini <address@hidden> writes:
> 
>> This saves a lot of memory compared to a statically-sized array.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
> 
> Saves 24KiB - d*32B.  That's "a lot" for an Atari ST or something, or if
> you're juggling hundreds of these things at the same time.
> 
> Allocating 24KiB and using only the first d*24 bytes is simpler and
> generally faster than allocating d chunks of 32B each.  But I won't
> argue, as I'm pretty confident that neither memory nor CPU use of this
> code matter.
> 
> Another reason not to argue: commit e38ac96 already added a per-depth
> allocation.
> 
> So all I ask for here is s/a lot of memory/some memory/.
> 
> The patch makes QmpInputVisitor more similar to QmpOutputVisitor, which
> is nice.
> 

Yes, this second effect is good enough reason for the patch, regardless
of the merits of any memory savings in the first effect.

>>      assert(obj);
>> -    if (qiv->nb_stack >= QIV_STACK_SIZE) {
>> -        error_setg(errp, "An internal buffer overran");
>> -        return NULL;
>> -    }
> 
> Removing the arbitrary limit is aesthetically pleasing.  But can
> untrusted input make us allocate unbounded amounts of memory now?

Elsewhere in the thread, we mentioned that the input visitor never
visits more than what an existing QObject already has in memory. Calling
that out in the commit message might have been nice (but v2 did not do
that).

> 
>> -
>>      tos->obj = obj;
>> -    assert(!tos->h);
>> -    assert(!tos->entry);
> 
> Why do you delete these two?
> 

Because pre-patch, these fields are inherited in the state left on the
stack from any earlier visits (we are asserting that other branches of
the visit left things in a sane state), but post-patch, we are
malloc0()ing each tos fresh (rather than reusing).



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

signature.asc
Description: OpenPGP digital signature