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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> "Daniel P. Berrange" <address@hidden> 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 <address@hidden>
>> Reviewed-by: Marc-André Lureau <address@hidden>
>> Signed-off-by: Daniel P. Berrange <address@hidden>
[...]
>> 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.

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

>> +        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)
>>  {
[...]