qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v9 10/37] qapi: Make all visitors supply uint64 callbacks
Date: Wed, 20 Jan 2016 11:10:03 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 01/20/2016 10:29 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Our qapi visitor contract supports multiple integer visitors,
>> but left the type_uint64 visitor as optional (falling back on
>> type_int64); it also marks the type_size visitor as optional
>> (falling back on type_uint64 or even type_int64).
>>
>> Note that the default of falling back on type_int for unsigned
>> visitors can cause confusing results for values larger than
>> INT64_MAX (such as having to pass in a negative 2s complement
>> value on input, and getting a negative result on output).
>>
>> This patch does not fully address the disparity in handling
>> large values as negatives, but does move towards a cleaner
>> situation where EVERY visitor provides both type_int64 and
>> type_uint64 variants as entry points; then each client can
>> either implement sane differences between the two, or document
>> in place with a FIXME that there is munging going on.
> 
> 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...)

But otherwise, yes, in this patch, we are not fixing insanity so much as
localizing and better documenting it.

I've given some thought about converting the QObject 'int' type to track
a sign bit, making it effectively a superset covering 3*2^63 values in
the range [INT64_MIN, UINT64_MAX], and then enhancing the parser to
track if a negative value was parsed and the formatter to output
negative if the sign bit is set, and then make the 'int64' and 'uint64'
parsers stick to stricter 2*64 subsets of that range.  But I haven't
actually written a patch along those lines yet.

> 
>> 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?

> 
>> Then, in qapi-visit-core.c, we can now use the guaranteed
>> type_uint64 callback as the fallback for all smaller unsigned
>> int visits.
> 
> The type_int64() callback works just fine for smaller unsigned ints, but
> I agree avoiding mixed signedness by using type_uint64() make sense once
> type_uint64() is mandatory.
> 

>> +++ b/qapi/qapi-visit-core.c
>> @@ -102,15 +102,15 @@ void visit_type_int(Visitor *v, int64_t *obj, const 
>> char *name, Error **errp)
>>
>>  void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error 
>> **errp)
>>  {
>> -    int64_t value;
>> +    uint64_t value;
>>
>>      if (v->type_uint8) {
>>          v->type_uint8(v, obj, name, errp);
>>      } else {
>>          value = *obj;
>> -        v->type_int64(v, &value, name, errp);
>> -        if (value < 0 || value > UINT8_MAX) {
>> -            /* FIXME questionable reuse of errp if type_int64() changes
>> +        v->type_uint64(v, &value, name, errp);
>> +        if (value > UINT8_MAX) {
>> +            /* FIXME questionable reuse of errp if type_uint64() changes
>>                 value on error */
>>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                         name ? name : "null", "uint8_t");
> 
> You could delay adding these FIXMEs until this patch, and reduce churn.
> Probably not worth the bother now.

Yeah, I'll see how the rest of the series review goes.

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]