On 09/05/2016 10:16 AM, Daniel P. Berrange 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 constructor for QmpInputVisitor > that will set it up such that it expects a QString for > all scalar types instead. > > This makes it possible to use QmpInputVisitor with a > QDict produced from QemuOpts, where everything is in > string format. > > Reviewed-by: Marc-André Lureau > Signed-off-by: Daniel P. Berrange > --- > include/qapi/qobject-input-visitor.h | 41 +++++++++- > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++- > tests/test-qobject-input-visitor.c | 152 ++++++++++++++++++++++++++++++++++- > 3 files changed, 298 insertions(+), 10 deletions(-) > > +/** > + * qobject_string_input_visitor_new: > + * @obj: the input object to visit > + * > + * Create a new input visitor that converts a QObject 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. Good; I like that strict mode on the new constructor is not optional. > +static void qobject_input_type_int64_str(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)); > + uint64_t ret; Uninitialized... > + > + parse_option_number(name, qstr ? qstr->string : NULL, &ret, errp); ...and parse_option_number() explicitly leaves *ret untouched on error... > + *obj = ret; so if errp was set, then *obj now contains uninitialized memory. I guess valgrind is smart enough to only complain if callers then try to branch based on that value (that is, assigning one uninit location to another silently propagates uninit status to the new location, but it is only when you branch or otherwise USE uninit data, not just copy it, that valgrind complains). On the other hand, if the caller explicitly set value = 0 before calling qobject_input_type_int64_str(&value), we've now messed with the caller's expectations of value being at its pre-set value on error. > @@ -312,8 +345,8 @@ static void qobject_input_type_str(Visitor *v, const char *name, char **obj, > } > > static void qobject_input_type_number(Visitor *v, const char *name, double *obj, > - Error **errp) > -{ > + Error **errp){ > + Spurious hunk. > +static void qobject_input_type_size_str(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)); > + int64_t val; > + char *endptr; > + > + if (qstr && qstr->string) { > + 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"); s/representible/representable/ > +++ b/tests/test-qobject-input-visitor.c > > +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, true, 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); > +} So we've previously tested that int->int works without autocast, and you add that str->int works with autocast, and that str->int fails without autocast. Is it also worth testing that int->int fails with autocast (that is, when doing string parsing, a QInt is intentionally rejected even though we are parsing to get an int result)? > @@ -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); Similar question for autocast causing QBool->bool, QInt->int under size, and QFloat->number failures. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org