On Mon, Sep 12, 2016 at 01:39:50PM -0500, Eric Blake wrote:
> 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 <marcandre.lur...@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> > ---
> > 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.
I'll use a local Error, so we can avoid the uninitialized
value and also avoid splattering *obj on failure.
> > +++ 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)?
[snip]
> Similar question for autocast causing QBool->bool, QInt->int under size,
> and QFloat->number failures.
You always notice all the edge cases :-)
I've added such tests and it exposed a bug in my impl which is nice :-)
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
