"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> > --- > 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 returned input visitor should be released by calling > + * visit_free() when no longer required. > */ > Visitor *qobject_input_visitor_new(QObject *obj, bool strict); > > +/** > + * Create a new input visitor that converts @obj to a QAPI object. > + * > + * Any scalar values in the @obj input data structure should always be > + * represented as strings. i.e. if visiting a boolean, the value should > + * be a QString whose contents represent a valid boolean. > + * > + * The visitor always operates in strict mode, requiring all dict keys > + * to be consumed during visitation. An error will be reported if this > + * does not happen. > + * > + * The returned input visitor should be released by calling > + * visit_free() when no longer required. > + */ > +Visitor *qobject_input_visitor_new_autocast(QObject *obj); Not thrilled about _autocast. It kind of suggests that scalar values are converted as necessary. They are, but the full truth is that all scalar values must be strings. _string_scalars is even longer, though. > + > #endif > 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"); > + 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"); > + 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) > { > @@ -294,6 +338,22 @@ static void qobject_input_type_bool(Visitor *v, const > char *name, bool *obj, > *obj = qbool_get_bool(qbool); > } > > +static void qobject_input_type_bool_autocast(Visitor *v, const char *name, > + bool *obj, Error **errp) > +{ > + QObjectInputVisitor *qiv = to_qiv(v); > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > + true)); > + > + if (!qstr || !qstr->string) { > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "string"); > + return; > + } > + > + parse_option_bool(name, qstr->string, obj, errp); To serve as replacement for the options visitor, this needs to parse exactly like opts_type_bool(). Which parses with parse_option_bool(), too. Good. QemuOpts parses the same way. JSON syntax is different. > +} > + > static void qobject_input_type_str(Visitor *v, const char *name, char **obj, > Error **errp) > { > @@ -335,6 +395,30 @@ static void qobject_input_type_number(Visitor *v, const > char *name, double *obj, > "number"); > } > > +static void qobject_input_type_number_autocast(Visitor *v, const char *name, > + double *obj, Error **errp) > +{ > + QObjectInputVisitor *qiv = to_qiv(v); > + QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name, > + true)); > + char *endp; > + > + if (!qstr || !qstr->string) { > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "string"); > + return; > + } > + > + errno = 0; > + *obj = strtod(qstr->string, &endp); > + if (errno == 0 && endp != qstr->string && *endp == '\0') { > + return; > + } > + > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "number"); > +} > + The options visitor does not implement this method. Your error checking looks good. The JSON parser also uses strtod(), in parse_literal(). When we get there, we know that the string is a valid JSON number. Therefore, strtod() can only fail with ERANGE. Bug (not yours): we ignore that. QemuOpts doesn't do floating-point. > static void qobject_input_type_any(Visitor *v, const char *name, QObject > **obj, > Error **errp) > { > @@ -356,6 +440,22 @@ static void qobject_input_type_null(Visitor *v, const > char *name, Error **errp) > } > } > > +static void qobject_input_type_size_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)); > + > + if (!qstr || !qstr->string) { > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "string"); > + return; > + } > + > + parse_option_size(name, qstr->string, obj, errp); The options visitor uses qemu_strtosz_suffix(). Are you sure parse_option_size() is a compatible replacement? (I'm getting tired...) > +} > + > static void qobject_input_optional(Visitor *v, const char *name, bool > *present) > { > QObjectInputVisitor *qiv = to_qiv(v); > @@ -413,3 +513,35 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool > strict) > > return &v->visitor; > } > + > +Visitor *qobject_input_visitor_new_autocast(QObject *obj) > +{ > + QObjectInputVisitor *v; > + > + v = g_malloc0(sizeof(*v)); > + > + v->visitor.type = VISITOR_INPUT; > + v->visitor.start_struct = qobject_input_start_struct; > + v->visitor.check_struct = qobject_input_check_struct; > + v->visitor.end_struct = qobject_input_pop; > + v->visitor.start_list = qobject_input_start_list; > + v->visitor.next_list = qobject_input_next_list; > + v->visitor.end_list = qobject_input_pop; > + v->visitor.start_alternate = qobject_input_start_alternate; > + v->visitor.type_int64 = qobject_input_type_int64_autocast; > + v->visitor.type_uint64 = qobject_input_type_uint64_autocast; > + v->visitor.type_bool = qobject_input_type_bool_autocast; > + v->visitor.type_str = qobject_input_type_str; > + v->visitor.type_number = qobject_input_type_number_autocast; > + v->visitor.type_any = qobject_input_type_any; > + v->visitor.type_null = qobject_input_type_null; > + v->visitor.type_size = qobject_input_type_size_autocast; > + v->visitor.optional = qobject_input_optional; > + v->visitor.free = qobject_input_free; > + v->strict = true; > + > + v->root = obj; > + qobject_incref(obj); > + > + return &v->visitor; > +} > diff --git a/tests/test-qobject-input-visitor.c > b/tests/test-qobject-input-visitor.c > index 26c5012..d5b8044 100644 > --- a/tests/test-qobject-input-visitor.c > +++ b/tests/test-qobject-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,31 @@ static Visitor > *visitor_input_test_init_internal(TestInputVisitorData *data, > data->obj = qobject_from_jsonv(json_string, ap); > g_assert(data->obj); > > - data->qiv = qobject_input_visitor_new(data->obj, false); > + if (autocast) { > + assert(strict); > + data->qiv = qobject_input_visitor_new_autocast(data->obj); > + } else { > + data->qiv = qobject_input_visitor_new(data->obj, strict); > + } > 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 +83,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, true, false, > + json_string, &ap); > va_end(ap); > return v; > } > @@ -77,7 +99,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, true, false, > + json_string, NULL); > } > > static void test_visitor_in_int(TestInputVisitorData *data, > @@ -109,6 +132,45 @@ 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; > + Error *err = NULL; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, > + "%" PRId64, value); > + visit_type_int(v, NULL, &res, &err); > + error_free_or_abort(&err); > +} > + > +static void test_visitor_in_int_str_autocast(TestInputVisitorData *data, > + const void *unused) > +{ > + int64_t res = 0, value = -42; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, > + "\"-42\""); > + > + visit_type_int(v, NULL, &res, &error_abort); > + g_assert_cmpint(res, ==, value); > +} > + > +static void test_visitor_in_int_str_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); > + error_free_or_abort(&err); > +} > + > static void test_visitor_in_bool(TestInputVisitorData *data, > const void *unused) > { > @@ -121,6 +183,44 @@ 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; > + Error *err = NULL; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, "true"); > + > + visit_type_bool(v, NULL, &res, &err); > + error_free_or_abort(&err); > +} > + > +static void test_visitor_in_bool_str_autocast(TestInputVisitorData *data, > + const void *unused) > +{ > + bool res = false; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, "\"yes\""); > + > + visit_type_bool(v, NULL, &res, &error_abort); > + g_assert_cmpint(res, ==, true); > +} > + > +static void test_visitor_in_bool_str_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); > + error_free_or_abort(&err); > +} > + > static void test_visitor_in_number(TestInputVisitorData *data, > const void *unused) > { > @@ -133,6 +233,69 @@ 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; > + Error *err = NULL; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, "%f", value); > + > + visit_type_number(v, NULL, &res, &err); > + error_free_or_abort(&err); > +} > + > +static void test_visitor_in_number_str_autocast(TestInputVisitorData *data, > + const void *unused) > +{ > + double res = 0, value = 3.14; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, "\"3.14\""); > + > + visit_type_number(v, NULL, &res, &error_abort); > + g_assert_cmpfloat(res, ==, value); > +} > + > +static void test_visitor_in_number_str_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); > + error_free_or_abort(&err); > +} > + > +static void test_visitor_in_size_str_autocast(TestInputVisitorData *data, > + const void *unused) > +{ > + uint64_t res, value = 500 * 1024 * 1024; > + Visitor *v; > + > + v = visitor_input_test_init_full(data, true, true, "\"500M\""); > + > + visit_type_size(v, NULL, &res, &error_abort); > + g_assert_cmpfloat(res, ==, value); > +} > + > +static void test_visitor_in_size_str_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); > + error_free_or_abort(&err); > +} > + > static void test_visitor_in_string(TestInputVisitorData *data, > const void *unused) > { > @@ -289,7 +452,8 @@ static void test_visitor_in_null(TestInputVisitorData > *data, > * when input is not null. > */ > > - v = visitor_input_test_init(data, "{ 'a': null, 'b': '' }"); > + v = visitor_input_test_init_full(data, false, false, > + "{ 'a': null, 'b': '' }"); > visit_start_struct(v, NULL, NULL, 0, &error_abort); > visit_type_null(v, "a", &error_abort); > visit_type_str(v, "a", &tmp, &err); > @@ -840,10 +1004,32 @@ int main(int argc, char **argv) > NULL, test_visitor_in_int); > input_visitor_test_add("/visitor/input/int_overflow", > NULL, test_visitor_in_int_overflow); > + input_visitor_test_add("/visitor/input/int_autocast", > + NULL, test_visitor_in_int_autocast); > + input_visitor_test_add("/visitor/input/int_str_autocast", > + NULL, test_visitor_in_int_str_autocast); > + input_visitor_test_add("/visitor/input/int_str_noautocast", > + NULL, test_visitor_in_int_str_noautocast); > input_visitor_test_add("/visitor/input/bool", > NULL, test_visitor_in_bool); > + input_visitor_test_add("/visitor/input/bool_autocast", > + NULL, test_visitor_in_bool_autocast); > + input_visitor_test_add("/visitor/input/bool_str_autocast", > + NULL, test_visitor_in_bool_str_autocast); > + input_visitor_test_add("/visitor/input/bool_str_noautocast", > + NULL, test_visitor_in_bool_str_noautocast); > input_visitor_test_add("/visitor/input/number", > NULL, test_visitor_in_number); > + input_visitor_test_add("/visitor/input/number_autocast", > + NULL, test_visitor_in_number_autocast); > + input_visitor_test_add("/visitor/input/number_str_autocast", > + NULL, test_visitor_in_number_str_autocast); > + input_visitor_test_add("/visitor/input/number_str_noautocast", > + NULL, test_visitor_in_number_str_noautocast); > + input_visitor_test_add("/visitor/input/size_str_autocast", > + NULL, test_visitor_in_size_str_autocast); > + input_visitor_test_add("/visitor/input/size_str_noautocast", > + NULL, test_visitor_in_size_str_noautocast); > input_visitor_test_add("/visitor/input/string", > NULL, test_visitor_in_string); > input_visitor_test_add("/visitor/input/enum", I don't like the test code duplication. The alternative would be a function that rewrites all the scalars in a QObject to strings. Probably not worthwhile. We'll live with the duplication then.