Re: [Qemu-block] [PATCH v14 10/21] qapi: permit auto-creating nested structs

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> Some of the historical command line opts that had their
> keys in in a completely flat namespace are now represented
> by QAPI schemas that use a nested structs. When converting

singular/plural mismatch; either s/a // or s/structs/struct/

> the QemuOpts to QObject, there is no information about
> compound types available, so the QObject will be completely
> flat, even after the qdict_crumple() call. So when starting
> a struct, we may not have a QDict available in the input
> data, so we auto-create a QDict containing all the currently
> unvisited input data keys. Not all historical command line
> opts require this, so the behaviour is opt-in, by specifying
> how many levels of structs are permitted to be auto-created.
> 
> Note that this only works if the child struct is the last
> field to the visited in the parent struct. This is always
> the case for currently existing legacy command line options.
> 
> The example is the NetLegacy type which has 3 levels of
> structs. The modern way to represent this in QemuOpts would
> be the dot-separated component approach
> 
>   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>        opts.data.fd=3,opts.data.script=ifup
> 
> The legacy syntax will just be presenting
> 
>   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup

So basically, any key=values that were not consumed as known keys in the
parent struct are reparsed as the (presumably lone) remaining QDict
child, recursing as needed to follow the QAPI nesting.

Does this still work if the command line is presented in a different order:

-net script=ifup,vlan=1,type=tap,fd=3,id=foo,name=bar

My guess is yes, but if the testsuite doesn't cover it, you may want to
add a test.

> 
> So we need to auto-create 3 levels of struct when visiting.
> 
> The implementation here will enable visiting in both the
> modern and legacy syntax, compared to OptsVisitor which
> only allows the legacy syntax.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  include/qapi/qobject-input-visitor.h |  22 +++++-
>  qapi/qobject-input-visitor.c         |  59 ++++++++++++++--
>  tests/test-qobject-input-visitor.c   | 130 
> ++++++++++++++++++++++++++++++++---
>  3 files changed, 194 insertions(+), 17 deletions(-)
> 
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 1809f48..94051f3 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -45,7 +45,7 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * If @autocreate_list is true, then as an alternative to a normal QList,
>   * list values can be stored as a QString or QDict instead, which will
>   * be interpreted as representing single element lists. This should only
> - * by used if compatibility is required with the OptsVisitor which allowed
> + * be used if compatibility is required with the OptsVisitor which allowed

This typo fix should be hoisted earlier in the series when it was
introduced.

>   * repeated keys, without list indexes, to represent lists. e.g. set this
>   * to true if you have compatibility requirements for
>   *
> @@ -55,6 +55,22 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   *
>   *   -arg foo.0=hello,foo.1=world
>   *
> + * If @autocreate_struct_levels is non-zero, then when visiting structs,
> + * if the corresponding QDict is not found, it will automatically create
> + * a QDict containing all remaining unvisited options. This should only
> + * be used if compatibility is required with the OptsVisitor which flatten
> + * structs so that all keys were at the same level. e.g. set this to a
> + * non-zero number if you compatibility requirements for
> + *
> + *   -arg type=person,surname=blogs,forename=fred
> + *
> + * to be treated as equivalent to the perferred syntax

s/perferred/preferred/


> +static void test_visitor_in_struct_autocreate(TestInputVisitorData *data,
> +                                              const void *unused)
> +{
> +    Visitor *v;
> +    int64_t vlan;
> +    char *id = NULL;
> +    char *type;
> +    int64_t fd;
> +    char *script = NULL;
> +
> +    v = visitor_input_test_init_internal(
> +        data, true, true, false, 3,
> +        "{ 'vlan': '1', 'id': 'foo', 'type': 'tap', 'fd': '3', "
> +        "'script': 'ifup' }", NULL);
> +

This is where intentionally mixing up the parameter orders might be
worthwhile.

Overall, the idea looks reasonable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature