| Message ID | 1468468228-27827-17-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 13, 2016 at 09:50:27PM -0600, Eric Blake wrote: > Currently the QmpInputVisitor assumes that all scalar > values are directly represented as their final types. > ie it assumes an 'int' is using QInt, and a 'bool' is > using QBool. > > This adds an alternative mode where a QString can also > be parsed in place of the native type, by adding a parameter > and updating all callers. For now, only the testsuite sets > the new autocast parameter. > > This makes it possible to use QmpInputVisitor with a QDict > produced from QemuOpts, where everything is in string format. > It also makes it possible for the next patch to support > back-compat in legacy commands like 'netdev_add' that no > longer use QemuOpts, but must parse the same set of QMP > constructs that QemuOpts would historically allow. I'm not really a huge fan of the approach there. IMHO the input visitor should strictly operate in "all native types" or "all string types" mode. The way you've done it here means that users can actually mix & match strings vs native types for each individual parameter :-( IMHO, I'd suggest you try to parse netdev_add args with it in "all native types" mode, if that fails, then re-parse in "all string types" mode. For that you'd not have to modify my patch at all. Regards, Daniel
On 07/14/2016 02:04 AM, Daniel P. Berrange wrote: > On Wed, Jul 13, 2016 at 09:50:27PM -0600, Eric Blake wrote: >> Currently the QmpInputVisitor assumes that all scalar >> values are directly represented as their final types. >> ie it assumes an 'int' is using QInt, and a 'bool' is >> using QBool. >> >> This adds an alternative mode where a QString can also >> be parsed in place of the native type, by adding a parameter >> and updating all callers. For now, only the testsuite sets >> the new autocast parameter. >> >> This makes it possible to use QmpInputVisitor with a QDict >> produced from QemuOpts, where everything is in string format. >> It also makes it possible for the next patch to support >> back-compat in legacy commands like 'netdev_add' that no >> longer use QemuOpts, but must parse the same set of QMP >> constructs that QemuOpts would historically allow. > > I'm not really a huge fan of the approach there. IMHO the > input visitor should strictly operate in "all native types" > or "all string types" mode. The way you've done it here > means that users can actually mix & match strings vs native > types for each individual parameter :-( > > IMHO, I'd suggest you try to parse netdev_add args with > it in "all native types" mode, if that fails, then re-parse > in "all string types" mode. Sadly, this idea won't work. Without this series, the existing QMP command 'netdev_add' takes mixed integers and strings in a single call, by virtue of converting QObject into QemuOpts and back into QObject. Forcing the parser to allow only integers or only strings would reject current uses of netdev_add, and we don't yet have a good idea whether any existing clients were depending on that behavior. Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse is by setting a single flag which controls which visitor constructor is used. Your idea of parsing once with integer-only, then falling back to parse a second time with string-only, would complicate the generator to have to create two different visitors, rather than passing a single boolean parameter through to the single visitor constructor. > > For that you'd not have to modify my patch at all. > Also not quite true - your patch no longer applies to master, so it needed rebasing anyway.
On Thu, Jul 14, 2016 at 06:43:16AM -0600, Eric Blake wrote: > On 07/14/2016 02:04 AM, Daniel P. Berrange wrote: > > On Wed, Jul 13, 2016 at 09:50:27PM -0600, Eric Blake wrote: > >> Currently the QmpInputVisitor assumes that all scalar > >> values are directly represented as their final types. > >> ie it assumes an 'int' is using QInt, and a 'bool' is > >> using QBool. > >> > >> This adds an alternative mode where a QString can also > >> be parsed in place of the native type, by adding a parameter > >> and updating all callers. For now, only the testsuite sets > >> the new autocast parameter. > >> > >> This makes it possible to use QmpInputVisitor with a QDict > >> produced from QemuOpts, where everything is in string format. > >> It also makes it possible for the next patch to support > >> back-compat in legacy commands like 'netdev_add' that no > >> longer use QemuOpts, but must parse the same set of QMP > >> constructs that QemuOpts would historically allow. > > > > I'm not really a huge fan of the approach there. IMHO the > > input visitor should strictly operate in "all native types" > > or "all string types" mode. The way you've done it here > > means that users can actually mix & match strings vs native > > types for each individual parameter :-( > > > > IMHO, I'd suggest you try to parse netdev_add args with > > it in "all native types" mode, if that fails, then re-parse > > in "all string types" mode. > > Sadly, this idea won't work. Without this series, the existing QMP > command 'netdev_add' takes mixed integers and strings in a single call, > by virtue of converting QObject into QemuOpts and back into QObject. Ok, so lemme check I understand. The original QObject has properties which are integers, but with existing code QMP allows them to be passed as strings or ints, converts them to QemuOpts which makes them all strings and converts them back to QObject leaving them all as strings. You've now got rid of the QObject -> QemuOpts conversion, so we're not string-ifying everything, and so have to cope with arbitrary mix directly. > Forcing the parser to allow only integers or only strings would reject > current uses of netdev_add, and we don't yet have a good idea whether > any existing clients were depending on that behavior. > > Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse > is by setting a single flag which controls which visitor constructor is > used. Your idea of parsing once with integer-only, then falling back to > parse a second time with string-only, would complicate the generator to > have to create two different visitors, rather than passing a single > boolean parameter through to the single visitor constructor. So I don't think a simple boolean flag is desirable, as we have 3 distinct scenarios: 1. Strongly typed only 2. String conversion only 3. Strongly typed, with string conversion fallback My current patch provides 1 + 2, your alternative patch provides 1 + 3. If we use option 3, in cases which only actually need option 2, then we are potentially introducing more scenarios where arbitrarily mixed types are accepted. IOW, I think we must implement 1, 2 and 3 and avoid using 3 except where absolutely needed. Regards, Daniel
On 07/14/2016 07:03 AM, Daniel P. Berrange wrote: >>> I'm not really a huge fan of the approach there. IMHO the >>> input visitor should strictly operate in "all native types" >>> or "all string types" mode. The way you've done it here >>> means that users can actually mix & match strings vs native >>> types for each individual parameter :-( >>> >>> IMHO, I'd suggest you try to parse netdev_add args with >>> it in "all native types" mode, if that fails, then re-parse >>> in "all string types" mode. >> >> Sadly, this idea won't work. Without this series, the existing QMP >> command 'netdev_add' takes mixed integers and strings in a single call, >> by virtue of converting QObject into QemuOpts and back into QObject. > > Ok, so lemme check I understand. The original QObject has properties > which are integers, but with existing code QMP allows them to be passed > as strings or ints, converts them to QemuOpts which makes them all > strings and converts them back to QObject leaving them all as strings. Yes, the existing code (using 'gen':false) just passes an entire QObject to hand-written code; the hand-written code converted everything to QemuOpts (which flattens both integers and strings into options), then expanded QemuOpts back into QObject (strings-only). > > You've now got rid of the QObject -> QemuOpts conversion, so we're > not string-ifying everything, and so have to cope with arbitrary > mix directly. Yes. > >> Forcing the parser to allow only integers or only strings would reject >> current uses of netdev_add, and we don't yet have a good idea whether >> any existing clients were depending on that behavior. >> >> Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse >> is by setting a single flag which controls which visitor constructor is >> used. Your idea of parsing once with integer-only, then falling back to >> parse a second time with string-only, would complicate the generator to >> have to create two different visitors, rather than passing a single >> boolean parameter through to the single visitor constructor. > > So I don't think a simple boolean flag is desirable, as we have 3 distinct > scenarios: > > 1. Strongly typed only > 2. String conversion only > 3. Strongly typed, with string conversion fallback > > My current patch provides 1 + 2, your alternative patch provides 1 + 3. > If we use option 3, in cases which only actually need option 2, then we > are potentially introducing more scenarios where arbitrarily mixed > types are accepted. IOW, I think we must implement 1, 2 and 3 and avoid > using 3 except where absolutely needed. 3 is a superset of 2, and your command-line conversion is the only case where we can achieve 2. That is, code dealing with QMP can only choose between 1 and 3, based on whether the QAPI .json file used 'autocast':true for back-compat reasons (the only candidates are 'netdev_add' and 'device_add'). And code dealing with command line parsing can only choose 2 (QemuOpts is string-only), but parsing string-only via 2 is no different than the result achieved from parsing strongly-typed with string fallback via 3. I still don't buy the fact that we need a string-only parser at the moment, but it would not be hard to change the 'bool autocast' into a tri-state enum, and then make the implementation specifically honor 1, 2, or 3 based on the enum value.
On Thu, Jul 14, 2016 at 08:04:29AM -0600, Eric Blake wrote: > On 07/14/2016 07:03 AM, Daniel P. Berrange wrote: > > >>> I'm not really a huge fan of the approach there. IMHO the > >>> input visitor should strictly operate in "all native types" > >>> or "all string types" mode. The way you've done it here > >>> means that users can actually mix & match strings vs native > >>> types for each individual parameter :-( > >>> > >>> IMHO, I'd suggest you try to parse netdev_add args with > >>> it in "all native types" mode, if that fails, then re-parse > >>> in "all string types" mode. > >> > >> Sadly, this idea won't work. Without this series, the existing QMP > >> command 'netdev_add' takes mixed integers and strings in a single call, > >> by virtue of converting QObject into QemuOpts and back into QObject. > > > > Ok, so lemme check I understand. The original QObject has properties > > which are integers, but with existing code QMP allows them to be passed > > as strings or ints, converts them to QemuOpts which makes them all > > strings and converts them back to QObject leaving them all as strings. > > Yes, the existing code (using 'gen':false) just passes an entire QObject > to hand-written code; the hand-written code converted everything to > QemuOpts (which flattens both integers and strings into options), then > expanded QemuOpts back into QObject (strings-only). > > > > > You've now got rid of the QObject -> QemuOpts conversion, so we're > > not string-ifying everything, and so have to cope with arbitrary > > mix directly. > > Yes. > > > > >> Forcing the parser to allow only integers or only strings would reject > >> current uses of netdev_add, and we don't yet have a good idea whether > >> any existing clients were depending on that behavior. > >> > >> Also, see patch 17/17 - the easiest way to implement a relaxed QMP parse > >> is by setting a single flag which controls which visitor constructor is > >> used. Your idea of parsing once with integer-only, then falling back to > >> parse a second time with string-only, would complicate the generator to > >> have to create two different visitors, rather than passing a single > >> boolean parameter through to the single visitor constructor. > > > > So I don't think a simple boolean flag is desirable, as we have 3 distinct > > scenarios: > > > > 1. Strongly typed only > > 2. String conversion only > > 3. Strongly typed, with string conversion fallback > > > > My current patch provides 1 + 2, your alternative patch provides 1 + 3. > > If we use option 3, in cases which only actually need option 2, then we > > are potentially introducing more scenarios where arbitrarily mixed > > types are accepted. IOW, I think we must implement 1, 2 and 3 and avoid > > using 3 except where absolutely needed. > > 3 is a superset of 2, and your command-line conversion is the only case > where we can achieve 2. That is, code dealing with QMP can only choose > between 1 and 3, based on whether the QAPI .json file used > 'autocast':true for back-compat reasons (the only candidates are > 'netdev_add' and 'device_add'). And code dealing with command line > parsing can only choose 2 (QemuOpts is string-only), but parsing > string-only via 2 is no different than the result achieved from parsing > strongly-typed with string fallback via 3. > > I still don't buy the fact that we need a string-only parser at the > moment, but it would not be hard to change the 'bool autocast' into a > tri-state enum, and then make the implementation specifically honor 1, > 2, or 3 based on the enum value. FYI, I just copied you on a patch that enables us to easily support all 3 options. Regards, Daniel
On 07/14/2016 08:16 AM, Daniel P. Berrange wrote: > On Thu, Jul 14, 2016 at 08:04:29AM -0600, Eric Blake wrote: >> On 07/14/2016 07:03 AM, Daniel P. Berrange wrote: >> >> 3 is a superset of 2, and your command-line conversion is the only case >> where we can achieve 2. That is, code dealing with QMP can only choose >> between 1 and 3, based on whether the QAPI .json file used >> 'autocast':true for back-compat reasons (the only candidates are >> 'netdev_add' and 'device_add'). And code dealing with command line >> parsing can only choose 2 (QemuOpts is string-only), but parsing >> string-only via 2 is no different than the result achieved from parsing >> strongly-typed with string fallback via 3. >> >> I still don't buy the fact that we need a string-only parser at the >> moment, but it would not be hard to change the 'bool autocast' into a >> tri-state enum, and then make the implementation specifically honor 1, >> 2, or 3 based on the enum value. > > FYI, I just copied you on a patch that enables us to easily support > all 3 options. Yours achieves that by having three separate constructors, instead of one constructor with a tri-state enum parameter. Either approach works; Markus, do you have any opinions on which looks nicer?
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index a06a2c4..f526c13 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -117,7 +117,7 @@ def gen_marshal(name, arg_type, boxed, ret_type): Visitor *v; %(c_name)s arg = {0}; - v = qmp_input_visitor_new(QOBJECT(args), true); + v = qmp_input_visitor_new(QOBJECT(args), true, false); visit_start_struct(v, NULL, NULL, 0, &err); if (err) { goto out; diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h index f3ff5f3..275a167 100644 --- a/include/qapi/qmp-input-visitor.h +++ b/include/qapi/qmp-input-visitor.h @@ -19,12 +19,26 @@ typedef struct QmpInputVisitor QmpInputVisitor; -/* - * Return a new input visitor that converts QMP to QAPI. +/** + * qmp_input_visitor_new: + * @obj: the input object to visit + * @strict: whether to require that all input keys are consumed + * @autocast: whether to allow strings in place of other scalars * - * Set @strict to reject a parse that doesn't consume all keys of a - * dictionary; otherwise excess input is ignored. + * Create a new input visitor that converts QMP to QAPI. + * + * If @strict is set to true, then an error will be reported if any + * dict keys are not consumed during visitation. + * + * When @autocast is false, any scalar values in the @obj input data + * structure should be in the required type already. ie if visiting a + * bool, the value should already be a QBool instance. Otherwise, a + * string value that can be parsed as the scalar is acceptable; + * however, it is not possible to parse QAPI alternates in autocast + * mode. + * + * Returns: a new input visitor */ -Visitor *qmp_input_visitor_new(QObject *obj, bool strict); +Visitor *qmp_input_visitor_new(QObject *obj, bool strict, bool autocast); #endif diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 64dd392..7a1b93a 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-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 @@ -47,6 +48,9 @@ struct QmpInputVisitor /* True to reject parse in visit_end_struct() if unvisited keys remain. */ bool strict; + + /* True to allow strings in place of native scalars. */ + bool autocast; }; static QmpInputVisitor *to_qiv(Visitor *v) @@ -234,6 +238,8 @@ static void qmp_input_start_alternate(Visitor *v, const char *name, QmpInputVisitor *qiv = to_qiv(v); QObject *qobj = qmp_input_get_object(qiv, name, false); + assert(!qiv->autocast); + if (!qobj) { *obj = NULL; error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); @@ -250,11 +256,21 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true); + QInt *qint = qobject_to_qint(qobj); if (!qint) { - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", - "integer"); + QString *qstr = qobject_to_qstring(qobj); + + if (qiv->autocast && qstr) { + uint64_t ret = *obj; + + parse_option_number(name, qstr->string, &ret, errp); + *obj = ret; + } else { + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "integer"); + } return; } @@ -266,11 +282,18 @@ static void qmp_input_type_uint64(Visitor *v, const char *name, uint64_t *obj, { /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ QmpInputVisitor *qiv = to_qiv(v); - QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true); + QInt *qint = qobject_to_qint(qobj); if (!qint) { - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", - "integer"); + QString *qstr = qobject_to_qstring(qobj); + + if (qiv->autocast && qstr) { + parse_option_number(name, qstr->string, obj, errp); + } else { + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "integer"); + } return; } @@ -281,11 +304,18 @@ static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); - QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true)); + QObject *qobj = qmp_input_get_object(qiv, name, true); + QBool *qbool = qobject_to_qbool(qobj); if (!qbool) { - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", - "boolean"); + QString *qstr = qobject_to_qstring(qobj); + + if (qiv->autocast && qstr) { + parse_option_bool(name, qstr->string, obj, errp); + } else { + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "boolean"); + } return; } @@ -315,6 +345,7 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj, QObject *qobj = qmp_input_get_object(qiv, name, true); QInt *qint; QFloat *qfloat; + QString *qstr; qint = qobject_to_qint(qobj); if (qint) { @@ -328,6 +359,19 @@ static void qmp_input_type_number(Visitor *v, const char *name, double *obj, return; } + qstr = qobject_to_qstring(qobj); + if (qiv->autocast && qstr) { + char *endp; + double value; + + errno = 0; + value = strtod(qstr->string, &endp); + if (errno == 0 && endp != qstr->string && *endp == '\0') { + *obj = value; + return; + } + } + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "number"); } @@ -353,6 +397,40 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp) } } +static void qmp_input_type_size(Visitor *v, const char *name, uint64_t *obj, + Error **errp) +{ + /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ + QmpInputVisitor *qiv = to_qiv(v); + QObject *qobj = qmp_input_get_object(qiv, name, true); + QInt *qint = qobject_to_qint(qobj); + + if (!qint) { + QString *qstr = qobject_to_qstring(qobj); + + if (qiv->autocast && qstr) { + int64_t val; + char *endptr; + + val = qemu_strtosz_suffix(qstr->string, &endptr, + QEMU_STRTOSZ_DEFSUFFIX_B); + if (val < 0 || *endptr) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, + "a size value representible as a non-negative" + " int64"); + } else { + *obj = val; + } + } else { + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", + "integer"); + } + return; + } + + *obj = qint_get_int(qint); +} + static void qmp_input_optional(Visitor *v, const char *name, bool *present) { QmpInputVisitor *qiv = to_qiv(v); @@ -380,7 +458,7 @@ static void qmp_input_free(Visitor *v) g_free(qiv); } -Visitor *qmp_input_visitor_new(QObject *obj, bool strict) +Visitor *qmp_input_visitor_new(QObject *obj, bool strict, bool autocast) { QmpInputVisitor *v; @@ -401,9 +479,11 @@ Visitor *qmp_input_visitor_new(QObject *obj, bool strict) v->visitor.type_number = qmp_input_type_number; v->visitor.type_any = qmp_input_type_any; v->visitor.type_null = qmp_input_type_null; + v->visitor.type_size = qmp_input_type_size; v->visitor.optional = qmp_input_optional; v->visitor.free = qmp_input_free; v->strict = strict; + v->autocast = autocast; v->root = obj; qobject_incref(obj); diff --git a/qmp.c b/qmp.c index b6d531e..ff93890 100644 --- a/qmp.c +++ b/qmp.c @@ -666,7 +666,7 @@ void qmp_object_add(const char *type, const char *id, } } - v = qmp_input_visitor_new(props, true); + v = qmp_input_visitor_new(props, true, false); obj = user_creatable_add_type(type, id, pdict, v, errp); visit_free(v); if (obj) { diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c index c225abc..974c7ea 100644 --- a/qom/qom-qobject.c +++ b/qom/qom-qobject.c @@ -23,7 +23,7 @@ void object_property_set_qobject(Object *obj, QObject *value, { Visitor *v; /* TODO: Should we reject, rather than ignore, excess input? */ - v = qmp_input_visitor_new(value, false); + v = qmp_input_visitor_new(value, false, false); object_property_set(obj, v, name, errp); visit_free(v); } diff --git a/tests/check-qnull.c b/tests/check-qnull.c index dc906b1..483ed6d 100644 --- a/tests/check-qnull.c +++ b/tests/check-qnull.c @@ -47,7 +47,7 @@ static void qnull_visit_test(void) g_assert(qnull_.refcnt == 1); obj = qnull(); - v = qmp_input_visitor_new(obj, true); + v = qmp_input_visitor_new(obj, true, false); qobject_decref(obj); visit_type_null(v, NULL, &error_abort); visit_free(v); diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index 5af1a46..974841c 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -229,7 +229,7 @@ static void test_dealloc_partial(void) ud2_dict = qdict_new(); qdict_put_obj(ud2_dict, "string0", QOBJECT(qstring_from_str(text))); - v = qmp_input_visitor_new(QOBJECT(ud2_dict), true); + v = qmp_input_visitor_new(QOBJECT(ud2_dict), true, false); visit_type_UserDefTwo(v, NULL, &ud2, &err); visit_free(v); QDECREF(ud2_dict); diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index 814550a..93ee137 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -53,7 +53,7 @@ static Visitor *validate_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qmp_input_visitor_new(data->obj, true); + data->qiv = qmp_input_visitor_new(data->obj, true, false); g_assert(data->qiv); return data->qiv; } diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index f583dce..29c4562 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -41,6 +41,7 @@ static void visitor_input_teardown(TestInputVisitorData *data, function so that the JSON string used by the tests are kept in the test functions (and not in main()). */ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, + bool strict, bool autocast, const char *json_string, va_list *ap) { @@ -49,11 +50,26 @@ static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data, data->obj = qobject_from_jsonv(json_string, ap); g_assert(data->obj); - data->qiv = qmp_input_visitor_new(data->obj, false); + data->qiv = qmp_input_visitor_new(data->obj, strict, autocast); g_assert(data->qiv); return data->qiv; } +static GCC_FMT_ATTR(4, 5) +Visitor *visitor_input_test_init_full(TestInputVisitorData *data, + bool strict, bool autocast, + const char *json_string, ...) +{ + Visitor *v; + va_list ap; + + va_start(ap, json_string); + v = visitor_input_test_init_internal(data, strict, autocast, + json_string, &ap); + va_end(ap); + return v; +} + static GCC_FMT_ATTR(2, 3) Visitor *visitor_input_test_init(TestInputVisitorData *data, const char *json_string, ...) @@ -62,7 +78,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, va_list ap; va_start(ap, json_string); - v = visitor_input_test_init_internal(data, json_string, &ap); + v = visitor_input_test_init_internal(data, false, false, + json_string, &ap); va_end(ap); return v; } @@ -77,7 +94,8 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data, static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data, const char *json_string) { - return visitor_input_test_init_internal(data, json_string, NULL); + return visitor_input_test_init_internal(data, false, false, + json_string, NULL); } static void test_visitor_in_int(TestInputVisitorData *data, @@ -109,6 +127,33 @@ static void test_visitor_in_int_overflow(TestInputVisitorData *data, error_free_or_abort(&err); } +static void test_visitor_in_int_autocast(TestInputVisitorData *data, + const void *unused) +{ + int64_t res = 0, value = -42; + Visitor *v; + + v = visitor_input_test_init_full(data, false, true, + "\"-42\""); + + visit_type_int(v, NULL, &res, &error_abort); + g_assert_cmpint(res, ==, value); +} + +static void test_visitor_in_int_noautocast(TestInputVisitorData *data, + const void *unused) +{ + int64_t res = 0; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"-42\""); + + visit_type_int(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + static void test_visitor_in_bool(TestInputVisitorData *data, const void *unused) { @@ -121,6 +166,32 @@ static void test_visitor_in_bool(TestInputVisitorData *data, g_assert_cmpint(res, ==, true); } +static void test_visitor_in_bool_autocast(TestInputVisitorData *data, + const void *unused) +{ + bool res = false; + Visitor *v; + + v = visitor_input_test_init_full(data, false, true, "\"yes\""); + + visit_type_bool(v, NULL, &res, &error_abort); + g_assert_cmpint(res, ==, true); +} + +static void test_visitor_in_bool_noautocast(TestInputVisitorData *data, + const void *unused) +{ + bool res = false; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"true\""); + + visit_type_bool(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + static void test_visitor_in_number(TestInputVisitorData *data, const void *unused) { @@ -133,6 +204,63 @@ static void test_visitor_in_number(TestInputVisitorData *data, g_assert_cmpfloat(res, ==, value); } +static void test_visitor_in_number_autocast(TestInputVisitorData *data, + const void *unused) +{ + double res = 0, value = 3.14; + Visitor *v; + + v = visitor_input_test_init_full(data, false, true, "\"3.14\""); + + visit_type_number(v, NULL, &res, &error_abort); + g_assert_cmpfloat(res, ==, value); +} + +static void test_visitor_in_number_noautocast(TestInputVisitorData *data, + const void *unused) +{ + double res = 0; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"3.14\""); + + visit_type_number(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + +static void test_visitor_in_size_autocast(TestInputVisitorData *data, + const void *unused) +{ + uint64_t res, value = 500 * 1024 * 1024; + Visitor *v; + + v = visitor_input_test_init_full(data, false, true, "\"500M\""); + + visit_type_size(v, NULL, &res, &error_abort); + g_assert_cmpfloat(res, ==, value); + + v = visitor_input_test_init_full(data, false, true, "524288000"); + + visit_type_size(v, NULL, &res, &error_abort); + g_assert_cmpfloat(res, ==, value); +} + +static void test_visitor_in_size_noautocast(TestInputVisitorData *data, + const void *unused) +{ + uint64_t res = 0; + Visitor *v; + Error *err = NULL; + + v = visitor_input_test_init(data, "\"500M\""); + + visit_type_size(v, NULL, &res, &err); + g_assert(err != NULL); + error_free(err); +} + static void test_visitor_in_string(TestInputVisitorData *data, const void *unused) { @@ -841,10 +969,26 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_int); input_visitor_test_add("/visitor/input/int_overflow", &in_visitor_data, test_visitor_in_int_overflow); + input_visitor_test_add("/visitor/input/int_autocast", + &in_visitor_data, test_visitor_in_int_autocast); + input_visitor_test_add("/visitor/input/int_noautocast", + &in_visitor_data, test_visitor_in_int_noautocast); input_visitor_test_add("/visitor/input/bool", &in_visitor_data, test_visitor_in_bool); + input_visitor_test_add("/visitor/input/bool_autocast", + &in_visitor_data, test_visitor_in_bool_autocast); + input_visitor_test_add("/visitor/input/bool_noautocast", + &in_visitor_data, test_visitor_in_bool_noautocast); input_visitor_test_add("/visitor/input/number", &in_visitor_data, test_visitor_in_number); + input_visitor_test_add("/visitor/input/number_autocast", + &in_visitor_data, test_visitor_in_number_autocast); + input_visitor_test_add("/visitor/input/number_noautocast", + &in_visitor_data, test_visitor_in_number_noautocast); + input_visitor_test_add("/visitor/input/size_autocast", + &in_visitor_data, test_visitor_in_size_autocast); + input_visitor_test_add("/visitor/input/size_noautocast", + &in_visitor_data, test_visitor_in_size_noautocast); input_visitor_test_add("/visitor/input/string", &in_visitor_data, test_visitor_in_string); input_visitor_test_add("/visitor/input/enum", diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index dba4670..ddd5837 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -1040,7 +1040,7 @@ static void qmp_deserialize(void **native_out, void *datap, obj = qobject_from_json(qstring_get_str(output_json)); QDECREF(output_json); - d->qiv = qmp_input_visitor_new(obj, true); + d->qiv = qmp_input_visitor_new(obj, true, false); qobject_decref(obj_orig); qobject_decref(obj); visit(d->qiv, native_out, errp); diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index de298dc..961015c 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -1024,7 +1024,7 @@ Example: Visitor *v; UserDefOneList *arg1 = NULL; - v = qmp_input_visitor_new(QOBJECT(args), true); + v = qmp_input_visitor_new(QOBJECT(args), true, false); visit_start_struct(v, NULL, NULL, 0, &err); if (err) { goto out;
Currently the QmpInputVisitor assumes that all scalar values are directly represented as their final types. ie it assumes an 'int' is using QInt, and a 'bool' is using QBool. This adds an alternative mode where a QString can also be parsed in place of the native type, by adding a parameter and updating all callers. For now, only the testsuite sets the new autocast parameter. This makes it possible to use QmpInputVisitor with a QDict produced from QemuOpts, where everything is in string format. It also makes it possible for the next patch to support back-compat in legacy commands like 'netdev_add' that no longer use QemuOpts, but must parse the same set of QMP constructs that QemuOpts would historically allow. Based heavily on a patch by Daniel P. Berrange <berrange@redhat.com>: Message-Id: <1467724312-9378-4-git-send-email-berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- v9: new patch (but heavily draws from 3/7 in Dan's v7 series, Provide a QOM-based authorization API) --- scripts/qapi-commands.py | 2 +- include/qapi/qmp-input-visitor.h | 24 ++++-- qapi/qmp-input-visitor.c | 100 ++++++++++++++++++++++--- qmp.c | 2 +- qom/qom-qobject.c | 2 +- tests/check-qnull.c | 2 +- tests/test-qmp-commands.c | 2 +- tests/test-qmp-input-strict.c | 2 +- tests/test-qmp-input-visitor.c | 150 ++++++++++++++++++++++++++++++++++++- tests/test-visitor-serialization.c | 2 +- docs/qapi-code-gen.txt | 2 +- 11 files changed, 264 insertions(+), 26 deletions(-)