Markus Armbruster <arm...@redhat.com> writes:
> "Daniel P. Berrange" <berra...@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 <kw...@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> Signed-off-by: Daniel P. Berrange <berra...@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.
>> + 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)
>> {
[...]
