Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor
Markus Armbruster writes:
> Markus Armbruster writes:
>
>> Markus Armbruster writes:
>>
>>> "Daniel P. Berrange" 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
Reviewed-by: Marc-André Lureau
Signed-off-by: Daniel P. Berrange
>> [...]
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.
Nevermind, I got confused. The type is actually always string here, so
your use of QERR_INVALID_PARAMETER_VALUE is appropriate.
[...]
Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor
Markus Armbruster writes:
> Markus Armbruster writes:
>
>> "Daniel P. Berrange" 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
>>> Reviewed-by: Marc-André Lureau
>>> Signed-off-by: Daniel P. Berrange
> [...]
>>> 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 :)
Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor
Markus Armbruster writes:
> "Daniel P. Berrange" 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
>> Reviewed-by: Marc-André Lureau
>> Signed-off-by: Daniel P. Berrange
[...]
>> 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.
Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor
"Daniel P. Berrange" 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 > Reviewed-by: Marc-André Lureau > Signed-off-by: Daniel P. Berrange > --- > include/qapi/qobject-input-visitor.h | 32 +- > qapi/qobject-input-visitor.c | 132 > tests/test-qobject-input-visitor.c | 194 > ++- > 3 files changed, 350 insertions(+), 8 deletions(-) Quite some code. Here's how I'm persuading myself to like it. We need to parse two and a half different languages into QAPI objects: JSON, command line key=value,... (QemuOpts), and just value (same syntax as in QemuOpts). Two language differences stand out: 1. JSON is recursive, key=value,... is flat (but see below), and 2. JSON values have syntactically obvious JSON types, key=value values are just strings. To parse JSON into a QAPI object, we first parse it into a QObject, then use the QObject input visitor to convert the QObject into a QAPI object. To parse key=value, we first parse it into a QemuOpts, then use either the options visitor to convert the QemuOpts into a QAPI object. We can also use the string input visitor to convert just an individual key's value. Or any string for that matter. Note that we always use the QemuOpts *string* value, never the integer or boolean value QemuOpts additionally provides when the key has been specified by a QemuOptDesc with a non-string type. Such QemuOptDesc are best avoided when we parse the string again with a visitor. For when it isn't avoided, the visitors' parsers need to match the QemuOpts parser. Yes, this is a mess. The flatness of key=value,... has become overly limiting. We've grown workarounds for lists in both the options and the string input visitor. Independent ones *sigh*. More recently, the block layer's dotted key convention has provided a way to overcome flatness completely. We could implement this convention in the options visitor. Instead, you're picking a different route: you reuse existing qemu_opts_to_qdict() to go from QemuOpts to flat qdict, create qdict_crumple() to unflatten using dotted key convention, then reuse the existing QObject input visitor to go to QAPI object. This decomposes the task into smaller ones, some of them solved already, at a tolerable cost in efficiency. Except the QObject input visitor won't do as is, because of language difference 2. This patch makes it cope. The need to do this negates much of the code reuse advantage. Doesn't look so hot by itself. However, you then do more work and replace the options visitor, with an obvious path to replacing the string input visitor as well. > diff --git a/include/qapi/qobject-input-visitor.h > b/include/qapi/qobject-input-visitor.h > index cde328d..5022297 100644 > --- a/include/qapi/qobject-input-visitor.h > +++ b/include/qapi/qobject-input-visitor.h > @@ -19,12 +19,36 @@ > > typedef struct QObjectInputVisitor QObjectInputVisitor; > > -/* > - * Return a new input visitor that converts a QObject to a QAPI object. > +/** > + * Create a new input visitor that converts @obj to a QAPI object. > + * > + * Any scalar values in the @obj input data structure should be in the > + * required type already. i.e. if visiting a bool, the value should > + * already be a QBool instance. > * > - * Set @strict to reject a parse that doesn't consume all keys of a > - * dictionary; otherwise excess input is ignored. > + * If @strict is set to true, then an error will be reported if any > + * dict keys are not consumed during visitation. If @strict is false > + * then extra dict keys are silently ignored. Aside: I want to get rid of non-strict, badly. Quoting commit 240f64b "qapi: Use strict QMP input visitor in more places": The only remaining uses of non-strict input visits are: - QMP 'qom-set' (which eventually executes object_property_set_qobject()) - mark it as something to revisit in the future (I didn't want to spend any more time on this patch auditing if we have any QOM dictionary properties that might be impacted, and couldn't easily prove whether this code path is shared with anything else). - test-qmp-input-visitor: explicit tests of non-strict mode. If we later get rid of users that don't need strictness, then this test should be merged with test-qmp-input-strict > + * > + * The retu
Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor
On 09/30/2016 09:45 AM, Daniel P. Berrange wrote: > 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 > 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 > Reviewed-by: Marc-André Lureau > Signed-off-by: Daniel P. Berrange > --- > include/qapi/qobject-input-visitor.h | 32 +- > qapi/qobject-input-visitor.c | 132 > tests/test-qobject-input-visitor.c | 194 > ++- > 3 files changed, 350 insertions(+), 8 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
