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