* [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:09 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 02/10] tests/qapi-schema: Cover optional command arguments Markus Armbruster
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response
2014-02-06 14:29 ` [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response Markus Armbruster
@ 2014-02-07 2:09 ` Eric Blake
2014-02-07 7:37 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2014-02-07 2:09 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, mdroth, aliguori
[-- 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 --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response
2014-02-07 2:09 ` Eric Blake
@ 2014-02-07 7:37 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2014-02-07 7:37 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, aliguori, mdroth
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!
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 02/10] tests/qapi-schema: Cover optional command arguments
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
2014-02-06 14:29 ` [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:30 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 03/10] tests/qapi-schema: Cover simple argument types Markus Armbruster
` (7 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 03/10] tests/qapi-schema: Cover simple argument types
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
2014-02-06 14:29 ` [Qemu-devel] [PATCH 01/10] tests/qapi-schema: Actually check successful QMP command response Markus Armbruster
2014-02-06 14:29 ` [Qemu-devel] [PATCH 02/10] tests/qapi-schema: Cover optional command arguments Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:32 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 04/10] tests/qapi-schema: Cover anonymous union types Markus Armbruster
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 04/10] tests/qapi-schema: Cover anonymous union types
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (2 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 03/10] tests/qapi-schema: Cover simple argument types Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:35 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 05/10] tests/qapi-schema: Cover complex types with base Markus Armbruster
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 05/10] tests/qapi-schema: Cover complex types with base
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (3 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 04/10] tests/qapi-schema: Cover anonymous union types Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:38 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union " Markus Armbruster
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union types with base
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (4 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 05/10] tests/qapi-schema: Cover complex types with base Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:40 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 07/10] tests/qapi-schema: Cover flat union types Markus Armbruster
` (3 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union types with base
2014-02-06 14:29 ` [Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union " Markus Armbruster
@ 2014-02-07 2:40 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2014-02-07 2:40 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, mdroth, aliguori
[-- 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 --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 07/10] tests/qapi-schema: Cover flat union types
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (5 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 06/10] tests/qapi-schema: Cover union " Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:51 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c Markus Armbruster
` (2 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (6 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 07/10] tests/qapi-schema: Cover flat union types Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:52 ` Eric Blake
2014-02-06 14:29 ` [Qemu-devel] [PATCH 09/10] qapi: Drop unused code in qapi-commands.py Markus Armbruster
2014-02-06 14:30 ` [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors Markus Armbruster
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c
2014-02-06 14:29 ` [Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c Markus Armbruster
@ 2014-02-07 2:52 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2014-02-07 2:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, mdroth, aliguori
[-- 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 --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 09/10] qapi: Drop unused code in qapi-commands.py
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (7 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 08/10] qapi: Drop nonsensical header guard in generated qapi-visit.c Markus Armbruster
@ 2014-02-06 14:29 ` Markus Armbruster
2014-02-07 2:56 ` Eric Blake
2014-02-06 14:30 ` [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors Markus Armbruster
9 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-06 14:29 [Qemu-devel] [PATCH 00/10] qapi: Test coverage & clean up generated code Markus Armbruster
` (8 preceding siblings ...)
2014-02-06 14:29 ` [Qemu-devel] [PATCH 09/10] qapi: Drop unused code in qapi-commands.py Markus Armbruster
@ 2014-02-06 14:30 ` Markus Armbruster
2014-02-07 3:10 ` Eric Blake
2014-02-07 12:42 ` Paolo Bonzini
9 siblings, 2 replies; 29+ messages in thread
From: Markus Armbruster @ 2014-02-06 14:30 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, aliguori
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-06 14:30 ` [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors Markus Armbruster
@ 2014-02-07 3:10 ` Eric Blake
2014-02-07 7:34 ` Markus Armbruster
2014-02-07 12:42 ` Paolo Bonzini
1 sibling, 1 reply; 29+ messages in thread
From: Eric Blake @ 2014-02-07 3:10 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, mdroth, aliguori
[-- 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 --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-07 3:10 ` Eric Blake
@ 2014-02-07 7:34 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2014-02-07 7:34 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, aliguori, mdroth
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>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-06 14:30 ` [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors Markus Armbruster
2014-02-07 3:10 ` Eric Blake
@ 2014-02-07 12:42 ` Paolo Bonzini
2014-02-07 14:23 ` Markus Armbruster
1 sibling, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2014-02-07 12:42 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, mdroth, aliguori
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)
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-07 12:42 ` Paolo Bonzini
@ 2014-02-07 14:23 ` Markus Armbruster
2014-02-07 14:45 ` Paolo Bonzini
2014-02-10 13:29 ` Markus Armbruster
0 siblings, 2 replies; 29+ messages in thread
From: Markus Armbruster @ 2014-02-07 14:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, aliguori, mdroth
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?
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-07 14:23 ` Markus Armbruster
@ 2014-02-07 14:45 ` Paolo Bonzini
2014-02-10 13:29 ` Markus Armbruster
1 sibling, 0 replies; 29+ messages in thread
From: Paolo Bonzini @ 2014-02-07 14:45 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, aliguori, mdroth
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-07 14:23 ` Markus Armbruster
2014-02-07 14:45 ` Paolo Bonzini
@ 2014-02-10 13:29 ` Markus Armbruster
2014-02-11 9:06 ` Paolo Bonzini
1 sibling, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2014-02-10 13:29 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, aliguori, mdroth
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.
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-10 13:29 ` Markus Armbruster
@ 2014-02-11 9:06 ` Paolo Bonzini
2014-02-11 12:35 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: Paolo Bonzini @ 2014-02-11 9:06 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, aliguori, mdroth
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.
>
> [...]
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 10/10] qapi: Clean up null checking in generated visitors
2014-02-11 9:06 ` Paolo Bonzini
@ 2014-02-11 12:35 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2014-02-11 12:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel, aliguori, mdroth
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.
^ permalink raw reply [flat|nested] 29+ messages in thread