Re: [Qemu-devel] [PATCH v11 5/6] qapi: add a QmpInputVisitor that does string conversion

[Daniel P. Berrange]

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 <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  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 :|