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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 01/20/2016 10:29 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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...)

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.

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

Yes, we'll want to get that right eventually, but it's not urgent.

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

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

Hope to make some more progress today.