Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>
>>> Currently the QObjectInputVisitor assumes that all scalar
>>> values are directly represented as the final types declared
>>> by the thing being visited. ie it assumes an 'int' is using
>>
>> i.e.
>>
>>> QInt, and a 'bool' is using QBool, etc.  This is good when
>>> QObjectInputVisitor is fed a QObject that came from a JSON
>>> document on the QMP monitor, as it will strictly validate
>>> correctness.
>>>
>>> To allow QObjectInputVisitor to be reused for visiting
>>> a QObject originating from QemuOpts, an alternative mode
>>> is needed where all the scalars types are represented as
>>> QString and converted on the fly to the final desired
>>> type.
>>>
>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> [...]
>>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>>> index 5ff3db3..cf41df6 100644
>>> --- a/qapi/qobject-input-visitor.c
>>> +++ b/qapi/qobject-input-visitor.c
>>> @@ -20,6 +20,7 @@
>>>  #include "qemu-common.h"
>>>  #include "qapi/qmp/types.h"
>>>  #include "qapi/qmp/qerror.h"
>>> +#include "qemu/cutils.h"
>>>  
>>>  #define QIV_STACK_SIZE 1024
>>>  
>>> @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, const char *name, int64_t *obj,
>>>      *obj = qint_get_int(qint);
>>>  }
>>>  
>>> +
>>> +static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
>>> +                                              int64_t *obj, Error **errp)
>>> +{
>>> +    QObjectInputVisitor *qiv = to_qiv(v);
>>> +    QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>>> +                                                                true));
>>
>> Needs a rebase for commit 1382d4a.  Same elsewhere.
>>
>>> +    int64_t ret;
>>> +
>>> +    if (!qstr || !qstr->string) {
>>
>> I don't think !qstr->string can happen.  Same elsewhere.
>>
>>> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>> +                   "string");
>>> +        return;
>>> +    }
>>
>> So far, this is basically qobject_input_type_str() less the g_strdup().
>> Worth factoring out?
>>
>> Now we're entering out parsing swamp:
>>
>>> +
>>> +    if (qemu_strtoll(qstr->string, NULL, 0, &ret) < 0) {
>>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>
> "integer", actually.

Wait!  You're using QERR_INVALID_PARAMETER_VALUE here, so it's "an
integer".

To be pedantically correct, we'd have to complain about the type when
qemu_strtoll() fails to parse, and about the value when it parses okay,
but the value is out of range.

>>> +        return;
>>> +    }
>>
>> To serve as replacement for the options visitor, this needs to parse
>> exactly like opts_type_int64().
>>
>> It should also match the JSON parser as far as possible, to minimize
>> difference between the two QObject input visitor variants, and the
>> QemuOpts parser, for command line consistency.
>>
>> opts_type_int64() uses strtoll() directly.  It carefully checks for no
>> conversion (both ways, EINVAL and endptr == str), range, and string not
>> fully consumed.
>>
>> Your code uses qemu_strtoll().  Bug#1: qemu_strtoll() assumes long long
>> is exactly 64 bits.  If it's wider, we fail to diagnose overflow.  If
>> it's narrower, we can't parse all values.  Bug#2: your code fails to
>> check the string is fully consumed.
>>
>> The JSON parser also uses strtoll() directly, in parse_literal().  When
>> we get there, we know that the string consists only of decimal digits,
>> possibly prefixed by a minus sign.  Therefore, strtoll() can only fail
>> with ERANGE, and parse_literal() handles that correctly.
>>
>> QemuOpts doesn't do signed integers.
>>
>>> +    *obj = ret;
>>> +}
>>> +
>>>  static void qobject_input_type_uint64(Visitor *v, const char *name,
>>>                                        uint64_t *obj, Error **errp)
>>>  {
>>> @@ -279,6 +302,27 @@ static void qobject_input_type_uint64(Visitor *v, const char *name,
>>>      *obj = qint_get_int(qint);
>>>  }
>>>  
>>> +static void qobject_input_type_uint64_autocast(Visitor *v, const char *name,
>>> +                                               uint64_t *obj, Error **errp)
>>> +{
>>> +    QObjectInputVisitor *qiv = to_qiv(v);
>>> +    QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>>> +                                                                true));
>>> +    unsigned long long ret;
>>> +
>>> +    if (!qstr || !qstr->string) {
>>> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>> +                   "string");
>>> +        return;
>>> +    }
>>> +
>>> +    if (parse_uint_full(qstr->string, &ret, 0) < 0) {
>>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>
> "integer".

Likewise.

>>> +        return;
>>> +    }
>>> +    *obj = ret;
>>
>> Differently wrong :)
>>
>> To serve as replacement for the options visitor, this needs to parse
>> exactly like opts_type_uint64().
>>
>> Again, this should also match the JSON parser and the QemuOpts parser as
>> far as possible.
>>
>> opts_type_uint64() uses parse_uint().  It carefully checks for no
>> conversion (EINVAL; parse_uint() normalizes), range, and string not
>> fully consumed.
>>
>> You use parse_uint_full().  You therefore don't have bug#2 here.  You do
>> have bug#1: you assume unsigned long long is exactly 64 bits.  If it's
>> wider, we fail to diagnose overflow.  If it's narrower, we can't parse
>> all values.
>>
>> There's a discrepancy with the JSON parser, but it's not your fault: the
>> JSON parser represents JSON numbers that fit into int64_t to QInt, and
>> any others as QFloat.  In particular, the integers between INT64_MAX+1
>> and UINT64_MAX are represented as QFloat.  qmp_input_type_uint64()
>> accepts only QInt.  You can declare things as uint64 in the schema, but
>> you're still limited to 0..UINT64_MAX in QMP.
>>
>> QemuOpts uses strtoull() directly, in parse_option_number().  Bug (not
>> yours): it happily ignores errors other than "string not fully
>> consumed".
>>
>>> +}
>>> +
>>>  static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
>>>                                      Error **errp)
>>>  {
> [...]