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

[Eric Blake]

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.

> @@ -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

signature.asc
Description: OpenPGP digital signature