[Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code

[Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code

[Markus Armbruster]

Coverity is unhappy with the generated code.  Nothing serious, just
heaps of valid DEADCODE defects topped off with a few bogus
FORWARD_NULL defects.

I had a look at the generator, and decided I don't want to mess with
it without decent test coverage.  Unfortunately, a few features have
been added without tests.  My first seven patches make the tests catch
up.  tests/qapi-schema/qapi-schema-test.json now covers all mcgen() in
scripts/qapi*.py, except for a few in qapi-commands.py that are
conditional on -m.

My last three patches clean up the generated code.

Markus Armbruster (10):
  tests/qapi-schema: Actually check successful QMP command response
  tests/qapi-schema: Cover optional command arguments
  tests/qapi-schema: Cover simple argument types
  tests/qapi-schema: Cover anonymous union types
  tests/qapi-schema: Cover complex types with base
  tests/qapi-schema: Cover union types with base
  tests/qapi-schema: Cover flat union types
  qapi: Drop nonsensical header guard in generated qapi-visit.c
  qapi: Drop unused code in qapi-commands.py
  qapi: Clean up null checking in generated visitors

 scripts/qapi-commands.py                | 20 ---------
 scripts/qapi-visit.py                   | 16 +++----
 tests/qapi-schema/qapi-schema-test.json | 24 +++++++++-
 tests/qapi-schema/qapi-schema-test.out  | 19 +++++---
 tests/test-qmp-commands.c               | 79 +++++++++++++++++++++++++++------
 tests/test-qmp-input-strict.c           | 69 +++++++++++++++++++++++++++-
 tests/test-qmp-input-visitor.c          | 45 +++++++++++++++++--
 tests/test-qmp-output-visitor.c         | 67 ++++++++++++++++++++++++++--
 tests/test-visitor-serialization.c      | 14 +++---
 9 files changed, 288 insertions(+), 65 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qmp-commands.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 5a3e82a..2416d07 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -71,6 +71,24 @@ static void test_dispatch_cmd_error(void)
     QDECREF(req);
 }
 
+static QObject *
+test_qmp_dispatch(QDict *req)
+{
+    QObject *resp_obj;
+    QDict *resp;
+    QObject *ret;
+
+    resp_obj = qmp_dispatch(QOBJECT(req));
+    assert(resp_obj);
+    resp = qobject_to_qdict(resp_obj);
+    assert(resp && !qdict_haskey(resp, "error"));
+    ret = qdict_get(resp, "return");
+    assert(ret);
+    qobject_incref(ret);
+    qobject_decref(resp_obj);
+    return ret;
+}
+
 /* test commands that involve both input parameters and return values */
 static void test_dispatch_cmd_io(void)
 {
@@ -78,7 +96,8 @@ static void test_dispatch_cmd_io(void)
     QDict *args = qdict_new();
     QDict *ud1a = qdict_new();
     QDict *ud1b = qdict_new();
-    QObject *resp;
+    QDict *ret, *ret_dict, *ret_dict_dict, *ret_dict_dict_userdef;
+    QDict *ret_dict_dict2, *ret_dict_dict2_userdef;
 
     qdict_put_obj(ud1a, "integer", QOBJECT(qint_from_int(42)));
     qdict_put_obj(ud1a, "string", QOBJECT(qstring_from_str("hello")));
@@ -87,15 +106,24 @@ static void test_dispatch_cmd_io(void)
     qdict_put_obj(args, "ud1a", QOBJECT(ud1a));
     qdict_put_obj(args, "ud1b", QOBJECT(ud1b));
     qdict_put_obj(req, "arguments", QOBJECT(args));
-
     qdict_put_obj(req, "execute", QOBJECT(qstring_from_str("user_def_cmd2")));
 
-    /* TODO: put in full payload and check for errors */
-    resp = qmp_dispatch(QOBJECT(req));
-    assert(resp != NULL);
-    assert(!qdict_haskey(qobject_to_qdict(resp), "error"));
-
-    qobject_decref(resp);
+    ret = qobject_to_qdict(test_qmp_dispatch(req));
+
+    assert(!strcmp(qdict_get_str(ret, "string"), "blah1"));
+    ret_dict = qdict_get_qdict(ret, "dict");
+    assert(!strcmp(qdict_get_str(ret_dict, "string"), "blah2"));
+    ret_dict_dict = qdict_get_qdict(ret_dict, "dict");
+    ret_dict_dict_userdef = qdict_get_qdict(ret_dict_dict, "userdef");
+    assert(qdict_get_int(ret_dict_dict_userdef, "integer") == 42);
+    assert(!strcmp(qdict_get_str(ret_dict_dict_userdef, "string"), "hello"));
+    assert(!strcmp(qdict_get_str(ret_dict_dict, "string"), "blah3"));
+    ret_dict_dict2 = qdict_get_qdict(ret_dict, "dict2");
+    ret_dict_dict2_userdef = qdict_get_qdict(ret_dict_dict2, "userdef");
+    assert(qdict_get_int(ret_dict_dict2_userdef, "integer") == 422);
+    assert(!strcmp(qdict_get_str(ret_dict_dict2_userdef, "string"), "hello2"));
+    assert(!strcmp(qdict_get_str(ret_dict_dict2, "string"), "blah4"));
+    QDECREF(ret);
     QDECREF(req);
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/10] tests/qapi-schema: Cover optional command arguments

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 4 +++-
 tests/qapi-schema/qapi-schema-test.out  | 2 +-
 tests/test-qmp-commands.c               | 8 +++++---
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index fe5af75..d2b1397 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -50,7 +50,9 @@
 # testing commands
 { 'command': 'user_def_cmd', 'data': {} }
 { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
-{ 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b': 'UserDefOne'}, 'returns': 'UserDefTwo' }
+{ 'command': 'user_def_cmd2',
+  'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
+  'returns': 'UserDefTwo' }
 
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3851880..af9829e 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -9,7 +9,7 @@
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
- OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
+ OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
 ['EnumOne', 'UserDefUnionKind', 'UserDefNativeListUnionKind']
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 2416d07..b385521 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -16,7 +16,9 @@ void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
 {
 }
 
-UserDefTwo * qmp_user_def_cmd2(UserDefOne * ud1a, UserDefOne * ud1b, Error **errp)
+UserDefTwo * qmp_user_def_cmd2(UserDefOne *ud1a,
+                               bool has_udb1, UserDefOne *ud1b,
+                               Error **errp)
 {
     UserDefTwo *ret;
     UserDefOne *ud1c = g_malloc0(sizeof(UserDefOne));
@@ -24,8 +26,8 @@ UserDefTwo * qmp_user_def_cmd2(UserDefOne * ud1a, UserDefOne * ud1b, Error **err
 
     ud1c->string = strdup(ud1a->string);
     ud1c->integer = ud1a->integer;
-    ud1d->string = strdup(ud1b->string);
-    ud1d->integer = ud1b->integer;
+    ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0");
+    ud1d->integer = has_udb1 ? ud1b->integer : 0;
 
     ret = g_malloc0(sizeof(UserDefTwo));
     ret->string = strdup("blah1");
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/10] tests/qapi-schema: Cover simple argument types

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  1 +
 tests/test-qmp-commands.c               | 16 ++++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index d2b1397..3f62821 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -53,6 +53,8 @@
 { 'command': 'user_def_cmd2',
   'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
   'returns': 'UserDefTwo' }
+{ 'command': 'user_def_cmd3', 'data': {'a': 'int', '*b': 'int' },
+  'returns': 'int' }
 
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index af9829e..59468ac 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -10,6 +10,7 @@
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
  OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
+ OrderedDict([('command', 'user_def_cmd3'), ('data', OrderedDict([('a', 'int'), ('*b', 'int')])), ('returns', 'int')]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
 ['EnumOne', 'UserDefUnionKind', 'UserDefNativeListUnionKind']
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index b385521..0e5f6a2 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -41,6 +41,11 @@ UserDefTwo * qmp_user_def_cmd2(UserDefOne *ud1a,
     return ret;
 }
 
+int64_t qmp_user_def_cmd3(int64_t a, bool has_b, int64_t b, Error **errp)
+{
+    return a + (has_b ? b : 0);
+}
+
 /* test commands with no input and no return value */
 static void test_dispatch_cmd(void)
 {
@@ -96,10 +101,12 @@ static void test_dispatch_cmd_io(void)
 {
     QDict *req = qdict_new();
     QDict *args = qdict_new();
+    QDict *args3 = qdict_new();
     QDict *ud1a = qdict_new();
     QDict *ud1b = qdict_new();
     QDict *ret, *ret_dict, *ret_dict_dict, *ret_dict_dict_userdef;
     QDict *ret_dict_dict2, *ret_dict_dict2_userdef;
+    QInt *ret3;
 
     qdict_put_obj(ud1a, "integer", QOBJECT(qint_from_int(42)));
     qdict_put_obj(ud1a, "string", QOBJECT(qstring_from_str("hello")));
@@ -126,6 +133,15 @@ static void test_dispatch_cmd_io(void)
     assert(!strcmp(qdict_get_str(ret_dict_dict2_userdef, "string"), "hello2"));
     assert(!strcmp(qdict_get_str(ret_dict_dict2, "string"), "blah4"));
     QDECREF(ret);
+
+    qdict_put(args3, "a", qint_from_int(66));
+    qdict_put(req, "arguments", args3);
+    qdict_put(req, "execute", qstring_from_str("user_def_cmd3"));
+
+    ret3 = qobject_to_qint(test_qmp_dispatch(req));
+    assert(qint_get_int(ret3) == 66);
+    QDECREF(ret);
+
     QDECREF(req);
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/10] tests/qapi-schema: Cover anonymous union types

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  4 ++++
 tests/qapi-schema/qapi-schema-test.out  |  6 +++++-
 tests/test-qmp-input-strict.c           | 32 ++++++++++++++++++++++++++++++++
 tests/test-qmp-input-visitor.c          | 18 ++++++++++++++++++
 tests/test-qmp-output-visitor.c         | 22 ++++++++++++++++++++++
 5 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 3f62821..8405021 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -32,6 +32,10 @@
 { 'union': 'UserDefUnion',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+{ 'union': 'UserDefAnonUnion',
+  'discriminator': {},
+  'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
+
 # for testing native lists
 { 'union': 'UserDefNativeListUnion',
   'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 59468ac..ac98f32 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -6,13 +6,17 @@
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('union', 'UserDefUnion'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
  OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
  OrderedDict([('command', 'user_def_cmd3'), ('data', OrderedDict([('a', 'int'), ('*b', 'int')])), ('returns', 'int')]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
-['EnumOne', 'UserDefUnionKind', 'UserDefNativeListUnionKind']
+['EnumOne',
+ 'UserDefUnionKind',
+ 'UserDefAnonUnionKind',
+ 'UserDefNativeListUnionKind']
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 6f68963..75dd49f 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -139,6 +139,20 @@ static void test_validate_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_validate_union_anon(TestInputVisitorData *data,
+                                     const void *unused)
+{
+    UserDefAnonUnion *tmp = NULL;
+    Visitor *v;
+    Error *errp = NULL;
+
+    v = validate_test_init(data, "42");
+
+    visit_type_UserDefAnonUnion(v, &tmp, NULL, &errp);
+    g_assert(!error_is_set(&errp));
+    qapi_free_UserDefAnonUnion(tmp);
+}
+
 static void test_validate_fail_struct(TestInputVisitorData *data,
                                        const void *unused)
 {
@@ -198,6 +212,20 @@ static void test_validate_fail_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_validate_fail_union_anon(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    UserDefAnonUnion *tmp = NULL;
+    Visitor *v;
+    Error *errp = NULL;
+
+    v = validate_test_init(data, "3.14");
+
+    visit_type_UserDefAnonUnion(v, &tmp, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    qapi_free_UserDefAnonUnion(tmp);
+}
+
 static void validate_test_add(const char *testpath,
                                TestInputVisitorData *data,
                                void (*test_func)(TestInputVisitorData *data, const void *user_data))
@@ -220,6 +248,8 @@ int main(int argc, char **argv)
                        &testdata, test_validate_list);
     validate_test_add("/visitor/input-strict/pass/union",
                        &testdata, test_validate_union);
+    validate_test_add("/visitor/input-strict/pass/union-anon",
+                       &testdata, test_validate_union_anon);
     validate_test_add("/visitor/input-strict/fail/struct",
                        &testdata, test_validate_fail_struct);
     validate_test_add("/visitor/input-strict/fail/struct-nested",
@@ -228,6 +258,8 @@ int main(int argc, char **argv)
                        &testdata, test_validate_fail_list);
     validate_test_add("/visitor/input-strict/fail/union",
                        &testdata, test_validate_fail_union);
+    validate_test_add("/visitor/input-strict/fail/union-anon",
+                       &testdata, test_validate_fail_union_anon);
 
     g_test_run();
 
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 1e1c6fa..89f845b 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -302,6 +302,22 @@ static void test_visitor_in_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_visitor_in_union_anon(TestInputVisitorData *data,
+                                       const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefAnonUnion *tmp;
+
+    v = visitor_input_test_init(data, "42");
+
+    visit_type_UserDefAnonUnion(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    g_assert_cmpint(tmp->kind, ==, USER_DEF_ANON_UNION_KIND_I);
+    g_assert_cmpint(tmp->i, ==, 42);
+    qapi_free_UserDefAnonUnion(tmp);
+}
+
 static void test_native_list_integer_helper(TestInputVisitorData *data,
                                             const void *unused,
                                             UserDefNativeListUnionKind kind)
@@ -635,6 +651,8 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_list);
     input_visitor_test_add("/visitor/input/union",
                             &in_visitor_data, test_visitor_in_union);
+    input_visitor_test_add("/visitor/input/union-anon",
+                            &in_visitor_data, test_visitor_in_union_anon);
     input_visitor_test_add("/visitor/input/errors",
                             &in_visitor_data, test_visitor_in_errors);
     input_visitor_test_add("/visitor/input/native_list/int",
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index e073d83..86efdb4 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -434,6 +434,26 @@ static void test_visitor_out_union(TestOutputVisitorData *data,
     QDECREF(qdict);
 }
 
+static void test_visitor_out_union_anon(TestOutputVisitorData *data,
+                                        const void *unused)
+{
+    QObject *arg;
+    Error *err = NULL;
+
+    UserDefAnonUnion *tmp = g_malloc0(sizeof(UserDefAnonUnion));
+    tmp->kind = USER_DEF_ANON_UNION_KIND_I;
+    tmp->i = 42;
+
+    visit_type_UserDefAnonUnion(data->ov, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    arg = qmp_output_get_qobject(data->qov);
+
+    g_assert(qobject_type(arg) == QTYPE_QINT);
+    g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42);
+
+    qapi_free_UserDefAnonUnion(tmp);
+}
+
 static void init_native_list(UserDefNativeListUnion *cvalue)
 {
     int i;
@@ -782,6 +802,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list_qapi_free);
     output_visitor_test_add("/visitor/output/union",
                             &out_visitor_data, test_visitor_out_union);
+    output_visitor_test_add("/visitor/output/union-anon",
+                            &out_visitor_data, test_visitor_out_union_anon);
     output_visitor_test_add("/visitor/output/native_list/int",
                             &out_visitor_data, test_visitor_out_native_list_int);
     output_visitor_test_add("/visitor/output/native_list/int8",
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/10] tests/qapi-schema: Cover complex types with base

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  6 +++++-
 tests/qapi-schema/qapi-schema-test.out  |  6 ++++--
 tests/test-qmp-commands.c               | 15 ++++++++++-----
 tests/test-qmp-input-visitor.c          |  4 ++--
 tests/test-qmp-output-visitor.c         | 12 ++++++++----
 tests/test-visitor-serialization.c      | 14 ++++++++------
 6 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 8405021..c7e6be8 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -7,8 +7,12 @@
   'data': { 'enum1': 'EnumOne', '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
 
 # for testing nested structs
+{ 'type': 'UserDefZero',
+  'data': { 'integer': 'int' } }
+
 { 'type': 'UserDefOne',
-  'data': { 'integer': 'int', 'string': 'str', '*enum1': 'EnumOne' } }
+  'base': 'UserDefZero',
+  'data': { 'string': 'str', '*enum1': 'EnumOne' } }
 
 { 'type': 'UserDefTwo',
   'data': { 'string': 'str',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index ac98f32..89e213a 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,6 +1,7 @@
 [OrderedDict([('enum', 'EnumOne'), ('data', ['value1', 'value2', 'value3'])]),
  OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
- OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]),
+ OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
@@ -18,7 +19,8 @@
  'UserDefAnonUnionKind',
  'UserDefNativeListUnionKind']
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
- OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]),
+ OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 0e5f6a2..899822d 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -25,9 +25,11 @@ UserDefTwo * qmp_user_def_cmd2(UserDefOne *ud1a,
     UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne));
 
     ud1c->string = strdup(ud1a->string);
-    ud1c->integer = ud1a->integer;
+    ud1c->base = g_new0(UserDefZero, 1);
+    ud1c->base->integer = ud1a->base->integer;
     ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0");
-    ud1d->integer = has_udb1 ? ud1b->integer : 0;
+    ud1d->base = g_new0(UserDefZero, 1);
+    ud1d->base->integer = has_udb1 ? ud1b->base->integer : 0;
 
     ret = g_malloc0(sizeof(UserDefTwo));
     ret->string = strdup("blah1");
@@ -152,17 +154,20 @@ static void test_dealloc_types(void)
     UserDefOneList *ud1list;
 
     ud1test = g_malloc0(sizeof(UserDefOne));
-    ud1test->integer = 42;
+    ud1test->base = g_new0(UserDefZero, 1);
+    ud1test->base->integer = 42;
     ud1test->string = g_strdup("hi there 42");
 
     qapi_free_UserDefOne(ud1test);
 
     ud1a = g_malloc0(sizeof(UserDefOne));
-    ud1a->integer = 43;
+    ud1a->base = g_new0(UserDefZero, 1);
+    ud1a->base->integer = 43;
     ud1a->string = g_strdup("hi there 43");
 
     ud1b = g_malloc0(sizeof(UserDefOne));
-    ud1b->integer = 44;
+    ud1b->base = g_new0(UserDefZero, 1);
+    ud1b->base->integer = 44;
     ud1b->string = g_strdup("hi there 44");
 
     ud1list = g_malloc0(sizeof(UserDefOneList));
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 89f845b..30d24e2 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -252,7 +252,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
 
     check_and_free_str(udp->string0, "string0");
     check_and_free_str(udp->dict1.string1, "string1");
-    g_assert_cmpint(udp->dict1.dict2.userdef1->integer, ==, 42);
+    g_assert_cmpint(udp->dict1.dict2.userdef1->base->integer, ==, 42);
     check_and_free_str(udp->dict1.dict2.userdef1->string, "string");
     check_and_free_str(udp->dict1.dict2.string2, "string2");
     g_assert(udp->dict1.has_dict3 == false);
@@ -280,7 +280,7 @@ static void test_visitor_in_list(TestInputVisitorData *data,
 
         snprintf(string, sizeof(string), "string%d", i);
         g_assert_cmpstr(item->value->string, ==, string);
-        g_assert_cmpint(item->value->integer, ==, 42 + i);
+        g_assert_cmpint(item->value->base->integer, ==, 42 + i);
     }
 
     qapi_free_UserDefOneList(head);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 86efdb4..3179430 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -231,13 +231,15 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
     ud2->dict1.string1 = g_strdup(strings[1]);
     ud2->dict1.dict2.userdef1 = g_malloc0(sizeof(UserDefOne));
     ud2->dict1.dict2.userdef1->string = g_strdup(string);
-    ud2->dict1.dict2.userdef1->integer = value;
+    ud2->dict1.dict2.userdef1->base = g_new0(UserDefZero, 1);
+    ud2->dict1.dict2.userdef1->base->integer = value;
     ud2->dict1.dict2.string2 = g_strdup(strings[2]);
 
     ud2->dict1.has_dict3 = true;
     ud2->dict1.dict3.userdef2 = g_malloc0(sizeof(UserDefOne));
     ud2->dict1.dict3.userdef2->string = g_strdup(string);
-    ud2->dict1.dict3.userdef2->integer = value;
+    ud2->dict1.dict3.userdef2->base = g_new0(UserDefZero, 1);
+    ud2->dict1.dict3.userdef2->base->integer = value;
     ud2->dict1.dict3.string3 = g_strdup(strings[3]);
 
     visit_type_UserDefNested(data->ov, &ud2, "unused", &errp);
@@ -279,7 +281,8 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
                                            const void *unused)
 {
     EnumOne bad_values[] = { ENUM_ONE_MAX, -1 };
-    UserDefOne u = { 0 }, *pu = &u;
+    UserDefZero b;
+    UserDefOne u = { .base = &b }, *pu = &u;
     Error *errp;
     int i;
 
@@ -391,7 +394,8 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
         p->value->dict1.string1 = g_strdup(string);
         p->value->dict1.dict2.userdef1 = g_malloc0(sizeof(UserDefOne));
         p->value->dict1.dict2.userdef1->string = g_strdup(string);
-        p->value->dict1.dict2.userdef1->integer = 42;
+        p->value->dict1.dict2.userdef1->base = g_new0(UserDefZero, 1);
+        p->value->dict1.dict2.userdef1->base->integer = 42;
         p->value->dict1.dict2.string2 = g_strdup(string);
         p->value->dict1.has_dict3 = false;
 
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 9aaa587..dfce5aa 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -239,12 +239,14 @@ static UserDefNested *nested_struct_create(void)
     udnp->string0 = strdup("test_string0");
     udnp->dict1.string1 = strdup("test_string1");
     udnp->dict1.dict2.userdef1 = g_malloc0(sizeof(UserDefOne));
-    udnp->dict1.dict2.userdef1->integer = 42;
+    udnp->dict1.dict2.userdef1->base = g_new0(UserDefZero, 1);
+    udnp->dict1.dict2.userdef1->base->integer = 42;
     udnp->dict1.dict2.userdef1->string = strdup("test_string");
     udnp->dict1.dict2.string2 = strdup("test_string2");
     udnp->dict1.has_dict3 = true;
     udnp->dict1.dict3.userdef2 = g_malloc0(sizeof(UserDefOne));
-    udnp->dict1.dict3.userdef2->integer = 43;
+    udnp->dict1.dict3.userdef2->base = g_new0(UserDefZero, 1);
+    udnp->dict1.dict3.userdef2->base->integer = 43;
     udnp->dict1.dict3.userdef2->string = strdup("test_string");
     udnp->dict1.dict3.string3 = strdup("test_string3");
     return udnp;
@@ -256,14 +258,14 @@ static void nested_struct_compare(UserDefNested *udnp1, UserDefNested *udnp2)
     g_assert(udnp2);
     g_assert_cmpstr(udnp1->string0, ==, udnp2->string0);
     g_assert_cmpstr(udnp1->dict1.string1, ==, udnp2->dict1.string1);
-    g_assert_cmpint(udnp1->dict1.dict2.userdef1->integer, ==,
-                    udnp2->dict1.dict2.userdef1->integer);
+    g_assert_cmpint(udnp1->dict1.dict2.userdef1->base->integer, ==,
+                    udnp2->dict1.dict2.userdef1->base->integer);
     g_assert_cmpstr(udnp1->dict1.dict2.userdef1->string, ==,
                     udnp2->dict1.dict2.userdef1->string);
     g_assert_cmpstr(udnp1->dict1.dict2.string2, ==, udnp2->dict1.dict2.string2);
     g_assert(udnp1->dict1.has_dict3 == udnp2->dict1.has_dict3);
-    g_assert_cmpint(udnp1->dict1.dict3.userdef2->integer, ==,
-                    udnp2->dict1.dict3.userdef2->integer);
+    g_assert_cmpint(udnp1->dict1.dict3.userdef2->base->integer, ==,
+                    udnp2->dict1.dict3.userdef2->base->integer);
     g_assert_cmpstr(udnp1->dict1.dict3.userdef2->string, ==,
                     udnp2->dict1.dict3.userdef2->string);
     g_assert_cmpstr(udnp1->dict1.dict3.string3, ==, udnp2->dict1.dict3.string3);
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union types with base

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 1 +
 tests/qapi-schema/qapi-schema-test.out  | 2 +-
 tests/test-qmp-input-strict.c           | 4 ++--
 tests/test-qmp-input-visitor.c          | 3 ++-
 tests/test-qmp-output-visitor.c         | 2 ++
 5 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c7e6be8..f5c5d37 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -34,6 +34,7 @@
   'data': { 'integer': 'int' } }
 
 { 'union': 'UserDefUnion',
+  'base': 'UserDefZero',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
 { 'union': 'UserDefAnonUnion',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 89e213a..3f51afd 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -6,7 +6,7 @@
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('union', 'UserDefUnion'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
  OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 75dd49f..a58f55f 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -132,7 +132,7 @@ static void test_validate_union(TestInputVisitorData *data,
     Visitor *v;
     Error *errp = NULL;
 
-    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
+    v = validate_test_init(data, "{ 'type': 'b', 'integer': 41, 'data' : { 'integer': 42 } }");
 
     visit_type_UserDefUnion(v, &tmp, NULL, &errp);
     g_assert(!error_is_set(&errp));
@@ -205,7 +205,7 @@ static void test_validate_fail_union(TestInputVisitorData *data,
     Error *errp = NULL;
     Visitor *v;
 
-    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 }, 'extra': 'yyy' }");
+    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
 
     visit_type_UserDefUnion(v, &tmp, NULL, &errp);
     g_assert(error_is_set(&errp));
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 30d24e2..1c92572 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -293,11 +293,12 @@ static void test_visitor_in_union(TestInputVisitorData *data,
     Error *err = NULL;
     UserDefUnion *tmp;
 
-    v = visitor_input_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
+    v = visitor_input_test_init(data, "{ 'type': 'b', 'integer': 41, '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->integer, ==, 41);
     g_assert_cmpint(tmp->b->integer, ==, 42);
     qapi_free_UserDefUnion(tmp);
 }
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 3179430..6ed7ffa 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -416,6 +416,7 @@ static void test_visitor_out_union(TestOutputVisitorData *data,
 
     UserDefUnion *tmp = g_malloc0(sizeof(UserDefUnion));
     tmp->kind = USER_DEF_UNION_KIND_A;
+    tmp->integer = 41;
     tmp->a = g_malloc0(sizeof(UserDefA));
     tmp->a->boolean = true;
 
@@ -427,6 +428,7 @@ static void test_visitor_out_union(TestOutputVisitorData *data,
     qdict = qobject_to_qdict(arg);
 
     g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "a");
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41);
 
     qvalue = qdict_get(qdict, "data");
     g_assert(data != NULL);
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/10] tests/qapi-schema: Cover flat union types

[Markus Armbruster]

The test demonstrates a generator bug: the generated struct
UserDefFlatUnion doesn't include members for the indirect base
UserDefZero.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  7 +++++++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 tests/test-qmp-input-strict.c           | 33 +++++++++++++++++++++++++++++++++
 tests/test-qmp-input-visitor.c          | 20 ++++++++++++++++++++
 tests/test-qmp-output-visitor.c         | 31 +++++++++++++++++++++++++++++++
 5 files changed, 93 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index f5c5d37..471ba47 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -37,6 +37,13 @@
   'base': 'UserDefZero',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+{ 'union': 'UserDefFlatUnion',
+  'base': 'UserDefOne',
+  'discriminator': 'string',
+  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+# FIXME generated struct UserDefFlatUnion has members for direct base
+# UserDefOne, but lacks members for indirect base UserDefZero
+
 { 'union': 'UserDefAnonUnion',
   'discriminator': {},
   'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3f51afd..89b53d4 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -7,6 +7,7 @@
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'), ('discriminator', 'string'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
  OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
@@ -16,6 +17,7 @@
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
 ['EnumOne',
  'UserDefUnionKind',
+ 'UserDefFlatUnionKind',
  'UserDefAnonUnionKind',
  'UserDefNativeListUnionKind']
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index a58f55f..09fc1ef 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -139,6 +139,21 @@ static void test_validate_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_validate_union_flat(TestInputVisitorData *data,
+                                     const void *unused)
+{
+    UserDefFlatUnion *tmp = NULL;
+    Visitor *v;
+    Error *errp = NULL;
+
+    v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
+    /* TODO when generator bug is fixed, add 'integer': 41 */
+
+    visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
+    g_assert(!error_is_set(&errp));
+    qapi_free_UserDefFlatUnion(tmp);
+}
+
 static void test_validate_union_anon(TestInputVisitorData *data,
                                      const void *unused)
 {
@@ -212,6 +227,20 @@ static void test_validate_fail_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_validate_fail_union_flat(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    UserDefFlatUnion *tmp = NULL;
+    Error *errp = NULL;
+    Visitor *v;
+
+    v = validate_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");
+
+    visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
+    g_assert(error_is_set(&errp));
+    qapi_free_UserDefFlatUnion(tmp);
+}
+
 static void test_validate_fail_union_anon(TestInputVisitorData *data,
                                           const void *unused)
 {
@@ -248,6 +277,8 @@ int main(int argc, char **argv)
                        &testdata, test_validate_list);
     validate_test_add("/visitor/input-strict/pass/union",
                        &testdata, test_validate_union);
+    validate_test_add("/visitor/input-strict/pass/union-flat",
+                       &testdata, test_validate_union_flat);
     validate_test_add("/visitor/input-strict/pass/union-anon",
                        &testdata, test_validate_union_anon);
     validate_test_add("/visitor/input-strict/fail/struct",
@@ -258,6 +289,8 @@ int main(int argc, char **argv)
                        &testdata, test_validate_fail_list);
     validate_test_add("/visitor/input-strict/fail/union",
                        &testdata, test_validate_fail_union);
+    validate_test_add("/visitor/input-strict/fail/union-flat",
+                       &testdata, test_validate_fail_union_flat);
     validate_test_add("/visitor/input-strict/fail/union-anon",
                        &testdata, test_validate_fail_union_anon);
 
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 1c92572..f1ab541 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -303,6 +303,24 @@ static void test_visitor_in_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_visitor_in_union_flat(TestInputVisitorData *data,
+                                       const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefFlatUnion *tmp;
+
+    v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
+    /* TODO when generator bug is fixed, add 'integer': 41 */
+
+    visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
+    /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
+    g_assert_cmpint(tmp->a->boolean, ==, true);
+    qapi_free_UserDefFlatUnion(tmp);
+}
+
 static void test_visitor_in_union_anon(TestInputVisitorData *data,
                                        const void *unused)
 {
@@ -652,6 +670,8 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_list);
     input_visitor_test_add("/visitor/input/union",
                             &in_visitor_data, test_visitor_in_union);
+    input_visitor_test_add("/visitor/input/union-flat",
+                            &in_visitor_data, test_visitor_in_union_flat);
     input_visitor_test_add("/visitor/input/union-anon",
                             &in_visitor_data, test_visitor_in_union_anon);
     input_visitor_test_add("/visitor/input/errors",
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 6ed7ffa..5613396 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -440,6 +440,35 @@ static void test_visitor_out_union(TestOutputVisitorData *data,
     QDECREF(qdict);
 }
 
+static void test_visitor_out_union_flat(TestOutputVisitorData *data,
+                                        const void *unused)
+{
+    QObject *arg;
+    QDict *qdict;
+
+    Error *err = NULL;
+
+    UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
+    tmp->kind = USER_DEF_UNION_KIND_A;
+    tmp->a = g_malloc0(sizeof(UserDefA));
+    /* TODO when generator bug is fixed: tmp->integer = 41; */
+    tmp->a->boolean = true;
+
+    visit_type_UserDefFlatUnion(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, "string"), ==, "a");
+    /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
+    g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
+
+    qapi_free_UserDefFlatUnion(tmp);
+    QDECREF(qdict);
+}
+
 static void test_visitor_out_union_anon(TestOutputVisitorData *data,
                                         const void *unused)
 {
@@ -808,6 +837,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list_qapi_free);
     output_visitor_test_add("/visitor/output/union",
                             &out_visitor_data, test_visitor_out_union);
+    output_visitor_test_add("/visitor/output/union-flat",
+                            &out_visitor_data, test_visitor_out_union_flat);
     output_visitor_test_add("/visitor/output/union-anon",
                             &out_visitor_data, test_visitor_out_union_anon);
     output_visitor_test_add("/visitor/output/native_list/int",
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 65f1a54..ff4239c 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -494,10 +494,8 @@ fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
 # have the functions defined, so we use -b option to provide control
 # over these cases
 if do_builtins:
-    fdef.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
     for typename in builtin_types:
         fdef.write(generate_visit_list(typename, None))
-    fdef.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
 
 for expr in exprs:
     if expr.has_key('type'):
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/10] qapi: Drop unused code in qapi-commands.py

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index b12b696..4bca121 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -23,13 +23,6 @@ def type_visitor(name):
     else:
         return 'visit_type_%s' % name
 
-def generate_decl_enum(name, members, genlist=True):
-    return mcgen('''
-
-void %(visitor)s(Visitor *m, %(name)s * obj, const char *name, Error **errp);
-''',
-                 visitor=type_visitor(name))
-
 def generate_command_decl(name, args, ret_type):
     arglist=""
     for argname, argtype, optional, structured in parse_args(args):
@@ -76,19 +69,6 @@ def gen_marshal_output_call(name, ret_type):
         return ""
     return "qmp_marshal_output_%s(retval, ret, errp);" % c_fun(name)
 
-def gen_visitor_output_containers_decl(ret_type):
-    ret = ""
-    push_indent()
-    if ret_type:
-        ret += mcgen('''
-QmpOutputVisitor *mo;
-QapiDeallocVisitor *md;
-Visitor *v;
-''')
-    pop_indent()
-
-    return ret
-
 def gen_visitor_input_containers_decl(args):
     ret = ""
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Markus Armbruster]

Visitors get passed a pointer to the visited object.  The generated
visitors try to cope with this pointer being null in some places, for
instance like this:

    visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);

visit_start_optional() passes its second argument to Visitor method
start_optional.  Two out of two methods dereference it
unconditionally.

I fail to see how hits pointer could legitimately be null.

All this useless null checking is highly redundant, which Coverity
duly reports.  About 200 times.

Remove the useless null checks.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ff4239c..3eb10c8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
 
     if base:
         ret += mcgen('''
-visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
+visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
 if (!err) {
-    visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
+    visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
     error_propagate(errp, err);
     err = NULL;
     visit_end_implicit_struct(m, &err);
@@ -61,8 +61,8 @@ if (!err) {
     for argname, argentry, optional, structured in parse_args(members):
         if optional:
             ret += mcgen('''
-visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
-if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
+visit_start_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
+if ((*obj)->%(prefix)shas_%(c_name)s) {
 ''',
                          c_prefix=c_var(field_prefix), prefix=field_prefix,
                          c_name=c_var(argname), name=argname)
@@ -72,7 +72,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
             ret += generate_visit_struct_body(full_name, argname, argentry)
         else:
             ret += mcgen('''
-visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
+visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
 ''',
                          c_prefix=c_var(field_prefix), prefix=field_prefix,
                          type=type_name(argentry), c_name=c_var(argname),
@@ -121,7 +121,7 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
 
     ret += mcgen('''
 if (!err) {
-    if (!obj || *obj) {
+    if (*obj) {
         visit_type_%(name)s_fields(m, obj, &err);
         error_propagate(errp, err);
         err = NULL;
@@ -273,7 +273,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
     if (!error_is_set(errp)) {
         visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
         if (!err) {
-            if (obj && *obj) {
+            if (*obj) {
 ''',
                  name=name)
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-qmp-commands.c | 44 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 5a3e82a..2416d07 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -71,6 +71,24 @@ static void test_dispatch_cmd_error(void)
>      QDECREF(req);
>  }
>  
> +static QObject *
> +test_qmp_dispatch(QDict *req)

Doesn't qemu style prefer this all on one line?

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 02/10] tests/qapi-schema: Cover optional command arguments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 4 +++-
>  tests/qapi-schema/qapi-schema-test.out  | 2 +-
>  tests/test-qmp-commands.c               | 8 +++++---
>  3 files changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 03/10] tests/qapi-schema: Cover simple argument types

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json |  2 ++
>  tests/qapi-schema/qapi-schema-test.out  |  1 +
>  tests/test-qmp-commands.c               | 16 ++++++++++++++++
>  3 files changed, 19 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 04/10] tests/qapi-schema: Cover anonymous union types

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json |  4 ++++
>  tests/qapi-schema/qapi-schema-test.out  |  6 +++++-
>  tests/test-qmp-input-strict.c           | 32 ++++++++++++++++++++++++++++++++
>  tests/test-qmp-input-visitor.c          | 18 ++++++++++++++++++
>  tests/test-qmp-output-visitor.c         | 22 ++++++++++++++++++++++
>  5 files changed, 81 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 05/10] tests/qapi-schema: Cover complex types with base

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json |  6 +++++-
>  tests/qapi-schema/qapi-schema-test.out  |  6 ++++--
>  tests/test-qmp-commands.c               | 15 ++++++++++-----
>  tests/test-qmp-input-visitor.c          |  4 ++--
>  tests/test-qmp-output-visitor.c         | 12 ++++++++----
>  tests/test-visitor-serialization.c      | 14 ++++++++------
>  6 files changed, 37 insertions(+), 20 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union types with base

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 1 +
>  tests/qapi-schema/qapi-schema-test.out  | 2 +-
>  tests/test-qmp-input-strict.c           | 4 ++--
>  tests/test-qmp-input-visitor.c          | 3 ++-
>  tests/test-qmp-output-visitor.c         | 2 ++
>  5 files changed, 8 insertions(+), 4 deletions(-)
> 

> - OrderedDict([('union', 'UserDefUnion'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
> + OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
>   OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
>   OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),

Good, we still have coverage of a union without a base.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 07/10] tests/qapi-schema: Cover flat union types

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> The test demonstrates a generator bug: the generated struct
> UserDefFlatUnion doesn't include members for the indirect base
> UserDefZero.

Good catch.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json |  7 +++++++
>  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>  tests/test-qmp-input-strict.c           | 33 +++++++++++++++++++++++++++++++++
>  tests/test-qmp-input-visitor.c          | 20 ++++++++++++++++++++
>  tests/test-qmp-output-visitor.c         | 31 +++++++++++++++++++++++++++++++
>  5 files changed, 93 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 65f1a54..ff4239c 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -494,10 +494,8 @@ fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
>  # have the functions defined, so we use -b option to provide control
>  # over these cases
>  if do_builtins:
> -    fdef.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
>      for typename in builtin_types:
>          fdef.write(generate_visit_list(typename, None))
> -    fdef.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DEF"))
>  
>  for expr in exprs:
>      if expr.has_key('type'):
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 09/10] qapi: Drop unused code in qapi-commands.py

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On 02/06/2014 07:29 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py | 20 --------------------
>  1 file changed, 20 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On 02/06/2014 07:30 AM, Markus Armbruster wrote:
> Visitors get passed a pointer to the visited object.  The generated
> visitors try to cope with this pointer being null in some places, for
> instance like this:
> 
>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
> 
> visit_start_optional() passes its second argument to Visitor method
> start_optional.  Two out of two methods dereference it
> unconditionally.

Let's see if I can find them without Coverity's help...

opts-visitor.c:opts_start_optional
qmp-input-visitor.c:qmp_input_start_optional
string-input-visitor.c:parse_start_optional

Your counting is off :)  All three unconditionally dereference.

[While at it, this code style from parse_start_optional, and similar in
other locations, is rather verbose:

    if (!siv->string) {
        *present = false;
        return;
    }

    *present = true;
}

Shorter would be:

    *present = !!siv->string;
}

but that doesn't affect this patch]

> 
> I fail to see how hits pointer could legitimately be null.
> 
> All this useless null checking is highly redundant, which Coverity
> duly reports.  About 200 times.
> 
> Remove the useless null checks.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/06/2014 07:30 AM, Markus Armbruster wrote:
>> Visitors get passed a pointer to the visited object.  The generated
>> visitors try to cope with this pointer being null in some places, for
>> instance like this:
>> 
>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>> 
>> visit_start_optional() passes its second argument to Visitor method
>> start_optional.  Two out of two methods dereference it
>> unconditionally.
>
> Let's see if I can find them without Coverity's help...
>
> opts-visitor.c:opts_start_optional
> qmp-input-visitor.c:qmp_input_start_optional
> string-input-visitor.c:parse_start_optional
>
> Your counting is off :)  All three unconditionally dereference.

Right.  I can't count :)

Most Coverity's defects are about silliness like this:

    visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
    if (obj && (*obj)->has_name) {
        visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err);
    }

The second "obj ? ... : ..." is guarded by if (obj), thus cannot take
the false branch.

> [While at it, this code style from parse_start_optional, and similar in
> other locations, is rather verbose:
>
>     if (!siv->string) {
>         *present = false;
>         return;
>     }
>
>     *present = true;
> }
>
> Shorter would be:
>
>     *present = !!siv->string;
> }
>
> but that doesn't affect this patch]

Coccinelle job, if I can teach it our macros.  Without that, it
frequently chokes on some macro usage, leaving too much code unexamined.

>> I fail to see how hits pointer could legitimately be null.
>> 
>> All this useless null checking is highly redundant, which Coverity
>> duly reports.  About 200 times.
>> 
>> Remove the useless null checks.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-visit.py | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/06/2014 07:29 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/test-qmp-commands.c | 44 ++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 36 insertions(+), 8 deletions(-)
>> 
>> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
>> index 5a3e82a..2416d07 100644
>> --- a/tests/test-qmp-commands.c
>> +++ b/tests/test-qmp-commands.c
>> @@ -71,6 +71,24 @@ static void test_dispatch_cmd_error(void)
>>      QDECREF(req);
>>  }
>>  
>> +static QObject *
>> +test_qmp_dispatch(QDict *req)
>
> Doesn't qemu style prefer this all on one line?
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

It does, but my fingers still hate it...

I'll wait a bit before I respin, to give others a chance to comment.

Thanks for your prompt review!

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Paolo Bonzini]

Il 06/02/2014 15:30, Markus Armbruster ha scritto:
> Visitors get passed a pointer to the visited object.  The generated
> visitors try to cope with this pointer being null in some places, for
> instance like this:
>
>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>
> visit_start_optional() passes its second argument to Visitor method
> start_optional.  Two out of two methods dereference it
> unconditionally.

Some visitor implementations however do not implement start_optional at 
all.  With these visitor implementations, you currently could pass a 
NULL object.  After your patch, you still can but you're passing a bad 
pointer which is also a problem (perhaps one that Coverity would also 
detect).

> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index ff4239c..3eb10c8 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
>
>      if base:
>          ret += mcgen('''
> -visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);

This is the implementation of start_implicit_struct:

static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
                                             size_t size, Error **errp)
{
     if (obj) {
         *obj = g_malloc0(size);
     }
}

Before your patch, if obj is NULL, *obj is not written.

After your patch, if obj is NULL, and c_name is not the first field in 
the struct, *obj is written and you get a NULL pointer dereference. 
Same for end_implicit_struct in qapi/qapi-dealloc-visitor.c.

So I think if you remove this checking, you need to do the same in the 
visitor implementations as well.

I think NULL pointer input can be used to *validate* input against QAPI 
types without building a throw-away object (which entails unbounded 
memory allocations for list types).  I don't know if the state of this 
is "broken", "it never worked", or "works but not tested and never 
used".  It's definitely not covered by the unit tests.

>  if (!err) {
> -    visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
> +    visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
>      error_propagate(errp, err);
>      err = NULL;
>      visit_end_implicit_struct(m, &err);
> @@ -61,8 +61,8 @@ if (!err) {
>      for argname, argentry, optional, structured in parse_args(members):
>          if optional:
>              ret += mcgen('''
> -visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
> -if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
> +visit_start_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
> +if ((*obj)->%(prefix)shas_%(c_name)s) {
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>                           c_name=c_var(argname), name=argname)
> @@ -72,7 +72,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
>              ret += generate_visit_struct_body(full_name, argname, argentry)
>          else:
>              ret += mcgen('''
> -visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
> +visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>                           type=type_name(argentry), c_name=c_var(argname),
> @@ -121,7 +121,7 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
>
>      ret += mcgen('''
>  if (!err) {
> -    if (!obj || *obj) {
> +    if (*obj) {
>          visit_type_%(name)s_fields(m, obj, &err);

This is a different problem, and I think a different Coverity error too, 
isn't it?

No objections to patch 1-9.

Paolo

>          error_propagate(errp, err);
>          err = NULL;
> @@ -273,7 +273,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>      if (!error_is_set(errp)) {
>          visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
>          if (!err) {
> -            if (obj && *obj) {
> +            if (*obj) {
>  ''',
>                   name=name)
>
>

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 06/02/2014 15:30, Markus Armbruster ha scritto:
>> Visitors get passed a pointer to the visited object.  The generated
>> visitors try to cope with this pointer being null in some places, for
>> instance like this:
>>
>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>>
>> visit_start_optional() passes its second argument to Visitor method
>> start_optional.  Two out of two methods dereference it
>> unconditionally.
>
> Some visitor implementations however do not implement start_optional
> at all.  With these visitor implementations, you currently could pass
> a NULL object.  After your patch, you still can but you're passing a
> bad pointer which is also a problem (perhaps one that Coverity would
> also detect).

We need to decide what the contract for the public visit_type_FOO() and
visit_type_FOOlist() is.

No existing user wants to pass a null pointer, the semantics of passing
a null pointer are not obvious to me, and as we'll see below, the
generated code isn't entirely successful in avoiding to dereference a
null argument :)

>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index ff4239c..3eb10c8 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
>>
>>      if base:
>>          ret += mcgen('''
>> -visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
>> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
>
> This is the implementation of start_implicit_struct:

One of two implementations.

> static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
>                                             size_t size, Error **errp)
> {
>     if (obj) {
>         *obj = g_malloc0(size);
>     }
> }
>
> Before your patch, if obj is NULL, *obj is not written.
>
> After your patch, if obj is NULL, and c_name is not the first field in
> the struct, *obj is written and you get a NULL pointer
> dereference. Same for end_implicit_struct in
> qapi/qapi-dealloc-visitor.c.
>
> So I think if you remove this checking, you need to do the same in the
> visitor implementations as well.

Can do.

Here's the other implementation:

static void qapi_dealloc_start_struct(Visitor *v, void **obj, const char *kind,
                                      const char *name, size_t unused,
                                      Error **errp)
{
    QapiDeallocVisitor *qov = to_qov(v);
    qapi_dealloc_push(qov, obj);
}

It happily passes null obj to qapi_dealloc_push().  There, null has a
special meaning, used by qapi_dealloc_start_list(): it's a list head
tracker.  I'd be surprised if this code could cope with null obj.

> I think NULL pointer input can be used to *validate* input against
> QAPI types without building a throw-away object (which entails
> unbounded memory allocations for list types).  I don't know if the
> state of this is "broken", "it never worked", or "works but not tested
> and never used".  It's definitely not covered by the unit tests.

Perhaps it worked in theory for some early version, but it doesn't work
for the current version.  Code that has never been used or even tested
not working shouldn't surprise anybody :)

Consider visit_type_NameInfo(m, NULL, "whatever", &local_err).  Assuming
no errors anywhere, this calls

    visit_start_struct(m, NULL, "NameInfo", "whatever", ...)

then enters visit_type_NameInfo_fields(), which calls

    visit_start_optional(m, NULL, "whatever", ...)

and then crashes dereferencing null obj:

    if (obj && (*obj)->has_name) {
        visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err);
    }

It's not clear to me what visit_type_NameInfo_fields() should do for a
null obj.  Should it visit_type_str(m, NULL, ...)?  That would assume
the optional member is present.  Why?  Feels arbitrary to me.

Should a use for null obj materialize, we can put back support for null
obj.  It'll even work then, having a user.

>>  if (!err) {
>> -    visit_type_%(type)s_fields(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, &err);
>> +    visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
>>      error_propagate(errp, err);
>>      err = NULL;
>>      visit_end_implicit_struct(m, &err);
>> @@ -61,8 +61,8 @@ if (!err) {
>>      for argname, argentry, optional, structured in parse_args(members):
>>          if optional:
>>              ret += mcgen('''
>> -visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
>> -if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
>> +visit_start_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
>> +if ((*obj)->%(prefix)shas_%(c_name)s) {
>>  ''',
>>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>>                           c_name=c_var(argname), name=argname)
>> @@ -72,7 +72,7 @@ if (obj && (*obj)->%(prefix)shas_%(c_name)s) {
>>              ret += generate_visit_struct_body(full_name, argname, argentry)
>>          else:
>>              ret += mcgen('''
>> -visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
>> +visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
>>  ''',
>>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>>                           type=type_name(argentry), c_name=c_var(argname),
>> @@ -121,7 +121,7 @@ visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
>>
>>      ret += mcgen('''
>>  if (!err) {
>> -    if (!obj || *obj) {
>> +    if (*obj) {
>>          visit_type_%(name)s_fields(m, obj, &err);
>
> This is a different problem, and I think a different Coverity error
> too, isn't it?

It's the same, actually: I'm deleting code attempting to cope with null
obj.

The bulk of the Coverity defects is like this one:

310  	static void visit_type_NameInfo_fields(Visitor *m, NameInfo ** obj, Error **errp)
311  	{
312  	    Error *err = NULL;

(1) Event cond_notnull: 	Condition "obj", taking true branch. Now the value of "obj" is not NULL.
Also see events: 	[notnull][dead_error_condition][dead_error_line]

313  	    visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
314  	    if (obj && (*obj)->has_name) {

(2) Event notnull: 	At condition "obj", the value of "obj" cannot be NULL.
(3) Event dead_error_condition: 	The condition "obj" must be true.
(4) Event dead_error_line: 	Execution cannot reach this expression "NULL" inside statement "visit_type_str(m, (obj ? &(...".
Also see events: 	[cond_notnull]

315  	        visit_type_str(m, obj ? &(*obj)->name : NULL, "name", &err);
316  	    }
317  	    visit_end_optional(m, &err);
318  	
319  	    error_propagate(errp, err);
320  	}

In addition, we get a few bogus defects like this one:

2470 	static void visit_type_PciBridgeInfo_bus_fields(Visitor *m, PciBridgeInfo ** obj, Error **errp)
2471 	{
2472 	    Error *err = NULL;
2473 	    visit_type_int(m, obj ? &(*obj)->bus.number : NULL, "number", &err);
2474 	    visit_type_int(m, obj ? &(*obj)->bus.secondary : NULL, "secondary", &err);
2475 	    visit_type_int(m, obj ? &(*obj)->bus.subordinate : NULL, "subordinate", &err);
2476 	    visit_type_PciMemoryRange(m, obj ? &(*obj)->bus.io_range : NULL, "io_range", &err);
2477 	    visit_type_PciMemoryRange(m, obj ? &(*obj)->bus.memory_range : NULL, "memory_range", &err);
2478 	    visit_type_PciMemoryRange(m, obj ? &(*obj)->bus.prefetchable_range : NULL, "prefetchable_range", &err);
2479 	
2480 	    error_propagate(errp, err);
2481 	}
2482 	
2483 	static void visit_type_PciBridgeInfo_fields(Visitor *m, PciBridgeInfo ** obj, Error **errp)
2484 	{
2485 	    Error *err = NULL;

(1) Event cond_true: 	Condition "!error_is_set(errp)", taking true branch

2486 	    if (!error_is_set(errp)) {
2487 	        Error **errp = &err; /* from outer scope */
2488 	        Error *err = NULL;
2489 	        visit_start_struct(m, NULL, "", "bus", 0, &err);

(2) Event cond_true: 	Condition "!err", taking true branch

2490 	        if (!err) {

(3) Event cond_false: 	Condition "!obj", taking false branch
(4) Event cond_false: 	Condition "*obj", taking false branch
(6) Event var_compare_op: 	Comparing "*obj" to null implies that "*obj" might be null.
Also see events: 	[var_deref_op]

2491 	            if (!obj || *obj) {
2492 	                visit_type_PciBridgeInfo_bus_fields(m, obj, &err);
2493 	                error_propagate(errp, err);
2494 	                err = NULL;

(5) Event if_end: 	End of if statement

2495 	            }
2496 	            /* Always call end_struct if start_struct succeeded.  */
2497 	            visit_end_struct(m, &err);
2498 	        }
2499 	        error_propagate(errp, err);
2500 	    }

(7) Event cond_true: 	Condition "obj", taking true branch

2501 	    visit_start_optional(m, obj ? &(*obj)->has_devices : NULL, "devices", &err);

(8) Event cond_true: 	Condition "obj", taking true branch
(9) Event var_deref_op: 	Dereferencing null pointer "*obj".
Also see events: 	[var_compare_op]

2502 	    if (obj && (*obj)->has_devices) {
2503 	        visit_type_PciDeviceInfoList(m, obj ? &(*obj)->devices : NULL, "devices", &err);
2504 	    }
2505 	    visit_end_optional(m, &err);
2506 	
2507 	    error_propagate(errp, err);
2508 	}

>
> No objections to patch 1-9.

What does it take make you accept PATCH 10?

[...]

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Paolo Bonzini]

Il 07/02/2014 15:23, Markus Armbruster ha scritto:
>> > No objections to patch 1-9.
> What does it take make you accept PATCH 10?

The email you just wrote, basically, :) and removing the NULL pointer 
checks in the visitor implementations.

Paolo

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 06/02/2014 15:30, Markus Armbruster ha scritto:
>>> Visitors get passed a pointer to the visited object.  The generated
>>> visitors try to cope with this pointer being null in some places, for
>>> instance like this:
>>>
>>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>>>
>>> visit_start_optional() passes its second argument to Visitor method
>>> start_optional.  Two out of two methods dereference it
>>> unconditionally.
>>
>> Some visitor implementations however do not implement start_optional
>> at all.  With these visitor implementations, you currently could pass
>> a NULL object.  After your patch, you still can but you're passing a
>> bad pointer which is also a problem (perhaps one that Coverity would
>> also detect).
>
> We need to decide what the contract for the public visit_type_FOO() and
> visit_type_FOOlist() is.
>
> No existing user wants to pass a null pointer, the semantics of passing
> a null pointer are not obvious to me, and as we'll see below, the
> generated code isn't entirely successful in avoiding to dereference a
> null argument :)
>
>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>> index ff4239c..3eb10c8 100644
>>> --- a/scripts/qapi-visit.py
>>> +++ b/scripts/qapi-visit.py
>>> @@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
>>>
>>>      if base:
>>>          ret += mcgen('''
>>> -visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
>>> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
>>
>> This is the implementation of start_implicit_struct:
>
> One of two implementations.
>
>> static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
>>                                             size_t size, Error **errp)
>> {
>>     if (obj) {
>>         *obj = g_malloc0(size);
>>     }
>> }
>>
>> Before your patch, if obj is NULL, *obj is not written.
>>
>> After your patch, if obj is NULL, and c_name is not the first field in
>> the struct, *obj is written and you get a NULL pointer
>> dereference. Same for end_implicit_struct in
>> qapi/qapi-dealloc-visitor.c.
>>
>> So I think if you remove this checking, you need to do the same in the
>> visitor implementations as well.
>
> Can do.

I'd like to keep this null check.  Let me explain why.

The start_struct() callback gets called in two separate ways.

1. Boxed struct: argument is a struct **.

2. Unboxed struct: argument is null.

Example: UserDefTwo from tests/qapi-schema/qapi-schema-test.json

Schema:

    { 'type': 'UserDefTwo',
      'data': { 'string': 'str',
                'dict': { 'string': 'str',
                          ... } } }

Generated type:

    struct UserDefTwo
    {
        char * string;
        struct 
        {
            char * string;
            ...
        } dict;
    };

The generated visit_type_UserDefTwo() takes a struct UserDefOne **
argument, which can't sensibly be null, as discussed earlier in this
thread.

It passes that argument to visit_start_struct().  This is the boxed
case.

When it's time to visit UserDefTwo member dict, it calls
visit_start_struct() again, but with a null argument.  This is the
unboxed case.

Therefore, implementations of start_struct() better be prepared for a
null argument.  opts_start_struct() isn't, and I'm going to fix it.

start_implicit_struct() is not currently called with a null argument as
far as I can tell, but I'd prefer to keep it close to start_struct().

The fact that we're still committing interfaces like
include/qapi/visitor.h without spelling out at least the non-obvious
parts of the callback contracts is depressing.

[...]

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Paolo Bonzini]

Il 10/02/2014 14:29, Markus Armbruster ha scritto:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 06/02/2014 15:30, Markus Armbruster ha scritto:
>>>> Visitors get passed a pointer to the visited object.  The generated
>>>> visitors try to cope with this pointer being null in some places, for
>>>> instance like this:
>>>>
>>>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>>>>
>>>> visit_start_optional() passes its second argument to Visitor method
>>>> start_optional.  Two out of two methods dereference it
>>>> unconditionally.
>>>
>>> Some visitor implementations however do not implement start_optional
>>> at all.  With these visitor implementations, you currently could pass
>>> a NULL object.  After your patch, you still can but you're passing a
>>> bad pointer which is also a problem (perhaps one that Coverity would
>>> also detect).
>>
>> We need to decide what the contract for the public visit_type_FOO() and
>> visit_type_FOOlist() is.
>>
>> No existing user wants to pass a null pointer, the semantics of passing
>> a null pointer are not obvious to me, and as we'll see below, the
>> generated code isn't entirely successful in avoiding to dereference a
>> null argument :)
>>
>>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>>> index ff4239c..3eb10c8 100644
>>>> --- a/scripts/qapi-visit.py
>>>> +++ b/scripts/qapi-visit.py
>>>> @@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
>>>>
>>>>      if base:
>>>>          ret += mcgen('''
>>>> -visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
>>>> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
>>>
>>> This is the implementation of start_implicit_struct:
>>
>> One of two implementations.
>>
>>> static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
>>>                                             size_t size, Error **errp)
>>> {
>>>     if (obj) {
>>>         *obj = g_malloc0(size);
>>>     }
>>> }
>>>
>>> Before your patch, if obj is NULL, *obj is not written.
>>>
>>> After your patch, if obj is NULL, and c_name is not the first field in
>>> the struct, *obj is written and you get a NULL pointer
>>> dereference. Same for end_implicit_struct in
>>> qapi/qapi-dealloc-visitor.c.
>>>
>>> So I think if you remove this checking, you need to do the same in the
>>> visitor implementations as well.
>>
>> Can do.
>
> I'd like to keep this null check.  Let me explain why.

Patch 10 is okay then!

We really should write down all of this.  Thanks for spelling it down 
for us! :(

Paolo

> The start_struct() callback gets called in two separate ways.
>
> 1. Boxed struct: argument is a struct **.
>
> 2. Unboxed struct: argument is null.
>
> Example: UserDefTwo from tests/qapi-schema/qapi-schema-test.json
>
> Schema:
>
>     { 'type': 'UserDefTwo',
>       'data': { 'string': 'str',
>                 'dict': { 'string': 'str',
>                           ... } } }
>
> Generated type:
>
>     struct UserDefTwo
>     {
>         char * string;
>         struct
>         {
>             char * string;
>             ...
>         } dict;
>     };
>
> The generated visit_type_UserDefTwo() takes a struct UserDefOne **
> argument, which can't sensibly be null, as discussed earlier in this
> thread.
>
> It passes that argument to visit_start_struct().  This is the boxed
> case.
>
> When it's time to visit UserDefTwo member dict, it calls
> visit_start_struct() again, but with a null argument.  This is the
> unboxed case.
>
> Therefore, implementations of start_struct() better be prepared for a
> null argument.  opts_start_struct() isn't, and I'm going to fix it.
>
> start_implicit_struct() is not currently called with a null argument as
> far as I can tell, but I'd prefer to keep it close to start_struct().
>
> The fact that we're still committing interfaces like
> include/qapi/visitor.h without spelling out at least the non-obvious
> parts of the callback contracts is depressing.
>
> [...]
>

Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/02/2014 14:29, Markus Armbruster ha scritto:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>> Il 06/02/2014 15:30, Markus Armbruster ha scritto:
>>>>> Visitors get passed a pointer to the visited object.  The generated
>>>>> visitors try to cope with this pointer being null in some places, for
>>>>> instance like this:
>>>>>
>>>>>     visit_start_optional(m, obj ? &(*obj)->has_name : NULL, "name", &err);
>>>>>
>>>>> visit_start_optional() passes its second argument to Visitor method
>>>>> start_optional.  Two out of two methods dereference it
>>>>> unconditionally.
>>>>
>>>> Some visitor implementations however do not implement start_optional
>>>> at all.  With these visitor implementations, you currently could pass
>>>> a NULL object.  After your patch, you still can but you're passing a
>>>> bad pointer which is also a problem (perhaps one that Coverity would
>>>> also detect).
>>>
>>> We need to decide what the contract for the public visit_type_FOO() and
>>> visit_type_FOOlist() is.
>>>
>>> No existing user wants to pass a null pointer, the semantics of passing
>>> a null pointer are not obvious to me, and as we'll see below, the
>>> generated code isn't entirely successful in avoiding to dereference a
>>> null argument :)
>>>
>>>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>>>> index ff4239c..3eb10c8 100644
>>>>> --- a/scripts/qapi-visit.py
>>>>> +++ b/scripts/qapi-visit.py
>>>>> @@ -47,9 +47,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
>>>>>
>>>>>      if base:
>>>>>          ret += mcgen('''
>>>>> -visit_start_implicit_struct(m, obj ? (void**) &(*obj)->%(c_name)s : NULL, sizeof(%(type)s), &err);
>>>>> +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
>>>>
>>>> This is the implementation of start_implicit_struct:
>>>
>>> One of two implementations.
>>>
>>>> static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
>>>>                                             size_t size, Error **errp)
>>>> {
>>>>     if (obj) {
>>>>         *obj = g_malloc0(size);
>>>>     }
>>>> }
>>>>
>>>> Before your patch, if obj is NULL, *obj is not written.
>>>>
>>>> After your patch, if obj is NULL, and c_name is not the first field in
>>>> the struct, *obj is written and you get a NULL pointer
>>>> dereference. Same for end_implicit_struct in
>>>> qapi/qapi-dealloc-visitor.c.
>>>>
>>>> So I think if you remove this checking, you need to do the same in the
>>>> visitor implementations as well.
>>>
>>> Can do.
>>
>> I'd like to keep this null check.  Let me explain why.
>
> Patch 10 is okay then!

Thanks!

> We really should write down all of this.  Thanks for spelling it down
> for us! :(

Yes, we should, and we should write it down before we commit the code!
Not two years later, when the original author has forgotten everything,
or has been run over by a bus[*], so the poor sod who needs to mess with
it gets to figure it all out, and gets his chance to claim his place in
git history as fabricator of "not quite correct, but we appreciate the
effort" documentation.

[...]

>> The fact that we're still committing interfaces like
>> include/qapi/visitor.h without spelling out at least the non-obvious
>> parts of the callback contracts is depressing.
>>
>> [...]
>>


[*] Or sucked into the guts of some corporation.  Same result for us,
but probably much preferred by the author.