Re: [Qemu-devel] [PATCH v3 2/8] qapi: complete implementation of unions

[Mark Wu <wudxw@linux.vnet.ibm.com>]

On 03/06/2012 01:33 AM, Paolo Bonzini wrote:
> Reviewed-by: Luiz Capitulino<lcapitulino@redhat.com>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   qapi-schema-test.json     |   10 ++++++++++
>   scripts/qapi-types.py     |    5 +++++
>   scripts/qapi-visit.py     |   31 ++++++++++++++++++++++++++++++-
>   test-qmp-input-visitor.c  |   18 ++++++++++++++++++
>   test-qmp-output-visitor.c |   34 ++++++++++++++++++++++++++++++++++
>   5 files changed, 97 insertions(+), 1 deletions(-)
>
> diff --git a/qapi-schema-test.json b/qapi-schema-test.json
> index 2b38919..8c7f9f7 100644
> --- a/qapi-schema-test.json
> +++ b/qapi-schema-test.json
> @@ -22,6 +22,16 @@
>                          'dict2': { 'userdef1': 'UserDefOne', 'string2': 'str' },
>                          '*dict3': { 'userdef2': 'UserDefOne', 'string3': 'str' } } } }
>
> +# for testing unions
> +{ 'type': 'UserDefA',
> +  'data': { 'boolean': 'bool' } }
> +
> +{ 'type': 'UserDefB',
> +  'data': { 'integer': 'int' } }
> +
> +{ 'union': 'UserDefUnion',
> +  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
> +
>   # testing commands
>   { 'command': 'user_def_cmd', 'data': {} }
>   { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index b56225b..6968e7f 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -269,6 +269,7 @@ for expr in exprs:
>       elif expr.has_key('union'):
>           ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>           ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
> +        fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
>       else:
>           continue
>       fdecl.write(ret)
> @@ -283,6 +284,10 @@ for expr in exprs:
>           fdef.write(generate_type_cleanup(expr['type']) + "\n")
>       elif expr.has_key('union'):
>           ret += generate_union(expr['union'], expr['data'])
> +        ret += generate_type_cleanup_decl(expr['union'] + "List")
> +        fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
> +        ret += generate_type_cleanup_decl(expr['union'])
> +        fdef.write(generate_type_cleanup(expr['union']) + "\n")
>       else:
>           continue
>       fdecl.write(ret)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 5160d83..54117d4 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -110,10 +110,38 @@ def generate_visit_union(name, members):
>
>   void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
>   {
> -}
> +    Error *err = NULL;
> +
> +    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s),&err);
> +    visit_type_%(name)sKind(m,&(*obj)->kind, "type",&err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        goto end;
> +    }
> +    switch ((*obj)->kind) {
>   ''',
>                    name=name)
>
> +    for key in members:
> +        ret += mcgen('''
> +    case %(abbrev)s_KIND_%(enum)s:
> +        visit_type_%(c_type)s(m,&(*obj)->%(c_name)s, "data", errp);
> +        break;
> +''',
> +                abbrev = de_camel_case(name).upper(),
> +                enum = de_camel_case(key).upper(),
> +                c_type=members[key],
> +                c_name=c_var(key))
> +
> +    ret += mcgen('''
> +    default:
> +        abort();
> +    }
> +end:
> +    visit_end_struct(m, errp);
> +}
> +''')
> +
>       return ret
>
>   def generate_declaration(name, members, genlist=True):
> @@ -242,6 +270,7 @@ for expr in exprs:
>           fdecl.write(ret)
>       elif expr.has_key('union'):
>           ret = generate_visit_union(expr['union'], expr['data'])
> +        ret += generate_visit_list(expr['union'], expr['data'])
>           fdef.write(ret)
>
>           ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
> diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
> index 926db5c..1996e49 100644
> --- a/test-qmp-input-visitor.c
> +++ b/test-qmp-input-visitor.c
> @@ -234,6 +234,22 @@ static void test_visitor_in_list(TestInputVisitorData *data,
>       qapi_free_UserDefOneList(head);
>   }
>
> +static void test_visitor_in_union(TestInputVisitorData *data,
> +                                  const void *unused)
> +{
> +    Visitor *v;
> +    Error *err = NULL;
> +    UserDefUnion *tmp;
> +
> +    v = visitor_input_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
> +
> +    visit_type_UserDefUnion(v,&tmp, NULL,&err);
> +    g_assert(err == NULL);
> +    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_B);
> +    g_assert_cmpint(tmp->b->integer, ==, 42);
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
>   static void input_visitor_test_add(const char *testpath,
>                                      TestInputVisitorData *data,
>                                      void (*test_func)(TestInputVisitorData *data, const void *user_data))
> @@ -264,6 +280,8 @@ int main(int argc, char **argv)
>                               &in_visitor_data, test_visitor_in_struct_nested);
>       input_visitor_test_add("/visitor/input/list",
>                               &in_visitor_data, test_visitor_in_list);
> +    input_visitor_test_add("/visitor/input/union",
> +&in_visitor_data, test_visitor_in_union);
>
>       g_test_run();
>
> diff --git a/test-qmp-output-visitor.c b/test-qmp-output-visitor.c
> index c94c208..78e5f2d 100644
> --- a/test-qmp-output-visitor.c
> +++ b/test-qmp-output-visitor.c
> @@ -380,6 +380,38 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
>       qapi_free_UserDefNestedList(head);
>   }
>
> +static void test_visitor_out_union(TestOutputVisitorData *data,
> +                                   const void *unused)
> +{
> +    QObject *arg, *qvalue;
> +    QDict *qdict, *value;
> +
> +    Error *err = NULL;
> +
> +    UserDefUnion *tmp = g_malloc0(sizeof(UserDefUnion));
> +    tmp->kind = USER_DEF_UNION_KIND_A;
> +    tmp->a = g_malloc0(sizeof(UserDefA));
> +    tmp->a->boolean = true;
> +
> +    visit_type_UserDefUnion(data->ov,&tmp, NULL,&err);
> +    g_assert(err == NULL);
> +    arg = qmp_output_get_qobject(data->qov);
> +
> +    g_assert(qobject_type(arg) == QTYPE_QDICT);
> +    qdict = qobject_to_qdict(arg);
> +
> +    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "a");
> +
> +    qvalue = qdict_get(qdict, "data");
> +    g_assert(data != NULL);
> +    g_assert(qobject_type(qvalue) == QTYPE_QDICT);
> +    value = qobject_to_qdict(qvalue);
> +    g_assert_cmpint(qdict_get_bool(value, "boolean"), ==, true);
> +
> +    qapi_free_UserDefUnion(tmp);
> +    QDECREF(qdict);
> +}
> +
>   static void output_visitor_test_add(const char *testpath,
>                                       TestOutputVisitorData *data,
>                                       void (*test_func)(TestOutputVisitorData *data, const void *user_data))
> @@ -416,6 +448,8 @@ int main(int argc, char **argv)
>                               &out_visitor_data, test_visitor_out_list);
>       output_visitor_test_add("/visitor/output/list-qapi-free",
>                               &out_visitor_data, test_visitor_out_list_qapi_free);
> +    output_visitor_test_add("/visitor/output/union",
> +&out_visitor_data, test_visitor_out_union);
>
>       g_test_run();
>
I got the following error when I tried to compile it:
blockdev.c: In function ‘qmp_blockdev_snapshot_sync’:
blockdev.c:664: error: unknown field ‘blockdev_snapshot_sync’ specified 
in initializer
cc1: warnings being treated as errors
blockdev.c:664: error: missing braces around initializer
blockdev.c:664: error: (near initialization for ‘action.<anonymous>’)
blockdev.c: In function ‘qmp_drive_mirror’:
blockdev.c:688: error: unknown field ‘drive_mirror’ specified in initializer
blockdev.c:688: error: missing braces around initializer
blockdev.c:688: error: (near initialization for ‘action.<anonymous>’)
blockdev.c:688: error: initialization from incompatible pointer type
make: *** [blockdev.o] Error 1
make: *** Waiting for unfinished jobs....

It seems we need a name for the union to reference its member. So I 
modified the scripts as the following patch. I also updated blockdev.c 
accordingly. After that I can compile it without error. Actually, I 
don't know why we need introduce a union for BlockdevAction. Can we just 
use a void pointer like "void *action_param" to replace the union? Or 
can we change the field ."snapshot_file to "target" too? Then they can 
share the same action parameter structure. It could make the code in 
qmp_transaction more compact because most of the code for cases mirror 
and snapshot are the same.

Mark



diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 6968e7f..de43790 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -128,7 +128,7 @@ struct %(name)s
c_name=c_var(key))

ret += mcgen('''
- };
+ } u;
};
''')

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 54117d4..4f8ac4d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -125,7 +125,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** 
obj, const char *name, Error **
for key in members:
ret += mcgen('''
case %(abbrev)s_KIND_%(enum)s:
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp);
+ visit_type_%(c_type)s(m, &(*obj)->u.%(c_name)s, "data", errp);
break;
''',
abbrev = de_camel_case(name).upper(),