Re: [Qemu-devel] [PATCH v9 10/37] qapi: Make all visitors supply uint64 callbacks

[Eric Blake]

On 01/21/2016 01:56 AM, Markus Armbruster wrote:

>>> Before: nobody implements type_uint64(), and the core falls back to
>>> type_int64(), casting negative values to large positive ones.  With an
>>> implementation of type_int64() that parses large positive values as
>>> negative, the two casts cancel out.
>>>
>>> After: everybody implements type_uint64() incorrectly, by reusing
>>> type_int64() code.  The problem moves from visitor core to visitor
>>> implementations, where we can actually fix it if we care.
>>>
>>> Correct?
>>
>> Close. opts-visitor.c already implemented type_uint64, but also has its
>> known bugs (and Paolo has been pinged about his claim for fixes in that
>> file...)
> 
> Ah, yes.  opts_type_uint64() doesn't reuse opts_type_int64(), but
> largely duplicates it.  Potentially less wrong :)
> 
>> But otherwise, yes, in this patch, we are not fixing insanity so much as
>> localizing and better documenting it.
> 
> Let me try to clarify the commit message a bit:
> 
>     This patch does not address the disparity in handling large values
>     as negatives.  It merely moves the fallback from uint64 to int64
>     from the visitor core to the visitors, where the issue can actually
>     be fixed, by implementing the missing type_uint64() callbacks on top
>     of the respective type_int64() callbacks, with a FIXME comment
>     explaining why that's wrong.

Looks good.  I think we're starting to rack up enough tweaks to make it
worth me posting a v10 respin to fold them all in.

>>>> The dealloc visitor no longer needs a type_size callback,
>>>> since that now safely falls back to the type_uint64 callback.
>>>
>>> Did it need the callback before this patch?
>>
>> Not really.  Should I split out the deletion of that callback as a
>> separate patch?
> 
> Probably cleanest, but merely adjusting the commit message would also
> work for me:
> 
>     The dealloc visitor doesn't actually need a type_size() callback,
>     since the visitor core can safely fall back to the type_uint64()
>     callback.  Drop it.

Okay, I won't bother to split this one; the commit message tweak seems
sufficient.

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

signature.asc
Description: OpenPGP digital signature