Markus Armbruster <arm...@redhat.com> 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 <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
>> ---
>> 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 :)
[...]
