* [Qemu-devel] [PATCH v10 01/13] qapi: Simplify excess input reporting in input visitors
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates Eric Blake
` (11 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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>
---
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] 39+ messages in thread* [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 01/13] qapi: Simplify excess input reporting in input visitors Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 16:08 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 03/13] qapi: Reposition error checks in gen_visit_fields() Eric Blake
` (10 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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 could inject
a dummy member (and in fact, we do for empty structs), but 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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates Eric Blake
@ 2016-02-16 16:08 ` Markus Armbruster
2016-02-16 23:18 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 16:08 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Empty unions serve no purpose, and while we compile with gcc
> which permits them, strict C99 forbids them. We could inject
> a dummy member (and in fact, we do for empty structs), but while
gen_variants() injects void *data.
> empty structs make sense in qapi,
Suggest to cut the paragaph until here.
> empty unions don't add any
> expressiveness to the QMP language. So prohibit them at parse
> time. Update the documentation and testsuite to match.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> 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)
This is stricter than the commit message announced. You can either
relax to at least one branch, or amend the commit message.
> 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:
Missing: update of section "Alternate types".
[tests/ diff looks good]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates
2016-02-16 16:08 ` Markus Armbruster
@ 2016-02-16 23:18 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 23:18 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]
On 02/16/2016 09:08 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Empty unions serve no purpose, and while we compile with gcc
>> which permits them, strict C99 forbids them. We could inject
>> a dummy member (and in fact, we do for empty structs), but while
>
> gen_variants() injects void *data.
>
>> empty structs make sense in qapi,
>
> Suggest to cut the paragaph until here.
Side effect of rebasing - I originally had this patch after one that
deletes the 'data' member, but that requires a few other patches. I'll
reword it to mention that we want to delete 'data', at which point we
would be left with an empty union if we didn't prohibit it at parse time.
>> @@ -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)
>
> This is stricter than the commit message announced. You can either
> relax to at least one branch, or amend the commit message.
Will amend; a later patch actually relies on the 2-or-more promise for
alternates, so it is worth documenting as intentional.
>> A simple union type defines a mapping from automatic discriminator
>> values to data types like in this example:
>
> Missing: update of section "Alternate types".
Already done there (commit 7b1b98c4):
"An alternate type is one that allows a choice between two or more JSON
data types (string, integer, number, or object, but currently not..."
I didn't see anything worth rewording.
>
> [tests/ diff 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] 39+ messages in thread
* [Qemu-devel] [PATCH v10 03/13] qapi: Reposition error checks in gen_visit_fields()
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 01/13] qapi: Simplify excess input reporting in input visitors Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code Eric Blake
` (9 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Change the generated code for error checking after an optional
field visit to have one less level of indentation. There is no
real semantic change (the compiler should be smart enough to
realize that err does not change if visit_optional() returns
false, and optimize out unneeded branching in that case); the
main reason for the change is making the next patch (removing
a pointless goto) easier to read.
| if (visit_optional(v, "node-name", &has_node_name)) {
| visit_type_str(v, "node-name", (char **)&node_name, &err);
|- if (err) {
|- goto out_obj;
|- }
|+ }
|+ if (err) {
|+ goto out_obj;
| }
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v10: new patch
---
scripts/qapi.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f97236f..fab5001 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1673,13 +1673,14 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
c_type=memb.type.c_name(), prefix=prefix, cast=cast,
c_name=c_name(memb.name), name=memb.name,
errp=errparg)
- ret += gen_err_check(skiperr=skiperr, label=label)
if memb.optional:
pop_indent()
ret += mcgen('''
}
''')
+ ret += gen_err_check(skiperr=skiperr, label=label)
+
return ret
--
2.5.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (2 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 03/13] qapi: Reposition error checks in gen_visit_fields() Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 16:27 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members Eric Blake
` (8 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
There's no point in emitting a goto if the very next thing
emitted will be the label. A bit of cleverness in gen_visit_fields()
will let us choose when to omit a final error check (basically,
swap the order of emitted text within the loop, with the error
check omitted on the first pass, then checking whether to emit a
final check after the loop); and in turn omit unused labels.
The change to generated code is a bit easier because we split out
the reindentation of the remaining gotos in the previous patch.
For example, in visit_type_ChardevReturn_fields():
| if (visit_optional(v, "pty", &obj->has_pty)) {
| visit_type_str(v, "pty", &obj->pty, &err);
| }
|- if (err) {
|- goto out;
|- }
|-
|-out:
| error_propagate(errp, err);
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v10: new patch
---
scripts/qapi-event.py | 7 +++++--
scripts/qapi-visit.py | 10 ++++++----
scripts/qapi.py | 8 ++++++--
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 544ae12..b50ac29 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -68,9 +68,12 @@ def gen_event_send(name, arg_type):
name=name)
ret += gen_err_check()
ret += gen_visit_fields(arg_type.members, need_cast=True,
- label='out_obj')
- ret += mcgen('''
+ label='out_obj', skiplast=True)
+ if len(arg_type.members) > 1:
+ ret += mcgen('''
out_obj:
+''')
+ ret += mcgen('''
visit_end_struct(v, err ? NULL : &err);
if (err) {
goto out;
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0cc9b08..0be396b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -92,12 +92,14 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
''',
c_type=base.c_name())
- ret += gen_err_check()
+ if members:
+ ret += gen_err_check()
- ret += gen_visit_fields(members, prefix='(*obj)->')
+ ret += gen_visit_fields(members, prefix='(*obj)->', skiplast=True)
- # 'goto out' produced for base, and by gen_visit_fields() for each member
- if base or members:
+ # 'goto out' produced for base, and by gen_visit_fields() for each
+ # member except the last
+ if bool(base) + len(members) > 1:
ret += mcgen('''
out:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index fab5001..4d75d75 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1645,14 +1645,17 @@ def gen_err_check(label='out', skiperr=False):
def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
- label='out'):
+ label='out', skiplast=False):
ret = ''
if skiperr:
errparg = 'NULL'
else:
errparg = '&err'
+ local_skiperr = True
for memb in members:
+ ret += gen_err_check(skiperr=local_skiperr, label=label)
+ local_skiperr = skiperr
if memb.optional:
ret += mcgen('''
if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
@@ -1679,8 +1682,9 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
ret += mcgen('''
}
''')
+
+ if members and not skiplast:
ret += gen_err_check(skiperr=skiperr, label=label)
-
return ret
--
2.5.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code Eric Blake
@ 2016-02-16 16:27 ` Markus Armbruster
2016-02-16 17:14 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 16:27 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> There's no point in emitting a goto if the very next thing
> emitted will be the label. A bit of cleverness in gen_visit_fields()
> will let us choose when to omit a final error check (basically,
> swap the order of emitted text within the loop, with the error
> check omitted on the first pass, then checking whether to emit a
> final check after the loop); and in turn omit unused labels.
>
> The change to generated code is a bit easier because we split out
> the reindentation of the remaining gotos in the previous patch.
> For example, in visit_type_ChardevReturn_fields():
>
> | if (visit_optional(v, "pty", &obj->has_pty)) {
> | visit_type_str(v, "pty", &obj->pty, &err);
> | }
> |- if (err) {
> |- goto out;
> |- }
> |-
> |-out:
> | error_propagate(errp, err);
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v10: new patch
> ---
> scripts/qapi-event.py | 7 +++++--
> scripts/qapi-visit.py | 10 ++++++----
> scripts/qapi.py | 8 ++++++--
> 3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 544ae12..b50ac29 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -68,9 +68,12 @@ def gen_event_send(name, arg_type):
> name=name)
> ret += gen_err_check()
> ret += gen_visit_fields(arg_type.members, need_cast=True,
> - label='out_obj')
> - ret += mcgen('''
> + label='out_obj', skiplast=True)
> + if len(arg_type.members) > 1:
> + ret += mcgen('''
> out_obj:
> +''')
> + ret += mcgen('''
> visit_end_struct(v, err ? NULL : &err);
> if (err) {
> goto out;
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 0cc9b08..0be396b 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -92,12 +92,14 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
> visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
> ''',
> c_type=base.c_name())
> - ret += gen_err_check()
> + if members:
> + ret += gen_err_check()
>
> - ret += gen_visit_fields(members, prefix='(*obj)->')
> + ret += gen_visit_fields(members, prefix='(*obj)->', skiplast=True)
>
> - # 'goto out' produced for base, and by gen_visit_fields() for each member
> - if base or members:
> + # 'goto out' produced for base, and by gen_visit_fields() for each
> + # member except the last
> + if bool(base) + len(members) > 1:
> ret += mcgen('''
>
> out:
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index fab5001..4d75d75 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1645,14 +1645,17 @@ def gen_err_check(label='out', skiperr=False):
>
>
> def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
> - label='out'):
> + label='out', skiplast=False):
> ret = ''
> if skiperr:
> errparg = 'NULL'
> else:
> errparg = '&err'
> + local_skiperr = True
>
> for memb in members:
> + ret += gen_err_check(skiperr=local_skiperr, label=label)
> + local_skiperr = skiperr
> if memb.optional:
> ret += mcgen('''
> if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
> @@ -1679,8 +1682,9 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
> ret += mcgen('''
> }
> ''')
> +
> + if members and not skiplast:
> ret += gen_err_check(skiperr=skiperr, label=label)
> -
> return ret
Is saving a goto the compiler can easily eliminate worth complicating
the code?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code
2016-02-16 16:27 ` Markus Armbruster
@ 2016-02-16 17:14 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 17:14 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]
On 02/16/2016 09:27 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> There's no point in emitting a goto if the very next thing
>> emitted will be the label. A bit of cleverness in gen_visit_fields()
>> will let us choose when to omit a final error check (basically,
>> swap the order of emitted text within the loop, with the error
>> check omitted on the first pass, then checking whether to emit a
>> final check after the loop); and in turn omit unused labels.
>>
>> The change to generated code is a bit easier because we split out
>> the reindentation of the remaining gotos in the previous patch.
>> For example, in visit_type_ChardevReturn_fields():
>>
>> | if (visit_optional(v, "pty", &obj->has_pty)) {
>> | visit_type_str(v, "pty", &obj->pty, &err);
>> | }
>> |- if (err) {
>> |- goto out;
>> |- }
>> |-
>> |-out:
>> | error_propagate(errp, err);
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> @@ -1679,8 +1682,9 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
>> ret += mcgen('''
>> }
>> ''')
>> +
>> + if members and not skiplast:
>> ret += gen_err_check(skiperr=skiperr, label=label)
>> -
>> return ret
>
> Is saving a goto the compiler can easily eliminate worth complicating
> the code?
I'm fine with dropping 3 and 4/13 if you think they don't add anything
(and they do indeed make it more complicated to reason about when it is
safe to drop a goto, and furthermore when to omit the label because all
gotos were dropped). The generated code will occupy more source code
bytes, but as you say, the optimizing compiler should not be bloating
the code any differently based on whether the goto is present or absent.
Okay, just in typing that, you've convinced me - ease of maintenance
should triumph over concise generated code.
--
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] 39+ messages in thread
* [Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (3 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 16:31 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 06/13] qapi-visit: Unify struct and union visit Eric Blake
` (7 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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);
+ 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);
+ 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 to avoid pointless gotos in new code]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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 0be396b..0530f2b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -240,11 +240,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
@@ -264,21 +261,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) {
@@ -378,11 +363,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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members Eric Blake
@ 2016-02-16 16:31 ` Markus Armbruster
2016-02-16 17:17 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 16:31 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);
> + 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);
> + 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 to avoid pointless gotos in new code]
Also: examples in commit message replaced by better ones :)
> Signed-off-by: Eric Blake <eblake@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members
2016-02-16 16:31 ` Markus Armbruster
@ 2016-02-16 17:17 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 17:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3893 bytes --]
On 02/16/2016 09:31 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:
>>
>> 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);
>> + error_propagate(errp, err);
>> +}
>> +
This is one of the places that is several lines longer if we always emit
the goto and label. If we drop 3 and 4, then the commit message needs a
rebase.
>> 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);
>> + error_propagate(errp, err);
>> +}
And another beneficiary of the omitted goto that gets longer.
>> +
>> 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 to avoid pointless gotos in new code]
>
> Also: examples in commit message replaced by better ones :)
I picked the shortest names possible, to minimize the length of the
commit message, and favoring qapi-schema.json over the testsuite. You
noticed :)
>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v10 06/13] qapi-visit: Unify struct and union visit
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (4 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 07/13] qapi-visit: Less indirection in visit_type_Foo_fields() Eric Blake
` (6 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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() plus additional
code to handle variants. Make that code conditional on variants, so
gen_visit_union() does exactly the same for structs as
gen_visit_struct(). Rename it to gen_visit_object(), use it for
structs, and drop gen_visit_struct().
Output is slightly changed due to an earlier placement of the
'out_obj' label for structs, but with no semantic impact:
| if (err) {
| goto out;
| }
| if (!*obj) {
| goto out_obj;
| }
| visit_type_ACPIOSTInfo_fields(v, obj, &err);
|+out_obj:
| error_propagate(errp, err);
| err = NULL;
|-out_obj:
| visit_end_struct(v, &err);
| out:
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1453902888-20457-4-git-send-email-armbru@redhat.com>
[rebase to earlier changes]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v10: new patch, replacing 31/37
---
scripts/qapi-visit.py | 96 ++++++++++++++++++---------------------------------
1 file changed, 34 insertions(+), 62 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0530f2b..786fe57 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -111,40 +111,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
@@ -240,14 +206,19 @@ 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)
- 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)
+ 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)
+ # 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)
@@ -264,43 +235,47 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
visit_type_%(c_name)s_fields(v, obj, &err);
''',
c_name=c_name(name))
- ret += gen_err_check(label='out_obj')
- ret += mcgen('''
+ if variants:
+ 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))
+ 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('''
+ 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))
- if simple_union_type:
- ret += mcgen('''
+ case=c_enum_const(variants.tag_member.type.name,
+ var.name))
+ 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('''
+ 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('''
+ c_type=var.type.c_name(),
+ c_name=c_name(var.name))
+ ret += mcgen('''
break;
''')
- ret += mcgen('''
+ ret += mcgen('''
default:
abort();
}
+''')
+
+ ret += mcgen('''
out_obj:
error_propagate(errp, err);
err = NULL;
@@ -362,10 +337,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] 39+ messages in thread* [Qemu-devel] [PATCH v10 07/13] qapi-visit: Less indirection in visit_type_Foo_fields()
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (5 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 06/13] qapi-visit: Unify struct and union visit Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 08/13] qapi: Adjust layout of FooList types Eric Blake
` (5 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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>
---
v10: new patch
---
scripts/qapi-visit.py | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 786fe57..177dfc4 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);
@@ -80,7 +80,7 @@ def gen_visit_struct_fields(name, base, members):
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;
@@ -89,13 +89,13 @@ 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())
if members:
ret += gen_err_check()
- ret += gen_visit_fields(members, prefix='(*obj)->', skiplast=True)
+ ret += gen_visit_fields(members, prefix='obj->', skiplast=True)
# 'goto out' produced for base, and by gen_visit_fields() for each
# member except the last
@@ -232,7 +232,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);
''',
c_name=c_name(name))
if variants:
--
2.5.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v10 08/13] qapi: Adjust layout of FooList types
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (6 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 07/13] qapi-visit: Less indirection in visit_type_Foo_fields() Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 16:55 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order Eric Blake
` (4 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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'.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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 | 14 +++++++-------
include/qapi/visitor-impl.h | 2 +-
scripts/qapi-types.py | 5 +----
scripts/qapi-visit.py | 2 +-
qapi/qapi-visit-core.c | 4 ++--
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..c131a32 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -19,13 +19,13 @@
#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 +36,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 177dfc4..9ff0337 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -129,7 +129,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..6fa66f1 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -50,9 +50,9 @@ 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);
+ 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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 08/13] qapi: Adjust layout of FooList types
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 08/13] qapi: Adjust layout of FooList types Eric Blake
@ 2016-02-16 16:55 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 16: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'.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> 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 | 14 +++++++-------
> include/qapi/visitor-impl.h | 2 +-
> scripts/qapi-types.py | 5 +----
> scripts/qapi-visit.py | 2 +-
> qapi/qapi-visit-core.c | 4 ++--
> 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..c131a32 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -19,13 +19,13 @@
> #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 +36,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 177dfc4..9ff0337 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -129,7 +129,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..6fa66f1 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -50,9 +50,9 @@ 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)
> {
Lost since v9: assert(size). In my review of v9, I suggested tightening
the assertion to size >= GenericList.
> - return v->next_list(v, list);
> + 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;
Doing this patch earlier in the series halved its size :)
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (7 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 08/13] qapi: Adjust layout of FooList types Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 17:03 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate Eric Blake
` (3 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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>
---
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..2f23432 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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order Eric Blake
@ 2016-02-16 17:03 ` Markus Armbruster
2016-02-16 17:32 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 17:03 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 (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>
>
> ---
> 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..2f23432 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)
PEP 8:
The preferred way of wrapping long lines is by using Python's
implied line continuation inside parentheses, brackets and
braces. Long lines can be broken over multiple lines by wrapping
expressions in parentheses. These should be used in preference to
using a backslash for line continuation.
In this case:
if (isinstance(v.type, QAPISchemaObjectType) and
not v.type.is_implicit()):
> +
> + ret += mcgen('''
>
> struct %(c_name)s {
> ''',
> - c_name=c_name(name))
> + c_name=c_name(name))
>
> if base:
> ret += mcgen('''
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order
2016-02-16 17:03 ` Markus Armbruster
@ 2016-02-16 17:32 ` Eric Blake
2016-02-16 21:00 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 17:32 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]
On 02/16/2016 10:03 AM, Markus Armbruster wrote:
> 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 (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).
>>
>> + 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)
>
> PEP 8:
>
> The preferred way of wrapping long lines is by using Python's
> implied line continuation inside parentheses, brackets and
> braces. Long lines can be broken over multiple lines by wrapping
> expressions in parentheses. These should be used in preference to
> using a backslash for line continuation.
>
> In this case:
>
> if (isinstance(v.type, QAPISchemaObjectType) and
> not v.type.is_implicit()):
pep8 silently accepted my version, but complains about yours:
scripts/qapi-types.py:65:5: E129 visually indented line with same indent
as next logical line
So the compromise for both of us is added indentation:
if (isinstance(v.type, QAPISchemaObjectType) and
not v.type.is_implicit()):
ret += ...
Or, I could revisit my earlier proposal of:
v.type.is_implicit(QAPISchemaObjectType)
of giving .is_implicit() an optional parameter; if absent, all types are
considered, but if present, the predicate is True only if the type of
the object being queried matches the parameter type name.
Here's the last time we discussed the tradeoffs of the shorter form:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02272.html
--
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order
2016-02-16 17:32 ` Eric Blake
@ 2016-02-16 21:00 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 21:00 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/16/2016 10:03 AM, Markus Armbruster wrote:
>> 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 (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).
>>>
>
>>> + 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)
>>
>> PEP 8:
>>
>> The preferred way of wrapping long lines is by using Python's
>> implied line continuation inside parentheses, brackets and
>> braces. Long lines can be broken over multiple lines by wrapping
>> expressions in parentheses. These should be used in preference to
>> using a backslash for line continuation.
>>
>> In this case:
>>
>> if (isinstance(v.type, QAPISchemaObjectType) and
>> not v.type.is_implicit()):
>
> pep8 silently accepted my version, but complains about yours:
>
> scripts/qapi-types.py:65:5: E129 visually indented line with same indent
> as next logical line
>
> So the compromise for both of us is added indentation:
>
> if (isinstance(v.type, QAPISchemaObjectType) and
> not v.type.is_implicit()):
> ret += ...
Sold.
>
> Or, I could revisit my earlier proposal of:
>
> v.type.is_implicit(QAPISchemaObjectType)
>
> of giving .is_implicit() an optional parameter; if absent, all types are
> considered, but if present, the predicate is True only if the type of
> the object being queried matches the parameter type name.
>
> Here's the last time we discussed the tradeoffs of the shorter form:
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02272.html
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (8 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-16 19:07 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions Eric Blake
` (2 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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; some testsuite coverage is added to avoid future
regressions.
Ultimately, we want to do the same treatment for QAPI unions, but
as that touches a lot more client code, it is better as a separate
patch. So in the meantime, I had to hack in a way to test if we
are visiting an alternate type, within qapi-types:gen_variants();
the hack is possible because an earlier patch guaranteed that all
alternates have at least two branches, with at most one object
branch; the hack will go away in a later patch.
The generated code difference to qapi-types.h is relatively small,
made possible by a new c_type(is_member) parameter in qapi.py:
| struct BlockdevRef {
| QType type;
| union { /* union tag is @type */
| void *data;
|- BlockdevOptions *definition;
|+ BlockdevOptions definition;
| char *reference;
| } u;
| };
meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(),
comparable to visit_type_implicit_Foo():
|+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions *obj, Error **errp)
|+{
|+ Error *err = NULL;
|+
|+ visit_start_struct(v, name, NULL, 0, &err);
|+ if (err) {
|+ goto out;
|+ }
|+ visit_type_BlockdevOptions_fields(v, obj, &err);
|+ error_propagate(errp, err);
|+ err = NULL;
|+ visit_end_struct(v, &err);
|+out:
|+ error_propagate(errp, err);
|+}
and use it like this:
| switch ((*obj)->type) {
| case QTYPE_QDICT:
|- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
|+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
| break;
| case QTYPE_QSTRING:
| visit_type_str(v, name, &(*obj)->u.reference, &err);
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v10: new patch
---
scripts/qapi-types.py | 10 ++++++-
scripts/qapi-visit.py | 49 +++++++++++++++++++++++++++++----
scripts/qapi.py | 10 ++++---
tests/test-qmp-input-visitor.c | 29 ++++++++++++++++++-
tests/qapi-schema/qapi-schema-test.json | 2 ++
tests/qapi-schema/qapi-schema-test.out | 2 ++
6 files changed, 91 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2f23432..aba2847 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.
+ inline = False
+ for v in variants.variants:
+ if not isinstance(v.type, QAPISchemaObjectType):
+ inline = 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_member=inline),
c_name=c_name(var.name))
ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 9ff0337..948bde4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,14 @@
from qapi import *
import re
-# visit_type_FOO_implicit() is emitted as needed; track if it has already
+# visit_type_implicit_FOO() is emitted as needed; track if it has already
# been output.
implicit_structs_seen = set()
+# visit_type_alternate_FOO() is emitted as needed; track if it has already
+# been output.
+alternate_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()
@@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
return ret
+def gen_visit_alternate_struct(typ):
+ if typ in alternate_structs_seen:
+ return ''
+ alternate_structs_seen.add(typ)
+
+ ret = gen_visit_fields_decl(typ)
+
+ ret += mcgen('''
+
+static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, %(c_type)s *obj, Error **errp)
+{
+ Error *err = NULL;
+
+ visit_start_struct(v, name, NULL, 0, &err);
+ if (err) {
+ goto out;
+ }
+ visit_type_%(c_type)s_fields(v, obj, &err);
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_struct(v, &err);
+out:
+ error_propagate(errp, err);
+}
+''',
+ c_type=typ.c_name())
+ return ret
+
+
def gen_visit_struct_fields(name, base, members):
ret = ''
@@ -158,11 +191,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_alternate_struct(var.type)
- ret = mcgen('''
+ ret += mcgen('''
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
{
@@ -178,15 +214,18 @@ 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:
+ prefix = 'visit_type_'
+ if isinstance(var.type, QAPISchemaObjectType):
+ prefix += 'alternate_'
ret += mcgen('''
case %(case)s:
- visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
+ %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
break;
''',
- case=var.type.alternate_qtype(),
+ prefix=prefix, case=var.type.alternate_qtype(),
c_type=var.type.c_name(),
c_name=c_name(var.name))
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4d75d75..dbb58da 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -821,7 +821,7 @@ class QAPISchemaVisitor(object):
class QAPISchemaType(QAPISchemaEntity):
- def c_type(self, is_param=False):
+ def c_type(self, is_param=False, is_member=False):
return c_name(self.name) + pointer_suffix
def c_null(self):
@@ -854,7 +854,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_member=False):
if is_param and self.name == 'str':
return 'const ' + self._c_type_name
return self._c_type_name
@@ -888,7 +888,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_member=False):
return c_name(self.name)
def member_names(self):
@@ -984,8 +984,10 @@ 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_member=False):
assert not self.is_implicit()
+ if is_member:
+ return c_name(self.name)
return QAPISchemaType.c_type(self)
def json_type(self):
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c72cdad..139ff7d 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,36 @@ 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, "{'boolean':true}");
+ visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QDICT);
+ g_assert_cmpint(tmp->u.uda.boolean, ==, true);
+ g_assert_cmpint(tmp->u.uda.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': {'boolean':true} }");
+ visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
+ g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT);
+ g_assert_cmpint(wrap->alt->u.uda.boolean, ==, true);
+ g_assert_cmpint(wrap->alt->u.uda.has_a_b, ==, false);
+ qapi_free_WrapAlternate(wrap);
}
static void test_visitor_in_alternate_number(TestInputVisitorData *data,
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 4b89527..b393572 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -82,6 +82,8 @@
'value2' : 'UserDefB',
'value3' : 'UserDefA' } }
+{ 'struct': 'WrapAlternate',
+ 'data': { 'alt': 'UserDefAlternate' } }
{ 'alternate': 'UserDefAlternate',
'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 2c546b7..11cdc9a 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -169,6 +169,8 @@ object UserDefUnionBase
member enum1: EnumOne 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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate Eric Blake
@ 2016-02-16 19:07 ` Markus Armbruster
2016-02-16 19:53 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-16 19:07 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; some testsuite coverage is added to avoid future
> regressions.
>
> Ultimately, we want to do the same treatment for QAPI unions, but
> as that touches a lot more client code, it is better as a separate
> patch. So in the meantime, I had to hack in a way to test if we
> are visiting an alternate type, within qapi-types:gen_variants();
> the hack is possible because an earlier patch guaranteed that all
> alternates have at least two branches, with at most one object
> branch; the hack will go away in a later patch.
Suggest:
Ultimately, we want to do the same treatment for QAPI unions, but as
that touches a lot more client code, it is better as a separate patch.
The two share gen_variants(), and I had to hack in a way to test if we
are visiting an alternate type: look for a non-object branch. This
works because alternates have at least two branches, with at most one
object branch, and 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,
> made possible by a new c_type(is_member) parameter in qapi.py:
Let's drop the "made possible" clause here.
>
> | struct BlockdevRef {
> | QType type;
> | union { /* union tag is @type */
> | void *data;
> |- BlockdevOptions *definition;
> |+ BlockdevOptions definition;
> | char *reference;
> | } u;
> | };
>
> meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(),
> comparable to visit_type_implicit_Foo():
>
> |+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions *obj, Error **errp)
> |+{
> |+ Error *err = NULL;
> |+
> |+ visit_start_struct(v, name, NULL, 0, &err);
> |+ if (err) {
> |+ goto out;
> |+ }
> |+ visit_type_BlockdevOptions_fields(v, obj, &err);
> |+ error_propagate(errp, err);
> |+ err = NULL;
> |+ visit_end_struct(v, &err);
> |+out:
> |+ error_propagate(errp, err);
> |+}
This is in addition to visit_type_BlockdevOptions(), so we need another
name.
I can't quite see how the function is tied to alternates, though.
>
> and use it like this:
>
> | switch ((*obj)->type) {
> | case QTYPE_QDICT:
> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
> |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
Let's compare the two functions. First visit_type_BlockdevOptions():
void visit_type_BlockdevOptions(Visitor *v,
const char *name,
BlockdevOptions **obj,
Error **errp)
{
Error *err = NULL;
visit_start_struct(v, name, (void **)obj, sizeof(BlockdevOptions), &err);
if (err) {
goto out;
}
if (!*obj) {
goto out_obj;
}
visit_type_BlockdevOptions_fields(v, *obj, &err);
if (err) {
goto out_obj;
}
if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
goto out_obj;
}
switch ((*obj)->driver) {
case BLOCKDEV_DRIVER_ARCHIPELAGO:
visit_type_implicit_BlockdevOptionsArchipelago(v, &(*obj)->u.archipelago, &err);
break;
[All the other cases...]
default:
abort();
}
out_obj:
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
out:
error_propagate(errp, err);
}
Now visit_type_alternate_BlockdevOptions(), with differences annotated
with //:
static void visit_type_alternate_BlockdevOptions(Visitor *v,
const char *name,
BlockdevOptions *obj, // one * less
Error **errp)
{
Error *err = NULL;
visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj
// suppresses malloc
if (err) {
goto out;
}
// null check dropped (obj can't be null)
visit_type_BlockdevOptions_fields(v, obj, &err);
// visit_start_union() + switch dropped
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
out:
error_propagate(errp, err);
}
Why can we drop visit_start_union() + switch?
> | break;
> | case QTYPE_QSTRING:
> | visit_type_str(v, name, &(*obj)->u.reference, &err);
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v10: new patch
> ---
> scripts/qapi-types.py | 10 ++++++-
> scripts/qapi-visit.py | 49 +++++++++++++++++++++++++++++----
> scripts/qapi.py | 10 ++++---
> tests/test-qmp-input-visitor.c | 29 ++++++++++++++++++-
> tests/qapi-schema/qapi-schema-test.json | 2 ++
> tests/qapi-schema/qapi-schema-test.out | 2 ++
> 6 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2f23432..aba2847 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.
> + inline = False
> + for v in variants.variants:
> + if not isinstance(v.type, QAPISchemaObjectType):
> + inline = 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_member=inline),
> c_name=c_name(var.name))
>
> ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9ff0337..948bde4 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,10 +15,14 @@
> from qapi import *
> import re
>
> -# visit_type_FOO_implicit() is emitted as needed; track if it has already
> +# visit_type_implicit_FOO() is emitted as needed; track if it has already
> # been output.
> implicit_structs_seen = set()
>
> +# visit_type_alternate_FOO() is emitted as needed; track if it has already
> +# been output.
> +alternate_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()
> @@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
> return ret
>
>
> +def gen_visit_alternate_struct(typ):
> + if typ in alternate_structs_seen:
> + return ''
> + alternate_structs_seen.add(typ)
> +
> + ret = gen_visit_fields_decl(typ)
> +
> + ret += mcgen('''
> +
> +static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, %(c_type)s *obj, Error **errp)
> +{
> + Error *err = NULL;
> +
> + visit_start_struct(v, name, NULL, 0, &err);
> + if (err) {
> + goto out;
> + }
> + visit_type_%(c_type)s_fields(v, obj, &err);
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_struct(v, &err);
> +out:
> + error_propagate(errp, err);
> +}
> +''',
> + c_type=typ.c_name())
> + return ret
> +
> +
> def gen_visit_struct_fields(name, base, members):
> ret = ''
>
> @@ -158,11 +191,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_alternate_struct(var.type)
>
> - ret = mcgen('''
> + ret += mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> {
> @@ -178,15 +214,18 @@ 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:
> + prefix = 'visit_type_'
> + if isinstance(var.type, QAPISchemaObjectType):
> + prefix += 'alternate_'
> ret += mcgen('''
> case %(case)s:
> - visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
> + %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
> break;
> ''',
> - case=var.type.alternate_qtype(),
> + prefix=prefix, case=var.type.alternate_qtype(),
> c_type=var.type.c_name(),
> c_name=c_name(var.name))
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4d75d75..dbb58da 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -821,7 +821,7 @@ class QAPISchemaVisitor(object):
>
>
> class QAPISchemaType(QAPISchemaEntity):
> - def c_type(self, is_param=False):
> + def c_type(self, is_param=False, is_member=False):
> return c_name(self.name) + pointer_suffix
>
> def c_null(self):
> @@ -854,7 +854,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_member=False):
> if is_param and self.name == 'str':
> return 'const ' + self._c_type_name
> return self._c_type_name
> @@ -888,7 +888,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_member=False):
> return c_name(self.name)
>
> def member_names(self):
> @@ -984,8 +984,10 @@ 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_member=False):
> assert not self.is_implicit()
> + if is_member:
> + return c_name(self.name)
> return QAPISchemaType.c_type(self)
To understand this, you have to peel off OO abstraction:
QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix.
I think we should keep it peeled off in the source code.
>
> def json_type(self):
[tests/ diff looks good]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate
2016-02-16 19:07 ` Markus Armbruster
@ 2016-02-16 19:53 ` Eric Blake
2016-02-17 14:40 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 19:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 7538 bytes --]
On 02/16/2016 12:07 PM, 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.
>>
>> Surprisingly, no clients were actually using the struct member prior
>> to this patch; some testsuite coverage is added to avoid future
>> regressions.
>>
>> Ultimately, we want to do the same treatment for QAPI unions, but
>> as that touches a lot more client code, it is better as a separate
>> patch. So in the meantime, I had to hack in a way to test if we
>> are visiting an alternate type, within qapi-types:gen_variants();
>> the hack is possible because an earlier patch guaranteed that all
>> alternates have at least two branches, with at most one object
>> branch; the hack will go away in a later patch.
>
> Suggest:
>
> Ultimately, we want to do the same treatment for QAPI unions, but as
> that touches a lot more client code, it is better as a separate patch.
> The two share gen_variants(), and I had to hack in a way to test if we
> are visiting an alternate type: look for a non-object branch. This
> works because alternates have at least two branches, with at most one
> object branch, and unions have only object branches. The hack will go
> away in a later patch.
Nicer.
>
>> The generated code difference to qapi-types.h is relatively small,
>> made possible by a new c_type(is_member) parameter in qapi.py:
>
> Let's drop the "made possible" clause here.
I was trying to document the new is_member parameter somewhere in the
commit message, but I can agree that it could be its own paragraph
rather than a clause here.
>
> This is in addition to visit_type_BlockdevOptions(), so we need another
> name.
>
> I can't quite see how the function is tied to alternates, though.
>
I'm open to naming suggestions. Also, I noticed that we have
'visit_type_FOO_fields' and 'visit_type_implicit_FOO'; so I didn't know
whether to name it 'visit_type_alternate_FOO' or
'visit_type_FOO_alternate'; it gets more interesting later in the series
when I completely delete 'visit_type_implicit_FOO'.
>>
>> and use it like this:
>>
>> | switch ((*obj)->type) {
>> | case QTYPE_QDICT:
>> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
>> |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
>
> Let's compare the two functions. First visit_type_BlockdevOptions():
>
>
> Now visit_type_alternate_BlockdevOptions(), with differences annotated
> with //:
>
> static void visit_type_alternate_BlockdevOptions(Visitor *v,
> const char *name,
> BlockdevOptions *obj, // one * less
Yep, because we no longer need to malloc a second object, so we no
longer need to propagate a change to obj back to the caller.
> Error **errp)
> {
> Error *err = NULL;
>
> visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj
> // suppresses malloc
> if (err) {
> goto out;
> }
> // null check dropped (obj can't be null)
> visit_type_BlockdevOptions_fields(v, obj, &err);
Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again,
because we have one less level of indirection, and 7/13 reduced the
indirection required in visit_type_FOO_fields()).
> // visit_start_union() + switch dropped
> error_propagate(errp, err);
> err = NULL;
> visit_end_struct(v, &err);
> out:
> error_propagate(errp, err);
> }
>
> Why can we drop visit_start_union() + switch?
visit_start_union() is dropped because its only purpose was to determine
if the dealloc visitor needs to visit the default branch. When we had a
separate allocation, we did not want to visit the branch if the
discriminator was not successfully parsed, because otherwise we would
dereference NULL. But now that we don't have a second pointer
allocation, we no longer have anything to dealloc, and we can no longer
dereference NULL. Explained better in 12/13, where I delete
visit_start_union() altogether. But maybe I could keep it in this patch
in the meantime, to minimize confusion.
Dropped switch, on the other hand, looks to be a genuine bug. Eeek.
That should absolutely be present, and it proves that our testsuite is
not strong enough for not catching me on it.
And now that you've made me think about it, maybe I have yet another
idea. Right now, we've split the visit of local members into
visit_type_FOO_fields(), while leaving the variant members to be visited
in visit_type_FOO()
visit_type_FOO_fields() is static, so we can change it without impacting
the entire tree; I could add a bool parameter to that function, and write:
visit_type_FOO() {
visit_start_struct(obj)
visit_type_FOO_fields(, false)
visit_end_struct()
}
visit_type_FOO_fields(, wrap) {
if (wrap) {
visit_start_struct(NULL)
}
visit local fields
visit variant fields
if (wrap) {
visit_end_struct()
}
}
and for the variant type that includes FOO as one of its options,
visit_type_VARIANT() {
allocate (currently spelled visit_start_implicit_struct, but see 13/13)
switch (type) {
case QTYPE_QINT:
visit_type_int(); break
case QTYPE_QDICT:
visit_type_FOO_fields(, true); break
}
visit_end
}
or not even create a new function, and just have:
visit_type_FOO() {
visit_start_struct(obj)
visit_type_FOO_fields()
visit_end_struct()
}
visit_type_FOO_fields() {
visit local fields
visit variant fields
}
and for the variant type,
visit_type_VARIANT() {
allocate (currently spelled visit_start_implicit_struct, but see 13/13)
switch (type) {
case QTYPE_QINT:
visit_type_int(); break
case QTYPE_QDICT:
visit_start_struct(NULL)
visit_type_FOO_fields()
visit_end_struct()
break
}
visit_end_implicit
}
where the logic gets a bit more complicated to do correct error handling.
>> @@ -984,8 +984,10 @@ 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_member=False):
>> assert not self.is_implicit()
>> + if is_member:
>> + return c_name(self.name)
>> return QAPISchemaType.c_type(self)
>
> To understand this, you have to peel off OO abstraction:
> QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix.
>
> I think we should keep it peeled off in the source code.
By this, you mean I should do:
def c_type(self, is_param=False, is_member=False):
assert not self.is_implicit()
if is_member:
return c_name(self.name)
return c_name(self.name) + pointer_suffix
?
or that I should hoist the suppression of pointer_suffix into the parent
class?
>
>>
>> def json_type(self):
> [tests/ diff looks good]
good for what they caught, but they didn't catch enough :)
--
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate
2016-02-16 19:53 ` Eric Blake
@ 2016-02-17 14:40 ` Markus Armbruster
2016-02-17 20:34 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-17 14:40 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/16/2016 12:07 PM, 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.
>>>
>>> Surprisingly, no clients were actually using the struct member prior
>>> to this patch; some testsuite coverage is added to avoid future
>>> regressions.
>>>
>>> Ultimately, we want to do the same treatment for QAPI unions, but
>>> as that touches a lot more client code, it is better as a separate
>>> patch. So in the meantime, I had to hack in a way to test if we
>>> are visiting an alternate type, within qapi-types:gen_variants();
>>> the hack is possible because an earlier patch guaranteed that all
>>> alternates have at least two branches, with at most one object
>>> branch; the hack will go away in a later patch.
>>
>> Suggest:
>>
>> Ultimately, we want to do the same treatment for QAPI unions, but as
>> that touches a lot more client code, it is better as a separate patch.
>> The two share gen_variants(), and I had to hack in a way to test if we
>> are visiting an alternate type: look for a non-object branch. This
>> works because alternates have at least two branches, with at most one
>> object branch, and unions have only object branches. The hack will go
>> away in a later patch.
>
> Nicer.
>
>>
>>> The generated code difference to qapi-types.h is relatively small,
>>> made possible by a new c_type(is_member) parameter in qapi.py:
>>
>> Let's drop the "made possible" clause here.
>
> I was trying to document the new is_member parameter somewhere in the
> commit message, but I can agree that it could be its own paragraph
> rather than a clause here.
That could work.
>> This is in addition to visit_type_BlockdevOptions(), so we need another
>> name.
>>
>> I can't quite see how the function is tied to alternates, though.
>>
>
> I'm open to naming suggestions. Also, I noticed that we have
> 'visit_type_FOO_fields' and 'visit_type_implicit_FOO'; so I didn't know
> whether to name it 'visit_type_alternate_FOO' or
> 'visit_type_FOO_alternate'; it gets more interesting later in the series
> when I completely delete 'visit_type_implicit_FOO'.
>
>>>
>>> and use it like this:
>>>
>>> | switch ((*obj)->type) {
>>> | case QTYPE_QDICT:
>>> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
>>> |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
>>
>> Let's compare the two functions. First visit_type_BlockdevOptions():
>>
>
>>
>> Now visit_type_alternate_BlockdevOptions(), with differences annotated
>> with //:
>>
>> static void visit_type_alternate_BlockdevOptions(Visitor *v,
>> const char *name,
>> BlockdevOptions *obj, // one * less
>
> Yep, because we no longer need to malloc a second object, so we no
> longer need to propagate a change to obj back to the caller.
>
>> Error **errp)
>> {
>> Error *err = NULL;
>>
>> visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj
>> // suppresses malloc
>> if (err) {
>> goto out;
>> }
>> // null check dropped (obj can't be null)
>> visit_type_BlockdevOptions_fields(v, obj, &err);
>
> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again,
> because we have one less level of indirection, and 7/13 reduced the
> indirection required in visit_type_FOO_fields()).
>
>> // visit_start_union() + switch dropped
>> error_propagate(errp, err);
>> err = NULL;
>> visit_end_struct(v, &err);
>> out:
>> error_propagate(errp, err);
>> }
>>
>> Why can we drop visit_start_union() + switch?
>
> visit_start_union() is dropped because its only purpose was to determine
> if the dealloc visitor needs to visit the default branch. When we had a
> separate allocation, we did not want to visit the branch if the
> discriminator was not successfully parsed, because otherwise we would
> dereference NULL. But now that we don't have a second pointer
> allocation, we no longer have anything to dealloc, and we can no longer
> dereference NULL. Explained better in 12/13, where I delete
> visit_start_union() altogether. But maybe I could keep it in this patch
> in the meantime, to minimize confusion.
Perhaps squashing the two patches together could be less confusing.
> Dropped switch, on the other hand, looks to be a genuine bug. Eeek.
> That should absolutely be present, and it proves that our testsuite is
> not strong enough for not catching me on it.
>
> And now that you've made me think about it, maybe I have yet another
> idea. Right now, we've split the visit of local members into
> visit_type_FOO_fields(), while leaving the variant members to be visited
> in visit_type_FOO()
Yes. I guess that's to support visiting "implicit" structs.
> visit_type_FOO_fields() is static, so we can change it without impacting
> the entire tree; I could add a bool parameter to that function, and write:
>
> visit_type_FOO() {
> visit_start_struct(obj)
> visit_type_FOO_fields(, false)
> visit_end_struct()
> }
>
> visit_type_FOO_fields(, wrap) {
> if (wrap) {
> visit_start_struct(NULL)
> }
> visit local fields
> visit variant fields
> if (wrap) {
> visit_end_struct()
> }
> }
>
> and for the variant type that includes FOO as one of its options,
>
> visit_type_VARIANT() {
> allocate (currently spelled visit_start_implicit_struct, but see 13/13)
> switch (type) {
> case QTYPE_QINT:
> visit_type_int(); break
> case QTYPE_QDICT:
> visit_type_FOO_fields(, true); break
> }
> visit_end
> }
>
> or not even create a new function, and just have:
>
>
> visit_type_FOO() {
> visit_start_struct(obj)
> visit_type_FOO_fields()
> visit_end_struct()
> }
>
> visit_type_FOO_fields() {
> visit local fields
> visit variant fields
> }
>
> and for the variant type,
>
> visit_type_VARIANT() {
> allocate (currently spelled visit_start_implicit_struct, but see 13/13)
> switch (type) {
> case QTYPE_QINT:
> visit_type_int(); break
> case QTYPE_QDICT:
> visit_start_struct(NULL)
> visit_type_FOO_fields()
> visit_end_struct()
> break
> }
> visit_end_implicit
> }
>
> where the logic gets a bit more complicated to do correct error handling.
Let me get to this result from a different angle. A "boxed" visit is
allocate hook
visit the members (common and variant)
cleanup hook
An "unboxed" visit is basically the same without the two hooks.
"Implicit" is like unboxed, except the members are inlined rather than
wrapped in a JSON object.
So the common code to factor out is "visit the members".
>>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>>> index 2f23432..aba2847 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.
>>> + inline = False
>>> + for v in variants.variants:
>>> + if not isinstance(v.type, QAPISchemaObjectType):
>>> + inline = 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_member=inline),
I don't like the name is_member. The thing we're dealing with here is a
member, regardless of the value of inline. I think it's boxed
vs. unboxed.
>>> c_name=c_name(var.name))
>>>
>>> ret += mcgen('''
[...]
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 4d75d75..dbb58da 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
[...]
>>> @@ -984,8 +984,10 @@ 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_member=False):
>>> assert not self.is_implicit()
>>> + if is_member:
>>> + return c_name(self.name)
>>> return QAPISchemaType.c_type(self)
>>
>> To understand this, you have to peel off OO abstraction:
>> QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix.
>>
>> I think we should keep it peeled off in the source code.
>
> By this, you mean I should do:
>
> def c_type(self, is_param=False, is_member=False):
> assert not self.is_implicit()
> if is_member:
> return c_name(self.name)
> return c_name(self.name) + pointer_suffix
>
> ?
Yes, that's what I had in mind.
> or that I should hoist the suppression of pointer_suffix into the parent
> class?
c_type()'s is_param is ugly, and is_member adds to the ugliness. If you
can come up with a clean way to avoid one or both, go for it.
>>> def json_type(self):
>> [tests/ diff looks good]
>
> good for what they caught, but they didn't catch enough :)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate
2016-02-17 14:40 ` Markus Armbruster
@ 2016-02-17 20:34 ` Eric Blake
2016-02-18 8:21 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-17 20:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4723 bytes --]
On 02/17/2016 07:40 AM, Markus Armbruster wrote:
>>>> 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.
>>>>
>> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again,
>> because we have one less level of indirection, and 7/13 reduced the
>> indirection required in visit_type_FOO_fields()).
>>
>>> // visit_start_union() + switch dropped
>>> error_propagate(errp, err);
>>> err = NULL;
>>> visit_end_struct(v, &err);
>>> out:
>>> error_propagate(errp, err);
>>> }
>>>
>>> Why can we drop visit_start_union() + switch?
>>
>> visit_start_union() is dropped because its only purpose was to determine
>> if the dealloc visitor needs to visit the default branch. When we had a
>> separate allocation, we did not want to visit the branch if the
>> discriminator was not successfully parsed, because otherwise we would
>> dereference NULL. But now that we don't have a second pointer
>> allocation, we no longer have anything to dealloc, and we can no longer
>> dereference NULL. Explained better in 12/13, where I delete
>> visit_start_union() altogether. But maybe I could keep it in this patch
>> in the meantime, to minimize confusion.
>
> Perhaps squashing the two patches together could be less confusing.
Yes, I'm closer to posting v11, and in that version, visit_start_union
is only dropped in a single patch; and this patch just inlines the
visit_start_struct() allocation directly into the visit_type_ALT()
rather than creating a new visit_type_alternate_ALT().
>
>> Dropped switch, on the other hand, looks to be a genuine bug. Eeek.
>> That should absolutely be present, and it proves that our testsuite is
>> not strong enough for not catching me on it.
>>
>> And now that you've made me think about it, maybe I have yet another
>> idea. Right now, we've split the visit of local members into
>> visit_type_FOO_fields(), while leaving the variant members to be visited
>> in visit_type_FOO()
>
> Yes. I guess that's to support visiting "implicit" structs.
Up to now, we've forbidden the use of one union as a branch of another
(but allowed a union as a branch of an alternate), so the types passed
to visit_struct_FOO_fields() never had variants. As part of our generic
"object" work, I _want_ to support one union as a branch of another (as
long as we can prove there will be no QMP name collisions); and that's
another reason why visit_struct_FOO_fields() would need to always visit
variants if present.
> Let me get to this result from a different angle. A "boxed" visit is
>
> allocate hook
> visit the members (common and variant)
> cleanup hook
Correct, and we have two choices of allocate hook: visit_start_struct()
[allocate and consume {}, for visit_type_FOO() in general], and
visit_start_implicit_struct() [allocate, but don't consume {}, for flat
union branches prior to this series].
>
> An "unboxed" visit is basically the same without the two hooks.
Done anywhere we don't have a separate C struct [base classes prior to
this series; and then this series is adding unboxed variant visits
within flat unions and alternates].
>
> "Implicit" is like unboxed, except the members are inlined rather than
> wrapped in a JSON object.
>
> So the common code to factor out is "visit the members".
Yep, we're on the same wavelength, and it is looking fairly nice for
what I'm about to post as v11. And I like 'unboxed' better than
'is_member':
>>>> - c_type=typ.c_type(),
>>>> + c_type=typ.c_type(is_member=inline),
>
> I don't like the name is_member. The thing we're dealing with here is a
> member, regardless of the value of inline. I think it's boxed
> vs. unboxed.
Hmm. I have later patches that add a 'box':true QAPI designation to
commands and events, to let us do qmp_command(Foo *arg, Error **errp)
instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error
**errp) (that is, instead of breaking out each parameter, we pass them
all boxed behind a single pointer). What we are doing here is in the
opposite direction - we are taking code that has boxed all the sub-type
fields behind a single pointer, and unboxing it so that they occur
inline in the parent type's storage. Works for me; I'm switching to
'is_unboxed' as the case for when we want to omit the pointer
designation during the member declaration.
--
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate
2016-02-17 20:34 ` Eric Blake
@ 2016-02-18 8:21 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2016-02-18 8:21 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/17/2016 07:40 AM, Markus Armbruster wrote:
>
>>>>> 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.
>>>>>
>
>>> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again,
>>> because we have one less level of indirection, and 7/13 reduced the
>>> indirection required in visit_type_FOO_fields()).
>>>
>>>> // visit_start_union() + switch dropped
>>>> error_propagate(errp, err);
>>>> err = NULL;
>>>> visit_end_struct(v, &err);
>>>> out:
>>>> error_propagate(errp, err);
>>>> }
>>>>
>>>> Why can we drop visit_start_union() + switch?
>>>
>>> visit_start_union() is dropped because its only purpose was to determine
>>> if the dealloc visitor needs to visit the default branch. When we had a
>>> separate allocation, we did not want to visit the branch if the
>>> discriminator was not successfully parsed, because otherwise we would
>>> dereference NULL. But now that we don't have a second pointer
>>> allocation, we no longer have anything to dealloc, and we can no longer
>>> dereference NULL. Explained better in 12/13, where I delete
>>> visit_start_union() altogether. But maybe I could keep it in this patch
>>> in the meantime, to minimize confusion.
>>
>> Perhaps squashing the two patches together could be less confusing.
>
> Yes, I'm closer to posting v11, and in that version, visit_start_union
> is only dropped in a single patch; and this patch just inlines the
> visit_start_struct() allocation directly into the visit_type_ALT()
> rather than creating a new visit_type_alternate_ALT().
>
>>
>>> Dropped switch, on the other hand, looks to be a genuine bug. Eeek.
>>> That should absolutely be present, and it proves that our testsuite is
>>> not strong enough for not catching me on it.
>>>
>>> And now that you've made me think about it, maybe I have yet another
>>> idea. Right now, we've split the visit of local members into
>>> visit_type_FOO_fields(), while leaving the variant members to be visited
>>> in visit_type_FOO()
>>
>> Yes. I guess that's to support visiting "implicit" structs.
>
> Up to now, we've forbidden the use of one union as a branch of another
> (but allowed a union as a branch of an alternate), so the types passed
> to visit_struct_FOO_fields() never had variants. As part of our generic
> "object" work, I _want_ to support one union as a branch of another (as
> long as we can prove there will be no QMP name collisions); and that's
> another reason why visit_struct_FOO_fields() would need to always visit
> variants if present.
>
>
>> Let me get to this result from a different angle. A "boxed" visit is
>>
>> allocate hook
>> visit the members (common and variant)
>> cleanup hook
>
> Correct, and we have two choices of allocate hook: visit_start_struct()
> [allocate and consume {}, for visit_type_FOO() in general], and
> visit_start_implicit_struct() [allocate, but don't consume {}, for flat
> union branches prior to this series].
>
>>
>> An "unboxed" visit is basically the same without the two hooks.
>
> Done anywhere we don't have a separate C struct [base classes prior to
> this series; and then this series is adding unboxed variant visits
> within flat unions and alternates].
Should work for visiting both "inlined" and "unboxed" members, shouldn't
it?
struct {
A a;
B b
} S;
struct {
S *ps; // boxed member of type S
S s; // unboxed member of type S
A a; B b; // inlined member of type S
}
>>
>> "Implicit" is like unboxed, except the members are inlined rather than
>> wrapped in a JSON object.
>>
>> So the common code to factor out is "visit the members".
>
> Yep, we're on the same wavelength, and it is looking fairly nice for
> what I'm about to post as v11. And I like 'unboxed' better than
> 'is_member':
>
>>>>> - c_type=typ.c_type(),
>>>>> + c_type=typ.c_type(is_member=inline),
>>
>> I don't like the name is_member. The thing we're dealing with here is a
>> member, regardless of the value of inline. I think it's boxed
>> vs. unboxed.
>
> Hmm. I have later patches that add a 'box':true QAPI designation to
> commands and events, to let us do qmp_command(Foo *arg, Error **errp)
> instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error
> **errp) (that is, instead of breaking out each parameter, we pass them
> all boxed behind a single pointer). What we are doing here is in the
> opposite direction - we are taking code that has boxed all the sub-type
> fields behind a single pointer, and unboxing it so that they occur
> inline in the parent type's storage. Works for me; I'm switching to
> 'is_unboxed' as the case for when we want to omit the pointer
> designation during the member declaration.
Better. "Unboxed" isn't tied to "member"; we could choose to unbox
elsewhere, too.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (9 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-17 17:44 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union() Eric Blake
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate Eric Blake
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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. It also lets us get rid of all traces
of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
not all) special cases of simplie unions.
Unfortunately, simple unions are not as easy to convert; 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. Hence,
gen_visit_fields_decl() has to special case implicit types (the
type for a simple union variant).
Note that after this patch, 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>
---
v10: new patch
If anything, we could match our simple union wire format more closely
by teaching qapi-types to expose implicit types inline, and write:
struct SU {
SUKind type;
union {
struct {
Branch1 *data;
} branch1;
struct {
Branch2 *data;
} branch2;
} u;
};
where we would then access su.u.branch1.data->member instead of
the current su.u.branch1->member.
---
scripts/qapi-types.py | 10 +--------
scripts/qapi-visit.py | 45 +++++++----------------------------------
cpus.c | 18 ++++++-----------
hmp.c | 12 +++++------
tests/test-qmp-input-visitor.c | 2 +-
tests/test-qmp-output-visitor.c | 5 ++---
6 files changed, 23 insertions(+), 69 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index aba2847..5071817 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.
- inline = False
- for v in variants.variants:
- if not isinstance(v.type, QAPISchemaObjectType):
- inline = 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
@@ -144,7 +136,7 @@ def gen_variants(variants):
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=typ.c_type(is_member=inline),
+ c_type=typ.c_type(is_member=not var.simple_union_type()),
c_name=c_name(var.name))
ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 948bde4..68354d8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
from qapi import *
import re
-# visit_type_implicit_FOO() is emitted as needed; track if it has already
-# been output.
-implicit_structs_seen = set()
-
# visit_type_alternate_FOO() is emitted as needed; track if it has already
# been output.
alternate_structs_seen = set()
@@ -39,40 +35,15 @@ 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.is_implicit() or 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
-
-
-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_alternate_struct(typ):
@@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants):
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)
+ ret += gen_visit_fields_decl(var.type)
# 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
@@ -300,7 +269,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
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 c6419da..1c16ed9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -313,22 +313,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 139ff7d..27e5ab9 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);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 965f298..c5514a1 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>
@@ -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);
--
2.5.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions Eric Blake
@ 2016-02-17 17:44 ` Markus Armbruster
2016-02-17 20:53 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-17 17:44 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. It also lets us get rid of all traces
> of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
> not all) special cases of simplie unions.
simple
>
> Unfortunately, simple unions are not as easy to convert; 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. Hence,
> gen_visit_fields_decl() has to special case implicit types (the
> type for a simple union variant).
Simple unions should be mere sugar for the equivalent flat union, as
explained in qapi-code-gen.txt. That they aren't now on the C side is
accidental complexity. I hope we can clean that up relatively soon.
In the long run, I'd like to replace the whole struct / flat union /
simple union mess by a variant record type.
> Note that after this patch, 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.
Remind me, what makes alternates need visit_start_implicit_struct()?
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v10: new patch
>
> If anything, we could match our simple union wire format more closely
> by teaching qapi-types to expose implicit types inline, and write:
>
> struct SU {
> SUKind type;
> union {
> struct {
> Branch1 *data;
> } branch1;
> struct {
> Branch2 *data;
> } branch2;
> } u;
> };
>
> where we would then access su.u.branch1.data->member instead of
> the current su.u.branch1->member.
Looks like the cleanup I mentioned above.
> ---
> scripts/qapi-types.py | 10 +--------
> scripts/qapi-visit.py | 45 +++++++----------------------------------
> cpus.c | 18 ++++++-----------
> hmp.c | 12 +++++------
> tests/test-qmp-input-visitor.c | 2 +-
> tests/test-qmp-output-visitor.c | 5 ++---
> 6 files changed, 23 insertions(+), 69 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index aba2847..5071817 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.
> - inline = False
> - for v in variants.variants:
> - if not isinstance(v.type, QAPISchemaObjectType):
> - inline = 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
> @@ -144,7 +136,7 @@ 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
> ret += mcgen('''
> %(c_type)s %(c_name)s;
> ''',
> - c_type=typ.c_type(is_member=inline),
> + c_type=typ.c_type(is_member=not var.simple_union_type()),
> c_name=c_name(var.name))
This is where we generate flat union members unboxed: is_member=True
suppresses the pointer suffix. Still dislike the name is_member :)
Perhaps:
# Ugly special case for simple union TODO get rid of it
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_member=not simple_union_type),
c_name=c_name(var.name))
Slightly more readable, and makes it more clear that "ugly special case"
applies to is_member=... as well.
>
> ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 948bde4..68354d8 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,10 +15,6 @@
> from qapi import *
> import re
>
> -# visit_type_implicit_FOO() is emitted as needed; track if it has already
> -# been output.
> -implicit_structs_seen = set()
> -
> # visit_type_alternate_FOO() is emitted as needed; track if it has already
> # been output.
> alternate_structs_seen = set()
> @@ -39,40 +35,15 @@ 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.is_implicit() or 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
Two changes squashed together. First step is mere style:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 948bde4..ac7d504 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -39,15 +39,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
def gen_visit_implicit_struct(typ):
The blank line before the return seems superfluous.
Second step is the actual change:
@@ -35,7 +39,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **
def gen_visit_fields_decl(typ):
- if typ.name in struct_fields_seen:
+ if typ.is_implicit() or typ.name in struct_fields_seen:
return ''
struct_fields_seen.add(typ.name)
Much easier to see what's going on now.
I guess you add .is_implicit() here so that gen_visit_object() can call
it unconditionally. It's odd; other gen_ functions don't check
.is_implicit().
> -
> -
> -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_alternate_struct(typ):
> @@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants):
>
> 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)
> + ret += gen_visit_fields_decl(var.type)
Before: if this is a flat union member of type FOO, we're going to call
visit_type_implicit_FOO(), as you can see in the next hunk. Ensure it's
in scope by generating it unless it's been generated already.
After: we're going to call visit_type_FOO_fields() instead. Generate a
forward declaration unless either the function or the forward
declaration has been generated already. Except don't generate it when
FOO is an implicit type, because then the member is simple rather than
flat.
Doesn't this unduly hide the ugly special case?
To keep it in view, I'd write
# 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)
and drop the .is_implicit() from gen_visit_fields_decl().
Would this work?
>
> # 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
> @@ -300,7 +269,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
> 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))
Before: we call visit_type_implicit_FOO() to visit the *boxed* member in
C and the *inlined* member in QMP. visit_type_implicit_FOO() looks like
this:
static void visit_type_implicit_FOO(Visitor *v, FOO **obj, Error **errp)
{
Error *err = NULL;
visit_start_implicit_struct(v, (void **)obj, sizeof(FOO), &err);
if (!err) {
visit_type_FOO_fields(v, *obj, errp);
visit_end_implicit_struct(v);
}
error_propagate(errp, err);
}
After: we skip visit_start_implicit_struct() and
visit_end_implicit_struct(), i.e. the memory allocation stuff, and pass
&(*obj)->u.MEMBER instead of (*obj)->u.MEMBER to
visit_type_FOO_fields(). Okay.
Every time I come across "implicit" structs, I get confused, and have to
dig to unconfuse myself. Good to get rid of one.
> 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 c6419da..1c16ed9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -313,22 +313,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;
That's not bad at all.
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 139ff7d..27e5ab9 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);
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 965f298..c5514a1 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>
> @@ -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);
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions
2016-02-17 17:44 ` Markus Armbruster
@ 2016-02-17 20:53 ` Eric Blake
2016-02-18 8:51 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-17 20:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Crosthwaite, qemu-devel, Michael Roth, Luiz Capitulino,
Paolo Bonzini, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 9055 bytes --]
On 02/17/2016 10:44 AM, Markus Armbruster wrote:
> 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. It also lets us get rid of all traces
>> of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
>> not all) special cases of simplie unions.
>
> simple
>
>>
>> Unfortunately, simple unions are not as easy to convert; 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. Hence,
>> gen_visit_fields_decl() has to special case implicit types (the
>> type for a simple union variant).
>
> Simple unions should be mere sugar for the equivalent flat union, as
> explained in qapi-code-gen.txt. That they aren't now on the C side is
> accidental complexity. I hope we can clean that up relatively soon.
>
> In the long run, I'd like to replace the whole struct / flat union /
> simple union mess by a variant record type.
We're getting closer :)
>
>> Note that after this patch, 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.
>
> Remind me, what makes alternates need visit_start_implicit_struct()?
Here's what the two functions do:
visit_start_struct(): optionally allocate, and consume {}
visit_start_implicit_struct(): allocate, but do not consume {}
When visiting an alternate, we need to allocate the C struct that
contains the various alternate branches (visit_start_implicit_struct(),
unchanged by this patch, but renamed visit_start_alternate in 13/13),
then within that struct, if the next token is '{' we need to visit the C
struct for the object branch of the alternate (pre-series, that was
boxed, so we used visit_type_FOO(&obj) which calls visit_start_struct()
for a full allocation and consumption of {}; but with the previous
patch, it is now already allocated, so we now use visit_type_FOO(NULL)
to skip the allocation while still consuming the {}).
I can try to work something along the lines of that text into my commit
messages for v11.
>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v10: new patch
>>
>> If anything, we could match our simple union wire format more closely
>> by teaching qapi-types to expose implicit types inline, and write:
>>
>> struct SU {
>> SUKind type;
>> union {
>> struct {
>> Branch1 *data;
>> } branch1;
>> struct {
>> Branch2 *data;
>> } branch2;
>> } u;
>> };
>>
>> where we would then access su.u.branch1.data->member instead of
>> the current su.u.branch1->member.
>
> Looks like the cleanup I mentioned above.
Yay, I'm glad you like it! I've already written the patch for it, but it
was big enough (and needs several other prerequisite cleanups in the
codebase to use C99 initializers for things like SocketAddress to make
the switch easier to review) that I haven't posted it yet. And yes, it
completely gets rid of the simple_union_type() hack.
>> @@ -144,7 +136,7 @@ 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
>> ret += mcgen('''
>> %(c_type)s %(c_name)s;
>> ''',
>> - c_type=typ.c_type(is_member=inline),
>> + c_type=typ.c_type(is_member=not var.simple_union_type()),
>> c_name=c_name(var.name))
>
> This is where we generate flat union members unboxed: is_member=True
> suppresses the pointer suffix. Still dislike the name is_member :)
>
> Perhaps:
>
> # Ugly special case for simple union TODO get rid of it
> 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_member=not simple_union_type),
> c_name=c_name(var.name))
>
> Slightly more readable, and makes it more clear that "ugly special case"
> applies to is_member=... as well.
It gets renamed to is_unboxed after the review on 10/13. But even after
my patch to convert simple unions, this code will still be
c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework
.c_type() to not need two separate boolean flags for the three different
contexts in which we use a type name (declaring an unboxed member to a
struct, declaring a local variable, and declaring a const parameter).
>> 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
>
> Two changes squashed together. First step is mere style:
Then I'll split into two patches for v11.
> Second step is the actual change:
>
> @@ -35,7 +39,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **
>
>
> def gen_visit_fields_decl(typ):
> - if typ.name in struct_fields_seen:
> + if typ.is_implicit() or typ.name in struct_fields_seen:
> return ''
> struct_fields_seen.add(typ.name)
>
> Much easier to see what's going on now.
>
> I guess you add .is_implicit() here so that gen_visit_object() can call
> it unconditionally. It's odd; other gen_ functions don't check
> .is_implicit().
Although I have more plans to use .is_implicit() - I have patches in my
local tree that allow:
{ 'enum': 'Enum', 'data': [ 'branch' ] }
{ 'union': 'U', 'base': { 'anonymous1': 'Enum' },
'discriminator': 'anonymous1',
'data': { 'branch': { 'anonymous2': 'str' } } }
that is, both an anonymous base, and an anonymous branch. It results in
more places where we'll need to suppress declarations if the type is
implicit; so doing it here instead of in each caller proves easier in
the long run.
But for this patch, I can probably go along with your idea of keeping
the complexity in the caller, where we highlight that simple unions are
still special cases for a bit longer...
>> @@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants):
>>
>> 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)
>> + ret += gen_visit_fields_decl(var.type)
>
> Before: if this is a flat union member of type FOO, we're going to call
> visit_type_implicit_FOO(), as you can see in the next hunk. Ensure it's
> in scope by generating it unless it's been generated already.
>
> After: we're going to call visit_type_FOO_fields() instead. Generate a
> forward declaration unless either the function or the forward
> declaration has been generated already. Except don't generate it when
> FOO is an implicit type, because then the member is simple rather than
> flat.
>
> Doesn't this unduly hide the ugly special case?
>
> To keep it in view, I'd write
>
> # 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)
>
> and drop the .is_implicit() from gen_visit_fields_decl().
>
> Would this work?
...It should; I'm testing it now.
>
> Every time I come across "implicit" structs, I get confused, and have to
> dig to unconfuse myself. Good to get rid of one.
Yep - and it makes my stalled work on documenting visitor.h easier with
fewer ugly things to document :)
>> 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;
>
> That's not bad at all.
I was actually pleasantly shocked at how few places in code needed
changing. The conversion of simple unions sitting in my local tree was
more complex (much of that because we use SocketAddress in a LOT more
places).
--
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions
2016-02-17 20:53 ` Eric Blake
@ 2016-02-18 8:51 ` Markus Armbruster
2016-02-18 16:51 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-18 8: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:
> On 02/17/2016 10:44 AM, Markus Armbruster wrote:
>> 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. It also lets us get rid of all traces
>>> of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
>>> not all) special cases of simplie unions.
>>
>> simple
>>
>>>
>>> Unfortunately, simple unions are not as easy to convert; 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. Hence,
>>> gen_visit_fields_decl() has to special case implicit types (the
>>> type for a simple union variant).
>>
>> Simple unions should be mere sugar for the equivalent flat union, as
>> explained in qapi-code-gen.txt. That they aren't now on the C side is
>> accidental complexity. I hope we can clean that up relatively soon.
>>
>> In the long run, I'd like to replace the whole struct / flat union /
>> simple union mess by a variant record type.
>
> We're getting closer :)
>
>>
>>> Note that after this patch, 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.
>>
>> Remind me, what makes alternates need visit_start_implicit_struct()?
>
> Here's what the two functions do:
>
> visit_start_struct(): optionally allocate, and consume {}
> visit_start_implicit_struct(): allocate, but do not consume {}
>
> When visiting an alternate, we need to allocate the C struct that
> contains the various alternate branches (visit_start_implicit_struct(),
> unchanged by this patch, but renamed visit_start_alternate in 13/13),
> then within that struct, if the next token is '{' we need to visit the C
> struct for the object branch of the alternate (pre-series, that was
> boxed, so we used visit_type_FOO(&obj) which calls visit_start_struct()
> for a full allocation and consumption of {}; but with the previous
> patch, it is now already allocated, so we now use visit_type_FOO(NULL)
> to skip the allocation while still consuming the {}).
>
> I can try to work something along the lines of that text into my commit
> messages for v11.
I've since made it to PATCH 13, and found the fused
visit_start_alternate(). Much easier to comprehend than the
visit_start_implicit_struct() + visit_get_next_type() combo.
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>>> ---
>>> v10: new patch
>>>
>>> If anything, we could match our simple union wire format more closely
>>> by teaching qapi-types to expose implicit types inline, and write:
>>>
>>> struct SU {
>>> SUKind type;
>>> union {
>>> struct {
>>> Branch1 *data;
>>> } branch1;
>>> struct {
>>> Branch2 *data;
>>> } branch2;
>>> } u;
>>> };
>>>
>>> where we would then access su.u.branch1.data->member instead of
>>> the current su.u.branch1->member.
>>
>> Looks like the cleanup I mentioned above.
>
> Yay, I'm glad you like it! I've already written the patch for it, but it
> was big enough (and needs several other prerequisite cleanups in the
> codebase to use C99 initializers for things like SocketAddress to make
> the switch easier to review) that I haven't posted it yet. And yes, it
> completely gets rid of the simple_union_type() hack.
Looking forward to its demise.
>>> @@ -144,7 +136,7 @@ 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
>>> ret += mcgen('''
>>> %(c_type)s %(c_name)s;
>>> ''',
>>> - c_type=typ.c_type(is_member=inline),
>>> + c_type=typ.c_type(is_member=not var.simple_union_type()),
>>> c_name=c_name(var.name))
>>
>> This is where we generate flat union members unboxed: is_member=True
>> suppresses the pointer suffix. Still dislike the name is_member :)
>>
>> Perhaps:
>>
>> # Ugly special case for simple union TODO get rid of it
>> 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_member=not simple_union_type),
>> c_name=c_name(var.name))
>>
>> Slightly more readable, and makes it more clear that "ugly special case"
>> applies to is_member=... as well.
>
> It gets renamed to is_unboxed after the review on 10/13. But even after
> my patch to convert simple unions, this code will still be
> c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework
> .c_type() to not need two separate boolean flags for the three different
> contexts in which we use a type name (declaring an unboxed member to a
> struct, declaring a local variable, and declaring a const parameter).
A possible alternative to a single c_type() with flags for context would
be separate c_CONTEXT_type().
In QAPISchemaType:
def c_type(self):
pass
def c_param_type(self):
return self.c_type()
QAPISchemaBuiltinType overrides:
def c_type(self):
return self._c_type_name
def c_param_type(self):
if self.name == 'str':
return 'const ' + self._c_type_name
return self._c_type_name
QAPISchemaEnumType:
def c_type(self):
return c_name(self.name)
QAPISchemaArrayType:
def c_type(self):
return c_name(self.name) + pointer_suffix
QAPISchemaObjectType:
def c_type(self):
assert not self.is_implicit()
return c_name(self.name) + pointer_suffix
def c_unboxed_type(self):
return c_name(self.name)
QAPISchemaAlternateType:
def c_type(self):
return c_name(self.name) + pointer_suffix
Lots of trivial code, as so often with OO.
>>> 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
>>
>> Two changes squashed together. First step is mere style:
>
> Then I'll split into two patches for v11.
>
>> Second step is the actual change:
>>
>> @@ -35,7 +39,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **
>>
>>
>> def gen_visit_fields_decl(typ):
>> - if typ.name in struct_fields_seen:
>> + if typ.is_implicit() or typ.name in struct_fields_seen:
>> return ''
>> struct_fields_seen.add(typ.name)
>>
>> Much easier to see what's going on now.
>>
>> I guess you add .is_implicit() here so that gen_visit_object() can call
>> it unconditionally. It's odd; other gen_ functions don't check
>> .is_implicit().
>
> Although I have more plans to use .is_implicit() - I have patches in my
> local tree that allow:
>
> { 'enum': 'Enum', 'data': [ 'branch' ] }
> { 'union': 'U', 'base': { 'anonymous1': 'Enum' },
> 'discriminator': 'anonymous1',
> 'data': { 'branch': { 'anonymous2': 'str' } } }
>
> that is, both an anonymous base, and an anonymous branch. It results in
> more places where we'll need to suppress declarations if the type is
> implicit; so doing it here instead of in each caller proves easier in
> the long run.
>
> But for this patch, I can probably go along with your idea of keeping
> the complexity in the caller, where we highlight that simple unions are
> still special cases for a bit longer...
Yes, please.
>>> @@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants):
>>>
>>> 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)
>>> + ret += gen_visit_fields_decl(var.type)
>>
>> Before: if this is a flat union member of type FOO, we're going to call
>> visit_type_implicit_FOO(), as you can see in the next hunk. Ensure it's
>> in scope by generating it unless it's been generated already.
>>
>> After: we're going to call visit_type_FOO_fields() instead. Generate a
>> forward declaration unless either the function or the forward
>> declaration has been generated already. Except don't generate it when
>> FOO is an implicit type, because then the member is simple rather than
>> flat.
>>
>> Doesn't this unduly hide the ugly special case?
>>
>> To keep it in view, I'd write
>>
>> # 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)
>>
>> and drop the .is_implicit() from gen_visit_fields_decl().
>>
>> Would this work?
>
> ...It should; I'm testing it now.
>
>>
>> Every time I come across "implicit" structs, I get confused, and have to
>> dig to unconfuse myself. Good to get rid of one.
>
> Yep - and it makes my stalled work on documenting visitor.h easier with
> fewer ugly things to document :)
I welcome a smaller visitor.h; reviewing the first iteration of the
documentation patch was tough going.
>>> 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;
>>
>> That's not bad at all.
>
> I was actually pleasantly shocked at how few places in code needed
> changing. The conversion of simple unions sitting in my local tree was
> more complex (much of that because we use SocketAddress in a LOT more
> places).
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions
2016-02-18 8:51 ` Markus Armbruster
@ 2016-02-18 16:51 ` Eric Blake
2016-02-18 17:11 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-18 16:51 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Crosthwaite, qemu-devel, Michael Roth, Luiz Capitulino,
Paolo Bonzini, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]
On 02/18/2016 01:51 AM, Markus Armbruster wrote:
>> It gets renamed to is_unboxed after the review on 10/13. But even after
>> my patch to convert simple unions, this code will still be
>> c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework
>> .c_type() to not need two separate boolean flags for the three different
>> contexts in which we use a type name (declaring an unboxed member to a
>> struct, declaring a local variable, and declaring a const parameter).
>
> A possible alternative to a single c_type() with flags for context would
> be separate c_CONTEXT_type().
>
> In QAPISchemaType:
>
> def c_type(self):
> pass
>
> def c_param_type(self):
> return self.c_type()
and
def c_unboxed_type(self):
return self.c_type()
so that c_unboxed_type() is callable on all types, not just objects.
>
> QAPISchemaBuiltinType overrides:
>
> def c_type(self):
> return self._c_type_name
>
> def c_param_type(self):
> if self.name == 'str':
> return 'const ' + self._c_type_name
> return self._c_type_name
...
>
> Lots of trivial code, as so often with OO.
But I'm liking it a bit better than the two flags. Your suggestion came
after my v11; at this point, if you want me to pursue the idea, I'm glad
to do it as a followup, and include it in my next series where I finish
the conversion of simple unions.
--
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions
2016-02-18 16:51 ` Eric Blake
@ 2016-02-18 17:11 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2016-02-18 17:11 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:
> On 02/18/2016 01:51 AM, Markus Armbruster wrote:
>
>>> It gets renamed to is_unboxed after the review on 10/13. But even after
>>> my patch to convert simple unions, this code will still be
>>> c_type=typ.c_type(is_unboxed=True), unless I figure out a way to rework
>>> .c_type() to not need two separate boolean flags for the three different
>>> contexts in which we use a type name (declaring an unboxed member to a
>>> struct, declaring a local variable, and declaring a const parameter).
>>
>> A possible alternative to a single c_type() with flags for context would
>> be separate c_CONTEXT_type().
>>
>> In QAPISchemaType:
>>
>> def c_type(self):
>> pass
>>
>> def c_param_type(self):
>> return self.c_type()
>
> and
>
> def c_unboxed_type(self):
> return self.c_type()
>
> so that c_unboxed_type() is callable on all types, not just objects.
I defined it only for object types because we box only object types.
Err, object types and lists.
If we have a need to call it for arbitrary types, we can do that, we
just need to pick names carefully, and write suitable contracts.
>>
>> QAPISchemaBuiltinType overrides:
>>
>> def c_type(self):
>> return self._c_type_name
>>
>> def c_param_type(self):
>> if self.name == 'str':
>> return 'const ' + self._c_type_name
>> return self._c_type_name
> ...
>
>>
>> Lots of trivial code, as so often with OO.
>
> But I'm liking it a bit better than the two flags. Your suggestion came
> after my v11; at this point, if you want me to pursue the idea, I'm glad
> to do it as a followup, and include it in my next series where I finish
> the conversion of simple unions.
I need to run an errand now. When I'm back, I'll review whether the
series can go to qapi-next without a respin.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (10 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-17 18:08 ` Markus Armbruster
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate Eric Blake
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Commit cee2dedb noticed that if you have a partial flat union
(such as if an input parse failed due to a missing
discriminator), calling the dealloc visitor could result in
trying to dereference a NULL pointer if we attempted to visit
an object branch without an earlier successful call to
visit_start_implicit_struct() allocating the pointer for that
branch. But the "fix" it implemented requires the use of a
'.data' member in the union, which may or may not be the same
size as other branches of the union (consider a 32-bit platform
where one of the branches is an int64), which feels fairly dirty.
Plus, as mentioned in that commit, it only works if you can
assume that '.data' would be zero-initialized even if '.kind' was
uninitialized, which is rather poor logic: our usage of
visit_start_struct() happens to zero-initialize both fields,
which means '.kind' is never truly uninitialized - but if we
changed visit_start_struct() to use g_new() instead of g_new0(),
then '.data' would not be any more reliable as a condition on
whether to visit the branch matching '.kind', regardless of
whether '.kind' was 0).
Menawhile, now that we have just inlined the fields of all flat
unions, there is no longer the possibility of a null pointer to
dereference in the first place. Where the branch structure used
to be separately allocated by visit_start_implicit_struct(), it
is now just pointing to a subset of the memory already
zero-allocated by visit_start_struct().
Thus, we can instead fix things to delete the misguided
visit_start_union(), as it is no longer providing any benefit.
And it finishes the cleanup we started in commit 7c91aabd when
we deleted visit_end_union(). Generated code changes as follows:
|@@ -2366,9 +2363,6 @@ void visit_type_ChardevBackend(Visitor *
| if (err) {
| goto out_obj;
| }
|- if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
|- goto out_obj;
|- }
| switch ((*obj)->type) {
| case CHARDEV_BACKEND_KIND_FILE:
| visit_type_ChardevFile(v, "data", &(*obj)->u.file, &err);
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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 | 3 ---
qapi/qapi-visit-core.c | 8 --------
qapi/qapi-dealloc-visitor.c | 26 --------------------------
5 files changed, 40 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index c131a32..b8ae1b5 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -80,6 +80,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 68354d8..02f0122 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
if variants:
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))
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6fa66f1..976106e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -60,14 +60,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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union() Eric Blake
@ 2016-02-17 18:08 ` Markus Armbruster
2016-02-17 21:19 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-17 18:08 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Commit cee2dedb noticed that if you have a partial flat union
> (such as if an input parse failed due to a missing
> discriminator), calling the dealloc visitor could result in
> trying to dereference a NULL pointer if we attempted to visit
> an object branch without an earlier successful call to
> visit_start_implicit_struct() allocating the pointer for that
> branch. But the "fix" it implemented requires the use of a
> '.data' member in the union, which may or may not be the same
> size as other branches of the union (consider a 32-bit platform
> where one of the branches is an int64), which feels fairly dirty.
Well, until the previous commit, it was the same, wasn't it? All
pointers.
> Plus, as mentioned in that commit, it only works if you can
> assume that '.data' would be zero-initialized even if '.kind' was
> uninitialized, which is rather poor logic: our usage of
> visit_start_struct() happens to zero-initialize both fields,
> which means '.kind' is never truly uninitialized - but if we
> changed visit_start_struct() to use g_new() instead of g_new0(),
> then '.data' would not be any more reliable as a condition on
> whether to visit the branch matching '.kind', regardless of
> whether '.kind' was 0).
>
> Menawhile, now that we have just inlined the fields of all flat
> unions, there is no longer the possibility of a null pointer to
> dereference in the first place. Where the branch structure used
> to be separately allocated by visit_start_implicit_struct(), it
> is now just pointing to a subset of the memory already
> zero-allocated by visit_start_struct().
>
> Thus, we can instead fix things to delete the misguided
> visit_start_union(), as it is no longer providing any benefit.
> And it finishes the cleanup we started in commit 7c91aabd when
> we deleted visit_end_union(). Generated code changes as follows:
>
> |@@ -2366,9 +2363,6 @@ void visit_type_ChardevBackend(Visitor *
> | if (err) {
> | goto out_obj;
> | }
> |- if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> |- goto out_obj;
> |- }
> | switch ((*obj)->type) {
> | case CHARDEV_BACKEND_KIND_FILE:
> | visit_type_ChardevFile(v, "data", &(*obj)->u.file, &err);
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> 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 | 3 ---
> qapi/qapi-visit-core.c | 8 --------
> qapi/qapi-dealloc-visitor.c | 26 --------------------------
> 5 files changed, 40 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index c131a32..b8ae1b5 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -80,6 +80,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 68354d8..02f0122 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
> if variants:
> ret += gen_err_check(label='out_obj')
> ret += mcgen('''
> - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> - goto out_obj;
> - }
I'm afraid the previous commit broke this for flat unions.
Before the previous commit, all members of (*obj)->u were pointers to
the struct holding the variant members both for flat and simple unions.
!!(*obj)->u.data tests whether the struct holding the variant members
has been allocated. This relies on uniform pointer format.
The dealloc visitor uses the "has been allocated" bit to suppress
visiting the struct when it hasn't been allocated.
The previous commit unboxes the struct for flat unions. Now ->u.data
reinterprets the first few bytes of that struct as pointer. If you're
"lucky", they're not all zero, and the struct gets visited.
Obvious fix: squash this hunk into the previous commit, then let this
commit drop the code that's no longer used.
However, simple unions are still boxed. Why can't their pointer be null
in the dealloc visitor?
> switch ((*obj)->%(c_name)s) {
> ''',
> c_name=c_name(variants.tag_member.name))
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6fa66f1..976106e 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -60,14 +60,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);
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()
2016-02-17 18:08 ` Markus Armbruster
@ 2016-02-17 21:19 ` Eric Blake
2016-02-18 8:24 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-17 21:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4815 bytes --]
On 02/17/2016 11:08 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Commit cee2dedb noticed that if you have a partial flat union
>> (such as if an input parse failed due to a missing
>> discriminator), calling the dealloc visitor could result in
>> trying to dereference a NULL pointer if we attempted to visit
>> an object branch without an earlier successful call to
>> visit_start_implicit_struct() allocating the pointer for that
>> branch. But the "fix" it implemented requires the use of a
>> '.data' member in the union, which may or may not be the same
>> size as other branches of the union (consider a 32-bit platform
>> where one of the branches is an int64), which feels fairly dirty.
>
> Well, until the previous commit, it was the same, wasn't it? All
> pointers.
For simple unions, you could have (well, still can have, until my later
patch gets rid of the simple_union_type() magic):
struct SU {
SUKind type;
union {
void *data;
int8_t byte;
} u;
};
But you're right - for flat unions, ALL branches were represented as
pointers (until this series unboxed them).
>
>> Plus, as mentioned in that commit, it only works if you can
>> assume that '.data' would be zero-initialized even if '.kind' was
>> uninitialized, which is rather poor logic: our usage of
>> visit_start_struct() happens to zero-initialize both fields,
>> which means '.kind' is never truly uninitialized - but if we
>> changed visit_start_struct() to use g_new() instead of g_new0(),
>> then '.data' would not be any more reliable as a condition on
>> whether to visit the branch matching '.kind', regardless of
>> whether '.kind' was 0).
>>
>> Menawhile, now that we have just inlined the fields of all flat
Meanwhile,
>> unions, there is no longer the possibility of a null pointer to
>> dereference in the first place. Where the branch structure used
>> to be separately allocated by visit_start_implicit_struct(), it
>> is now just pointing to a subset of the memory already
>> zero-allocated by visit_start_struct().
I guess I may try and reword this slightly, and point to the fact that
the NULL dereference was due to calling visit_start_implicit_FOO() (only
done for flat unions; for simple unions the branches call
visit_type_FOO(), and that call safely handled NULL); because we were
using visit_start/end_implicit_struct() for its allocation effects. But
the net result is the same - now that we no longer call
visit_start_implicit_struct() for a union visit, the dealloc visitor no
longer has to worry about a NULL dereference on a partially constructed
object, so we no longer need to probe if the union contains any data.
>> +++ b/scripts/qapi-visit.py
>> @@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>> if variants:
>> ret += gen_err_check(label='out_obj')
>> ret += mcgen('''
>> - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
>> - goto out_obj;
>> - }
>
> I'm afraid the previous commit broke this for flat unions.
>
> Before the previous commit, all members of (*obj)->u were pointers to
> the struct holding the variant members both for flat and simple unions.
> !!(*obj)->u.data tests whether the struct holding the variant members
> has been allocated. This relies on uniform pointer format.
>
> The dealloc visitor uses the "has been allocated" bit to suppress
> visiting the struct when it hasn't been allocated.
>
> The previous commit unboxes the struct for flat unions. Now ->u.data
> reinterprets the first few bytes of that struct as pointer. If you're
> "lucky", they're not all zero, and the struct gets visited.
You're right - and I bet I could come up with a case where valgrind
could call me on it.
>
> Obvious fix: squash this hunk into the previous commit, then let this
> commit drop the code that's no longer used.
Yep, for bisectability, I think that's what I'll end up doing.
>
> However, simple unions are still boxed. Why can't their pointer be null
> in the dealloc visitor?
Simple unions still go through visit_type_FOO(), and _that_ function
properly checks for NULL. It was only visit_type_implicit_FOO() that
blindly dereferenced things. In fact, in the earlier incantation of
this patch, my fix was to teach visit_type_implicit_FOO() how to check
for NULL:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05442.html
But now that visit_type_implicit_FOO() is gone, my earlier incantation
got reduced in size. I guess it's all in how I document the commit message.
--
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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()
2016-02-17 21:19 ` Eric Blake
@ 2016-02-18 8:24 ` Markus Armbruster
2016-02-18 16:47 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-02-18 8:24 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 02/17/2016 11:08 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Commit cee2dedb noticed that if you have a partial flat union
>>> (such as if an input parse failed due to a missing
>>> discriminator), calling the dealloc visitor could result in
>>> trying to dereference a NULL pointer if we attempted to visit
>>> an object branch without an earlier successful call to
>>> visit_start_implicit_struct() allocating the pointer for that
>>> branch. But the "fix" it implemented requires the use of a
>>> '.data' member in the union, which may or may not be the same
>>> size as other branches of the union (consider a 32-bit platform
>>> where one of the branches is an int64), which feels fairly dirty.
>>
>> Well, until the previous commit, it was the same, wasn't it? All
>> pointers.
>
> For simple unions, you could have (well, still can have, until my later
> patch gets rid of the simple_union_type() magic):
>
> struct SU {
> SUKind type;
> union {
> void *data;
> int8_t byte;
> } u;
> };
Begs the question why that works :)
> But you're right - for flat unions, ALL branches were represented as
> pointers (until this series unboxed them).
>
>>
>>> Plus, as mentioned in that commit, it only works if you can
>>> assume that '.data' would be zero-initialized even if '.kind' was
>>> uninitialized, which is rather poor logic: our usage of
>>> visit_start_struct() happens to zero-initialize both fields,
>>> which means '.kind' is never truly uninitialized - but if we
>>> changed visit_start_struct() to use g_new() instead of g_new0(),
>>> then '.data' would not be any more reliable as a condition on
>>> whether to visit the branch matching '.kind', regardless of
>>> whether '.kind' was 0).
>>>
>>> Menawhile, now that we have just inlined the fields of all flat
>
> Meanwhile,
>
>>> unions, there is no longer the possibility of a null pointer to
>>> dereference in the first place. Where the branch structure used
>>> to be separately allocated by visit_start_implicit_struct(), it
>>> is now just pointing to a subset of the memory already
>>> zero-allocated by visit_start_struct().
>
> I guess I may try and reword this slightly, and point to the fact that
> the NULL dereference was due to calling visit_start_implicit_FOO() (only
> done for flat unions; for simple unions the branches call
> visit_type_FOO(), and that call safely handled NULL);
That's why it works?
> because we were
> using visit_start/end_implicit_struct() for its allocation effects. But
> the net result is the same - now that we no longer call
> visit_start_implicit_struct() for a union visit, the dealloc visitor no
> longer has to worry about a NULL dereference on a partially constructed
> object, so we no longer need to probe if the union contains any data.
>
>>> +++ b/scripts/qapi-visit.py
>>> @@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>>> if variants:
>>> ret += gen_err_check(label='out_obj')
>>> ret += mcgen('''
>>> - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
>>> - goto out_obj;
>>> - }
>>
>> I'm afraid the previous commit broke this for flat unions.
>>
>> Before the previous commit, all members of (*obj)->u were pointers to
>> the struct holding the variant members both for flat and simple unions.
>> !!(*obj)->u.data tests whether the struct holding the variant members
>> has been allocated. This relies on uniform pointer format.
>>
>> The dealloc visitor uses the "has been allocated" bit to suppress
>> visiting the struct when it hasn't been allocated.
>>
>> The previous commit unboxes the struct for flat unions. Now ->u.data
>> reinterprets the first few bytes of that struct as pointer. If you're
>> "lucky", they're not all zero, and the struct gets visited.
>
> You're right - and I bet I could come up with a case where valgrind
> could call me on it.
>
>>
>> Obvious fix: squash this hunk into the previous commit, then let this
>> commit drop the code that's no longer used.
>
> Yep, for bisectability, I think that's what I'll end up doing.
>
>>
>> However, simple unions are still boxed. Why can't their pointer be null
>> in the dealloc visitor?
>
> Simple unions still go through visit_type_FOO(), and _that_ function
> properly checks for NULL. It was only visit_type_implicit_FOO() that
> blindly dereferenced things. In fact, in the earlier incantation of
> this patch, my fix was to teach visit_type_implicit_FOO() how to check
> for NULL:
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05442.html
>
> But now that visit_type_implicit_FOO() is gone, my earlier incantation
> got reduced in size. I guess it's all in how I document the commit message.
Give it a try :)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()
2016-02-18 8:24 ` Markus Armbruster
@ 2016-02-18 16:47 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-02-18 16:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]
On 02/18/2016 01:24 AM, Markus Armbruster wrote:
>> For simple unions, you could have (well, still can have, until my later
>> patch gets rid of the simple_union_type() magic):
>>
>> struct SU {
>> SUKind type;
>> union {
>> void *data;
>> int8_t byte;
>> } u;
>> };
>
> Begs the question why that works :)
By sheer luck, and (poorly?) documented in a hairy comment in
qapi-dealloc-visitor.c (at least, until I delete visit_start_union). We
have a data-dependent decision (not only the contents of 'byte', but
ALSO the contents of the padding bits), but either the decision results
in calling visit_type_int8() (and doing nothing) or skipping the call
(and likewise doing nothing).
>> I guess I may try and reword this slightly, and point to the fact that
>> the NULL dereference was due to calling visit_start_implicit_FOO() (only
>> done for flat unions; for simple unions the branches call
>> visit_type_FOO(), and that call safely handled NULL);
>
> That's why it works?
>
>> But now that visit_type_implicit_FOO() is gone, my earlier incantation
>> got reduced in size. I guess it's all in how I document the commit message.
>
> Give it a try :)
I gave it my best in v11 :) Maybe you'll still have wording
improvements, but this back-and-forth has helped both of us try to
actually characterize what is going on.
--
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] 39+ messages in thread
* [Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate
2016-02-16 0:20 [Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E) Eric Blake
` (11 preceding siblings ...)
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union() Eric Blake
@ 2016-02-16 0:20 ` Eric Blake
2016-02-17 18:13 ` Markus Armbruster
12 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-02-16 0:20 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_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
| break;
...
| }
|-out_obj:
|- visit_end_implicit_struct(v);
|+ visit_end_alternate(v);
| out:
| error_propagate(errp, err);
| }
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v10: new patch
---
include/qapi/visitor.h | 50 ++++++++++++++++++++++++++++++++++-----------
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(+), 72 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b8ae1b5..83cad74 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -19,7 +19,6 @@
#include "qapi/error.h"
#include <stdlib.h>
-
/* This struct is layout-compatible with all other *List structs
* created by the qapi generator. It is used as a typical
* singly-linked list. */
@@ -28,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
@@ -47,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 02f0122..2749331 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -175,14 +175,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)
@@ -205,8 +202,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 976106e..973ab72 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);
@@ -60,6 +45,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) {
@@ -68,14 +70,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] 39+ messages in thread* Re: [Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate
2016-02-16 0:20 ` [Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate Eric Blake
@ 2016-02-17 18:13 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2016-02-17 18:13 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> 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_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
> | break;
> ...
> | }
> |-out_obj:
> |- visit_end_implicit_struct(v);
> |+ visit_end_alternate(v);
> | out:
> | error_propagate(errp, err);
> | }
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v10: new patch
> ---
> include/qapi/visitor.h | 50 ++++++++++++++++++++++++++++++++++-----------
> 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(+), 72 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index b8ae1b5..83cad74 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -19,7 +19,6 @@
> #include "qapi/error.h"
> #include <stdlib.h>
>
> -
> /* This struct is layout-compatible with all other *List structs
> * created by the qapi generator. It is used as a typical
> * singly-linked list. */
> @@ -28,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
> @@ -47,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 02f0122..2749331 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -175,14 +175,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)
> @@ -205,8 +202,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 976106e..973ab72 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);
> @@ -60,6 +45,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) {
> @@ -68,14 +70,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);
Okay, this start_alternate stuff looks like it could actually have a
chance not to confuse me every time I run across it, unlike the
implicit_struct stuff it replaces.
^ permalink raw reply [flat|nested] 39+ messages in thread