Re: [Qemu-devel] [PATCH 08/17] qapi: update the qobject visitor to use QUInt

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> On the subject: there is no such thing as "QUInt".  I guess you mean
> "uint type" (like in PATCH 06's subject).  Could also say "QNUM_U64".
>
> Apropos subject: humor me, and start your subjects with a capital
> letter, like this:
>
>     qapi: Update the qobject visitor ...
>
> Marc-André Lureau <address@hidden> writes:
>
>> Switch to use QNum/uint where appropriate to remove i64 limitation.
>>
>> The input visitor will cast i64 input to u64 for compatibility
>> reasons (existing json QMP client already use negative i64 for large
>> u64, and expect an implicit cast in qemu).
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  qapi/qobject-input-visitor.c        | 13 +++++++++++--
>>  qapi/qobject-output-visitor.c       |  3 +--
>>  tests/test-qobject-output-visitor.c | 21 ++++++++++++++++-----
>>  3 files changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 785949ebab..72cefcf677 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor *v, 
>> const char *name,
>>  static void qobject_input_type_uint64(Visitor *v, const char *name,
>>                                        uint64_t *obj, Error **errp)
>>  {
>> -    /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */
>>      QObjectInputVisitor *qiv = to_qiv(v);
>>      QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
>> +    Error *err = NULL;
>>      QNum *qnum;
>>  
>>      if (!qobj) {
>> @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v, const 
>> char *name,
>>          return;
>>      }
>>  
>> -    *obj = qnum_get_int(qnum, errp);
>> +    /* XXX: compatibility case, accept negative values as u64 */
>
> What does "XXX" signify?
>
>> +    *obj = qnum_get_int(qnum, &err);
>> +
>
> Shouldn't the comment go right here?

Nope, I misread the code.

We first try qnum_get_int().  Works for integers between -2^63 and
2^63-1.  The assignment to *obj adds 2^64 to negative ones.

When it doesn't work, we try qnum_get_uint().  Works for integers
between 0 and 2^64-1, but only integers between 2^63 and 2^64-1 can
occur.

>> +    if (err) {
>> +        error_free(err);
>> +        err = NULL;
>> +        *obj = qnum_get_uint(qnum, &err);
>> +    }
>> +
>> +    error_propagate(errp, err);
>>  }

Let's do this the other way round:

    *obj = qnum_get_uint(qnum, &err);
    if (err) {
        /* Need to accept negative values for backward compatibility */
        error_free(err);
        err = NULL;
        *obj = qnum_get_int(qnum, &err);
    }

    error_propagate(errp, err);

This way, the backward compatibility code is entirely contained in the
conditional.

It's also how I misread the code :)

[...]