* [Qemu-devel] [PATCH v11 01/15] qapi: Simplify excess input reporting in input visitors
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 02/15] qapi: Forbid empty unions and useless alternates Eric Blake
` (15 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
When reporting that an unvisited member remains at the end of an
input visit for a struct, we were using g_hash_table_find()
coupled with a callback function that always returns true, to
locate an arbitrary member of the hash table. But if all we
need is an arbitrary entry, we can get that from a single-use
iterator, without needing a tautological callback function.
Technically, our cast of &(GQueue *) to (void **) is not strict
C (while void * must be able to hold all other pointers, nothing
says a void ** has to be the same width or representation as a
GQueue **). The kosher way to write it would be the verbose:
void *tmp;
GQueue *any;
if (g_hash_table_iter_next(&iter, NULL, &tmp)) {
any = tmp;
But our code base (not to mention glib itself) already has other
cases of assuming that ALL pointers have the same width and
representation, where a compiler would have to go out of its way
to mis-compile our borderline behavior.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
v11: no change
v10: enhance commit message
v9: no change
v8: rebase to earlier changes
v7: retitle, rebase to earlier context changes
v6: new patch, based on comments on RFC against v5 7/46
---
qapi/opts-visitor.c | 12 +++---------
qapi/qmp-input-visitor.c | 14 +++++---------
2 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index d54f75b..ae5b955 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -157,17 +157,11 @@ opts_start_struct(Visitor *v, const char *name, void **obj,
}
-static gboolean
-ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
-{
- return TRUE;
-}
-
-
static void
opts_end_struct(Visitor *v, Error **errp)
{
OptsVisitor *ov = to_ov(v);
+ GHashTableIter iter;
GQueue *any;
if (--ov->depth > 0) {
@@ -175,8 +169,8 @@ opts_end_struct(Visitor *v, Error **errp)
}
/* we should have processed all (distinct) QemuOpt instances */
- any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
- if (any) {
+ g_hash_table_iter_init(&iter, ov->unprocessed_opts);
+ if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) {
const QemuOpt *first;
first = g_queue_peek_head(any);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 362a1a3..2f48b95 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -90,12 +90,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
qiv->nb_stack++;
}
-/** Only for qmp_input_pop. */
-static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
-{
- *(const char **)user_pkey = (const char *)key;
- return TRUE;
-}
static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
{
@@ -104,9 +98,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
if (qiv->strict) {
GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
if (top_ht) {
- if (g_hash_table_size(top_ht)) {
- const char *key;
- g_hash_table_find(top_ht, always_true, &key);
+ GHashTableIter iter;
+ const char *key;
+
+ g_hash_table_iter_init(&iter, top_ht);
+ if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
}
g_hash_table_unref(top_ht);
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v11 02/15] qapi: Forbid empty unions and useless alternates
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 01/15] qapi: Simplify excess input reporting in input visitors Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate Eric Blake
` (14 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Empty unions serve no purpose, and while we compile with gcc
which permits them, strict C99 forbids them. We happen to inject
a dummy 'void *data' member into the C unions that represent QAPI
unions and alternates, but we want to get rid of that member (it
pollutes the namespace for no good reason), which would leave us
with an empty union if the user didn't provide any branches. While
empty structs make sense in QAPI, empty unions don't add any
expressiveness to the QMP language. So prohibit them at parse
time. Update the documentation and testsuite to match.
Note that the documentation already mentioned that alternates
should have "two or more JSON data types"; so this also fixes
the code to enforce that. However, we have existing uses of a
union type with only one branch, so the 2-or-more strictness
is intentionally limited to alternates.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: improve commit message
v10: hoist into earlier series
[no v7, v8, or v9]
v6: rebase to earlier qapi.py cleanups
---
scripts/qapi.py | 12 ++++++++++--
docs/qapi-code-gen.txt | 15 ++++++++-------
tests/qapi-schema/alternate-empty.err | 1 +
tests/qapi-schema/alternate-empty.exit | 2 +-
tests/qapi-schema/alternate-empty.json | 2 +-
tests/qapi-schema/alternate-empty.out | 5 -----
tests/qapi-schema/flat-union-empty.err | 1 +
tests/qapi-schema/flat-union-empty.exit | 2 +-
tests/qapi-schema/flat-union-empty.json | 2 +-
tests/qapi-schema/flat-union-empty.out | 9 ---------
tests/qapi-schema/union-empty.err | 1 +
tests/qapi-schema/union-empty.exit | 2 +-
tests/qapi-schema/union-empty.json | 2 +-
tests/qapi-schema/union-empty.out | 6 ------
14 files changed, 27 insertions(+), 35 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f40dc9e..f97236f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -590,7 +590,10 @@ def check_union(expr, expr_info):
"Discriminator '%s' must be of enumeration "
"type" % discriminator)
- # Check every branch
+ # Check every branch; don't allow an empty union
+ if len(members) == 0:
+ raise QAPIExprError(expr_info,
+ "Union '%s' cannot have empty 'data'" % name)
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
@@ -613,7 +616,11 @@ def check_alternate(expr, expr_info):
members = expr['data']
types_seen = {}
- # Check every branch
+ # Check every branch; require at least two branches
+ if len(members) < 2:
+ raise QAPIExprError(expr_info,
+ "Alternate '%s' should have at least two branches "
+ "in 'data'" % name)
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
@@ -1059,6 +1066,7 @@ class QAPISchemaObjectTypeVariants(object):
assert bool(tag_member) != bool(tag_name)
assert (isinstance(tag_name, str) or
isinstance(tag_member, QAPISchemaObjectTypeMember))
+ assert len(variants) > 0
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self.tag_name = tag_name
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 128f074..999f3b9 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -187,11 +187,11 @@ prevent incomplete include files.
Usage: { 'struct': STRING, 'data': DICT, '*base': STRUCT-NAME }
-A struct is a dictionary containing a single 'data' key whose
-value is a dictionary. This corresponds to a struct in C or an Object
-in JSON. Each value of the 'data' dictionary must be the name of a
-type, or a one-element array containing a type name. An example of a
-struct is:
+A struct is a dictionary containing a single 'data' key whose value is
+a dictionary; the dictionary may be empty. This corresponds to a
+struct in C or an Object in JSON. Each value of the 'data' dictionary
+must be the name of a type, or a one-element array containing a type
+name. An example of a struct is:
{ 'struct': 'MyType',
'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } }
@@ -288,9 +288,10 @@ or: { 'union': STRING, 'data': DICT, 'base': STRUCT-NAME,
Union types are used to let the user choose between several different
variants for an object. There are two flavors: simple (no
-discriminator or base), flat (both discriminator and base). A union
+discriminator or base), and flat (both discriminator and base). A union
type is defined using a data dictionary as explained in the following
-paragraphs.
+paragraphs. The data dictionary for either type of union must not
+be empty.
A simple union type defines a mapping from automatic discriminator
values to data types like in this example:
diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err
index e69de29..bb06c5b 100644
--- a/tests/qapi-schema/alternate-empty.err
+++ b/tests/qapi-schema/alternate-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least two branches in 'data'
diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/alternate-empty.exit
+++ b/tests/qapi-schema/alternate-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json
index db3820f..fff15ba 100644
--- a/tests/qapi-schema/alternate-empty.json
+++ b/tests/qapi-schema/alternate-empty.json
@@ -1,2 +1,2 @@
-# FIXME - alternates should list at least two types to be useful
+# alternates must list at least two types to be useful
{ 'alternate': 'Alt', 'data': { 'i': 'int' } }
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
index f78f174..e69de29 100644
--- a/tests/qapi-schema/alternate-empty.out
+++ b/tests/qapi-schema/alternate-empty.out
@@ -1,5 +0,0 @@
-object :empty
-alternate Alt
- case i: int
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
- prefix QTYPE
diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err
index e69de29..15754f5 100644
--- a/tests/qapi-schema/flat-union-empty.err
+++ b/tests/qapi-schema/flat-union-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-empty.json:4: Union 'Union' cannot have empty 'data'
diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-empty.exit
+++ b/tests/qapi-schema/flat-union-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json
index 67dd297..77f1d9a 100644
--- a/tests/qapi-schema/flat-union-empty.json
+++ b/tests/qapi-schema/flat-union-empty.json
@@ -1,4 +1,4 @@
-# FIXME - flat unions should not be empty
+# flat unions cannot be empty
{ 'enum': 'Empty', 'data': [ ] }
{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
index eade2d5..e69de29 100644
--- a/tests/qapi-schema/flat-union-empty.out
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -1,9 +0,0 @@
-object :empty
-object Base
- member type: Empty optional=False
-enum Empty []
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
- prefix QTYPE
-object Union
- base Base
- tag type
diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err
index e69de29..12c2022 100644
--- a/tests/qapi-schema/union-empty.err
+++ b/tests/qapi-schema/union-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-empty.json:2: Union 'Union' cannot have empty 'data'
diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/union-empty.exit
+++ b/tests/qapi-schema/union-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json
index 1785007..1f0b13c 100644
--- a/tests/qapi-schema/union-empty.json
+++ b/tests/qapi-schema/union-empty.json
@@ -1,2 +1,2 @@
-# FIXME - unions should not be empty
+# unions cannot be empty
{ 'union': 'Union', 'data': { } }
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
index bdf17e5..e69de29 100644
--- a/tests/qapi-schema/union-empty.out
+++ b/tests/qapi-schema/union-empty.out
@@ -1,6 +0,0 @@
-object :empty
-enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
- prefix QTYPE
-object Union
- member type: UnionKind optional=False
-enum UnionKind []
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 01/15] qapi: Simplify excess input reporting in input visitors Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 02/15] qapi: Forbid empty unions and useless alternates Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 12:05 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 04/15] qapi: Add tests of complex objects within alternate Eric Blake
` (13 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The whole point of an alternate is to allow some type-safety while
still accepting more than one JSON type. Meanwhile, the 'any'
type exists to bypass type-safety altogether. The two are
incompatible: you can't accept every type, and still tell which
branch of the alternate to use for the parse; fix this to give a
sane error instead of a Python stack trace.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: new patch
---
scripts/qapi.py | 5 ++++-
tests/Makefile | 1 +
tests/qapi-schema/alternate-any.err | 1 +
tests/qapi-schema/alternate-any.exit | 1 +
tests/qapi-schema/alternate-any.json | 4 ++++
tests/qapi-schema/alternate-any.out | 0
6 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/alternate-any.err
create mode 100644 tests/qapi-schema/alternate-any.exit
create mode 100644 tests/qapi-schema/alternate-any.json
create mode 100644 tests/qapi-schema/alternate-any.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f97236f..17bf633 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -629,7 +629,10 @@ def check_alternate(expr, expr_info):
value,
allow_metas=['built-in', 'union', 'struct', 'enum'])
qtype = find_alternate_member_qtype(value)
- assert qtype
+ if not qtype:
+ raise QAPIExprError(expr_info,
+ "Alternate '%s' member '%s' cannot use "
+ "type '%s'" % (name, key, value))
if qtype in types_seen:
raise QAPIExprError(expr_info,
"Alternate '%s' member '%s' can't "
diff --git a/tests/Makefile b/tests/Makefile
index c1c605f..7c66d16 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -241,6 +241,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
check-qtest-generic-y += tests/qom-test$(EXESUF)
+qapi-schema += alternate-any.json
qapi-schema += alternate-array.json
qapi-schema += alternate-base.json
qapi-schema += alternate-clash.json
diff --git a/tests/qapi-schema/alternate-any.err b/tests/qapi-schema/alternate-any.err
new file mode 100644
index 0000000..aaa0154
--- /dev/null
+++ b/tests/qapi-schema/alternate-any.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-any.json:2: Alternate 'Alt' member 'one' cannot use type 'any'
diff --git a/tests/qapi-schema/alternate-any.exit b/tests/qapi-schema/alternate-any.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/alternate-any.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-any.json b/tests/qapi-schema/alternate-any.json
new file mode 100644
index 0000000..e47a73a
--- /dev/null
+++ b/tests/qapi-schema/alternate-any.json
@@ -0,0 +1,4 @@
+# we do not allow the 'any' type as an alternate branch
+{ 'alternate': 'Alt',
+ 'data': { 'one': 'any',
+ 'two': 'int' } }
diff --git a/tests/qapi-schema/alternate-any.out b/tests/qapi-schema/alternate-any.out
new file mode 100644
index 0000000..e69de29
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate Eric Blake
@ 2016-02-18 12:05 ` Markus Armbruster
2016-02-18 14:40 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 12:05 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> The whole point of an alternate is to allow some type-safety while
> still accepting more than one JSON type. Meanwhile, the 'any'
> type exists to bypass type-safety altogether. The two are
> incompatible: you can't accept every type, and still tell which
> branch of the alternate to use for the parse; fix this to give a
> sane error instead of a Python stack trace.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: new patch
> ---
> scripts/qapi.py | 5 ++++-
> tests/Makefile | 1 +
> tests/qapi-schema/alternate-any.err | 1 +
> tests/qapi-schema/alternate-any.exit | 1 +
> tests/qapi-schema/alternate-any.json | 4 ++++
> tests/qapi-schema/alternate-any.out | 0
> 6 files changed, 11 insertions(+), 1 deletion(-)
> create mode 100644 tests/qapi-schema/alternate-any.err
> create mode 100644 tests/qapi-schema/alternate-any.exit
> create mode 100644 tests/qapi-schema/alternate-any.json
> create mode 100644 tests/qapi-schema/alternate-any.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f97236f..17bf633 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info):
> value,
> allow_metas=['built-in', 'union', 'struct', 'enum'])
> qtype = find_alternate_member_qtype(value)
> - assert qtype
> + if not qtype:
> + raise QAPIExprError(expr_info,
> + "Alternate '%s' member '%s' cannot use "
> + "type '%s'" % (name, key, value))
> if qtype in types_seen:
> raise QAPIExprError(expr_info,
> "Alternate '%s' member '%s' can't "
Interestingly, find_alternate_member_qtype(T) can return None in two
ways: when builtin_types[T] is None (only for T == 'any'), and when T is
neither built-in, struct, enum or union (it must be alternate then).
This leads to the question whether this patch catches exactly 'any', as
the commit message claims, or alternate as well.
> diff --git a/tests/Makefile b/tests/Makefile
> index c1c605f..7c66d16 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -241,6 +241,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>
> check-qtest-generic-y += tests/qom-test$(EXESUF)
>
> +qapi-schema += alternate-any.json
> qapi-schema += alternate-array.json
> qapi-schema += alternate-base.json
> qapi-schema += alternate-clash.json
> diff --git a/tests/qapi-schema/alternate-any.err b/tests/qapi-schema/alternate-any.err
> new file mode 100644
> index 0000000..aaa0154
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-any.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/alternate-any.json:2: Alternate 'Alt' member 'one' cannot use type 'any'
> diff --git a/tests/qapi-schema/alternate-any.exit b/tests/qapi-schema/alternate-any.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-any.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/alternate-any.json b/tests/qapi-schema/alternate-any.json
> new file mode 100644
> index 0000000..e47a73a
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-any.json
> @@ -0,0 +1,4 @@
> +# we do not allow the 'any' type as an alternate branch
> +{ 'alternate': 'Alt',
> + 'data': { 'one': 'any',
> + 'two': 'int' } }
> diff --git a/tests/qapi-schema/alternate-any.out b/tests/qapi-schema/alternate-any.out
> new file mode 100644
> index 0000000..e69de29
Could use a test for alternate member of alternate type.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate
2016-02-18 12:05 ` Markus Armbruster
@ 2016-02-18 14:40 ` Eric Blake
2016-02-18 17:03 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 14:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]
On 02/18/2016 05:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> The whole point of an alternate is to allow some type-safety while
>> still accepting more than one JSON type. Meanwhile, the 'any'
>> type exists to bypass type-safety altogether. The two are
>> incompatible: you can't accept every type, and still tell which
>> branch of the alternate to use for the parse; fix this to give a
>> sane error instead of a Python stack trace.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>
> Interestingly, find_alternate_member_qtype(T) can return None in two
> ways: when builtin_types[T] is None (only for T == 'any'), and when T is
> neither built-in, struct, enum or union (it must be alternate then).
>
> This leads to the question whether this patch catches exactly 'any', as
> the commit message claims, or alternate as well.
>
>>
>> @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info):
>> value,
>> allow_metas=['built-in', 'union', 'struct', 'enum'])
>> qtype = find_alternate_member_qtype(value)
>
>
> Could use a test for alternate member of alternate type.
One step ahead of you: commit 3d0c4829 added the test
alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the
parser to reject it (first by a hard-coded check, then via allow_metas[]
excluding alternates). 'any' is the only value that could sneak
through, because it is a subset of 'built-in' which allow_metas[]
whitelisted.
If you want to squash any of this information into the commit message,
though, I don't mind.
--
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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate
2016-02-18 14:40 ` Eric Blake
@ 2016-02-18 17:03 ` Markus Armbruster
2016-02-18 20:11 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 17:03 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/18/2016 05:05 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> The whole point of an alternate is to allow some type-safety while
>>> still accepting more than one JSON type. Meanwhile, the 'any'
>>> type exists to bypass type-safety altogether. The two are
>>> incompatible: you can't accept every type, and still tell which
>>> branch of the alternate to use for the parse; fix this to give a
>>> sane error instead of a Python stack trace.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>
>>
>> Interestingly, find_alternate_member_qtype(T) can return None in two
>> ways: when builtin_types[T] is None (only for T == 'any'), and when T is
>> neither built-in, struct, enum or union (it must be alternate then).
>>
>> This leads to the question whether this patch catches exactly 'any', as
>> the commit message claims, or alternate as well.
>>
>
>>>
>>> @@ -629,7 +629,10 @@ def check_alternate(expr, expr_info):
>>> value,
>>> allow_metas=['built-in', 'union', 'struct', 'enum'])
>>> qtype = find_alternate_member_qtype(value)
>>
>
>>
>> Could use a test for alternate member of alternate type.
>
> One step ahead of you: commit 3d0c4829 added the test
> alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the
> parser to reject it (first by a hard-coded check, then via allow_metas[]
> excluding alternates). 'any' is the only value that could sneak
> through, because it is a subset of 'built-in' which allow_metas[]
> whitelisted.
Then find_alternate_member_qtype()'s final return None is unreachable,
correct?
> If you want to squash any of this information into the commit message,
> though, I don't mind.
I'll consider it when I apply.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate
2016-02-18 17:03 ` Markus Armbruster
@ 2016-02-18 20:11 ` Eric Blake
2016-02-19 8:32 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 20:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]
On 02/18/2016 10:03 AM, Markus Armbruster wrote:
>>> Could use a test for alternate member of alternate type.
>>
>> One step ahead of you: commit 3d0c4829 added the test
>> alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the
>> parser to reject it (first by a hard-coded check, then via allow_metas[]
>> excluding alternates). 'any' is the only value that could sneak
>> through, because it is a subset of 'built-in' which allow_metas[]
>> whitelisted.
>
> Then find_alternate_member_qtype()'s final return None is unreachable,
> correct?
Indeed, the testsuite still passes with:
diff --git i/scripts/qapi.py w/scripts/qapi.py
index 8497777..81d435f 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -345,7 +345,7 @@ def find_alternate_member_qtype(qapi_type):
return "QTYPE_QSTRING"
elif find_union(qapi_type):
return "QTYPE_QDICT"
- return None
+ assert False
# Return the discriminator enum define if discriminator is specified as an
That said, even though we currently filter out unknown types before
deciding to call find_alternate_member_qtype, it's not out of the
question that future work to move ad hoc front-end tests into formal
QAPISchema .check() methods may cause us to call
find_alternate_member_qtype('unknown'). Leaving it as return None
instead of asserting would make the error message added in this patch
nicer. Then again, refactoring would move the error message of this
patch to the .check() methods. So I won't worry about it for now.
--
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 related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate
2016-02-18 20:11 ` Eric Blake
@ 2016-02-19 8:32 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-19 8:32 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/18/2016 10:03 AM, Markus Armbruster wrote:
>
>>>> Could use a test for alternate member of alternate type.
>>>
>>> One step ahead of you: commit 3d0c4829 added the test
>>> alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the
>>> parser to reject it (first by a hard-coded check, then via allow_metas[]
>>> excluding alternates). 'any' is the only value that could sneak
>>> through, because it is a subset of 'built-in' which allow_metas[]
>>> whitelisted.
>>
>> Then find_alternate_member_qtype()'s final return None is unreachable,
>> correct?
>
> Indeed, the testsuite still passes with:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index 8497777..81d435f 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -345,7 +345,7 @@ def find_alternate_member_qtype(qapi_type):
> return "QTYPE_QSTRING"
> elif find_union(qapi_type):
> return "QTYPE_QDICT"
> - return None
> + assert False
>
>
> # Return the discriminator enum define if discriminator is specified as an
>
>
> That said, even though we currently filter out unknown types before
> deciding to call find_alternate_member_qtype, it's not out of the
> question that future work to move ad hoc front-end tests into formal
> QAPISchema .check() methods may cause us to call
> find_alternate_member_qtype('unknown'). Leaving it as return None
> instead of asserting would make the error message added in this patch
> nicer. Then again, refactoring would move the error message of this
> patch to the .check() methods. So I won't worry about it for now.
Makes sense. I was just making sure I understand how this works.
Thanks!
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 04/15] qapi: Add tests of complex objects within alternate
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (2 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 03/15] qapi: Forbid 'any' inside an alternate Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 05/15] qapi-visit: Simplify how we visit common union members Eric Blake
` (12 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Upcoming patches will adjust how we visit an object branch of an
alternate; but we were completely lacking testsuite coverage.
Rectify this, so that the future patches will be able to highlight
the changes and still prove that we avoided regressions.
In particular, the use of a flat union UserDefFlatUnion rather
than a simple struct UserDefA as the branch will give us coverage
of an object with variants. And visiting an alternate as both
the top level and as a nested member gives confidence in correct
memory allocation handling, especially if the test is run under
valgrind.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: new patch
---
tests/test-qmp-input-visitor.c | 37 ++++++++++++++++++++++++++++++++-
tests/test-qmp-output-visitor.c | 26 ++++++++++++++++++++++-
tests/qapi-schema/qapi-schema-test.json | 4 +++-
tests/qapi-schema/qapi-schema-test.out | 4 +++-
4 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c72cdad..ef836d5 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -1,7 +1,7 @@
/*
* QMP Input Visitor unit-tests.
*
- * Copyright (C) 2011, 2015 Red Hat Inc.
+ * Copyright (C) 2011-2016 Red Hat Inc.
*
* Authors:
* Luiz Capitulino <lcapitulino@redhat.com>
@@ -309,6 +309,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
Visitor *v;
Error *err = NULL;
UserDefAlternate *tmp;
+ WrapAlternate *wrap;
v = visitor_input_test_init(data, "42");
visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
@@ -322,10 +323,44 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpstr(tmp->u.s, ==, "string");
qapi_free_UserDefAlternate(tmp);
+ v = visitor_input_test_init(data, "{'integer':1, 'string':'str', "
+ "'enum1':'value1', 'boolean':true}");
+ visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QDICT);
+ g_assert_cmpint(tmp->u.udfu->integer, ==, 1);
+ g_assert_cmpstr(tmp->u.udfu->string, ==, "str");
+ g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
+ g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true);
+ g_assert_cmpint(tmp->u.udfu->u.value1->has_a_b, ==, false);
+ qapi_free_UserDefAlternate(tmp);
+
v = visitor_input_test_init(data, "false");
visit_type_UserDefAlternate(v, NULL, &tmp, &err);
error_free_or_abort(&err);
qapi_free_UserDefAlternate(tmp);
+
+ v = visitor_input_test_init(data, "{ 'alt': 42 }");
+ visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+ g_assert_cmpint(wrap->alt->type, ==, QTYPE_QINT);
+ g_assert_cmpint(wrap->alt->u.i, ==, 42);
+ qapi_free_WrapAlternate(wrap);
+
+ v = visitor_input_test_init(data, "{ 'alt': 'string' }");
+ visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+ g_assert_cmpint(wrap->alt->type, ==, QTYPE_QSTRING);
+ g_assert_cmpstr(wrap->alt->u.s, ==, "string");
+ qapi_free_WrapAlternate(wrap);
+
+ v = visitor_input_test_init(data, "{ 'alt': {'integer':1, 'string':'str', "
+ "'enum1':'value1', 'boolean':true} }");
+ visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+ g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT);
+ g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1);
+ g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str");
+ g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
+ g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true);
+ g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false);
+ qapi_free_WrapAlternate(wrap);
}
static void test_visitor_in_alternate_number(TestInputVisitorData *data,
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 965f298..2b0f7e9 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -1,7 +1,7 @@
/*
* QMP Output Visitor unit-tests.
*
- * Copyright (C) 2011, 2015 Red Hat Inc.
+ * Copyright (C) 2011-2016 Red Hat Inc.
*
* Authors:
* Luiz Capitulino <lcapitulino@redhat.com>
@@ -427,6 +427,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
{
QObject *arg;
UserDefAlternate *tmp;
+ QDict *qdict;
tmp = g_new0(UserDefAlternate, 1);
tmp->type = QTYPE_QINT;
@@ -453,6 +454,29 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
qapi_free_UserDefAlternate(tmp);
qobject_decref(arg);
+
+ tmp = g_new0(UserDefAlternate, 1);
+ tmp->type = QTYPE_QDICT;
+ tmp->u.udfu = g_new0(UserDefFlatUnion, 1);
+ tmp->u.udfu->integer = 1;
+ tmp->u.udfu->string = g_strdup("str");
+ tmp->u.udfu->enum1 = ENUM_ONE_VALUE1;
+ tmp->u.udfu->u.value1 = g_new0(UserDefA, 1);
+ tmp->u.udfu->u.value1->boolean = true;
+
+ visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
+ arg = qmp_output_get_qobject(data->qov);
+
+ g_assert_cmpint(qobject_type(arg), ==, QTYPE_QDICT);
+ qdict = qobject_to_qdict(arg);
+ g_assert_cmpint(qdict_size(qdict), ==, 4);
+ g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 1);
+ g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
+ g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
+ g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
+
+ qapi_free_UserDefAlternate(tmp);
+ qobject_decref(arg);
}
static void test_visitor_out_empty(TestOutputVisitorData *data,
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 353a34e..632964a 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -85,8 +85,10 @@
'data': { 'value1' : 'UserDefC', # intentional forward reference
'value2' : 'UserDefB' } }
+{ 'struct': 'WrapAlternate',
+ 'data': { 'alt': 'UserDefAlternate' } }
{ 'alternate': 'UserDefAlternate',
- 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
+ 'data': { 'udfu': 'UserDefFlatUnion', 's': 'str', 'i': 'int' } }
{ 'struct': 'UserDefC',
'data': { 'string1': 'str', 'string2': 'str' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 241aadb..f5e2a73 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -105,7 +105,7 @@ object UserDefA
member boolean: bool optional=False
member a_b: int optional=True
alternate UserDefAlternate
- case uda: UserDefA
+ case udfu: UserDefFlatUnion
case s: str
case i: int
object UserDefB
@@ -172,6 +172,8 @@ object UserDefUnionBase2
member enum1: QEnumTwo optional=False
object UserDefZero
member integer: int optional=False
+object WrapAlternate
+ member alt: UserDefAlternate optional=False
event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v11 05/15] qapi-visit: Simplify how we visit common union members
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (3 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 04/15] qapi: Add tests of complex objects within alternate Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 12:16 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields() Eric Blake
` (11 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
From: Markus Armbruster <armbru@redhat.com>
For a simple union SU, gen_visit_union() generates a visit of its
single tag member, like this:
visit_type_SUKind(v, "type", &(*obj)->type, &err);
For a flat union FU with base B, it generates a visit of its base
fields:
visit_type_B_fields(v, (B **)obj, &err);
Instead, we can simply visit the common members using the same fields
visit function we use for structs, generated with
gen_visit_struct_fields(). This function visits the base if any, then
the local members.
For a simple union SU, visit_type_SU_fields() contains exactly the old
tag member visit, because there is no base, and the tag member is the
only member. For instance, the code generated for qapi-schema.json's
KeyValue changes like this:
+static void visit_type_KeyValue_fields(Visitor *v, KeyValue **obj, Error **errp)
+{
+ Error *err = NULL;
+
+ visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
+ if (err) {
+ goto out;
+ }
+
+out:
+ error_propagate(errp, err);
+}
+
void visit_type_KeyValue(Visitor *v, const char *name, KeyValue **obj, Error **errp)
{
Error *err = NULL;
@@ -4863,7 +4911,7 @@ void visit_type_KeyValue(Visitor *v, con
if (!*obj) {
goto out_obj;
}
- visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
+ visit_type_KeyValue_fields(v, obj, &err);
if (err) {
goto out_obj;
}
For a flat union FU, visit_type_FU_fields() contains exactly the old
base fields visit, because there is a base, but no members. For
instance, the code generated for qapi-schema.json's CpuInfo changes
like this:
static void visit_type_CpuInfoBase_fields(Visitor *v, CpuInfoBase **obj, Error **errp);
+static void visit_type_CpuInfo_fields(Visitor *v, CpuInfo **obj, Error **errp)
+{
+ Error *err = NULL;
+
+ visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
+ if (err) {
+ goto out;
+ }
+
+out:
+ error_propagate(errp, err);
+}
+
static void visit_type_CpuInfoX86_fields(Visitor *v, CpuInfoX86 **obj, Error **errp)
...
@@ -3485,7 +3509,7 @@ void visit_type_CpuInfo(Visitor *v, cons
if (!*obj) {
goto out_obj;
}
- visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
+ visit_type_CpuInfo_fields(v, obj, &err);
if (err) {
goto out_obj;
}
As you see, the generated code grows a bit, but in practice, it's lost
in the noise: qapi-schema.json's qapi-visit.c gains roughly 1%.
This simplification became possible with commit 441cbac "qapi-visit:
Convert to QAPISchemaVisitor, fixing bugs". It's a step towards
unifying gen_struct() and gen_union().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1453902888-20457-2-git-send-email-armbru@redhat.com>
[rebase, improve commit message example]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: update commit message now that goto cleanup is removed from series
v10: new patch, but effectively split out from 31/37
---
scripts/qapi-visit.py | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2bdb5a1..1051710 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -238,11 +238,8 @@ out:
return ret
-def gen_visit_union(name, base, variants):
- ret = ''
-
- if base:
- ret += gen_visit_fields_decl(base)
+def gen_visit_union(name, base, members, variants):
+ ret = gen_visit_struct_fields(name, base, members)
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
@@ -262,21 +259,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
if (!*obj) {
goto out_obj;
}
+ visit_type_%(c_name)s_fields(v, obj, &err);
''',
c_name=c_name(name))
-
- if base:
- ret += mcgen('''
- visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
-''',
- c_name=base.c_name())
- else:
- ret += mcgen('''
- visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, &err);
-''',
- c_type=variants.tag_member.type.c_name(),
- c_name=c_name(variants.tag_member.name),
- name=variants.tag_member.name)
ret += gen_err_check(label='out_obj')
ret += mcgen('''
if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
@@ -377,11 +362,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
def visit_object_type(self, name, info, base, members, variants):
self.decl += gen_visit_decl(name)
if variants:
- if members:
- # Members other than variants.tag_member not implemented
- assert len(members) == 1
- assert members[0] == variants.tag_member
- self.defn += gen_visit_union(name, base, variants)
+ self.defn += gen_visit_union(name, base, members, variants)
else:
self.defn += gen_visit_struct(name, base, members)
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 05/15] qapi-visit: Simplify how we visit common union members
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 05/15] qapi-visit: Simplify how we visit common union members Eric Blake
@ 2016-02-18 12:16 ` Markus Armbruster
2016-02-18 14:40 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 12:16 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> From: Markus Armbruster <armbru@redhat.com>
>
> For a simple union SU, gen_visit_union() generates a visit of its
> single tag member, like this:
>
> visit_type_SUKind(v, "type", &(*obj)->type, &err);
>
> For a flat union FU with base B, it generates a visit of its base
> fields:
>
> visit_type_B_fields(v, (B **)obj, &err);
>
> Instead, we can simply visit the common members using the same fields
> visit function we use for structs, generated with
> gen_visit_struct_fields(). This function visits the base if any, then
> the local members.
>
> For a simple union SU, visit_type_SU_fields() contains exactly the old
> tag member visit, because there is no base, and the tag member is the
> only member. For instance, the code generated for qapi-schema.json's
> KeyValue changes like this:
>
> +static void visit_type_KeyValue_fields(Visitor *v, KeyValue **obj, Error **errp)
> +{
> + Error *err = NULL;
> +
> + visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
> + if (err) {
> + goto out;
> + }
> +
> +out:
> + error_propagate(errp, err);
> +}
> +
> void visit_type_KeyValue(Visitor *v, const char *name, KeyValue **obj, Error **errp)
> {
> Error *err = NULL;
> @@ -4863,7 +4911,7 @@ void visit_type_KeyValue(Visitor *v, con
> if (!*obj) {
> goto out_obj;
> }
> - visit_type_KeyValueKind(v, "type", &(*obj)->type, &err);
> + visit_type_KeyValue_fields(v, obj, &err);
> if (err) {
> goto out_obj;
> }
>
> For a flat union FU, visit_type_FU_fields() contains exactly the old
> base fields visit, because there is a base, but no members. For
> instance, the code generated for qapi-schema.json's CpuInfo changes
> like this:
>
> static void visit_type_CpuInfoBase_fields(Visitor *v, CpuInfoBase **obj, Error **errp);
>
> +static void visit_type_CpuInfo_fields(Visitor *v, CpuInfo **obj, Error **errp)
> +{
> + Error *err = NULL;
> +
> + visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
> + if (err) {
> + goto out;
> + }
> +
> +out:
> + error_propagate(errp, err);
> +}
> +
> static void visit_type_CpuInfoX86_fields(Visitor *v, CpuInfoX86 **obj, Error **errp)
> ...
> @@ -3485,7 +3509,7 @@ void visit_type_CpuInfo(Visitor *v, cons
> if (!*obj) {
> goto out_obj;
> }
> - visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, &err);
> + visit_type_CpuInfo_fields(v, obj, &err);
> if (err) {
> goto out_obj;
> }
>
> As you see, the generated code grows a bit, but in practice, it's lost
> in the noise: qapi-schema.json's qapi-visit.c gains roughly 1%.
>
> This simplification became possible with commit 441cbac "qapi-visit:
> Convert to QAPISchemaVisitor, fixing bugs". It's a step towards
> unifying gen_struct() and gen_union().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <1453902888-20457-2-git-send-email-armbru@redhat.com>
> [rebase, improve commit message example]
> Signed-off-by: Eric Blake <eblake@redhat.com>
v10 had a noteworthy rebase, but v11 is the original patch again, with
only the line numbers shifted a bit. Suggest to drop "rebase, ".
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 05/15] qapi-visit: Simplify how we visit common union members
2016-02-18 12:16 ` Markus Armbruster
@ 2016-02-18 14:40 ` Eric Blake
0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 14:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 749 bytes --]
On 02/18/2016 05:16 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> For a simple union SU, gen_visit_union() generates a visit of its
>> single tag member, like this:
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Message-Id: <1453902888-20457-2-git-send-email-armbru@redhat.com>
>> [rebase, improve commit message example]
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> v10 had a noteworthy rebase, but v11 is the original patch again, with
> only the line numbers shifted a bit. Suggest to drop "rebase, ".
Concur.
--
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] 41+ messages in thread
* [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields()
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (4 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 05/15] qapi-visit: Simplify how we visit common union members Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 13:58 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 07/15] qapi-visit: Unify struct and union visit Eric Blake
` (10 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We initially created the static visit_type_FOO_fields() helper
function for reuse of code - we have cases where the initial
setup for a visit has different allocation (depending on whether
the fields represent a stand-alone type or are embedded as part
of a larger type), but where the actual field visits are
identical once a pointer is available.
Up until the previous patch, visit_type_FOO_fields() was only
used for structs (no variants), so it was covering every field
for each type where it was emitted.
Meanwhile, the code for visiting unions looks like:
static visit_type_U_fields() {
visit base;
visit local_members;
}
visit_type_U() {
visit_start_struct();
visit_type_U_fields();
visit variants;
visit_end_struct();
}
which splits the fields of the union visit across two functions.
Move the code to visit variants to live inside visit_type_U_fields(),
while making it conditional on having variants so that all other
instances of the helper function remain unchanged. This is also
a step closer towards unifying struct and union visits, and towards
allowing one union type to be the branch of another flat union.
The resulting diff to the generated code is a bit hard to read,
but it can be verified that it touches only union types, and that
the end result is the following general structure:
static visit_type_U_fields() {
visit base;
visit local_members;
visit variants;
}
visit_type_U() {
visit_start_struct();
visit_type_U_fields();
visit_end_struct();
}
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: new patch
---
scripts/qapi-visit.py | 104 +++++++++++++++++++++++++-------------------------
1 file changed, 53 insertions(+), 51 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 1051710..6cea7d6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -71,11 +71,16 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
return ret
-def gen_visit_struct_fields(name, base, members):
+def gen_visit_struct_fields(name, base, members, variants=None):
ret = ''
if base:
ret += gen_visit_fields_decl(base)
+ if variants:
+ for var in variants.variants:
+ # Ugly special case for simple union TODO get rid of it
+ if not var.simple_union_type():
+ ret += gen_visit_implicit_struct(var.type)
struct_fields_seen.add(name)
ret += mcgen('''
@@ -96,8 +101,49 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
ret += gen_visit_fields(members, prefix='(*obj)->')
- # 'goto out' produced for base, and by gen_visit_fields() for each member
- if base or members:
+ if variants:
+ ret += mcgen('''
+ if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
+ goto out;
+ }
+ switch ((*obj)->%(c_name)s) {
+''',
+ c_name=c_name(variants.tag_member.name))
+
+ for var in variants.variants:
+ # TODO ugly special case for simple union
+ simple_union_type = var.simple_union_type()
+ ret += mcgen('''
+ case %(case)s:
+''',
+ case=c_enum_const(variants.tag_member.type.name,
+ var.name,
+ variants.tag_member.type.prefix))
+ if simple_union_type:
+ ret += mcgen('''
+ visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
+''',
+ c_type=simple_union_type.c_name(),
+ c_name=c_name(var.name))
+ else:
+ ret += mcgen('''
+ visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
+''',
+ c_type=var.type.c_name(),
+ c_name=c_name(var.name))
+ ret += mcgen('''
+ break;
+''')
+
+ ret += mcgen('''
+ default:
+ abort();
+ }
+''')
+
+ # 'goto out' produced for base, by gen_visit_fields() for each member,
+ # and if variants were present
+ if base or members or variants:
ret += mcgen('''
out:
@@ -239,12 +285,7 @@ out:
def gen_visit_union(name, base, members, variants):
- ret = gen_visit_struct_fields(name, base, members)
-
- for var in variants.variants:
- # Ugly special case for simple union TODO get rid of it
- if not var.simple_union_type():
- ret += gen_visit_implicit_struct(var.type)
+ ret = gen_visit_struct_fields(name, base, members, variants)
ret += mcgen('''
@@ -260,54 +301,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
goto out_obj;
}
visit_type_%(c_name)s_fields(v, obj, &err);
-''',
- c_name=c_name(name))
- ret += gen_err_check(label='out_obj')
- ret += mcgen('''
- if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
- goto out_obj;
- }
- switch ((*obj)->%(c_name)s) {
-''',
- c_name=c_name(variants.tag_member.name))
-
- for var in variants.variants:
- # TODO ugly special case for simple union
- simple_union_type = var.simple_union_type()
- ret += mcgen('''
- case %(case)s:
-''',
- case=c_enum_const(variants.tag_member.type.name,
- var.name,
- variants.tag_member.type.prefix))
- if simple_union_type:
- ret += mcgen('''
- visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
-''',
- c_type=simple_union_type.c_name(),
- c_name=c_name(var.name))
- else:
- ret += mcgen('''
- visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
-''',
- c_type=var.type.c_name(),
- c_name=c_name(var.name))
- ret += mcgen('''
- break;
-''')
-
- ret += mcgen('''
- default:
- abort();
- }
-out_obj:
error_propagate(errp, err);
err = NULL;
+out_obj:
visit_end_struct(v, &err);
out:
error_propagate(errp, err);
}
-''')
+''',
+ c_name=c_name(name))
return ret
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields()
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields() Eric Blake
@ 2016-02-18 13:58 ` Markus Armbruster
2016-02-18 14:43 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 13:58 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> We initially created the static visit_type_FOO_fields() helper
> function for reuse of code - we have cases where the initial
> setup for a visit has different allocation (depending on whether
> the fields represent a stand-alone type or are embedded as part
> of a larger type), but where the actual field visits are
> identical once a pointer is available.
>
> Up until the previous patch, visit_type_FOO_fields() was only
> used for structs (no variants), so it was covering every field
> for each type where it was emitted.
>
> Meanwhile, the code for visiting unions looks like:
>
> static visit_type_U_fields() {
> visit base;
> visit local_members;
> }
> visit_type_U() {
> visit_start_struct();
> visit_type_U_fields();
> visit variants;
> visit_end_struct();
> }
>
> which splits the fields of the union visit across two functions.
> Move the code to visit variants to live inside visit_type_U_fields(),
> while making it conditional on having variants so that all other
> instances of the helper function remain unchanged. This is also
> a step closer towards unifying struct and union visits, and towards
> allowing one union type to be the branch of another flat union.
>
> The resulting diff to the generated code is a bit hard to read,
Mostly because a few visit_type_T_fields() get generated in a different
place. If I move them back manually for a diff, the patch's effect
becomes clear enough: the switch to visit the variants and the
visit_start_union() guarding it moves from visit_type_T() to
visit_type_T_fields(). That's what the commit message promises.
> but it can be verified that it touches only union types, and that
> the end result is the following general structure:
>
> static visit_type_U_fields() {
> visit base;
> visit local_members;
> visit variants;
> }
> visit_type_U() {
> visit_start_struct();
> visit_type_U_fields();
> visit_end_struct();
> }
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: new patch
> ---
> scripts/qapi-visit.py | 104 +++++++++++++++++++++++++-------------------------
> 1 file changed, 53 insertions(+), 51 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 1051710..6cea7d6 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -71,11 +71,16 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
> return ret
>
>
> -def gen_visit_struct_fields(name, base, members):
> +def gen_visit_struct_fields(name, base, members, variants=None):
Two callers:
* gen_visit_union(): the new code here comes from there, except it's now
completely guarded by if variants. Effect: generated code motion.
* gen_visit_struct(): passes no variants, guard false, thus no change.
I think the patch becomes slightly clearer if you make the variants
parameter mandatory. It'll probably become mandatory anyway when we
unify gen_visit_struct() and gen_visit_union().
> ret = ''
>
> if base:
> ret += gen_visit_fields_decl(base)
> + if variants:
> + for var in variants.variants:
> + # Ugly special case for simple union TODO get rid of it
> + if not var.simple_union_type():
> + ret += gen_visit_implicit_struct(var.type)
>
> struct_fields_seen.add(name)
> ret += mcgen('''
> @@ -96,8 +101,49 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
>
> ret += gen_visit_fields(members, prefix='(*obj)->')
>
> - # 'goto out' produced for base, and by gen_visit_fields() for each member
> - if base or members:
> + if variants:
> + ret += mcgen('''
> + if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> + goto out;
> + }
> + switch ((*obj)->%(c_name)s) {
> +''',
> + c_name=c_name(variants.tag_member.name))
> +
> + for var in variants.variants:
> + # TODO ugly special case for simple union
> + simple_union_type = var.simple_union_type()
> + ret += mcgen('''
> + case %(case)s:
> +''',
> + case=c_enum_const(variants.tag_member.type.name,
> + var.name,
> + variants.tag_member.type.prefix))
> + if simple_union_type:
> + ret += mcgen('''
> + visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
> +''',
> + c_type=simple_union_type.c_name(),
> + c_name=c_name(var.name))
> + else:
> + ret += mcgen('''
> + visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
> +''',
> + c_type=var.type.c_name(),
> + c_name=c_name(var.name))
> + ret += mcgen('''
> + break;
> +''')
> +
> + ret += mcgen('''
> + default:
> + abort();
> + }
> +''')
> +
> + # 'goto out' produced for base, by gen_visit_fields() for each member,
> + # and if variants were present
> + if base or members or variants:
> ret += mcgen('''
>
> out:
> @@ -239,12 +285,7 @@ out:
>
>
> def gen_visit_union(name, base, members, variants):
> - ret = gen_visit_struct_fields(name, base, members)
> -
> - for var in variants.variants:
> - # Ugly special case for simple union TODO get rid of it
> - if not var.simple_union_type():
> - ret += gen_visit_implicit_struct(var.type)
> + ret = gen_visit_struct_fields(name, base, members, variants)
>
> ret += mcgen('''
>
> @@ -260,54 +301,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
> goto out_obj;
> }
> visit_type_%(c_name)s_fields(v, obj, &err);
> -''',
> - c_name=c_name(name))
> - ret += gen_err_check(label='out_obj')
> - ret += mcgen('''
> - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> - goto out_obj;
> - }
> - switch ((*obj)->%(c_name)s) {
> -''',
> - c_name=c_name(variants.tag_member.name))
> -
> - for var in variants.variants:
> - # TODO ugly special case for simple union
> - simple_union_type = var.simple_union_type()
> - ret += mcgen('''
> - case %(case)s:
> -''',
> - case=c_enum_const(variants.tag_member.type.name,
> - var.name,
> - variants.tag_member.type.prefix))
> - if simple_union_type:
> - ret += mcgen('''
> - visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
> -''',
> - c_type=simple_union_type.c_name(),
> - c_name=c_name(var.name))
> - else:
> - ret += mcgen('''
> - visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
> -''',
> - c_type=var.type.c_name(),
> - c_name=c_name(var.name))
> - ret += mcgen('''
> - break;
> -''')
> -
> - ret += mcgen('''
> - default:
> - abort();
> - }
> -out_obj:
> error_propagate(errp, err);
> err = NULL;
> +out_obj:
> visit_end_struct(v, &err);
> out:
> error_propagate(errp, err);
> }
> -''')
> +''',
> + c_name=c_name(name))
>
> return ret
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields()
2016-02-18 13:58 ` Markus Armbruster
@ 2016-02-18 14:43 ` Eric Blake
2016-02-18 17:04 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 14:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]
On 02/18/2016 06:58 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> We initially created the static visit_type_FOO_fields() helper
>> function for reuse of code - we have cases where the initial
>> setup for a visit has different allocation (depending on whether
>> the fields represent a stand-alone type or are embedded as part
>> of a larger type), but where the actual field visits are
>> identical once a pointer is available.
>>
>> Up until the previous patch, visit_type_FOO_fields() was only
>> used for structs (no variants), so it was covering every field
>> for each type where it was emitted.
>>
>> Meanwhile, the code for visiting unions looks like:
Maybe reword this to:
Meanwhile, the previous patch updated the code for visiting unions to
look like:
>> The resulting diff to the generated code is a bit hard to read,
>
> Mostly because a few visit_type_T_fields() get generated in a different
> place. If I move them back manually for a diff, the patch's effect
> becomes clear enough: the switch to visit the variants and the
> visit_start_union() guarding it moves from visit_type_T() to
> visit_type_T_fields(). That's what the commit message promises.
I debated about splitting out a separate patch so that the motion of
visit_type_T_fields() is separate from the variant visiting, but it
wasn't worth the effort. I'm glad you managed to see through the churn
in the generated code.
>> -def gen_visit_struct_fields(name, base, members):
>> +def gen_visit_struct_fields(name, base, members, variants=None):
>
> Two callers:
>
> * gen_visit_union(): the new code here comes from there, except it's now
> completely guarded by if variants. Effect: generated code motion.
>
> * gen_visit_struct(): passes no variants, guard false, thus no change.
>
> I think the patch becomes slightly clearer if you make the variants
> parameter mandatory. It'll probably become mandatory anyway when we
> unify gen_visit_struct() and gen_visit_union().
You beat me to it - yes, I realized that after sending the series. We
can either squash in the fix here to make variants mandatory (and
gen_visit_struct() pass an explicit None, which disappears in the next
patch), or squash in a fix to 7/15 to delete the '=None'. Either way,
I'm fine if you tackle that, if no other major findings turn up.
--
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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields()
2016-02-18 14:43 ` Eric Blake
@ 2016-02-18 17:04 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 17:04 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/18/2016 06:58 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> We initially created the static visit_type_FOO_fields() helper
>>> function for reuse of code - we have cases where the initial
>>> setup for a visit has different allocation (depending on whether
>>> the fields represent a stand-alone type or are embedded as part
>>> of a larger type), but where the actual field visits are
>>> identical once a pointer is available.
>>>
>>> Up until the previous patch, visit_type_FOO_fields() was only
>>> used for structs (no variants), so it was covering every field
>>> for each type where it was emitted.
>>>
>>> Meanwhile, the code for visiting unions looks like:
>
> Maybe reword this to:
>
> Meanwhile, the previous patch updated the code for visiting unions to
> look like:
I don't mind.
>>> The resulting diff to the generated code is a bit hard to read,
>>
>> Mostly because a few visit_type_T_fields() get generated in a different
>> place. If I move them back manually for a diff, the patch's effect
>> becomes clear enough: the switch to visit the variants and the
>> visit_start_union() guarding it moves from visit_type_T() to
>> visit_type_T_fields(). That's what the commit message promises.
>
> I debated about splitting out a separate patch so that the motion of
> visit_type_T_fields() is separate from the variant visiting, but it
> wasn't worth the effort. I'm glad you managed to see through the churn
> in the generated code.
>
>
>>> -def gen_visit_struct_fields(name, base, members):
>>> +def gen_visit_struct_fields(name, base, members, variants=None):
>>
>> Two callers:
>>
>> * gen_visit_union(): the new code here comes from there, except it's now
>> completely guarded by if variants. Effect: generated code motion.
>>
>> * gen_visit_struct(): passes no variants, guard false, thus no change.
>>
>> I think the patch becomes slightly clearer if you make the variants
>> parameter mandatory. It'll probably become mandatory anyway when we
>> unify gen_visit_struct() and gen_visit_union().
>
> You beat me to it - yes, I realized that after sending the series. We
> can either squash in the fix here to make variants mandatory (and
> gen_visit_struct() pass an explicit None, which disappears in the next
> patch), or squash in a fix to 7/15 to delete the '=None'. Either way,
> I'm fine if you tackle that, if no other major findings turn up.
Can do.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 07/15] qapi-visit: Unify struct and union visit
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (5 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 06/15] qapi: Visit variants in visit_type_FOO_fields() Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 14:05 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 08/15] qapi-visit: Less indirection in visit_type_Foo_fields() Eric Blake
` (9 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
From: Markus Armbruster <armbru@redhat.com>
gen_visit_union() is now just like gen_visit_struct(). Rename
it to gen_visit_object(), use it for structs, and drop
gen_visit_struct(). Output is unchanged.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1453902888-20457-4-git-send-email-armbru@redhat.com>
[split out variant handling, rebase to earlier changes]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rebase on top of moved variant code
v10: new patch, replacing 31/37
---
scripts/qapi-visit.py | 45 ++++++---------------------------------------
1 file changed, 6 insertions(+), 39 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 6cea7d6..4650c5b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -155,40 +155,6 @@ out:
return ret
-def gen_visit_struct(name, base, members):
- ret = gen_visit_struct_fields(name, base, members)
-
- # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
- # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
- # rather than leaving it non-NULL. As currently written, the caller must
- # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
- ret += mcgen('''
-
-void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
-{
- Error *err = NULL;
-
- visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
- if (err) {
- goto out;
- }
- if (!*obj) {
- goto out_obj;
- }
- visit_type_%(c_name)s_fields(v, obj, &err);
- error_propagate(errp, err);
- err = NULL;
-out_obj:
- visit_end_struct(v, &err);
-out:
- error_propagate(errp, err);
-}
-''',
- c_name=c_name(name))
-
- return ret
-
-
def gen_visit_list(name, element_type):
# FIXME: if *obj is NULL on entry, and the first visit_next_list()
# assigns to *obj, while a later one fails, we should clean up *obj
@@ -284,9 +250,13 @@ out:
return ret
-def gen_visit_union(name, base, members, variants):
+def gen_visit_object(name, base, members, variants):
ret = gen_visit_struct_fields(name, base, members, variants)
+ # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
+ # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
+ # rather than leaving it non-NULL. As currently written, the caller must
+ # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
ret += mcgen('''
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
@@ -363,10 +333,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
def visit_object_type(self, name, info, base, members, variants):
self.decl += gen_visit_decl(name)
- if variants:
- self.defn += gen_visit_union(name, base, members, variants)
- else:
- self.defn += gen_visit_struct(name, base, members)
+ self.defn += gen_visit_object(name, base, members, variants)
def visit_alternate_type(self, name, info, variants):
self.decl += gen_visit_decl(name)
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 07/15] qapi-visit: Unify struct and union visit
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 07/15] qapi-visit: Unify struct and union visit Eric Blake
@ 2016-02-18 14:05 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 14:05 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> From: Markus Armbruster <armbru@redhat.com>
>
> gen_visit_union() is now just like gen_visit_struct(). Rename
> it to gen_visit_object(), use it for structs, and drop
> gen_visit_struct(). Output is unchanged.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <1453902888-20457-4-git-send-email-armbru@redhat.com>
> [split out variant handling, rebase to earlier changes]
Less than half of the size now :)
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: rebase on top of moved variant code
> v10: new patch, replacing 31/37
> ---
> scripts/qapi-visit.py | 45 ++++++---------------------------------------
> 1 file changed, 6 insertions(+), 39 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 6cea7d6..4650c5b 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -155,40 +155,6 @@ out:
> return ret
>
>
> -def gen_visit_struct(name, base, members):
> - ret = gen_visit_struct_fields(name, base, members)
> -
> - # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> - # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> - # rather than leaving it non-NULL. As currently written, the caller must
> - # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
> - ret += mcgen('''
> -
> -void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> -{
> - Error *err = NULL;
> -
> - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
> - if (err) {
> - goto out;
> - }
> - if (!*obj) {
> - goto out_obj;
> - }
> - visit_type_%(c_name)s_fields(v, obj, &err);
> - error_propagate(errp, err);
> - err = NULL;
> -out_obj:
> - visit_end_struct(v, &err);
> -out:
> - error_propagate(errp, err);
> -}
> -''',
> - c_name=c_name(name))
> -
> - return ret
> -
> -
> def gen_visit_list(name, element_type):
> # FIXME: if *obj is NULL on entry, and the first visit_next_list()
> # assigns to *obj, while a later one fails, we should clean up *obj
> @@ -284,9 +250,13 @@ out:
> return ret
>
>
> -def gen_visit_union(name, base, members, variants):
> +def gen_visit_object(name, base, members, variants):
> ret = gen_visit_struct_fields(name, base, members, variants)
This is now the only caller of gen_visit_struct_fields(), and leaving
parameter variant optional makes no sense.
>
> + # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> + # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
> + # rather than leaving it non-NULL. As currently written, the caller must
> + # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
> ret += mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> @@ -363,10 +333,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
> def visit_object_type(self, name, info, base, members, variants):
> self.decl += gen_visit_decl(name)
> - if variants:
> - self.defn += gen_visit_union(name, base, members, variants)
> - else:
> - self.defn += gen_visit_struct(name, base, members)
> + self.defn += gen_visit_object(name, base, members, variants)
>
> def visit_alternate_type(self, name, info, variants):
> self.decl += gen_visit_decl(name)
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 08/15] qapi-visit: Less indirection in visit_type_Foo_fields()
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (6 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 07/15] qapi-visit: Unify struct and union visit Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 09/15] qapi: Adjust layout of FooList types Eric Blake
` (8 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We were passing 'Foo **obj' to the internal helper function, but
all uses within the helper were via reads of '*obj'. Refactor
things to pass one less level of indirection, by having the
callers dereference before calling.
For an example of the generated code change:
|-static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo **obj, Error **errp)
|+static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo *obj, Error **errp)
| {
| Error *err = NULL;
|
|- visit_type_int(v, "actual", &(*obj)->actual, &err);
|+ visit_type_int(v, "actual", &obj->actual, &err);
| error_propagate(errp, err);
| }
|
|@@ -261,7 +261,7 @@ void visit_type_BalloonInfo(Visitor *v,
| if (!*obj) {
| goto out_obj;
| }
|- visit_type_BalloonInfo_fields(v, obj, &err);
|+ visit_type_BalloonInfo_fields(v, *obj, &err);
| out_obj:
The refactoring will also make it easier to reuse the helpers in
a future patch when implicit structs are stored directly in the
parent struct rather than boxed through a pointer.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rebase to include variants, drop skiplast handling
v10: new patch
---
scripts/qapi-visit.py | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4650c5b..231e424 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -39,7 +39,7 @@ def gen_visit_fields_decl(typ):
if typ.name not in struct_fields_seen:
ret += mcgen('''
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp);
''',
c_type=typ.c_name())
struct_fields_seen.add(typ.name)
@@ -61,7 +61,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
if (!err) {
- visit_type_%(c_type)s_fields(v, obj, errp);
+ visit_type_%(c_type)s_fields(v, *obj, errp);
visit_end_implicit_struct(v);
}
error_propagate(errp, err);
@@ -85,7 +85,7 @@ def gen_visit_struct_fields(name, base, members, variants=None):
struct_fields_seen.add(name)
ret += mcgen('''
-static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **errp)
{
Error *err = NULL;
@@ -94,19 +94,19 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
if base:
ret += mcgen('''
- visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
+ visit_type_%(c_type)s_fields(v, (%(c_type)s *)obj, &err);
''',
c_type=base.c_name())
ret += gen_err_check()
- ret += gen_visit_fields(members, prefix='(*obj)->')
+ ret += gen_visit_fields(members, prefix='obj->')
if variants:
ret += mcgen('''
- if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
+ if (!visit_start_union(v, !!obj->u.data, &err) || err) {
goto out;
}
- switch ((*obj)->%(c_name)s) {
+ switch (obj->%(c_name)s) {
''',
c_name=c_name(variants.tag_member.name))
@@ -121,13 +121,13 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
variants.tag_member.type.prefix))
if simple_union_type:
ret += mcgen('''
- visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);
+ visit_type_%(c_type)s(v, "data", &obj->u.%(c_name)s, &err);
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
+ visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
@@ -270,7 +270,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
if (!*obj) {
goto out_obj;
}
- visit_type_%(c_name)s_fields(v, obj, &err);
+ visit_type_%(c_name)s_fields(v, *obj, &err);
error_propagate(errp, err);
err = NULL;
out_obj:
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v11 09/15] qapi: Adjust layout of FooList types
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (7 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 08/15] qapi-visit: Less indirection in visit_type_Foo_fields() Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 15:55 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 10/15] qapi: Emit structs used as variants in topological order Eric Blake
` (7 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
By sticking the next pointer first, we don't need a union with
64-bit padding for smaller types. On 32-bit platforms, this
can reduce the size of uint8List from 16 bytes (or 12, depending
on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
It has no effect on 64-bit platforms (where alignment still
dictates a 16-byte struct); but fewer anonymous unions is still
a win in my book.
It requires visit_next_list() to gain a size parameter, to know
what size element to allocate; comparable to the size parameter
of visit_start_struct().
I debated about going one step further, to allow for fewer casts,
by doing:
typedef GenericList GenericList;
struct GenericList {
GenericList *next;
};
struct FooList {
GenericList base;
Foo *value;
};
so that you convert to 'GenericList *' by '&foolist->base', and
back by 'container_of(generic, GenericList, base)' (as opposed to
the existing '(GenericList *)foolist' and '(FooList *)generic').
But doing that would require hoisting the declaration of
GenericList prior to inclusion of qapi-types.h, rather than its
current spot in visitor.h; it also makes iteration a bit more
verbose through 'foolist->base.next' instead of 'foolist->next'.
Note that for lists of objects, the 'value' payload is still
hidden behind a boxed pointer. Someday, it would be nice to do:
struct FooList {
FooList *next;
Foo value;
};
for one less level of malloc for each list element. This patch
is a step in that direction (now that 'next' is no longer at a
fixed non-zero offset within the struct, we can store more than
just a pointer's-worth of data as the value payload), but the
actual conversion would be a task for another series, as it will
touch a lot of code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: assert that size is sane, improve commit message
v10: hoist earlier in series
v9: no change
v8: rebase to earlier changes
v7: new patch; probably too invasive to be worth it, especially if
we can't prove that our current size for uint8List is a bottleneck
---
include/qapi/visitor.h | 13 ++++++-------
include/qapi/visitor-impl.h | 2 +-
scripts/qapi-types.py | 5 +----
scripts/qapi-visit.py | 2 +-
qapi/qapi-visit-core.c | 5 +++--
qapi/opts-visitor.c | 4 ++--
qapi/qapi-dealloc-visitor.c | 3 ++-
qapi/qmp-input-visitor.c | 5 +++--
qapi/qmp-output-visitor.c | 3 ++-
qapi/string-input-visitor.c | 4 ++--
qapi/string-output-visitor.c | 2 +-
11 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5e581dc..8a2d5cc 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -19,13 +19,12 @@
#include "qapi/error.h"
#include <stdlib.h>
-typedef struct GenericList
-{
- union {
- void *value;
- uint64_t padding;
- };
+/* This struct is layout-compatible with all other *List structs
+ * created by the qapi generator. It is used as a typical
+ * singly-linked list. */
+typedef struct GenericList {
struct GenericList *next;
+ char padding[];
} GenericList;
void visit_start_struct(Visitor *v, const char *name, void **obj,
@@ -36,7 +35,7 @@ void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
void visit_end_implicit_struct(Visitor *v);
void visit_start_list(Visitor *v, const char *name, Error **errp);
-GenericList *visit_next_list(Visitor *v, GenericList **list);
+GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
void visit_end_list(Visitor *v);
/**
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index ea252f8..7905a28 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -29,7 +29,7 @@ struct Visitor
void (*start_list)(Visitor *v, const char *name, Error **errp);
/* Must be set */
- GenericList *(*next_list)(Visitor *v, GenericList **list);
+ GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
/* Must be set */
void (*end_list)(Visitor *v);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7b0dca8..83f230a 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -26,11 +26,8 @@ def gen_array(name, element_type):
return mcgen('''
struct %(c_name)s {
- union {
- %(c_type)s value;
- uint64_t padding;
- };
%(c_name)s *next;
+ %(c_type)s value;
};
''',
c_name=c_name(name), c_type=element_type.c_type())
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 231e424..48ebd2b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -173,7 +173,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
}
for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(v, prev)) != NULL;
+ !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
prev = &i) {
%(c_name)s *native_i = (%(c_name)s *)i;
visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index f856286..b4a0f21 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -50,9 +50,10 @@ void visit_start_list(Visitor *v, const char *name, Error **errp)
v->start_list(v, name, errp);
}
-GenericList *visit_next_list(Visitor *v, GenericList **list)
+GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size)
{
- return v->next_list(v, list);
+ assert(list && size >= sizeof(GenericList));
+ return v->next_list(v, list, size);
}
void visit_end_list(Visitor *v)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ae5b955..73e4ace 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -215,7 +215,7 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
static GenericList *
-opts_next_list(Visitor *v, GenericList **list)
+opts_next_list(Visitor *v, GenericList **list, size_t size)
{
OptsVisitor *ov = to_ov(v);
GenericList **link;
@@ -258,7 +258,7 @@ opts_next_list(Visitor *v, GenericList **list)
abort();
}
- *link = g_malloc0(sizeof **link);
+ *link = g_malloc0(size);
return *link;
}
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 2659d3f..6667e8c 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -100,7 +100,8 @@ static void qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp)
qapi_dealloc_push(qov, NULL);
}
-static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp)
+static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
+ size_t size)
{
GenericList *list = *listp;
QapiDeallocVisitor *qov = to_qov(v);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 2f48b95..2621660 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -165,7 +165,8 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
qmp_input_push(qiv, qobj, errp);
}
-static GenericList *qmp_input_next_list(Visitor *v, GenericList **list)
+static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
+ size_t size)
{
QmpInputVisitor *qiv = to_qiv(v);
GenericList *entry;
@@ -184,7 +185,7 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list)
return NULL;
}
- entry = g_malloc0(sizeof(*entry));
+ entry = g_malloc0(size);
if (first) {
*list = entry;
} else {
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f47eefa..d44c676 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -126,7 +126,8 @@ static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
qmp_output_push(qov, list);
}
-static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp)
+static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
+ size_t size)
{
GenericList *list = *listp;
QmpOutputVisitor *qov = to_qov(v);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 18b9339..59eb5dc 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -139,7 +139,7 @@ start_list(Visitor *v, const char *name, Error **errp)
}
}
-static GenericList *next_list(Visitor *v, GenericList **list)
+static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
{
StringInputVisitor *siv = to_siv(v);
GenericList **link;
@@ -173,7 +173,7 @@ static GenericList *next_list(Visitor *v, GenericList **list)
link = &(*list)->next;
}
- *link = g_malloc0(sizeof **link);
+ *link = g_malloc0(size);
return *link;
}
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index b980bd3..c2e5c5b 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -276,7 +276,7 @@ start_list(Visitor *v, const char *name, Error **errp)
sov->head = true;
}
-static GenericList *next_list(Visitor *v, GenericList **list)
+static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
{
StringOutputVisitor *sov = to_sov(v);
GenericList *ret = NULL;
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 09/15] qapi: Adjust layout of FooList types
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 09/15] qapi: Adjust layout of FooList types Eric Blake
@ 2016-02-18 15:55 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 15:55 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> By sticking the next pointer first, we don't need a union with
> 64-bit padding for smaller types. On 32-bit platforms, this
> can reduce the size of uint8List from 16 bytes (or 12, depending
> on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
> It has no effect on 64-bit platforms (where alignment still
> dictates a 16-byte struct); but fewer anonymous unions is still
> a win in my book.
>
> It requires visit_next_list() to gain a size parameter, to know
> what size element to allocate; comparable to the size parameter
> of visit_start_struct().
>
> I debated about going one step further, to allow for fewer casts,
> by doing:
> typedef GenericList GenericList;
> struct GenericList {
> GenericList *next;
> };
> struct FooList {
> GenericList base;
> Foo *value;
> };
> so that you convert to 'GenericList *' by '&foolist->base', and
> back by 'container_of(generic, GenericList, base)' (as opposed to
> the existing '(GenericList *)foolist' and '(FooList *)generic').
> But doing that would require hoisting the declaration of
> GenericList prior to inclusion of qapi-types.h, rather than its
> current spot in visitor.h; it also makes iteration a bit more
> verbose through 'foolist->base.next' instead of 'foolist->next'.
>
> Note that for lists of objects, the 'value' payload is still
> hidden behind a boxed pointer. Someday, it would be nice to do:
>
> struct FooList {
> FooList *next;
> Foo value;
> };
>
> for one less level of malloc for each list element. This patch
> is a step in that direction (now that 'next' is no longer at a
> fixed non-zero offset within the struct, we can store more than
> just a pointer's-worth of data as the value payload), but the
> actual conversion would be a task for another series, as it will
> touch a lot of code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Bonus: qapi-types.h shrinks by 5%.
qapi-types.h | 350 +++++++++++------------------------------------------------
qapi-visit.c | 140 +++++++++++------------
2 files changed, 140 insertions(+), 350 deletions(-)
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 10/15] qapi: Emit structs used as variants in topological order
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (8 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 09/15] qapi: Adjust layout of FooList types Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 14:35 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 11/15] qapi-visit: Use common idiom in gen_visit_fields_decl() Eric Blake
` (6 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Right now, we emit the branches of union types as a boxed pointer,
and it suffices to have a forward declaration of the type. However,
a future patch will swap things to directly use the branch type,
instead of hiding it behind a pointer. For this to work, the
compiler needs the full definition of the type, not just a forward
declaration, prior to the union that is including the branch type.
This patch just adds topological sorting to hoist all types
mentioned in a branch of a union to be fully declared before the
union itself. The sort is always possible, because we do not
allow circular union types that include themselves as a direct
branch (it is, however, still possible to include a branch type
that itself has a pointer to the union, for a type that can
indirectly recursively nest itself - that remains safe, because
that the member of the branch type will remain a pointer, and the
QMP representation of such a type adds another {} for each recurring
layer of the union type).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: pep8 cleanup
v10: new patch
---
scripts/qapi-types.py | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 83f230a..6ea0ae6 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -2,7 +2,7 @@
# QAPI types generator
#
# Copyright IBM, Corp. 2011
-# Copyright (c) 2013-2015 Red Hat Inc.
+# Copyright (c) 2013-2016 Red Hat Inc.
#
# Authors:
# Anthony Liguori <aliguori@us.ibm.com>
@@ -14,6 +14,11 @@
from qapi import *
+# variants must be emitted before their container; track what has already
+# been output
+objects_seen = set()
+
+
def gen_fwd_object_or_array(name):
return mcgen('''
@@ -49,11 +54,23 @@ def gen_struct_fields(members):
def gen_object(name, base, members, variants):
- ret = mcgen('''
+ if name in objects_seen:
+ return ''
+ objects_seen.add(name)
+
+ ret = ''
+ if variants:
+ for v in variants.variants:
+ if (isinstance(v.type, QAPISchemaObjectType) and
+ not v.type.is_implicit()):
+ ret += gen_object(v.type.name, v.type.base,
+ v.type.local_members, v.type.variants)
+
+ ret += mcgen('''
struct %(c_name)s {
''',
- c_name=c_name(name))
+ c_name=c_name(name))
if base:
ret += mcgen('''
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 10/15] qapi: Emit structs used as variants in topological order
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 10/15] qapi: Emit structs used as variants in topological order Eric Blake
@ 2016-02-18 14:35 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 14:35 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Right now, we emit the branches of union types as a boxed pointer,
> and it suffices to have a forward declaration of the type. However,
> a future patch will swap things to directly use the branch type,
> instead of hiding it behind a pointer. For this to work, the
> compiler needs the full definition of the type, not just a forward
> declaration, prior to the union that is including the branch type.
> This patch just adds topological sorting to hoist all types
> mentioned in a branch of a union to be fully declared before the
> union itself. The sort is always possible, because we do not
> allow circular union types that include themselves as a direct
> branch
When a patch makes a "can't happen" claim, I normally ask for assertions
to make sure we go down in flames should it happen anyway. But in this
case, the C compiler will shoot us down then, and that'll do nicely.
> (it is, however, still possible to include a branch type
> that itself has a pointer to the union, for a type that can
> indirectly recursively nest itself - that remains safe, because
> that the member of the branch type will remain a pointer, and the
> QMP representation of such a type adds another {} for each recurring
> layer of the union type).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 11/15] qapi-visit: Use common idiom in gen_visit_fields_decl()
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (9 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 10/15] qapi: Emit structs used as variants in topological order Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate Eric Blake
` (5 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We have several instances of methods that do an early exit if
output is not needed, then log that output is being generated,
and finally produce the output; see qapi-types.py:gen_object()
and qapi-visit.py:gen_visit_implicit_struct(). The odd man
out was gen_visit_fields_decl(); rearrange it to be more like
the others. No semantic change or difference to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: split out from 11/13
---
scripts/qapi-visit.py | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 48ebd2b..089f4c4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -35,15 +35,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **
def gen_visit_fields_decl(typ):
- ret = ''
- if typ.name not in struct_fields_seen:
- ret += mcgen('''
+ if typ.name in struct_fields_seen:
+ return ''
+ struct_fields_seen.add(typ.name)
+ return mcgen('''
static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **errp);
''',
- c_type=typ.c_name())
- struct_fields_seen.add(typ.name)
- return ret
+ c_type=typ.c_name())
def gen_visit_implicit_struct(typ):
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (10 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 11/15] qapi-visit: Use common idiom in gen_visit_fields_decl() Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 16:43 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 13/15] qapi: Don't box branches of flat unions Eric Blake
` (4 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
There's no reason to do two malloc's for an alternate type visiting
a QAPI struct; let's just inline the struct directly as the C union
branch of the struct.
Surprisingly, no clients were actually using the struct member prior
to this patch outside of the testsuite; an earlier patch in the series
added some testsuite coverage to make the effect of this patch more
obvious.
In qapi.py, c_type() gains a new is_unboxed flag to control when we
are emitting a C struct unboxed within the context of an outer
struct (different from our other two modes of usage with no flags
for normal local variable declarations, and with is_param for adding
'const' in a parameter list). I don't know if there is any more
pythonic way of collapsing the two flags into a single parameter,
as we never have a caller setting both flags at once.
Ultimately, we want to also unbox branches for QAPI unions, but as
that touches a lot more client code, it is better as separate
patches. But since unions and alternates share gen_variants(), I
had to hack in a way to test if we are visiting an alternate type
for setting the is_unboxed flag: look for a non-object branch.
This works because alternates have at least two branches, with at
most one object branch, while unions have only object branches.
The hack will go away in a later patch.
The generated code difference to qapi-types.h is relatively small:
| struct BlockdevRef {
| QType type;
| union { /* union tag is @type */
| void *data;
|- BlockdevOptions *definition;
|+ BlockdevOptions definition;
| char *reference;
| } u;
| };
meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj)
(which allocates a pointer, handles a layer of {} from the JSON stream,
and visits the fields) with an inline call to visit_type_FOO(NULL) (to
consume the {} without allocation) and a direct visit of the fields:
| switch ((*obj)->type) {
| case QTYPE_QDICT:
|- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
|+ visit_start_struct(v, name, NULL, 0, &err);
|+ if (err) {
|+ break;
|+ }
|+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err);
|+ error_propagate(errp, err);
|+ err = NULL;
|+ visit_end_struct(v, &err);
| break;
| case QTYPE_QSTRING:
| visit_type_str(v, name, &(*obj)->u.reference, &err);
The visit of non-object fields is unchanged.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: drop visit_type_alternate_FOO(), improve commit message,
fix and test bug regarding visits to a union, rework some of
the hacks to make them more obvious
v10: new patch
---
scripts/qapi.py | 12 +++++++-----
scripts/qapi-types.py | 10 +++++++++-
scripts/qapi-visit.py | 33 +++++++++++++++++++++++++++------
tests/test-qmp-input-visitor.c | 20 ++++++++++----------
tests/test-qmp-output-visitor.c | 11 +++++------
5 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 17bf633..8497777 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -824,7 +824,7 @@ class QAPISchemaVisitor(object):
class QAPISchemaType(QAPISchemaEntity):
- def c_type(self, is_param=False):
+ def c_type(self, is_param=False, is_unboxed=False):
return c_name(self.name) + pointer_suffix
def c_null(self):
@@ -857,7 +857,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
def c_name(self):
return self.name
- def c_type(self, is_param=False):
+ def c_type(self, is_param=False, is_unboxed=False):
if is_param and self.name == 'str':
return 'const ' + self._c_type_name
return self._c_type_name
@@ -891,7 +891,7 @@ class QAPISchemaEnumType(QAPISchemaType):
# See QAPISchema._make_implicit_enum_type()
return self.name.endswith('Kind')
- def c_type(self, is_param=False):
+ def c_type(self, is_param=False, is_unboxed=False):
return c_name(self.name)
def member_names(self):
@@ -987,9 +987,11 @@ class QAPISchemaObjectType(QAPISchemaType):
assert not self.is_implicit()
return QAPISchemaType.c_name(self)
- def c_type(self, is_param=False):
+ def c_type(self, is_param=False, is_unboxed=False):
assert not self.is_implicit()
- return QAPISchemaType.c_type(self)
+ if is_unboxed:
+ return c_name(self.name)
+ return c_name(self.name) + pointer_suffix
def json_type(self):
return 'object'
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 6ea0ae6..4dabe91 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
def gen_variants(variants):
+ # HACK: Determine if this is an alternate (at least one variant
+ # is not an object); unions have all branches as objects.
+ unboxed = False
+ for v in variants.variants:
+ if not isinstance(v.type, QAPISchemaObjectType):
+ unboxed = True
+ break
+
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
# whether to bypass the switch statement if visiting the discriminator
@@ -136,7 +144,7 @@ def gen_variants(variants):
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=typ.c_type(),
+ c_type=typ.c_type(is_unboxed=unboxed),
c_name=c_name(var.name))
ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 089f4c4..61b7e72 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -201,11 +201,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error
def gen_visit_alternate(name, variants):
promote_int = 'true'
+ ret = ''
for var in variants.variants:
if var.type.alternate_qtype() == 'QTYPE_QINT':
promote_int = 'false'
+ if isinstance(var.type, QAPISchemaObjectType):
+ ret += gen_visit_fields_decl(var.type)
- ret = mcgen('''
+ ret += mcgen('''
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
{
@@ -221,17 +224,35 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
}
switch ((*obj)->type) {
''',
- c_name=c_name(name), promote_int=promote_int)
+ c_name=c_name(name), promote_int=promote_int)
for var in variants.variants:
ret += mcgen('''
case %(case)s:
+''',
+ case=var.type.alternate_qtype())
+ if isinstance(var.type, QAPISchemaObjectType):
+ ret += mcgen('''
+ visit_start_struct(v, name, NULL, 0, &err);
+ if (err) {
+ break;
+ }
+ visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_struct(v, &err);
+''',
+ c_type=var.type.c_name(),
+ c_name=c_name(var.name))
+ else:
+ ret += mcgen('''
visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
- break;
''',
- case=var.type.alternate_qtype(),
- c_type=var.type.c_name(),
- c_name=c_name(var.name))
+ c_type=var.type.c_name(),
+ c_name=c_name(var.name))
+ ret += mcgen('''
+ break;
+''')
ret += mcgen('''
default:
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index ef836d5..47cf6aa 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -327,11 +327,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
"'enum1':'value1', 'boolean':true}");
visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
g_assert_cmpint(tmp->type, ==, QTYPE_QDICT);
- g_assert_cmpint(tmp->u.udfu->integer, ==, 1);
- g_assert_cmpstr(tmp->u.udfu->string, ==, "str");
- g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
- g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true);
- g_assert_cmpint(tmp->u.udfu->u.value1->has_a_b, ==, false);
+ g_assert_cmpint(tmp->u.udfu.integer, ==, 1);
+ g_assert_cmpstr(tmp->u.udfu.string, ==, "str");
+ g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
+ g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true);
+ g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false);
qapi_free_UserDefAlternate(tmp);
v = visitor_input_test_init(data, "false");
@@ -355,11 +355,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
"'enum1':'value1', 'boolean':true} }");
visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT);
- g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1);
- g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str");
- g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1);
- g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true);
- g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false);
+ g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1);
+ g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str");
+ g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
+ g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true);
+ g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false);
qapi_free_WrapAlternate(wrap);
}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 2b0f7e9..fe2f1a1 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -457,12 +457,11 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
tmp = g_new0(UserDefAlternate, 1);
tmp->type = QTYPE_QDICT;
- tmp->u.udfu = g_new0(UserDefFlatUnion, 1);
- tmp->u.udfu->integer = 1;
- tmp->u.udfu->string = g_strdup("str");
- tmp->u.udfu->enum1 = ENUM_ONE_VALUE1;
- tmp->u.udfu->u.value1 = g_new0(UserDefA, 1);
- tmp->u.udfu->u.value1->boolean = true;
+ tmp->u.udfu.integer = 1;
+ tmp->u.udfu.string = g_strdup("str");
+ tmp->u.udfu.enum1 = ENUM_ONE_VALUE1;
+ tmp->u.udfu.u.value1 = g_new0(UserDefA, 1);
+ tmp->u.udfu.u.value1->boolean = true;
visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
arg = qmp_output_get_qobject(data->qov);
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate Eric Blake
@ 2016-02-18 16:43 ` Markus Armbruster
2016-02-18 16:56 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 16:43 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> There's no reason to do two malloc's for an alternate type visiting
> a QAPI struct; let's just inline the struct directly as the C union
> branch of the struct.
>
> Surprisingly, no clients were actually using the struct member prior
> to this patch outside of the testsuite; an earlier patch in the series
> added some testsuite coverage to make the effect of this patch more
> obvious.
>
> In qapi.py, c_type() gains a new is_unboxed flag to control when we
> are emitting a C struct unboxed within the context of an outer
> struct (different from our other two modes of usage with no flags
> for normal local variable declarations, and with is_param for adding
> 'const' in a parameter list). I don't know if there is any more
> pythonic way of collapsing the two flags into a single parameter,
> as we never have a caller setting both flags at once.
>
> Ultimately, we want to also unbox branches for QAPI unions, but as
> that touches a lot more client code, it is better as separate
> patches. But since unions and alternates share gen_variants(), I
> had to hack in a way to test if we are visiting an alternate type
> for setting the is_unboxed flag: look for a non-object branch.
> This works because alternates have at least two branches, with at
> most one object branch, while unions have only object branches.
> The hack will go away in a later patch.
>
> The generated code difference to qapi-types.h is relatively small:
>
> | struct BlockdevRef {
> | QType type;
> | union { /* union tag is @type */
> | void *data;
> |- BlockdevOptions *definition;
> |+ BlockdevOptions definition;
> | char *reference;
> | } u;
> | };
>
> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj)
Meanwhile
> (which allocates a pointer, handles a layer of {} from the JSON stream,
> and visits the fields) with an inline call to visit_type_FOO(NULL) (to
> consume the {} without allocation) and a direct visit of the fields:
I don't see a call to visit_type_FOO(NULL).
Suggest not to abbreviate argument lists, it's mildly confusing.
>
> | switch ((*obj)->type) {
> | case QTYPE_QDICT:
> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
> |+ visit_start_struct(v, name, NULL, 0, &err);
> |+ if (err) {
> |+ break;
> |+ }
> |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err);
> |+ error_propagate(errp, err);
> |+ err = NULL;
> |+ visit_end_struct(v, &err);
> | break;
> | case QTYPE_QSTRING:
> | visit_type_str(v, name, &(*obj)->u.reference, &err);
>
> The visit of non-object fields is unchanged.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Patch looks good.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate
2016-02-18 16:43 ` Markus Armbruster
@ 2016-02-18 16:56 ` Eric Blake
2016-02-18 18:56 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 16:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]
On 02/18/2016 09:43 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> There's no reason to do two malloc's for an alternate type visiting
>> a QAPI struct; let's just inline the struct directly as the C union
>> branch of the struct.
>>
>>
>> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj)
>
> Meanwhile
>
>> (which allocates a pointer, handles a layer of {} from the JSON stream,
>> and visits the fields)
Maybe:
(which first calls visit_start_struct(v, name, obj, size, &err) to
allocate a pointer and handle a layer of {} from the JSON stream, then
visits the fields)
>> with an inline call to visit_type_FOO(NULL) (to
>> consume the {} without allocation) and a direct visit of the fields:
>
> I don't see a call to visit_type_FOO(NULL).
Whoops, wrong function name. I meant:
visit_start_struct(v, name, NULL, 0, &err).
^^^^
Especially useful if you take my earlier text suggestion to point out
the difference in the visit_start_struct() calls as being the
intentional so that we continue to consume {} but now opt out of a
second allocation.
>
> Suggest not to abbreviate argument lists, it's mildly confusing.
Sure. Are we still at a point that you could touch up the commit message
without a patch respin?
>
> Patch looks good.
>
--
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] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate
2016-02-18 16:56 ` Eric Blake
@ 2016-02-18 18:56 ` Markus Armbruster
2016-02-18 20:00 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 18:56 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/18/2016 09:43 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> There's no reason to do two malloc's for an alternate type visiting
>>> a QAPI struct; let's just inline the struct directly as the C union
>>> branch of the struct.
>>>
>>>
>>> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj)
>>
>> Meanwhile
>>
>>> (which allocates a pointer, handles a layer of {} from the JSON stream,
>>> and visits the fields)
>
> Maybe:
>
> (which first calls visit_start_struct(v, name, obj, size, &err) to
> allocate a pointer and handle a layer of {} from the JSON stream, then
> visits the fields)
>
>>> with an inline call to visit_type_FOO(NULL) (to
>>> consume the {} without allocation) and a direct visit of the fields:
>>
>> I don't see a call to visit_type_FOO(NULL).
>
> Whoops, wrong function name. I meant:
>
> visit_start_struct(v, name, NULL, 0, &err).
> ^^^^
> Especially useful if you take my earlier text suggestion to point out
> the difference in the visit_start_struct() calls as being the
> intentional so that we continue to consume {} but now opt out of a
> second allocation.
Better, but the sentence is long enough to confuse even a German. What
about:
The corresponding spot in qapi-visit.c calls visit_type_FOO(), which
first calls visit_start_struct() to allocate or deallocate the member
and handle a layer of {} from the JSON stream, then visits the
members. To peel off the indirection and the memory management that
comes with it, we inline this call, then suppress allocation /
deallocation by passing NULL to visit_start_struct(), and adjust the
member visit:
>>
>> Suggest not to abbreviate argument lists, it's mildly confusing.
>
> Sure. Are we still at a point that you could touch up the commit message
> without a patch respin?
Looks like it so far.
>>
>> Patch looks good.
>>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate
2016-02-18 18:56 ` Markus Armbruster
@ 2016-02-18 20:00 ` Eric Blake
0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 20:00 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 943 bytes --]
On 02/18/2016 11:56 AM, Markus Armbruster wrote:
> Better, but the sentence is long enough to confuse even a German. What
> about:
lol
>
> The corresponding spot in qapi-visit.c calls visit_type_FOO(), which
> first calls visit_start_struct() to allocate or deallocate the member
> and handle a layer of {} from the JSON stream, then visits the
> members. To peel off the indirection and the memory management that
> comes with it, we inline this call, then suppress allocation /
> deallocation by passing NULL to visit_start_struct(), and adjust the
> member visit:
Works for me. [Technically, visit_start_struct() never deallocates, it
merely records a pointer for later deallocation during
visit_end_struct(); but I don't think we need to worry about the
imprecision here]
--
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] 41+ messages in thread
* [Qemu-devel] [PATCH v11 13/15] qapi: Don't box branches of flat unions
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (11 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 16:51 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct() Eric Blake
` (3 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Crosthwaite, armbru, Michael Roth, Luiz Capitulino,
Paolo Bonzini, Richard Henderson
There's no reason to do two malloc's for a flat union; let's just
inline the branch struct directly into the C union branch of the
flat union.
Surprisingly, fewer clients were actually using explicit references
to the branch types in comparison to the number of flat unions
thus modified.
This lets us reduce the hack in qapi-types:gen_variants() added in
the previous patch; we no longer need to distinguish between
alternates and flat unions.
The change to unboxed structs means that u.data (added in commit
cee2dedb) is now coincident with random fields of each branch of
the flat union, whereas beforehand it was only coincident with
pointers (since all branches of a flat union have to be objects).
Note that this was already the case for simple unions - but there
we got lucky. Remember, visit_start_union() blindly returns true
for all visitors except for the dealloc visitor, where it returns
the value !!obj->u.data, and that this result then controls
whether to proceed with the visit to the variant. Pre-patch,
this meant that flat unions were testing whether the boxed pointer
was still NULL, and thereby skipping visit_end_implicit_struct()
and avoiding a NULL dereference if the pointer had not been
allocated. The same was true for simple unions where the current
branch had pointer type, except there we bypassed visit_type_FOO().
But for simple unions where the current branch had scalar type, the
contents of that scalar meant that the decision to call
visit_type_FOO() was data-dependent - the reason we got lucky there
is that visit_type_FOO() for all scalar types in the dealloc visitor
is a no-op (only the pointer variants had anything to free), so it
did not matter whether the dealloc visit was skipped. But with this
patch, we would risk leaking memory if we could skip a call to
visit_type_FOO_fields() based solely on a data-dependent decision.
But notice: in the dealloc visitor, visit_type_FOO() already handles
a NULL obj - it was only the visit_type_implicit_FOO() that was
failing to check for NULL. And now that we have refactored things to
have the branch be part of the parent struct, we no longer have a
separate pointer that can be NULL in the first place. So we can just
delete the call to visit_start_union() altogether, and blindly visit
the branch type; there is no change in behavior except to the dealloc
visitor, where we now unconditionally visit the branch, but where that
visit is now always safe (for a flat union, we can no longer
dereference NULL, and for a simple union, visit_type_FOO() was already
safely handling NULL on pointer types).
Unfortunately, simple unions are not as easy to switch to unboxed
layout; because we are special-casing the hidden implicit type with
a single 'data' member, we really DO need to keep calling another
layer of visit_start_struct(), with a second malloc; although there
are some cleanups planned for simple unions in later patches.
Note that after this patch, visit_start_union() is unused, and the
only remaining use of visit_start_implicit_struct() is for alternate
types; the next couple of patches will do further cleanups based on
that fact.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rebase to earlier changes, save cleanups for later, fix bug
with visit_start_union now being actively wrong
v10: new patch
---
scripts/qapi-types.py | 13 +++----------
scripts/qapi-visit.py | 7 ++-----
cpus.c | 18 ++++++------------
hmp.c | 12 ++++++------
tests/test-qmp-input-visitor.c | 10 +++++-----
tests/test-qmp-output-visitor.c | 6 ++----
6 files changed, 24 insertions(+), 42 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4dabe91..eac90d2 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
def gen_variants(variants):
- # HACK: Determine if this is an alternate (at least one variant
- # is not an object); unions have all branches as objects.
- unboxed = False
- for v in variants.variants:
- if not isinstance(v.type, QAPISchemaObjectType):
- unboxed = True
- break
-
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
# whether to bypass the switch statement if visiting the discriminator
@@ -140,11 +132,12 @@ def gen_variants(variants):
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
- typ = var.simple_union_type() or var.type
+ simple_union_type = var.simple_union_type()
+ typ = simple_union_type or var.type
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=typ.c_type(is_unboxed=unboxed),
+ c_type=typ.c_type(is_unboxed=not simple_union_type),
c_name=c_name(var.name))
ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 61b7e72..367c459 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -79,7 +79,7 @@ def gen_visit_struct_fields(name, base, members, variants=None):
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
if not var.simple_union_type():
- ret += gen_visit_implicit_struct(var.type)
+ ret += gen_visit_fields_decl(var.type)
struct_fields_seen.add(name)
ret += mcgen('''
@@ -102,9 +102,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er
if variants:
ret += mcgen('''
- if (!visit_start_union(v, !!obj->u.data, &err) || err) {
- goto out;
- }
switch (obj->%(c_name)s) {
''',
c_name=c_name(variants.tag_member.name))
@@ -126,7 +123,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er
c_name=c_name(var.name))
else:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err);
+ visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
diff --git a/cpus.c b/cpus.c
index 898426c..9592163 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1568,28 +1568,22 @@ CpuInfoList *qmp_query_cpus(Error **errp)
info->value->thread_id = cpu->thread_id;
#if defined(TARGET_I386)
info->value->arch = CPU_INFO_ARCH_X86;
- info->value->u.x86 = g_new0(CpuInfoX86, 1);
- info->value->u.x86->pc = env->eip + env->segs[R_CS].base;
+ info->value->u.x86.pc = env->eip + env->segs[R_CS].base;
#elif defined(TARGET_PPC)
info->value->arch = CPU_INFO_ARCH_PPC;
- info->value->u.ppc = g_new0(CpuInfoPPC, 1);
- info->value->u.ppc->nip = env->nip;
+ info->value->u.ppc.nip = env->nip;
#elif defined(TARGET_SPARC)
info->value->arch = CPU_INFO_ARCH_SPARC;
- info->value->u.q_sparc = g_new0(CpuInfoSPARC, 1);
- info->value->u.q_sparc->pc = env->pc;
- info->value->u.q_sparc->npc = env->npc;
+ info->value->u.q_sparc.pc = env->pc;
+ info->value->u.q_sparc.npc = env->npc;
#elif defined(TARGET_MIPS)
info->value->arch = CPU_INFO_ARCH_MIPS;
- info->value->u.q_mips = g_new0(CpuInfoMIPS, 1);
- info->value->u.q_mips->PC = env->active_tc.PC;
+ info->value->u.q_mips.PC = env->active_tc.PC;
#elif defined(TARGET_TRICORE)
info->value->arch = CPU_INFO_ARCH_TRICORE;
- info->value->u.tricore = g_new0(CpuInfoTricore, 1);
- info->value->u.tricore->PC = env->PC;
+ info->value->u.tricore.PC = env->PC;
#else
info->value->arch = CPU_INFO_ARCH_OTHER;
- info->value->u.other = g_new0(CpuInfoOther, 1);
#endif
/* XXX: waiting for the qapi to support GSList */
diff --git a/hmp.c b/hmp.c
index 996cb91..bfbd667 100644
--- a/hmp.c
+++ b/hmp.c
@@ -314,22 +314,22 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
switch (cpu->value->arch) {
case CPU_INFO_ARCH_X86:
- monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86->pc);
+ monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc);
break;
case CPU_INFO_ARCH_PPC:
- monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc->nip);
+ monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip);
break;
case CPU_INFO_ARCH_SPARC:
monitor_printf(mon, " pc=0x%016" PRIx64,
- cpu->value->u.q_sparc->pc);
+ cpu->value->u.q_sparc.pc);
monitor_printf(mon, " npc=0x%016" PRIx64,
- cpu->value->u.q_sparc->npc);
+ cpu->value->u.q_sparc.npc);
break;
case CPU_INFO_ARCH_MIPS:
- monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips->PC);
+ monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC);
break;
case CPU_INFO_ARCH_TRICORE:
- monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore->PC);
+ monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
break;
default:
break;
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 47cf6aa..b05da5b 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -295,7 +295,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1);
g_assert_cmpstr(tmp->string, ==, "str");
g_assert_cmpint(tmp->integer, ==, 41);
- g_assert_cmpint(tmp->u.value1->boolean, ==, true);
+ g_assert_cmpint(tmp->u.value1.boolean, ==, true);
base = qapi_UserDefFlatUnion_base(tmp);
g_assert(&base->enum1 == &tmp->enum1);
@@ -330,8 +330,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(tmp->u.udfu.integer, ==, 1);
g_assert_cmpstr(tmp->u.udfu.string, ==, "str");
g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
- g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true);
- g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false);
+ g_assert_cmpint(tmp->u.udfu.u.value1.boolean, ==, true);
+ g_assert_cmpint(tmp->u.udfu.u.value1.has_a_b, ==, false);
qapi_free_UserDefAlternate(tmp);
v = visitor_input_test_init(data, "false");
@@ -358,8 +358,8 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1);
g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str");
g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1);
- g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true);
- g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false);
+ g_assert_cmpint(wrap->alt->u.udfu.u.value1.boolean, ==, true);
+ g_assert_cmpint(wrap->alt->u.udfu.u.value1.has_a_b, ==, false);
qapi_free_WrapAlternate(wrap);
}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index fe2f1a1..a7f8b45 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -403,9 +403,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
tmp->enum1 = ENUM_ONE_VALUE1;
tmp->string = g_strdup("str");
- tmp->u.value1 = g_malloc0(sizeof(UserDefA));
tmp->integer = 41;
- tmp->u.value1->boolean = true;
+ tmp->u.value1.boolean = true;
visit_type_UserDefFlatUnion(data->ov, NULL, &tmp, &error_abort);
arg = qmp_output_get_qobject(data->qov);
@@ -460,8 +459,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
tmp->u.udfu.integer = 1;
tmp->u.udfu.string = g_strdup("str");
tmp->u.udfu.enum1 = ENUM_ONE_VALUE1;
- tmp->u.udfu.u.value1 = g_new0(UserDefA, 1);
- tmp->u.udfu.u.value1->boolean = true;
+ tmp->u.udfu.u.value1.boolean = true;
visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
arg = qmp_output_get_qobject(data->qov);
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 13/15] qapi: Don't box branches of flat unions
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 13/15] qapi: Don't box branches of flat unions Eric Blake
@ 2016-02-18 16:51 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 16:51 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, qemu-devel, Michael Roth, Luiz Capitulino,
Paolo Bonzini, Richard Henderson
Eric Blake <eblake@redhat.com> writes:
> There's no reason to do two malloc's for a flat union; let's just
> inline the branch struct directly into the C union branch of the
> flat union.
>
> Surprisingly, fewer clients were actually using explicit references
> to the branch types in comparison to the number of flat unions
> thus modified.
>
> This lets us reduce the hack in qapi-types:gen_variants() added in
> the previous patch; we no longer need to distinguish between
> alternates and flat unions.
>
> The change to unboxed structs means that u.data (added in commit
> cee2dedb) is now coincident with random fields of each branch of
> the flat union, whereas beforehand it was only coincident with
> pointers (since all branches of a flat union have to be objects).
> Note that this was already the case for simple unions - but there
> we got lucky. Remember, visit_start_union() blindly returns true
> for all visitors except for the dealloc visitor, where it returns
> the value !!obj->u.data, and that this result then controls
> whether to proceed with the visit to the variant. Pre-patch,
> this meant that flat unions were testing whether the boxed pointer
> was still NULL, and thereby skipping visit_end_implicit_struct()
> and avoiding a NULL dereference if the pointer had not been
> allocated. The same was true for simple unions where the current
> branch had pointer type, except there we bypassed visit_type_FOO().
> But for simple unions where the current branch had scalar type, the
> contents of that scalar meant that the decision to call
> visit_type_FOO() was data-dependent - the reason we got lucky there
> is that visit_type_FOO() for all scalar types in the dealloc visitor
> is a no-op (only the pointer variants had anything to free), so it
> did not matter whether the dealloc visit was skipped. But with this
> patch, we would risk leaking memory if we could skip a call to
> visit_type_FOO_fields() based solely on a data-dependent decision.
>
> But notice: in the dealloc visitor, visit_type_FOO() already handles
> a NULL obj - it was only the visit_type_implicit_FOO() that was
> failing to check for NULL. And now that we have refactored things to
> have the branch be part of the parent struct, we no longer have a
> separate pointer that can be NULL in the first place. So we can just
> delete the call to visit_start_union() altogether, and blindly visit
> the branch type; there is no change in behavior except to the dealloc
> visitor, where we now unconditionally visit the branch, but where that
> visit is now always safe (for a flat union, we can no longer
> dereference NULL, and for a simple union, visit_type_FOO() was already
> safely handling NULL on pointer types).
>
> Unfortunately, simple unions are not as easy to switch to unboxed
> layout; because we are special-casing the hidden implicit type with
> a single 'data' member, we really DO need to keep calling another
> layer of visit_start_struct(), with a second malloc; although there
> are some cleanups planned for simple unions in later patches.
>
> Note that after this patch, visit_start_union() is unused, and the
> only remaining use of visit_start_implicit_struct() is for alternate
> types; the next couple of patches will do further cleanups based on
> that fact.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Bonus: qapi-visit.c shrinks a bit, shedding all the
visit_type_implicit_FOO().
> ---
> v11: rebase to earlier changes, save cleanups for later, fix bug
> with visit_start_union now being actively wrong
> v10: new patch
> ---
> scripts/qapi-types.py | 13 +++----------
> scripts/qapi-visit.py | 7 ++-----
> cpus.c | 18 ++++++------------
> hmp.c | 12 ++++++------
> tests/test-qmp-input-visitor.c | 10 +++++-----
> tests/test-qmp-output-visitor.c | 6 ++----
> 6 files changed, 24 insertions(+), 42 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4dabe91..eac90d2 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
>
>
> def gen_variants(variants):
> - # HACK: Determine if this is an alternate (at least one variant
> - # is not an object); unions have all branches as objects.
> - unboxed = False
> - for v in variants.variants:
> - if not isinstance(v.type, QAPISchemaObjectType):
> - unboxed = True
> - break
> -
> # FIXME: What purpose does data serve, besides preventing a union that
> # has a branch named 'data'? We use it in qapi-visit.py to decide
> # whether to bypass the switch statement if visiting the discriminator
> @@ -140,11 +132,12 @@ def gen_variants(variants):
>
> for var in variants.variants:
> # Ugly special case for simple union TODO get rid of it
> - typ = var.simple_union_type() or var.type
> + simple_union_type = var.simple_union_type()
> + typ = simple_union_type or var.type
> ret += mcgen('''
> %(c_type)s %(c_name)s;
> ''',
> - c_type=typ.c_type(is_unboxed=unboxed),
> + c_type=typ.c_type(is_unboxed=not simple_union_type),
> c_name=c_name(var.name))
>
> ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 61b7e72..367c459 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -79,7 +79,7 @@ def gen_visit_struct_fields(name, base, members, variants=None):
> for var in variants.variants:
> # Ugly special case for simple union TODO get rid of it
> if not var.simple_union_type():
> - ret += gen_visit_implicit_struct(var.type)
> + ret += gen_visit_fields_decl(var.type)
>
> struct_fields_seen.add(name)
> ret += mcgen('''
> @@ -102,9 +102,6 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er
>
> if variants:
> ret += mcgen('''
> - if (!visit_start_union(v, !!obj->u.data, &err) || err) {
> - goto out;
> - }
> switch (obj->%(c_name)s) {
> ''',
> c_name=c_name(variants.tag_member.name))
> @@ -126,7 +123,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **er
> c_name=c_name(var.name))
> else:
> ret += mcgen('''
> - visit_type_implicit_%(c_type)s(v, &obj->u.%(c_name)s, &err);
> + visit_type_%(c_type)s_fields(v, &obj->u.%(c_name)s, &err);
> ''',
> c_type=var.type.c_name(),
> c_name=c_name(var.name))
The change has become quite simple. I take that as a good sign.
[...]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct()
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (12 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 13/15] qapi: Don't box branches of flat unions Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 16:52 ` Markus Armbruster
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 15/15] qapi: Change visit_start_implicit_struct to visit_start_alternate Eric Blake
` (2 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Delete code rendered dead in the previous patch. As explained
there, we no longer have any need to use visit_start_struct(),
and we no longer have any more code trying to create or use
visit_type_implicit_FOO().
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: retitle, merge multiple cleanups, avoid bisection bug by hoisting
visit_start_union() hunk to patch where it matters
v10: retitle, hoist earlier in series, rebase, drop R-b
v9: no change
v8: rebase to 'name' motion
v7: rebase to earlier context changes, simplify 'obj && !*obj'
condition based on contract
v6: rebase due to deferring 7/46, and gen_err_check() improvements;
rewrite gen_visit_implicit_struct() more like other patterns
---
include/qapi/visitor.h | 1 -
include/qapi/visitor-impl.h | 2 --
scripts/qapi-visit.py | 29 -----------------------------
qapi/qapi-visit-core.c | 8 --------
qapi/qapi-dealloc-visitor.c | 26 --------------------------
5 files changed, 66 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 8a2d5cc..a6678b1 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -79,6 +79,5 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
void visit_type_number(Visitor *v, const char *name, double *obj,
Error **errp);
void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);
#endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7905a28..c4af3e0 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -58,8 +58,6 @@ struct Visitor
/* May be NULL; most useful for input visitors. */
void (*optional)(Visitor *v, const char *name, bool *present);
-
- bool (*start_union)(Visitor *v, bool data_present, Error **errp);
};
void input_type_enum(Visitor *v, const char *name, int *obj,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 367c459..d2c2dc0 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
from qapi import *
import re
-# visit_type_FOO_implicit() is emitted as needed; track if it has already
-# been output.
-implicit_structs_seen = set()
-
# visit_type_FOO_fields() is always emitted; track if a forward declaration
# or implementation has already been output.
struct_fields_seen = set()
@@ -45,31 +41,6 @@ static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **er
c_type=typ.c_name())
-def gen_visit_implicit_struct(typ):
- if typ in implicit_structs_seen:
- return ''
- implicit_structs_seen.add(typ)
-
- ret = gen_visit_fields_decl(typ)
-
- ret += mcgen('''
-
-static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp)
-{
- Error *err = NULL;
-
- visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
- if (!err) {
- visit_type_%(c_type)s_fields(v, *obj, errp);
- visit_end_implicit_struct(v);
- }
- error_propagate(errp, err);
-}
-''',
- c_type=typ.c_name())
- return ret
-
-
def gen_visit_struct_fields(name, base, members, variants=None):
ret = ''
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index b4a0f21..f7b9980 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -61,14 +61,6 @@ void visit_end_list(Visitor *v)
v->end_list(v);
}
-bool visit_start_union(Visitor *v, bool data_present, Error **errp)
-{
- if (v->start_union) {
- return v->start_union(v, data_present, errp);
- }
- return true;
-}
-
bool visit_optional(Visitor *v, const char *name, bool *present)
{
if (v->optional) {
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 6667e8c..4eae555 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -169,31 +169,6 @@ static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj,
{
}
-/* If there's no data present, the dealloc visitor has nothing to free.
- * Thus, indicate to visitor code that the subsequent union fields can
- * be skipped. This is not an error condition, since the cleanup of the
- * rest of an object can continue unhindered, so leave errp unset in
- * these cases.
- *
- * NOTE: In cases where we're attempting to deallocate an object that
- * may have missing fields, the field indicating the union type may
- * be missing. In such a case, it's possible we don't have enough
- * information to differentiate data_present == false from a case where
- * data *is* present but happens to be a scalar with a value of 0.
- * This is okay, since in the case of the dealloc visitor there's no
- * work that needs to done in either situation.
- *
- * The current inability in QAPI code to more thoroughly verify a union
- * type in such cases will likely need to be addressed if we wish to
- * implement this interface for other types of visitors in the future,
- * however.
- */
-static bool qapi_dealloc_start_union(Visitor *v, bool data_present,
- Error **errp)
-{
- return data_present;
-}
-
Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
{
return &v->visitor;
@@ -224,7 +199,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
v->visitor.type_str = qapi_dealloc_type_str;
v->visitor.type_number = qapi_dealloc_type_number;
v->visitor.type_any = qapi_dealloc_type_anything;
- v->visitor.start_union = qapi_dealloc_start_union;
QTAILQ_INIT(&v->stack);
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct()
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct() Eric Blake
@ 2016-02-18 16:52 ` Markus Armbruster
2016-02-18 17:00 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 16:52 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Delete code rendered dead in the previous patch. As explained
> there, we no longer have any need to use visit_start_struct(),
> and we no longer have any more code trying to create or use
> visit_type_implicit_FOO().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
The previous patch might be simple enough to permit squashing this one
in. Your choice.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct()
2016-02-18 16:52 ` Markus Armbruster
@ 2016-02-18 17:00 ` Eric Blake
2016-02-18 17:12 ` Markus Armbruster
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2016-02-18 17:00 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 934 bytes --]
On 02/18/2016 09:52 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Delete code rendered dead in the previous patch. As explained
>> there, we no longer have any need to use visit_start_struct(),
>> and we no longer have any more code trying to create or use
>> visit_type_implicit_FOO().
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> The previous patch might be simple enough to permit squashing this one
> in. Your choice.
>
13/15:
6 files changed, 24 insertions(+), 42 deletions(-)
14/15:
5 files changed, 66 deletions(-)
Yeah, squashing them makes for a more impressive diffstat, while still
not being too overwhelming with too much done in one patch. I'm
assuming you could handle the squashing as part of promoting to your
qapi-next, if you want?
--
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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct()
2016-02-18 17:00 ` Eric Blake
@ 2016-02-18 17:12 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 17:12 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/18/2016 09:52 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Delete code rendered dead in the previous patch. As explained
>>> there, we no longer have any need to use visit_start_struct(),
>>> and we no longer have any more code trying to create or use
>>> visit_type_implicit_FOO().
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> The previous patch might be simple enough to permit squashing this one
>> in. Your choice.
>>
>
> 13/15:
> 6 files changed, 24 insertions(+), 42 deletions(-)
> 14/15:
> 5 files changed, 66 deletions(-)
>
> Yeah, squashing them makes for a more impressive diffstat, while still
> not being too overwhelming with too much done in one patch. I'm
> assuming you could handle the squashing as part of promoting to your
> qapi-next, if you want?
Can do.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v11 15/15] qapi: Change visit_start_implicit_struct to visit_start_alternate
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (13 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 14/15] qapi: Delete visit_start_union(), gen_visit_implicit_struct() Eric Blake
@ 2016-02-18 6:48 ` Eric Blake
2016-02-18 10:09 ` [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Markus Armbruster
2016-02-18 20:08 ` Markus Armbruster
16 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2016-02-18 6:48 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
After recent changes, the only remaining use of
visit_start_implicit_struct() is for allocating the space needed
when visiting an alternate. Since the term 'implicit struct' is
hard to explain, rename the function to its current usage. While
at it, we can merge the functionality of visit_get_next_type()
into the same function, making it more like visit_start_struct().
Generated code is now slightly smaller:
| {
| Error *err = NULL;
|
|- visit_start_implicit_struct(v, (void**) obj, sizeof(BlockdevRef), &err);
|+ visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
|+ true, &err);
| if (err) {
| goto out;
| }
|- visit_get_next_type(v, name, &(*obj)->type, true, &err);
|- if (err) {
|- goto out_obj;
|- }
| switch ((*obj)->type) {
| case QTYPE_QDICT:
| visit_start_struct(v, name, NULL, 0, &err);
...
| }
|-out_obj:
|- visit_end_implicit_struct(v);
|+ visit_end_alternate(v);
| out:
| error_propagate(errp, err);
| }
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rebase to earlier changes
v10: new patch
---
include/qapi/visitor.h | 49 +++++++++++++++++++++++++++++++++++----------
include/qapi/visitor-impl.h | 17 ++++++++--------
scripts/qapi-visit.py | 10 +++------
qapi/qapi-visit-core.c | 40 ++++++++++++++++--------------------
qapi/qapi-dealloc-visitor.c | 13 ++++++------
qapi/qmp-input-visitor.c | 24 +++++++++-------------
6 files changed, 82 insertions(+), 71 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a6678b1..83cad74 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,17 +27,52 @@ typedef struct GenericList {
char padding[];
} GenericList;
+/* This struct is layout-compatible with all Alternate types
+ * created by the qapi generator. */
+typedef struct GenericAlternate {
+ QType type;
+ char padding[];
+} GenericAlternate;
+
void visit_start_struct(Visitor *v, const char *name, void **obj,
size_t size, Error **errp);
void visit_end_struct(Visitor *v, Error **errp);
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
- Error **errp);
-void visit_end_implicit_struct(Visitor *v);
void visit_start_list(Visitor *v, const char *name, Error **errp);
GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
void visit_end_list(Visitor *v);
+/*
+ * Start the visit of an alternate @obj with the given @size.
+ *
+ * @name specifies the relationship to the containing struct (ignored
+ * for a top level visit, the name of the key if this alternate is
+ * part of an object, or NULL if this alternate is part of a list).
+ *
+ * @obj must not be NULL. Input visitors will allocate @obj and
+ * determine the qtype of the next thing to be visited, stored in
+ * (*@obj)->type. Other visitors will leave @obj unchanged.
+ *
+ * If @promote_int, treat integers as QTYPE_FLOAT.
+ *
+ * If successful, this must be paired with visit_end_alternate(), even
+ * if visiting the contents of the alternate fails.
+ */
+void visit_start_alternate(Visitor *v, const char *name,
+ GenericAlternate **obj, size_t size,
+ bool promote_int, Error **errp);
+
+/*
+ * Finish visiting an alternate type.
+ *
+ * Must be called after a successful visit_start_alternate(), even if
+ * an error occurred in the meantime.
+ *
+ * TODO: Should all the visit_end_* interfaces take obj parameter, so
+ * that dealloc visitor need not track what was passed in visit_start?
+ */
+void visit_end_alternate(Visitor *v);
+
/**
* Check if an optional member @name of an object needs visiting.
* For input visitors, set *@present according to whether the
@@ -46,14 +81,6 @@ void visit_end_list(Visitor *v);
*/
bool visit_optional(Visitor *v, const char *name, bool *present);
-/**
- * Determine the qtype of the item @name in the current object visit.
- * For input visitors, set *@type to the correct qtype of a qapi
- * alternate type; for other visitors, leave *@type unchanged.
- * If @promote_int, treat integers as QTYPE_FLOAT.
- */
-void visit_get_next_type(Visitor *v, const char *name, QType *type,
- bool promote_int, Error **errp);
void visit_type_enum(Visitor *v, const char *name, int *obj,
const char *const strings[], Error **errp);
void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index c4af3e0..6a1ddfb 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -22,22 +22,23 @@ struct Visitor
size_t size, Error **errp);
void (*end_struct)(Visitor *v, Error **errp);
- void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
- Error **errp);
- /* May be NULL */
- void (*end_implicit_struct)(Visitor *v);
-
void (*start_list)(Visitor *v, const char *name, Error **errp);
/* Must be set */
GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
/* Must be set */
void (*end_list)(Visitor *v);
+ /* Optional, needed for input and dealloc visitors. */
+ void (*start_alternate)(Visitor *v, const char *name,
+ GenericAlternate **obj, size_t size,
+ bool promote_int, Error **errp);
+
+ /* Optional, needed for dealloc visitor. */
+ void (*end_alternate)(Visitor *v);
+
+ /* Must be set. */
void (*type_enum)(Visitor *v, const char *name, int *obj,
const char *const strings[], Error **errp);
- /* May be NULL; only needed for input visitors. */
- void (*get_next_type)(Visitor *v, const char *name, QType *type,
- bool promote_int, Error **errp);
/* Must be set. */
void (*type_int64)(Visitor *v, const char *name, int64_t *obj,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d2c2dc0..69a6f8e 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -182,14 +182,11 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
{
Error *err = NULL;
- visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
+ visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
+ %(promote_int)s, &err);
if (err) {
goto out;
}
- visit_get_next_type(v, name, &(*obj)->type, %(promote_int)s, &err);
- if (err) {
- goto out_obj;
- }
switch ((*obj)->type) {
''',
c_name=c_name(name), promote_int=promote_int)
@@ -227,8 +224,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"%(name)s");
}
-out_obj:
- visit_end_implicit_struct(v);
+ visit_end_alternate(v);
out:
error_propagate(errp, err);
}
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index f7b9980..856606b 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -30,21 +30,6 @@ void visit_end_struct(Visitor *v, Error **errp)
v->end_struct(v, errp);
}
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
- Error **errp)
-{
- if (v->start_implicit_struct) {
- v->start_implicit_struct(v, obj, size, errp);
- }
-}
-
-void visit_end_implicit_struct(Visitor *v)
-{
- if (v->end_implicit_struct) {
- v->end_implicit_struct(v);
- }
-}
-
void visit_start_list(Visitor *v, const char *name, Error **errp)
{
v->start_list(v, name, errp);
@@ -61,6 +46,23 @@ void visit_end_list(Visitor *v)
v->end_list(v);
}
+void visit_start_alternate(Visitor *v, const char *name,
+ GenericAlternate **obj, size_t size,
+ bool promote_int, Error **errp)
+{
+ assert(obj && size >= sizeof(GenericAlternate));
+ if (v->start_alternate) {
+ v->start_alternate(v, name, obj, size, promote_int, errp);
+ }
+}
+
+void visit_end_alternate(Visitor *v)
+{
+ if (v->end_alternate) {
+ v->end_alternate(v);
+ }
+}
+
bool visit_optional(Visitor *v, const char *name, bool *present)
{
if (v->optional) {
@@ -69,14 +71,6 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
return *present;
}
-void visit_get_next_type(Visitor *v, const char *name, QType *type,
- bool promote_int, Error **errp)
-{
- if (v->get_next_type) {
- v->get_next_type(v, name, type, promote_int, errp);
- }
-}
-
void visit_type_enum(Visitor *v, const char *name, int *obj,
const char *const strings[], Error **errp)
{
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 4eae555..6922179 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -76,16 +76,15 @@ static void qapi_dealloc_end_struct(Visitor *v, Error **errp)
}
}
-static void qapi_dealloc_start_implicit_struct(Visitor *v,
- void **obj,
- size_t size,
- Error **errp)
+static void qapi_dealloc_start_alternate(Visitor *v, const char *name,
+ GenericAlternate **obj, size_t size,
+ bool promote_int, Error **errp)
{
QapiDeallocVisitor *qov = to_qov(v);
qapi_dealloc_push(qov, obj);
}
-static void qapi_dealloc_end_implicit_struct(Visitor *v)
+static void qapi_dealloc_end_alternate(Visitor *v)
{
QapiDeallocVisitor *qov = to_qov(v);
void **obj = qapi_dealloc_pop(qov);
@@ -187,8 +186,8 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
v->visitor.start_struct = qapi_dealloc_start_struct;
v->visitor.end_struct = qapi_dealloc_end_struct;
- v->visitor.start_implicit_struct = qapi_dealloc_start_implicit_struct;
- v->visitor.end_implicit_struct = qapi_dealloc_end_implicit_struct;
+ v->visitor.start_alternate = qapi_dealloc_start_alternate;
+ v->visitor.end_alternate = qapi_dealloc_end_alternate;
v->visitor.start_list = qapi_dealloc_start_list;
v->visitor.next_list = qapi_dealloc_next_list;
v->visitor.end_list = qapi_dealloc_end_list;
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 2621660..e659832 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -143,14 +143,6 @@ static void qmp_input_end_struct(Visitor *v, Error **errp)
qmp_input_pop(qiv, errp);
}
-static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
- size_t size, Error **errp)
-{
- if (obj) {
- *obj = g_malloc0(size);
- }
-}
-
static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -202,19 +194,22 @@ static void qmp_input_end_list(Visitor *v)
qmp_input_pop(qiv, &error_abort);
}
-static void qmp_input_get_next_type(Visitor *v, const char *name, QType *type,
- bool promote_int, Error **errp)
+static void qmp_input_start_alternate(Visitor *v, const char *name,
+ GenericAlternate **obj, size_t size,
+ bool promote_int, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
QObject *qobj = qmp_input_get_object(qiv, name, false);
if (!qobj) {
+ *obj = NULL;
error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
return;
}
- *type = qobject_type(qobj);
- if (promote_int && *type == QTYPE_QINT) {
- *type = QTYPE_QFLOAT;
+ *obj = g_malloc0(size);
+ (*obj)->type = qobject_type(qobj);
+ if (promote_int && (*obj)->type == QTYPE_QINT) {
+ (*obj)->type = QTYPE_QFLOAT;
}
}
@@ -345,10 +340,10 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
v->visitor.start_struct = qmp_input_start_struct;
v->visitor.end_struct = qmp_input_end_struct;
- v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
v->visitor.start_list = qmp_input_start_list;
v->visitor.next_list = qmp_input_next_list;
v->visitor.end_list = qmp_input_end_list;
+ v->visitor.start_alternate = qmp_input_start_alternate;
v->visitor.type_enum = input_type_enum;
v->visitor.type_int64 = qmp_input_type_int64;
v->visitor.type_uint64 = qmp_input_type_uint64;
@@ -357,7 +352,6 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
v->visitor.type_number = qmp_input_type_number;
v->visitor.type_any = qmp_input_type_any;
v->visitor.optional = qmp_input_optional;
- v->visitor.get_next_type = qmp_input_get_next_type;
qmp_input_push(v, obj, NULL);
qobject_incref(obj);
--
2.5.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E)
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (14 preceding siblings ...)
2016-02-18 6:48 ` [Qemu-devel] [PATCH v11 15/15] qapi: Change visit_start_implicit_struct to visit_start_alternate Eric Blake
@ 2016-02-18 10:09 ` Markus Armbruster
2016-02-18 14:44 ` Eric Blake
2016-02-18 20:08 ` Markus Armbruster
16 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 10:09 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> I'm still working on my documentation patches for QAPI visitors
> (https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03504.html),
> but am finding it easier to nuke bad code up front than to document
> that it is bad only to later nuke it. So this pulls bits and pieces
> of other patches that Markus I have previously posted, along with
> some new glue, to get rid of some of the worst of the cruft.
Based on my qapi-next commit fb8a18d. Try to remember mentioning a base
other than current master in the future :)
[...]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E)
2016-02-18 6:48 [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (15 preceding siblings ...)
2016-02-18 10:09 ` [Qemu-devel] [PATCH v11 00/15] prune some QAPI visitor cruft (was qapi cleanups subset E) Markus Armbruster
@ 2016-02-18 20:08 ` Markus Armbruster
2016-02-18 20:24 ` Eric Blake
16 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2016-02-18 20:08 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> I'm still working on my documentation patches for QAPI visitors
> (https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03504.html),
> but am finding it easier to nuke bad code up front than to document
> that it is bad only to later nuke it. So this pulls bits and pieces
> of other patches that Markus I have previously posted, along with
> some new glue, to get rid of some of the worst of the cruft.
>
> v10 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03282.html
>
> Since then, I've folded in fixes for Markus' review comments,
> including rearranging some hunks and retitling some patches,
> and dropping the attempt to minimize generated gotos. The biggest
> changes are moving where variants get visited (during
> visit_type_FOO_fields() rather than visit_type_FOO()), dropping
> visit_type_alternate_FOO() (open-coding it during
> gen_visit_alternate()), and fixing bugs (properly handle
> visit_start_union() so that there are no bisection points where
> we are handling 'void *data' incorrectly).
>
> 001/15:[----] [--] 'qapi: Simplify excess input reporting in input visitors'
> 002/15:[----] [--] 'qapi: Forbid empty unions and useless alternates'
> 003/15:[down] 'qapi: Forbid 'any' inside an alternate'
> 004/15:[down] 'qapi: Add tests of complex objects within alternate'
> 005/15:[----] [--] 'qapi-visit: Simplify how we visit common union members'
> 006/15:[down] 'qapi: Visit variants in visit_type_FOO_fields()'
> 007/15:[0051] [FC] 'qapi-visit: Unify struct and union visit'
> 008/15:[0012] [FC] 'qapi-visit: Less indirection in visit_type_Foo_fields()'
> 009/15:[0002] [FC] 'qapi: Adjust layout of FooList types'
> 010/15:[0004] [FC] 'qapi: Emit structs used as variants in topological order'
> 011/15:[down] 'qapi-visit: Use common idiom in gen_visit_fields_decl()'
> 012/15:[0148] [FC] 'qapi: Don't box struct branch of alternate'
> 013/15:[0074] [FC] 'qapi: Don't box branches of flat unions'
> 014/15:[down] 'qapi: Delete visit_start_union(), gen_visit_implicit_struct()'
> 015/15:[0001] [FC] 'qapi: Change visit_start_implicit_struct to visit_start_alternate'
Applied to qapi-next with minor tweaks. Diff between the final
appended. I'd appreciate a look-over.
Thanks!
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -41,7 +41,7 @@ static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error **er
c_type=typ.c_name())
-def gen_visit_struct_fields(name, base, members, variants=None):
+def gen_visit_struct_fields(name, base, members, variants):
ret = ''
if base:
^ permalink raw reply [flat|nested] 41+ messages in thread