[v9,10/37] qapi: Make all visitors supply uint64 callbacks

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.

The dealloc visitor no longer needs a type_size callback,
since that now safely falls back to the type_uint64 callback.

Then, in qapi-visit-core.c, we can now use the guaranteed
type_uint64 callback as the fallback for all smaller unsigned
int visits.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v9: hoist in part of 11/35, drop Marc-Andre's R-b
v8: no change
v7: split off int64 callbacks and retitle, add more FIXMEs in the
code, hoist use of type_uint64 here from 3/23, improved commit
message
v6: new patch, but stems from v5 23/46
---
 include/qapi/visitor-impl.h  |  9 ++++++---
 qapi/qapi-dealloc-visitor.c  | 12 ++++++------
 qapi/qapi-visit-core.c       | 42 ++++++++++++++----------------------------
 qapi/qmp-input-visitor.c     | 17 +++++++++++++++++
 qapi/qmp-output-visitor.c    |  9 +++++++++
 qapi/string-input-visitor.c  | 15 +++++++++++++++
 qapi/string-output-visitor.c |  9 +++++++++
 7 files changed, 76 insertions(+), 37 deletions(-)

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?

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

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

> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v9: hoist in part of 11/35, drop Marc-Andre's R-b
> v8: no change
> v7: split off int64 callbacks and retitle, add more FIXMEs in the
> code, hoist use of type_uint64 here from 3/23, improved commit
> message
> v6: new patch, but stems from v5 23/46
> ---
>  include/qapi/visitor-impl.h  |  9 ++++++---
>  qapi/qapi-dealloc-visitor.c  | 12 ++++++------
>  qapi/qapi-visit-core.c       | 42 ++++++++++++++----------------------------
>  qapi/qmp-input-visitor.c     | 17 +++++++++++++++++
>  qapi/qmp-output-visitor.c    |  9 +++++++++
>  qapi/string-input-visitor.c  | 15 +++++++++++++++
>  qapi/string-output-visitor.c |  9 +++++++++
>  7 files changed, 76 insertions(+), 37 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index c263a26..45c1d3e 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -40,6 +40,12 @@ struct Visitor
>      void (*type_int64)(Visitor *v, int64_t *obj, const char *name,
>                         Error **errp);
>      /* Must be set. */
> +    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
> +                        Error **errp);
> +    /* Optional; fallback is type_uint64().  */
> +    void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
> +                      Error **errp);
> +    /* Must be set. */
>      void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
>      void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
>      void (*type_number)(Visitor *v, double *obj, const char *name,
> @@ -53,12 +59,9 @@ struct Visitor
>      void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
>      void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
>      void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
> -    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
>      void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp);
>      void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
>      void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp);
> -    /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
> -    void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
>      bool (*start_union)(Visitor *v, bool data_present, Error **errp);
>      void (*end_union)(Visitor *v, bool data_present, Error **errp);
>  };
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index e9b9f3f..11eb828 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -140,6 +140,11 @@ static void qapi_dealloc_type_int64(Visitor *v, int64_t *obj, const char *name,
>  {
>  }
>
> +static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj,
> +                                     const char *name, Error **errp)
> +{
> +}
> +
>  static void qapi_dealloc_type_bool(Visitor *v, bool *obj, const char *name,
>                                     Error **errp)
>  {
> @@ -158,11 +163,6 @@ static void qapi_dealloc_type_anything(Visitor *v, QObject **obj,
>      }
>  }
>
> -static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name,
> -                                   Error **errp)
> -{
> -}
> -
>  static void qapi_dealloc_type_enum(Visitor *v, int *obj,
>                                     const char * const strings[],
>                                     const char *kind, const char *name,
> @@ -220,11 +220,11 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.end_list = qapi_dealloc_end_list;
>      v->visitor.type_enum = qapi_dealloc_type_enum;
>      v->visitor.type_int64 = qapi_dealloc_type_int64;
> +    v->visitor.type_uint64 = qapi_dealloc_type_uint64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
>      v->visitor.type_str = qapi_dealloc_type_str;
>      v->visitor.type_number = qapi_dealloc_type_number;
>      v->visitor.type_any = qapi_dealloc_type_anything;
> -    v->visitor.type_size = qapi_dealloc_type_size;
>      v->visitor.start_union = qapi_dealloc_start_union;
>
>      QTAILQ_INIT(&v->stack);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6295fa8..4a8ad43 100644
> --- a/qapi/qapi-visit-core.c
> +++ 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.

[...]

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

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

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.