* [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D)
@ 2015-11-20 17:24 Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject Eric Blake
` (14 more replies)
0 siblings, 15 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
Pending prerequisites:
+ Markus' "typedefs: Put them back into alphabetical order"
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04417.html
+ Markus' qapi-next branch
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/qapi-next
Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv13d
and will soon be part of my branch with the rest of the v5 series, at:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
v13 notes:
1-26 of v12 were taken into qapi-next. 27 and 34 are dropped,
replaced by new patches for whitelisting exceptions to the
lower-case member name rule. Pick up the tail end of v11 that
was dropped in v12 (although 26/28 and 27/28 of v11 are merged
as a single 13/14 here).
backport diff (confused by rename of 1, 3, and 13)
001/14:[down] 'qobject: Simplify QObject'
002/14:[0004] [FC] 'qobject: Rename qtype_code to QType'
003/14:[down] 'qapi: Convert QType into QAPI built-in enum type'
004/14:[0001] [FC] 'qapi: Simplify visiting of alternate types'
005/14:[----] [--] 'qapi: Inline _make_implicit_tag()'
006/14:[----] [--] 'qapi: Fix alternates that accept 'number' but not 'int''
007/14:[----] [--] 'qapi: Simplify visits of optional fields'
008/14:[----] [--] 'qapi: Shorter visits of optional fields'
009/14:[down] 'qapi: Prepare new QAPISchemaMember base class'
010/14:[down] 'qapi: Track enum values by QAPISchemaMember, not string'
011/14:[down] 'qapi: Populate info['name'] for each entity'
012/14:[down] 'qapi: Enforce (or whitelist) case conventions on qapi members'
013/14:[down] 'qapi: Move duplicate collision checks to schema check()'
014/14:[0010] [FC] 'qapi: Detect base class loops'
v12 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04033.html
Delay 26-28 to a later subset, and instead add lots of new prereq
patches that tackle some cleanups that make case-insensitive
collision detection easier. Series is now slated for 2.6.
v11 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg02563.html
First half of v10 subset C has been applied, so consolidate the last
half of that along with all of subset D (which was at v9) into one
group. Address list reviews, in particular, add a new patch 21 that
makes alternate layouts a lot nicer by making qtype_code a builtin
qapi type; and new patches 18-19 that try to reduce confusion on
the use of camel_to_upper() in c_enum_const().
Probably too late to get these into 2.5, in which case 17/28 will need
tweaks to call out 2.6.
v10 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg01249.html
Split several patches, redo the middle patches from Markus to be
back in the order they were first posted, some fallout change to
my patches due to the nicer pattern of minimizing conditionals
inside .check(), by instead calling .check_clash() as needed.
Change data->err magic in tests to instead use a new helper
error_free_or_abort(). Add a patch that would prevent qapi
case-insensitive clashes.
I am redoing my subset boundaries slightly: patches 23-27 of
v9 (updating the alternate layout) will be delayed to subset D,
and 2 other patches previously posted in subset D are now here
(turning qapi clash checking into actual error messages), so
the subject line of this cover letter is slightly different.
Hopefully, we are converging on something that will be ready
for a pull request, especially for the earlier patches of this
subset.
v9 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00652.html
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06999.html
More patches added, and several reorganized. Lots of new patches
from Markus, although not in the order originally proposed.
The first 8 patches are fairly straightforward, and could probably
be taken as-is. Patch 9 is a rewrite of v8 4/17, but in the opposite
direction (document that no sorting is done, rather than attempting
to sort), so it may need further fine-tuning. Patches 12-21
represents a fusion of Markus' and my attempts to rewrite v5 7/17
into a more-reviewable set of patches, and caused further churn
later in the series.
Patch 23 still uses tag_member.type == None; I ran out of time to
work on Markus' idea of providing an instance of QAPISchemaBuiltinType
to fill the role for 'qtype_code' without being exposed through .json
files and without breaking the invariant of a valid member.type after
check(), and wanted to get the rest of the series started under revew.
So I may need a followup patch or even a v10 of the later half of
this series after exploring that idea more.
v8 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06674.html
Minor changes when rebasing to latest, improved commit messages in
a few places, plus the addition of a few new patches. Markus started
reviewing v7, but hadn't gotten very far.
v7 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04112.html
Patches 1-3 of the previous round have moved to subset B; patch 7
is gone, and a couple of new patches are present in this round. The
latest version of subset B made it much easier to reason about
collision detection (namely, tag values can't collide with QMP values
thanks to a named rather than anonymous union in the C type), and I
like how things turned out. I suspect the hardest part of the review
will be patches 5/14 and 7/14, although none of this has really
had much review in any earlier versions.
v6 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01980.html
Add some patches and rebase onto work on subset B. Rearrange some
patches from v5 (this set includes 17-20, 23, 25-27). Backport diff
gets a bit confused by one patch title changing.
Not much direct review comments, although some of the changes here
are updated based on comments made on other patches in the v5 series.
Subset D (and more?) will come later.
In v5:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05410.html
I _did_ rearrange patches to try and group related features:
1-2: Groundwork cleanups
3-5: Add more test cases
6-16: Front-end cleanups
17-18: Introspection output cleanups
19-20: 'alternate' type cleanups
21-29: qapi visitor cleanups
30-45: qapi-ify netdev_add
46: add qapi shorthand for flat unions
Lots of fixes based on additional testing, and rebased to
track other changes that happened in the meantime. The series
is huge; I can split off smaller portions as requested.
In v4:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html
add some more clean up patches
rebase to Markus' recent work
pull in part of Zoltán's work to make netdev_add a flat union,
further enhancing it to be introspectible
I might be able to rearrange some of these patches, or separate
it into smaller independent series, if requested; but I'm
posting now to get review started.
In v3:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02059.html
redo cleanup of dealloc of partial struct
add patches to make all visit_type_*() avoid leaks on failure
add patches to allow boxed command arguments and events
In v2:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00900.html
rebase to Markus' v3 series
rework how comments are emitted for fields inherited from base
additional patches added for deleting colliding 'void *data'
documentation updates to match code changes
v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05266.html
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05325.html
Eric Blake (14):
qobject: Simplify QObject
qobject: Rename qtype_code to QType
qapi: Convert QType into QAPI built-in enum type
qapi: Simplify visiting of alternate types
qapi: Inline _make_implicit_tag()
qapi: Fix alternates that accept 'number' but not 'int'
qapi: Simplify visits of optional fields
qapi: Shorter visits of optional fields
qapi: Prepare new QAPISchemaMember base class
qapi: Track enum values by QAPISchemaMember, not string
qapi: Populate info['name'] for each entity
qapi: Enforce (or whitelist) case conventions on qapi members
qapi: Move duplicate collision checks to schema check()
qapi: Detect base class loops
block/qapi.c | 4 +-
docs/qapi-code-gen.txt | 4 +-
include/hw/qdev-core.h | 2 +-
include/qapi/qmp/qbool.h | 1 +
include/qapi/qmp/qdict.h | 1 +
include/qapi/qmp/qfloat.h | 1 +
include/qapi/qmp/qint.h | 1 +
include/qapi/qmp/qlist.h | 1 +
include/qapi/qmp/qobject.h | 56 +++----
include/qapi/qmp/qstring.h | 1 +
include/qapi/visitor-impl.h | 8 +-
include/qapi/visitor.h | 19 ++-
include/qemu/typedefs.h | 1 +
qapi/opts-visitor.c | 2 +-
qapi/qapi-visit-core.c | 10 +-
qapi/qmp-input-visitor.c | 10 +-
qapi/string-input-visitor.c | 3 +-
qobject/Makefile.objs | 2 +-
qobject/qbool.c | 11 +-
qobject/qdict.c | 14 +-
qobject/qfloat.c | 11 +-
qobject/qint.c | 11 +-
qobject/qlist.c | 11 +-
qobject/qnull.c | 12 +-
qobject/qobject.c | 34 ++++
qobject/qstring.c | 11 +-
scripts/qapi-types.py | 47 ++----
scripts/qapi-visit.py | 31 +++-
scripts/qapi.py | 179 ++++++++++-----------
tests/Makefile | 6 +-
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/alternate-empty.out | 3 +-
tests/qapi-schema/args-member-case.err | 1 +
...union-bad-branch.exit => args-member-case.exit} | 0
tests/qapi-schema/args-member-case.json | 3 +
.../{union-bad-branch.out => args-member-case.out} | 0
tests/qapi-schema/base-cycle-direct.err | 1 +
tests/qapi-schema/base-cycle-direct.exit | 1 +
tests/qapi-schema/base-cycle-direct.json | 2 +
tests/qapi-schema/base-cycle-direct.out | 0
tests/qapi-schema/base-cycle-indirect.err | 1 +
tests/qapi-schema/base-cycle-indirect.exit | 1 +
tests/qapi-schema/base-cycle-indirect.json | 3 +
tests/qapi-schema/base-cycle-indirect.out | 0
tests/qapi-schema/comments.out | 2 +
tests/qapi-schema/empty.out | 2 +
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/enum-clash-member.json | 2 +-
tests/qapi-schema/enum-member-case.err | 1 +
tests/qapi-schema/enum-member-case.exit | 1 +
tests/qapi-schema/enum-member-case.json | 3 +
tests/qapi-schema/enum-member-case.out | 0
tests/qapi-schema/event-case.out | 2 +
tests/qapi-schema/flat-union-clash-member.err | 2 +-
tests/qapi-schema/flat-union-empty.out | 2 +
tests/qapi-schema/ident-with-escape.out | 2 +
tests/qapi-schema/include-relpath.out | 2 +
tests/qapi-schema/include-repetition.out | 2 +
tests/qapi-schema/include-simple.out | 2 +
tests/qapi-schema/indented-expr.out | 2 +
tests/qapi-schema/qapi-schema-test.out | 10 +-
tests/qapi-schema/struct-base-clash-deep.err | 2 +-
tests/qapi-schema/struct-base-clash.err | 2 +-
tests/qapi-schema/union-bad-branch.err | 1 -
tests/qapi-schema/union-bad-branch.json | 8 -
tests/qapi-schema/union-branch-case.err | 1 +
tests/qapi-schema/union-branch-case.exit | 1 +
tests/qapi-schema/union-branch-case.json | 3 +
tests/qapi-schema/union-branch-case.out | 0
tests/qapi-schema/union-clash-branches.err | 2 +-
tests/qapi-schema/union-clash-branches.json | 2 +-
tests/qapi-schema/union-clash-data.out | 2 +
tests/qapi-schema/union-empty.out | 2 +
tests/test-qmp-input-visitor.c | 29 ++--
tests/test-qmp-output-visitor.c | 4 +-
75 files changed, 315 insertions(+), 306 deletions(-)
create mode 100644 qobject/qobject.c
create mode 100644 tests/qapi-schema/args-member-case.err
rename tests/qapi-schema/{union-bad-branch.exit => args-member-case.exit} (100%)
create mode 100644 tests/qapi-schema/args-member-case.json
rename tests/qapi-schema/{union-bad-branch.out => args-member-case.out} (100%)
create mode 100644 tests/qapi-schema/base-cycle-direct.err
create mode 100644 tests/qapi-schema/base-cycle-direct.exit
create mode 100644 tests/qapi-schema/base-cycle-direct.json
create mode 100644 tests/qapi-schema/base-cycle-direct.out
create mode 100644 tests/qapi-schema/base-cycle-indirect.err
create mode 100644 tests/qapi-schema/base-cycle-indirect.exit
create mode 100644 tests/qapi-schema/base-cycle-indirect.json
create mode 100644 tests/qapi-schema/base-cycle-indirect.out
create mode 100644 tests/qapi-schema/enum-member-case.err
create mode 100644 tests/qapi-schema/enum-member-case.exit
create mode 100644 tests/qapi-schema/enum-member-case.json
create mode 100644 tests/qapi-schema/enum-member-case.out
delete mode 100644 tests/qapi-schema/union-bad-branch.err
delete mode 100644 tests/qapi-schema/union-bad-branch.json
create mode 100644 tests/qapi-schema/union-branch-case.err
create mode 100644 tests/qapi-schema/union-branch-case.exit
create mode 100644 tests/qapi-schema/union-branch-case.json
create mode 100644 tests/qapi-schema/union-branch-case.out
--
2.4.3
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-26 14:27 ` Markus Armbruster
2015-11-26 15:06 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 02/14] qobject: Rename qtype_code to QType Eric Blake
` (13 subsequent siblings)
14 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Luiz Capitulino
The QObject hierarchy is small enough, and unlikely to grow further
(since we only use it to map to JSON and already cover all JSON
types), that we can simplify things by not tracking a separate
vtable, but just inline the refcnt element of the vtable QType
directly into QObject, and track a separate array of destroy
functions. We can drop qnull_destroy_obj() in the process.
The remaining QObject subclasses must export their destructor.
This also has the nice benefit of moving the typename 'QType'
out of the way, so that the next patch can repurpose it for a
nicer name for 'qtype_code'.
The various objects are still the same size (so no change in cache
line pressure), but now have less indirection (although I didn't
bother benchmarking to see if there is a noticeable speedup, as
we don't have hard evidence that this was in a performance hotspot
in the first place).
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: retitle; private destructor table through new qobject_destroy()
v12: new patch, split off from 21/28
---
include/qapi/qmp/qbool.h | 1 +
include/qapi/qmp/qdict.h | 1 +
include/qapi/qmp/qfloat.h | 1 +
include/qapi/qmp/qint.h | 1 +
include/qapi/qmp/qlist.h | 1 +
include/qapi/qmp/qobject.h | 37 +++++++++++++++++++++----------------
include/qapi/qmp/qstring.h | 1 +
qobject/Makefile.objs | 2 +-
qobject/qbool.c | 11 ++---------
qobject/qdict.c | 11 ++---------
qobject/qfloat.c | 11 ++---------
qobject/qint.c | 11 ++---------
qobject/qlist.c | 11 ++---------
qobject/qnull.c | 12 +-----------
qobject/qobject.c | 34 ++++++++++++++++++++++++++++++++++
qobject/qstring.c | 11 ++---------
16 files changed, 75 insertions(+), 82 deletions(-)
create mode 100644 qobject/qobject.c
diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index d9256e4..836d078 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -25,5 +25,6 @@ typedef struct QBool {
QBool *qbool_from_bool(bool value);
bool qbool_get_bool(const QBool *qb);
QBool *qobject_to_qbool(const QObject *obj);
+void qbool_destroy_obj(QObject *obj);
#endif /* QBOOL_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 787c658..6c2a0e5 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -48,6 +48,7 @@ void qdict_iter(const QDict *qdict,
void *opaque);
const QDictEntry *qdict_first(const QDict *qdict);
const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
+void qdict_destroy_obj(QObject *obj);
/* Helper to qdict_put_obj(), accepts any object */
#define qdict_put(qdict, key, obj) \
diff --git a/include/qapi/qmp/qfloat.h b/include/qapi/qmp/qfloat.h
index 46745e5..a8af2a8 100644
--- a/include/qapi/qmp/qfloat.h
+++ b/include/qapi/qmp/qfloat.h
@@ -25,5 +25,6 @@ typedef struct QFloat {
QFloat *qfloat_from_double(double value);
double qfloat_get_double(const QFloat *qi);
QFloat *qobject_to_qfloat(const QObject *obj);
+void qfloat_destroy_obj(QObject *obj);
#endif /* QFLOAT_H */
diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
index 339a9ab..049e528 100644
--- a/include/qapi/qmp/qint.h
+++ b/include/qapi/qmp/qint.h
@@ -24,5 +24,6 @@ typedef struct QInt {
QInt *qint_from_int(int64_t value);
int64_t qint_get_int(const QInt *qi);
QInt *qobject_to_qint(const QObject *obj);
+void qint_destroy_obj(QObject *obj);
#endif /* QINT_H */
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index b1bf785..a84117e 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -49,6 +49,7 @@ QObject *qlist_peek(QList *qlist);
int qlist_empty(const QList *qlist);
size_t qlist_size(const QList *qlist);
QList *qobject_to_qlist(const QObject *obj);
+void qlist_destroy_obj(QObject *obj);
static inline const QListEntry *qlist_first(const QList *qlist)
{
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 4b96ed5..7e5b9b1 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -47,21 +47,20 @@ typedef enum {
QTYPE_MAX,
} qtype_code;
-struct QObject;
-
-typedef struct QType {
- qtype_code code;
- void (*destroy)(struct QObject *);
-} QType;
-
typedef struct QObject {
- const QType *type;
+ qtype_code type;
size_t refcnt;
} QObject;
+typedef void (*QDestroy)(QObject *);
+
/* Get the 'base' part of an object */
#define QOBJECT(obj) (&(obj)->base)
+/* High-level interface for qobject_init() */
+#define QOBJECT_INIT(obj, type) \
+ qobject_init(QOBJECT(obj), type)
+
/* High-level interface for qobject_incref() */
#define QINCREF(obj) \
qobject_incref(QOBJECT(obj))
@@ -71,9 +70,12 @@ typedef struct QObject {
qobject_decref(obj ? QOBJECT(obj) : NULL)
/* Initialize an object to default values */
-#define QOBJECT_INIT(obj, qtype_type) \
- obj->base.refcnt = 1; \
- obj->base.type = qtype_type
+static inline void qobject_init(QObject *obj, qtype_code type)
+{
+ assert(type);
+ obj->refcnt = 1;
+ obj->type = type;
+}
/**
* qobject_incref(): Increment QObject's reference count
@@ -85,6 +87,11 @@ static inline void qobject_incref(QObject *obj)
}
/**
+ * qobject_destroy(): Free resources used by the object
+ */
+void qobject_destroy(QObject *obj);
+
+/**
* qobject_decref(): Decrement QObject's reference count, deallocate
* when it reaches zero
*/
@@ -92,9 +99,7 @@ static inline void qobject_decref(QObject *obj)
{
assert(!obj || obj->refcnt);
if (obj && --obj->refcnt == 0) {
- assert(obj->type != NULL);
- assert(obj->type->destroy != NULL);
- obj->type->destroy(obj);
+ qobject_destroy(obj);
}
}
@@ -103,8 +108,8 @@ static inline void qobject_decref(QObject *obj)
*/
static inline qtype_code qobject_type(const QObject *obj)
{
- assert(obj->type != NULL);
- return obj->type->code;
+ assert(obj->type);
+ return obj->type;
}
extern QObject qnull_;
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 34675a7..df7df55 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -32,5 +32,6 @@ void qstring_append_int(QString *qstring, int64_t value);
void qstring_append(QString *qstring, const char *str);
void qstring_append_chr(QString *qstring, int c);
QString *qobject_to_qstring(const QObject *obj);
+void qstring_destroy_obj(QObject *obj);
#endif /* QSTRING_H */
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index 0031e8b..bed5508 100644
--- a/qobject/Makefile.objs
+++ b/qobject/Makefile.objs
@@ -1,2 +1,2 @@
util-obj-y = qnull.o qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
-util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
+util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
diff --git a/qobject/qbool.c b/qobject/qbool.c
index bc6535f..895fd4d 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -15,13 +15,6 @@
#include "qapi/qmp/qobject.h"
#include "qemu-common.h"
-static void qbool_destroy_obj(QObject *obj);
-
-static const QType qbool_type = {
- .code = QTYPE_QBOOL,
- .destroy = qbool_destroy_obj,
-};
-
/**
* qbool_from_bool(): Create a new QBool from a bool
*
@@ -33,7 +26,7 @@ QBool *qbool_from_bool(bool value)
qb = g_malloc(sizeof(*qb));
qb->value = value;
- QOBJECT_INIT(qb, &qbool_type);
+ QOBJECT_INIT(qb, QTYPE_QBOOL);
return qb;
}
@@ -61,7 +54,7 @@ QBool *qobject_to_qbool(const QObject *obj)
* qbool_destroy_obj(): Free all memory allocated by a
* QBool object
*/
-static void qbool_destroy_obj(QObject *obj)
+void qbool_destroy_obj(QObject *obj)
{
assert(obj != NULL);
g_free(qobject_to_qbool(obj));
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 2d67bf1..dd3f1c2 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -19,13 +19,6 @@
#include "qemu/queue.h"
#include "qemu-common.h"
-static void qdict_destroy_obj(QObject *obj);
-
-static const QType qdict_type = {
- .code = QTYPE_QDICT,
- .destroy = qdict_destroy_obj,
-};
-
/**
* qdict_new(): Create a new QDict
*
@@ -36,7 +29,7 @@ QDict *qdict_new(void)
QDict *qdict;
qdict = g_malloc0(sizeof(*qdict));
- QOBJECT_INIT(qdict, &qdict_type);
+ QOBJECT_INIT(qdict, QTYPE_QDICT);
return qdict;
}
@@ -441,7 +434,7 @@ void qdict_del(QDict *qdict, const char *key)
/**
* qdict_destroy_obj(): Free all the memory allocated by a QDict
*/
-static void qdict_destroy_obj(QObject *obj)
+void qdict_destroy_obj(QObject *obj)
{
int i;
QDict *qdict;
diff --git a/qobject/qfloat.c b/qobject/qfloat.c
index c865163..1dffb54 100644
--- a/qobject/qfloat.c
+++ b/qobject/qfloat.c
@@ -15,13 +15,6 @@
#include "qapi/qmp/qobject.h"
#include "qemu-common.h"
-static void qfloat_destroy_obj(QObject *obj);
-
-static const QType qfloat_type = {
- .code = QTYPE_QFLOAT,
- .destroy = qfloat_destroy_obj,
-};
-
/**
* qfloat_from_int(): Create a new QFloat from a float
*
@@ -33,7 +26,7 @@ QFloat *qfloat_from_double(double value)
qf = g_malloc(sizeof(*qf));
qf->value = value;
- QOBJECT_INIT(qf, &qfloat_type);
+ QOBJECT_INIT(qf, QTYPE_QFLOAT);
return qf;
}
@@ -61,7 +54,7 @@ QFloat *qobject_to_qfloat(const QObject *obj)
* qfloat_destroy_obj(): Free all memory allocated by a
* QFloat object
*/
-static void qfloat_destroy_obj(QObject *obj)
+void qfloat_destroy_obj(QObject *obj)
{
assert(obj != NULL);
g_free(qobject_to_qfloat(obj));
diff --git a/qobject/qint.c b/qobject/qint.c
index 999688e..112ee68 100644
--- a/qobject/qint.c
+++ b/qobject/qint.c
@@ -14,13 +14,6 @@
#include "qapi/qmp/qobject.h"
#include "qemu-common.h"
-static void qint_destroy_obj(QObject *obj);
-
-static const QType qint_type = {
- .code = QTYPE_QINT,
- .destroy = qint_destroy_obj,
-};
-
/**
* qint_from_int(): Create a new QInt from an int64_t
*
@@ -32,7 +25,7 @@ QInt *qint_from_int(int64_t value)
qi = g_malloc(sizeof(*qi));
qi->value = value;
- QOBJECT_INIT(qi, &qint_type);
+ QOBJECT_INIT(qi, QTYPE_QINT);
return qi;
}
@@ -60,7 +53,7 @@ QInt *qobject_to_qint(const QObject *obj)
* qint_destroy_obj(): Free all memory allocated by a
* QInt object
*/
-static void qint_destroy_obj(QObject *obj)
+void qint_destroy_obj(QObject *obj)
{
assert(obj != NULL);
g_free(qobject_to_qint(obj));
diff --git a/qobject/qlist.c b/qobject/qlist.c
index 298003a..28be4f6 100644
--- a/qobject/qlist.c
+++ b/qobject/qlist.c
@@ -15,13 +15,6 @@
#include "qemu/queue.h"
#include "qemu-common.h"
-static void qlist_destroy_obj(QObject *obj);
-
-static const QType qlist_type = {
- .code = QTYPE_QLIST,
- .destroy = qlist_destroy_obj,
-};
-
/**
* qlist_new(): Create a new QList
*
@@ -33,7 +26,7 @@ QList *qlist_new(void)
qlist = g_malloc(sizeof(*qlist));
QTAILQ_INIT(&qlist->head);
- QOBJECT_INIT(qlist, &qlist_type);
+ QOBJECT_INIT(qlist, QTYPE_QLIST);
return qlist;
}
@@ -151,7 +144,7 @@ QList *qobject_to_qlist(const QObject *obj)
/**
* qlist_destroy_obj(): Free all the memory allocated by a QList
*/
-static void qlist_destroy_obj(QObject *obj)
+void qlist_destroy_obj(QObject *obj)
{
QList *qlist;
QListEntry *entry, *next_entry;
diff --git a/qobject/qnull.c b/qobject/qnull.c
index 9873e26..5f7ba4d 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -13,17 +13,7 @@
#include "qemu-common.h"
#include "qapi/qmp/qobject.h"
-static void qnull_destroy_obj(QObject *obj)
-{
- assert(0);
-}
-
-static const QType qnull_type = {
- .code = QTYPE_QNULL,
- .destroy = qnull_destroy_obj,
-};
-
QObject qnull_ = {
- .type = &qnull_type,
+ .type = QTYPE_QNULL,
.refcnt = 1,
};
diff --git a/qobject/qobject.c b/qobject/qobject.c
new file mode 100644
index 0000000..5325447
--- /dev/null
+++ b/qobject/qobject.c
@@ -0,0 +1,34 @@
+/*
+ * QObject
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later. See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu-common.h"
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qfloat.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qstring.h"
+
+static QDestroy qdestroy[QTYPE_MAX] = {
+ [QTYPE_NONE] = NULL, /* No such object exists */
+ [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
+ [QTYPE_QINT] = qint_destroy_obj,
+ [QTYPE_QSTRING] = qstring_destroy_obj,
+ [QTYPE_QDICT] = qdict_destroy_obj,
+ [QTYPE_QLIST] = qlist_destroy_obj,
+ [QTYPE_QFLOAT] = qfloat_destroy_obj,
+ [QTYPE_QBOOL] = qbool_destroy_obj,
+};
+
+void qobject_destroy(QObject *obj)
+{
+ assert(!obj->refcnt);
+ assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
+ qdestroy[obj->type](obj);
+}
diff --git a/qobject/qstring.c b/qobject/qstring.c
index cb72dfb..a2f5903 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -14,13 +14,6 @@
#include "qapi/qmp/qstring.h"
#include "qemu-common.h"
-static void qstring_destroy_obj(QObject *obj);
-
-static const QType qstring_type = {
- .code = QTYPE_QSTRING,
- .destroy = qstring_destroy_obj,
-};
-
/**
* qstring_new(): Create a new empty QString
*
@@ -57,7 +50,7 @@ QString *qstring_from_substr(const char *str, int start, int end)
memcpy(qstring->string, str + start, qstring->length);
qstring->string[qstring->length] = 0;
- QOBJECT_INIT(qstring, &qstring_type);
+ QOBJECT_INIT(qstring, QTYPE_QSTRING);
return qstring;
}
@@ -138,7 +131,7 @@ const char *qstring_get_str(const QString *qstring)
* qstring_destroy_obj(): Free all memory allocated by a QString
* object
*/
-static void qstring_destroy_obj(QObject *obj)
+void qstring_destroy_obj(QObject *obj)
{
QString *qs;
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 02/14] qobject: Rename qtype_code to QType
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type Eric Blake
` (12 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Michael Roth, armbru, open list:Block layer core,
Luiz Capitulino
The name QType matches our CODING_STYLE conventions for type names
in CamelCase. It also matches the fact that we are already naming
all the enum members with a prefix of QTYPE, not QTYPE_CODE. And
doing the rename will also make it easier for the next patch to use
QAPI for providing the enum, which also wants CamelCase type names.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: commit message touchup, rebase to cleaner QObject
v12: new patch split off of 21/28
---
block/qapi.c | 4 ++--
include/hw/qdev-core.h | 2 +-
include/qapi/qmp/qobject.h | 8 ++++----
qobject/qdict.c | 3 +--
scripts/qapi.py | 2 +-
5 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index d20262d..4eb48fc 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -590,7 +590,7 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
int i = 0;
for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
- qtype_code type = qobject_type(entry->value);
+ QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: ";
@@ -608,7 +608,7 @@ static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
const QDictEntry *entry;
for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
- qtype_code type = qobject_type(entry->value);
+ QType type = qobject_type(entry->value);
bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
char key[strlen(entry->key) + 1];
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index c537969..abcdee8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -239,7 +239,7 @@ struct Property {
PropertyInfo *info;
ptrdiff_t offset;
uint8_t bitnr;
- qtype_code qtype;
+ QType qtype;
int64_t defval;
int arrayoffset;
PropertyInfo *arrayinfo;
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 7e5b9b1..525fb39 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,10 +45,10 @@ typedef enum {
QTYPE_QFLOAT,
QTYPE_QBOOL,
QTYPE_MAX,
-} qtype_code;
+} QType;
typedef struct QObject {
- qtype_code type;
+ QType type;
size_t refcnt;
} QObject;
@@ -70,7 +70,7 @@ typedef void (*QDestroy)(QObject *);
qobject_decref(obj ? QOBJECT(obj) : NULL)
/* Initialize an object to default values */
-static inline void qobject_init(QObject *obj, qtype_code type)
+static inline void qobject_init(QObject *obj, QType type)
{
assert(type);
obj->refcnt = 1;
@@ -106,7 +106,7 @@ static inline void qobject_decref(QObject *obj)
/**
* qobject_type(): Return the QObject's type
*/
-static inline qtype_code qobject_type(const QObject *obj)
+static inline QType qobject_type(const QObject *obj)
{
assert(obj->type);
return obj->type;
diff --git a/qobject/qdict.c b/qobject/qdict.c
index dd3f1c2..d087410 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -177,8 +177,7 @@ size_t qdict_size(const QDict *qdict)
/**
* qdict_get_obj(): Get a QObject of a specific type
*/
-static QObject *qdict_get_obj(const QDict *qdict, const char *key,
- qtype_code type)
+static QObject *qdict_get_obj(const QDict *qdict, const char *key, QType type)
{
QObject *obj;
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 72925c3..9fe9c95 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -33,7 +33,7 @@ builtin_types = {
'uint32': 'QTYPE_QINT',
'uint64': 'QTYPE_QINT',
'size': 'QTYPE_QINT',
- 'any': None, # any qtype_code possible, actually
+ 'any': None, # any QType possible, actually
}
# Whitelist of commands allowed to return a non-dictionary
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 02/14] qobject: Rename qtype_code to QType Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-26 14:51 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 04/14] qapi: Simplify visiting of alternate types Eric Blake
` (11 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: Luiz Capitulino, armbru, Michael Roth
What's more meta than using qapi to define qapi? :)
Convert QType into a full-fledged[*] builtin qapi enum type, so
that a subsequent patch can then use it as the discriminator
type of qapi alternate types. Fortunately, the judicious use of
'prefix' in the qapi definition avoids churn to the spelling of
the enum constants.
To avoid circular definitions, we have to flip the order of
inclusion between "qobject.h" vs. "qapi-types.h". Back in commit
28770e0, we had the latter include the former, so that we could
use 'QObject *' for our implementation of 'any'. But that usage
also works with only a forward declaration, whereas the
definition of QObject requires QType to be a complete type.
[*] The type has to be builtin, rather than declared in
qapi/common.json, because we want to use it for alternates even
when common.json is not included. But since it is the first
builtin enum type, we have to add special cases to qapi-types
and qapi-visit to only emit definitions once, even when two
qapi files are being compiled into the same binary (the way we
already handled builtin list types like 'intList'). We may
need to revisit how multiple qapi files share common types,
but that's a project for another day.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: change title, use typedefs.h, rebase to cleaner QObject,
rebase to Markus' typedefs.h re-sort
v12: split out preparatory renames, retitle patch, use info rather
than name comparison
v11: new patch
---
docs/qapi-code-gen.txt | 1 +
include/qapi/qmp/qobject.h | 17 +++--------------
include/qemu/typedefs.h | 1 +
qobject/qobject.c | 4 ++--
scripts/qapi-types.py | 15 +++++++++++----
scripts/qapi-visit.py | 11 +++++++++--
scripts/qapi.py | 6 ++++++
tests/qapi-schema/alternate-empty.out | 2 ++
| 2 ++
tests/qapi-schema/empty.out | 2 ++
tests/qapi-schema/event-case.out | 2 ++
tests/qapi-schema/flat-union-empty.out | 2 ++
tests/qapi-schema/ident-with-escape.out | 2 ++
tests/qapi-schema/include-relpath.out | 2 ++
tests/qapi-schema/include-repetition.out | 2 ++
tests/qapi-schema/include-simple.out | 2 ++
tests/qapi-schema/indented-expr.out | 2 ++
tests/qapi-schema/qapi-schema-test.out | 2 ++
tests/qapi-schema/union-clash-data.out | 2 ++
tests/qapi-schema/union-empty.out | 2 ++
20 files changed, 59 insertions(+), 22 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 2becba9..79bf072 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -160,6 +160,7 @@ The following types are predefined, and map to C as follows:
accepts size suffixes
bool bool JSON true or false
any QObject * any JSON value
+ QType QType JSON string matching enum QType values
=== Includes ===
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 525fb39..295aa76 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -34,23 +34,12 @@
#include <stddef.h>
#include <assert.h>
+#include "qapi-types.h"
-typedef enum {
- QTYPE_NONE, /* sentinel value, no QObject has this type code */
- QTYPE_QNULL,
- QTYPE_QINT,
- QTYPE_QSTRING,
- QTYPE_QDICT,
- QTYPE_QLIST,
- QTYPE_QFLOAT,
- QTYPE_QBOOL,
- QTYPE_MAX,
-} QType;
-
-typedef struct QObject {
+struct QObject {
QType type;
size_t refcnt;
-} QObject;
+};
typedef void (*QDestroy)(QObject *);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 3eedcf4..78fe6e8 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -80,6 +80,7 @@ typedef struct QEMUSGList QEMUSGList;
typedef struct QEMUSizedBuffer QEMUSizedBuffer;
typedef struct QEMUTimer QEMUTimer;
typedef struct QEMUTimerListGroup QEMUTimerListGroup;
+typedef struct QObject QObject;
typedef struct RAMBlock RAMBlock;
typedef struct Range Range;
typedef struct SerialState SerialState;
diff --git a/qobject/qobject.c b/qobject/qobject.c
index 5325447..d0eb7f1 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -15,7 +15,7 @@
#include "qapi/qmp/qlist.h"
#include "qapi/qmp/qstring.h"
-static QDestroy qdestroy[QTYPE_MAX] = {
+static QDestroy qdestroy[QTYPE__MAX] = {
[QTYPE_NONE] = NULL, /* No such object exists */
[QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
[QTYPE_QINT] = qint_destroy_obj,
@@ -29,6 +29,6 @@ static QDestroy qdestroy[QTYPE_MAX] = {
void qobject_destroy(QObject *obj)
{
assert(!obj->refcnt);
- assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
+ assert(QTYPE_QNULL < obj->type && obj->type < QTYPE__MAX);
qdestroy[obj->type](obj);
}
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2f2f7df..82635b0 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -112,7 +112,7 @@ extern const int %(c_name)s_qtypes[];
def gen_alternate_qtypes(name, variants):
ret = mcgen('''
-const int %(c_name)s_qtypes[QTYPE_MAX] = {
+const int %(c_name)s_qtypes[QTYPE__MAX] = {
''',
c_name=c_name(name))
@@ -233,8 +233,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.defn += gen_type_cleanup(name)
def visit_enum_type(self, name, info, values, prefix):
- self._fwdecl += gen_enum(name, values, prefix)
- self._fwdefn += gen_enum_lookup(name, values, prefix)
+ # Special case for our lone builtin enum type
+ # TODO use something cleaner than existence of info
+ if not info:
+ self._btin += gen_enum(name, values, prefix)
+ if do_builtins:
+ self.defn += gen_enum_lookup(name, values, prefix)
+ else:
+ self._fwdecl += gen_enum(name, values, prefix)
+ self._fwdefn += gen_enum_lookup(name, values, prefix)
def visit_array_type(self, name, info, element_type):
if isinstance(element_type, QAPISchemaBuiltinType):
@@ -319,7 +326,7 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include <stdbool.h>
#include <stdint.h>
-#include "qapi/qmp/qobject.h"
+#include "qemu/typedefs.h"
'''))
schema = QAPISchema(input_file)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 94cd113..7ceda18 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -347,8 +347,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
isinstance(entity, QAPISchemaObjectType))
def visit_enum_type(self, name, info, values, prefix):
- self.decl += gen_visit_decl(name, scalar=True)
- self.defn += gen_visit_enum(name)
+ # Special case for our lone builtin enum type
+ # TODO use something cleaner than existence of info
+ if not info:
+ self._btin += gen_visit_decl(name, scalar=True)
+ if do_builtins:
+ self.defn += gen_visit_enum(name)
+ else:
+ self.decl += gen_visit_decl(name, scalar=True)
+ self.defn += gen_visit_enum(name)
def visit_array_type(self, name, info, element_type):
decl = gen_visit_decl(name)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 9fe9c95..3c8cb13 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -34,6 +34,7 @@ builtin_types = {
'uint64': 'QTYPE_QINT',
'size': 'QTYPE_QINT',
'any': None, # any QType possible, actually
+ 'QType': 'QTYPE_QSTRING',
}
# Whitelist of commands allowed to return a non-dictionary
@@ -1243,6 +1244,11 @@ class QAPISchema(object):
self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
[], None)
self._def_entity(self.the_empty_object_type)
+ self._def_entity(QAPISchemaEnumType('QType', None,
+ ['none', 'qnull', 'qint',
+ 'qstring', 'qdict', 'qlist',
+ 'qfloat', 'qbool'],
+ 'QTYPE'))
def _make_implicit_enum_type(self, name, info, values):
name = name + 'Kind' # Use namespace reserved by add_name()
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
index 0f153b6..02b9876 100644
--- a/tests/qapi-schema/alternate-empty.out
+++ b/tests/qapi-schema/alternate-empty.out
@@ -2,3 +2,5 @@ object :empty
alternate Alt
case i: int
enum AltKind ['i']
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
--git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 9e2c656..97be601 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,2 +1,4 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
enum Status ['good', 'bad', 'ugly']
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 272b161..6522940 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1 +1,3 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index cdfd264..6350d64 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,2 +1,4 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
event oops None
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
index 0e0665a..eade2d5 100644
--- a/tests/qapi-schema/flat-union-empty.out
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -2,6 +2,8 @@ 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/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index f4542b1..453e0b2 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -1,5 +1,7 @@
object :empty
object :obj-fooA-arg
member bar1: str optional=False
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
command fooA :obj-fooA-arg -> None
gen=True success_response=True
diff --git a/tests/qapi-schema/include-relpath.out b/tests/qapi-schema/include-relpath.out
index 9e2c656..97be601 100644
--- a/tests/qapi-schema/include-relpath.out
+++ b/tests/qapi-schema/include-relpath.out
@@ -1,2 +1,4 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
enum Status ['good', 'bad', 'ugly']
diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out
index 9e2c656..97be601 100644
--- a/tests/qapi-schema/include-repetition.out
+++ b/tests/qapi-schema/include-repetition.out
@@ -1,2 +1,4 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
enum Status ['good', 'bad', 'ugly']
diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out
index 9e2c656..97be601 100644
--- a/tests/qapi-schema/include-simple.out
+++ b/tests/qapi-schema/include-simple.out
@@ -1,2 +1,4 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
enum Status ['good', 'bad', 'ugly']
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 226d300..ce37ff5 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,4 +1,6 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
command eins None -> None
gen=True success_response=True
command zwei None -> None
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 0724a9f..b87cfba 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -101,6 +101,8 @@ object NestedEnumsOne
member enum4: EnumOne optional=True
enum QEnumTwo ['value1', 'value2']
prefix QENUM_TWO
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
object TestStruct
member integer: int optional=False
member boolean: bool optional=False
diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
index cea8551..f5752f4 100644
--- a/tests/qapi-schema/union-clash-data.out
+++ b/tests/qapi-schema/union-clash-data.out
@@ -1,6 +1,8 @@
object :empty
object :obj-int-wrapper
member data: int optional=False
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
object TestUnion
member type: TestUnionKind optional=False
case data: :obj-int-wrapper
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
index 9c89fd1..bdf17e5 100644
--- a/tests/qapi-schema/union-empty.out
+++ b/tests/qapi-schema/union-empty.out
@@ -1,4 +1,6 @@
object :empty
+enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
+ prefix QTYPE
object Union
member type: UnionKind optional=False
enum UnionKind []
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 04/14] qapi: Simplify visiting of alternate types
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (2 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 05/14] qapi: Inline _make_implicit_tag() Eric Blake
` (10 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Previously, working with alternates required two lookup arrays
and some indirection: for type Foo, we created Foo_qtypes[]
which maps each qtype to a value of the generated FooKind enum,
then look up that value in FooKind_lookup[] like we do for other
union types.
This has a couple of subtle bugs. First, the generator was
creating a call with a parameter '(int *) &(*obj)->type' where
type is an enum type; this is unsafe if the compiler chooses
to store the enum type in a different size than int, where
assigning through the wrong size pointer can corrupt data or
cause a SIGBUS.
Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.
Second, since the values of the FooKind enum start at zero, all
entries of the Foo_qtypes[] array that were not explicitly
initialized will map to the same branch of the union as the
first member of the alternate, rather than triggering a desired
failure in visit_get_next_type(). Fortunately, the bug seldom
bites; the very next thing the input visitor does is try to
parse the incoming JSON with the wrong parser, which normally
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).
However, the second bug IS observable in one case: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it). A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.
This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode parameter. This in turn fixes the type-casting bug,
as we are no longer casting a pointer to enum to a questionable
size. There is no longer a need to generate an implicit FooKind
enum associated with the alternate type (since the QMP wire
format never uses the stringized counterparts of the C union
member names). Since the updated visit_get_next_type() does not
know which qtypes are expected, the generated visitor is
modified to generate an error statement if an unexpected type is
encountered.
Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types. I considered the possibility of
keeping the internal enum FooKind, but initialized differently
than most generated arrays, as in:
typedef enum FooKind {
FOO_KIND_A = QTYPE_QDICT,
FOO_KIND_B = QTYPE_QINT,
} FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->type; but it turned out to add too much
complexity, especially without a client.
There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
{"execute":"blockdev-add", "arguments":{"options":
{"driver":"raw", "id":"a", "file":true}}}
failed with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: QDict"}}
(visit_get_next_type() succeeded, and the error comes from the
visit_type_BlockdevOptions() expecting {}; there is no mention of
the fact that a string would also work). Now it fails with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
(the error when the next type doesn't match any expected types for
the overall alternate).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: touch up commit message and add FIXME
v12: rebase to earlier 'max' collision avoidance, some variable renames
v11 (no v10): rebase to new QTypeCode, with fewer special cases; tweak
commit message to match
v9: rebase to earlier changes, rework commit message to mention second
bug fix; move positive test in qapi-schema-test to later patch
v8: no change
v7: rebase onto earlier changes, rework how subtype makes things work
v6: rebase onto tag_member subclass, testsuite, gen_err_check(),
and info improvements
---
docs/qapi-code-gen.txt | 3 ---
include/qapi/visitor-impl.h | 3 ++-
include/qapi/visitor.h | 8 +++++++-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 4 ++--
scripts/qapi-types.py | 34 ----------------------------------
scripts/qapi-visit.py | 15 ++++++++++-----
scripts/qapi.py | 18 +++++++++++++-----
tests/qapi-schema/alternate-empty.out | 1 -
tests/qapi-schema/qapi-schema-test.out | 8 --------
tests/test-qmp-input-visitor.c | 31 ++++++++++++++++---------------
tests/test-qmp-output-visitor.c | 4 ++--
12 files changed, 54 insertions(+), 79 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 79bf072..128f074 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -384,9 +384,6 @@ where each branch of the union names a QAPI type. For example:
'data': { 'definition': 'BlockdevOptions',
'reference': 'str' } }
-Just like for a simple union, an implicit C enum 'NameKind' is created
-to enumerate the branches for the alternate 'Name'.
-
Unlike a union, the discriminator string is never passed on the wire
for the Client JSON Protocol. Instead, the value's JSON type serves
as an implicit discriminator, which in turn means that an alternate
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8c0ba57..7cd1313 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -32,7 +32,8 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
- void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
+ /* May be NULL; only needed for input visitors. */
+ void (*get_next_type)(Visitor *v, QType *type,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a2ad66c..6d25ad2 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -38,7 +38,13 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp);
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+
+/**
+ * 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.
+ */
+void visit_get_next_type(Visitor *v, QType *type,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 59ed506..850ca03 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
}
}
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+void visit_get_next_type(Visitor *v, QType *type,
const char *name, Error **errp)
{
if (v->get_next_type) {
- v->get_next_type(v, obj, qtypes, name, errp);
+ v->get_next_type(v, type, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index eb6e110..d398de7 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
qmp_input_pop(qiv, errp);
}
-static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
+static void qmp_input_get_next_type(Visitor *v, QType *type,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -218,7 +218,7 @@ static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
return;
}
- *kind = qobjects[qobject_type(qobj)];
+ *type = qobject_type(qobj);
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 82635b0..d0e1f74 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -101,38 +101,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
c_name=c_name(name), base=base.c_name())
-def gen_alternate_qtypes_decl(name):
- return mcgen('''
-
-extern const int %(c_name)s_qtypes[];
-''',
- c_name=c_name(name))
-
-
-def gen_alternate_qtypes(name, variants):
- ret = mcgen('''
-
-const int %(c_name)s_qtypes[QTYPE__MAX] = {
-''',
- c_name=c_name(name))
-
- for var in variants.variants:
- qtype = var.type.alternate_qtype()
- assert qtype
-
- ret += mcgen('''
- [%(qtype)s] = %(enum_const)s,
-''',
- qtype=qtype,
- enum_const=c_enum_const(variants.tag_member.type.name,
- var.name))
-
- ret += mcgen('''
-};
-''')
- return ret
-
-
def gen_variants(variants):
# 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
@@ -264,9 +232,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_alternate_type(self, name, info, variants):
self._fwdecl += gen_fwd_object_or_array(name)
- self._fwdefn += gen_alternate_qtypes(name, variants)
self.decl += gen_object(name, None, [variants.tag_member], variants)
- self.decl += gen_alternate_qtypes_decl(name)
self._gen_type_cleanup(name)
# If you link code generated from multiple schemata, you want only one
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 7ceda18..e254ca9 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,6 +172,7 @@ out:
def gen_visit_enum(name):
+ # FIXME cast from enum *obj to int * invalidely assumes enum is int
return mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
@@ -193,7 +194,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
+ visit_get_next_type(v, &(*obj)->type, name, &err);
if (err) {
goto out_obj;
}
@@ -201,20 +202,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
''',
c_name=c_name(name))
+ # FIXME: When 'number' but not 'int' is present in the alternate, we
+ # should allow QTYPE_INT to promote to QTYPE_FLOAT.
for var in variants.variants:
ret += mcgen('''
case %(case)s:
visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
break;
''',
- case=c_enum_const(variants.tag_member.type.name,
- var.name),
+ case=var.type.alternate_qtype(),
c_type=var.type.c_name(),
c_name=c_name(var.name))
ret += mcgen('''
default:
- abort();
+ error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+ "%(name)s");
}
out_obj:
error_propagate(errp, err);
@@ -223,7 +226,8 @@ out_obj:
out:
error_propagate(errp, err);
}
-''')
+''',
+ name=name)
return ret
@@ -437,6 +441,7 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
#include "%(prefix)sqapi-types.h"
''',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3c8cb13..44b6126 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -635,8 +635,8 @@ def check_alternate(expr, expr_info):
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
- # Check for conflicts in the generated enum
- c_key = camel_to_upper(key)
+ # Check for conflicts in the branch names
+ c_key = c_name(key)
if c_key in values:
raise QAPIExprError(expr_info,
"Alternate '%s' member '%s' clashes with '%s'"
@@ -1091,8 +1091,11 @@ class QAPISchemaObjectTypeVariants(object):
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
v.check(schema)
- assert v.name in self.tag_member.type.values
- if isinstance(v.type, QAPISchemaObjectType):
+ # Union names must match enum values; alternate names are
+ # checked separately. Use 'seen' to tell the two apart.
+ if seen:
+ assert v.name in self.tag_member.type.values
+ assert isinstance(v.type, QAPISchemaObjectType)
v.type.check(schema)
def check_clash(self, schema, info, seen):
@@ -1134,6 +1137,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
# Not calling self.variants.check_clash(), because there's nothing
# to clash with
self.variants.check(schema, {})
+ # Alternate branch names have no relation to the tag enum values;
+ # so we have to check for potential name collisions ourselves.
+ seen = {}
+ for v in self.variants.variants:
+ v.check_clash(self.info, seen)
def json_type(self):
return 'value'
@@ -1341,7 +1349,7 @@ class QAPISchema(object):
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
- tag_member = self._make_implicit_tag(name, info, variants)
+ tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
self._def_entity(
QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
index 02b9876..f78f174 100644
--- a/tests/qapi-schema/alternate-empty.out
+++ b/tests/qapi-schema/alternate-empty.out
@@ -1,6 +1,5 @@
object :empty
alternate Alt
case i: int
-enum AltKind ['i']
enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
prefix QTYPE
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index b87cfba..2c546b7 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -56,27 +56,21 @@ object :obj-user_def_cmd2-arg
alternate AltIntNum
case i: int
case n: number
-enum AltIntNumKind ['i', 'n']
alternate AltNumInt
case n: number
case i: int
-enum AltNumIntKind ['n', 'i']
alternate AltNumStr
case n: number
case s: str
-enum AltNumStrKind ['n', 's']
alternate AltStrBool
case s: str
case b: bool
-enum AltStrBoolKind ['s', 'b']
alternate AltStrInt
case s: str
case i: int
-enum AltStrIntKind ['s', 'i']
alternate AltStrNum
case s: str
case n: number
-enum AltStrNumKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
@@ -114,7 +108,6 @@ alternate UserDefAlternate
case uda: UserDefA
case s: str
case i: int
-enum UserDefAlternateKind ['uda', 's', 'i']
object UserDefB
member intb: int optional=False
member a-b: bool optional=True
@@ -180,7 +173,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str
case b: __org.qemu_x-Base
-enum __org.qemu_x-AltKind ['__org.qemu_x-branch', 'b']
object __org.qemu_x-Base
member __org.qemu_x-member1: __org.qemu_x-Enum optional=False
enum __org.qemu_x-Enum ['__org.qemu_x-value']
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index d48ebdd..43b9e18 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -312,13 +312,13 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QINT);
g_assert_cmpint(tmp->u.i, ==, 42);
qapi_free_UserDefAlternate(tmp);
v = visitor_input_test_init(data, "'string'");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QSTRING);
g_assert_cmpstr(tmp->u.s, ==, "string");
qapi_free_UserDefAlternate(tmp);
@@ -347,36 +347,37 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free_or_abort(&err);
qapi_free_AltStrBool(asb);
- /* FIXME: Order of alternate should not affect semantics; asn should
- * parse the same as ans */
+ /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
visit_type_AltStrNum(v, &asn, NULL, &err);
- /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
+ /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */
/* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
error_free_or_abort(&err);
qapi_free_AltStrNum(asn);
+ /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
- visit_type_AltNumStr(v, &ans, NULL, &error_abort);
- g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
- g_assert_cmpfloat(ans->u.n, ==, 42);
+ visit_type_AltNumStr(v, &ans, NULL, &err);
+ /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */
+ /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */
+ error_free_or_abort(&err);
qapi_free_AltNumStr(ans);
v = visitor_input_test_init(data, "42");
visit_type_AltStrInt(v, &asi, NULL, &error_abort);
- g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
+ g_assert_cmpint(asi->type, ==, QTYPE_QINT);
g_assert_cmpint(asi->u.i, ==, 42);
qapi_free_AltStrInt(asi);
v = visitor_input_test_init(data, "42");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
- g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
+ g_assert_cmpint(ain->type, ==, QTYPE_QINT);
g_assert_cmpint(ain->u.i, ==, 42);
qapi_free_AltIntNum(ain);
v = visitor_input_test_init(data, "42");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
- g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
+ g_assert_cmpint(ani->type, ==, QTYPE_QINT);
g_assert_cmpint(ani->u.i, ==, 42);
qapi_free_AltNumInt(ani);
@@ -389,13 +390,13 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltStrNum(v, &asn, NULL, &error_abort);
- g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N);
+ g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(asn->u.n, ==, 42.5);
qapi_free_AltStrNum(asn);
v = visitor_input_test_init(data, "42.5");
visit_type_AltNumStr(v, &ans, NULL, &error_abort);
- g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
+ g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ans->u.n, ==, 42.5);
qapi_free_AltNumStr(ans);
@@ -406,13 +407,13 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
- g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N);
+ g_assert_cmpint(ain->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ain->u.n, ==, 42.5);
qapi_free_AltIntNum(ain);
v = visitor_input_test_init(data, "42.5");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
- g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N);
+ g_assert_cmpint(ani->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ani->u.n, ==, 42.5);
qapi_free_AltNumInt(ani);
}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 4196e66..3078442 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -428,7 +428,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
UserDefAlternate *tmp;
tmp = g_new0(UserDefAlternate, 1);
- tmp->type = USER_DEF_ALTERNATE_KIND_I;
+ tmp->type = QTYPE_QINT;
tmp->u.i = 42;
visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
@@ -441,7 +441,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
qobject_decref(arg);
tmp = g_new0(UserDefAlternate, 1);
- tmp->type = USER_DEF_ALTERNATE_KIND_S;
+ tmp->type = QTYPE_QSTRING;
tmp->u.s = g_strdup("hello");
visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 05/14] qapi: Inline _make_implicit_tag()
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (3 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 04/14] qapi: Simplify visiting of alternate types Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 06/14] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
` (9 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Now that alternates no longer use an implicit tag, we can
inline _make_implicit_tag() into its one caller,
_def_union_type().
No change to generated code.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: commit message touchup
v12: new patch
---
scripts/qapi.py | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 44b6126..264de63 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1318,11 +1318,6 @@ class QAPISchema(object):
typ, info, 'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
- def _make_implicit_tag(self, type_name, info, variants):
- typ = self._make_implicit_enum_type(type_name, info,
- [v.name for v in variants])
- return QAPISchemaObjectTypeMember('type', typ, False)
-
def _def_union_type(self, expr, info):
name = expr['union']
data = expr['data']
@@ -1336,7 +1331,9 @@ class QAPISchema(object):
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
- tag_member = self._make_implicit_tag(name, info, variants)
+ typ = self._make_implicit_enum_type(name, info,
+ [v.name for v in variants])
+ tag_member = QAPISchemaObjectTypeMember('type', typ, False)
members = [tag_member]
self._def_entity(
QAPISchemaObjectType(name, info, base, members,
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 06/14] qapi: Fix alternates that accept 'number' but not 'int'
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (4 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 05/14] qapi: Inline _make_implicit_tag() Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 07/14] qapi: Simplify visits of optional fields Eric Blake
` (8 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The QMP input visitor allows integral values to be assigned by
promotion to a QTYPE_QFLOAT. However, when parsing an alternate,
we did not take this into account, such that an alternate that
accepts 'number' and some other type, but not 'int', would reject
integral values.
With this patch, we now have the following desirable table:
alternate has case selected for
'int' 'number' QTYPE_QINT QTYPE_QFLOAT
no no error error
no yes 'number' 'number'
yes no 'int' error
yes yes 'int' 'number'
While it is unlikely that we will ever use 'number' in an
alternate other than in the testsuite, it never hurts to be
more precise in what we allow.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: no change
v12: rebase to QType cleanups
v11 (no v10): slight commit message tweak, rebase to earlier changes
v9: rebase to earlier changes
v8: no change
v7: rebase to named .u union
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor-impl.h | 2 +-
include/qapi/visitor.h | 3 ++-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 5 ++++-
scripts/qapi-visit.py | 11 +++++++----
tests/test-qmp-input-visitor.c | 16 ++++++----------
6 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7cd1313..7419684 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -33,7 +33,7 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
/* May be NULL; only needed for input visitors. */
- void (*get_next_type)(Visitor *v, QType *type,
+ void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 6d25ad2..1414de1 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -43,8 +43,9 @@ void visit_optional(Visitor *v, bool *present, const char *name,
* 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, QType *type,
+void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 850ca03..cee76bc 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
}
}
-void visit_get_next_type(Visitor *v, QType *type,
+void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp)
{
if (v->get_next_type) {
- v->get_next_type(v, type, name, errp);
+ v->get_next_type(v, type, promote_int, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index d398de7..26b7414 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
qmp_input_pop(qiv, errp);
}
-static void qmp_input_get_next_type(Visitor *v, QType *type,
+static void qmp_input_get_next_type(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -219,6 +219,9 @@ static void qmp_input_get_next_type(Visitor *v, QType *type,
return;
}
*type = qobject_type(qobj);
+ if (promote_int && *type == QTYPE_QINT) {
+ *type = QTYPE_QFLOAT;
+ }
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e254ca9..dc2a336 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -184,6 +184,11 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error
def gen_visit_alternate(name, variants):
+ promote_int = 'true'
+ for var in variants.variants:
+ if var.type.alternate_qtype() == 'QTYPE_QINT':
+ promote_int = 'false'
+
ret = mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
@@ -194,16 +199,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(v, &(*obj)->type, name, &err);
+ visit_get_next_type(v, &(*obj)->type, %(promote_int)s, name, &err);
if (err) {
goto out_obj;
}
switch ((*obj)->type) {
''',
- c_name=c_name(name))
+ c_name=c_name(name), promote_int=promote_int)
- # FIXME: When 'number' but not 'int' is present in the alternate, we
- # should allow QTYPE_INT to promote to QTYPE_FLOAT.
for var in variants.variants:
ret += mcgen('''
case %(case)s:
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 43b9e18..b4a5bee 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -347,20 +347,16 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free_or_abort(&err);
qapi_free_AltStrBool(asb);
- /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
- visit_type_AltStrNum(v, &asn, NULL, &err);
- /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */
- /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
- error_free_or_abort(&err);
+ visit_type_AltStrNum(v, &asn, NULL, &error_abort);
+ g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(asn->u.n, ==, 42);
qapi_free_AltStrNum(asn);
- /* FIXME: integer should parse as number */
v = visitor_input_test_init(data, "42");
- visit_type_AltNumStr(v, &ans, NULL, &err);
- /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */
- /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */
- error_free_or_abort(&err);
+ visit_type_AltNumStr(v, &ans, NULL, &error_abort);
+ g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(ans->u.n, ==, 42);
qapi_free_AltNumStr(ans);
v = visitor_input_test_init(data, "42");
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 07/14] qapi: Simplify visits of optional fields
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (5 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 06/14] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 08/14] qapi: Shorter " Eric Blake
` (7 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
None of the visitor callbacks would set an error when testing
if an optional field was present; make this part of the interface
contract by eliminating the errp argument.
The resulting generated code has a nice diff:
|- visit_optional(v, &has_fdset_id, "fdset-id", &err);
|- if (err) {
|- goto out;
|- }
|+ visit_optional(v, &has_fdset_id, "fdset-id");
| if (has_fdset_id) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: commit message tweak
v12: split error removal from 'if' compression
v11 (no v10): no change
v9: no change
v8: no change
v7: rebase to no member.c_name()
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor-impl.h | 5 ++---
include/qapi/visitor.h | 10 ++++++++--
qapi/opts-visitor.c | 2 +-
qapi/qapi-visit-core.c | 5 ++---
qapi/qmp-input-visitor.c | 3 +--
qapi/string-input-visitor.c | 3 +--
scripts/qapi.py | 8 ++------
7 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7419684..44a21b7 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -44,9 +44,8 @@ struct Visitor
void (*type_any)(Visitor *v, QObject **obj, const char *name,
Error **errp);
- /* May be NULL */
- void (*optional)(Visitor *v, bool *present, const char *name,
- Error **errp);
+ /* May be NULL; most useful for input visitors. */
+ void (*optional)(Visitor *v, bool *present, const char *name);
void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1414de1..9be60d4 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -36,8 +36,14 @@ void visit_end_implicit_struct(Visitor *v, Error **errp);
void visit_start_list(Visitor *v, const char *name, Error **errp);
GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
-void visit_optional(Visitor *v, bool *present, const char *name,
- Error **errp);
+
+/**
+ * Check if an optional member @name of an object needs visiting.
+ * For input visitors, set *@present according to whether the
+ * corresponding visit_type_*() needs calling; for other visitors,
+ * leave *@present unchanged.
+ */
+void visit_optional(Visitor *v, bool *present, const char *name);
/**
* Determine the qtype of the item @name in the current object visit.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index cd10392..ef5fb8b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -488,7 +488,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
static void
-opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
+opts_optional(Visitor *v, bool *present, const char *name)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index cee76bc..e07d6f9 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -73,11 +73,10 @@ void visit_end_union(Visitor *v, bool data_present, Error **errp)
}
}
-void visit_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+void visit_optional(Visitor *v, bool *present, const char *name)
{
if (v->optional) {
- v->optional(v, present, name, errp);
+ v->optional(v, present, name);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 26b7414..932b5f3 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -303,8 +303,7 @@ static void qmp_input_type_any(Visitor *v, QObject **obj, const char *name,
*obj = qobj;
}
-static void qmp_input_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+static void qmp_input_optional(Visitor *v, bool *present, const char *name)
{
QmpInputVisitor *qiv = to_qiv(v);
QObject *qobj = qmp_input_get_object(qiv, name, true);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index bbd6a54..dee780a 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -299,8 +299,7 @@ static void parse_type_number(Visitor *v, double *obj, const char *name,
*obj = val;
}
-static void parse_optional(Visitor *v, bool *present, const char *name,
- Error **errp)
+static void parse_optional(Visitor *v, bool *present, const char *name)
{
StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 264de63..fe76a6e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1655,15 +1655,11 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
for memb in members:
if memb.optional:
ret += mcgen('''
- visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
+ visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s");
+ if (%(prefix)shas_%(c_name)s) {
''',
prefix=prefix, c_name=c_name(memb.name),
name=memb.name, errp=errparg)
- ret += gen_err_check(skiperr=skiperr)
- ret += mcgen('''
- if (%(prefix)shas_%(c_name)s) {
-''',
- prefix=prefix, c_name=c_name(memb.name))
push_indent()
# Ugly: sometimes we need to cast away const
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 08/14] qapi: Shorter visits of optional fields
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (6 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 07/14] qapi: Simplify visits of optional fields Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 09/14] qapi: Prepare new QAPISchemaMember base class Eric Blake
` (6 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
For less code, reflect the determined boolean value of an optional
visit back to the caller instead of making the caller read the
boolean after the fact.
The resulting generated code has the following diff:
|- visit_optional(v, &has_fdset_id, "fdset-id");
|- if (has_fdset_id) {
|+ if (visit_optional(v, &has_fdset_id, "fdset-id")) {
| visit_type_int(v, &fdset_id, "fdset-id", &err);
| if (err) {
| goto out;
| }
| }
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: no change
v12: split error removal from 'if' compression
v11 (no v10): no change
v9: no change
v8: no change
v7: rebase to no member.c_name()
v6: rebase onto earlier testsuite and gen_err_check() improvements
---
include/qapi/visitor.h | 4 ++--
qapi/qapi-visit-core.c | 3 ++-
scripts/qapi.py | 3 +--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 9be60d4..a14a16d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -41,9 +41,9 @@ void visit_end_list(Visitor *v, Error **errp);
* Check if an optional member @name of an object needs visiting.
* For input visitors, set *@present according to whether the
* corresponding visit_type_*() needs calling; for other visitors,
- * leave *@present unchanged.
+ * leave *@present unchanged. Return *@present for convenience.
*/
-void visit_optional(Visitor *v, bool *present, const char *name);
+bool visit_optional(Visitor *v, bool *present, const char *name);
/**
* Determine the qtype of the item @name in the current object visit.
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index e07d6f9..6d63e40 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -73,11 +73,12 @@ void visit_end_union(Visitor *v, bool data_present, Error **errp)
}
}
-void visit_optional(Visitor *v, bool *present, const char *name)
+bool visit_optional(Visitor *v, bool *present, const char *name)
{
if (v->optional) {
v->optional(v, present, name);
}
+ return *present;
}
void visit_get_next_type(Visitor *v, QType *type, bool promote_int,
diff --git a/scripts/qapi.py b/scripts/qapi.py
index fe76a6e..085455b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1655,8 +1655,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
for memb in members:
if memb.optional:
ret += mcgen('''
- visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s");
- if (%(prefix)shas_%(c_name)s) {
+ if (visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s")) {
''',
prefix=prefix, c_name=c_name(memb.name),
name=memb.name, errp=errparg)
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 09/14] qapi: Prepare new QAPISchemaMember base class
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (7 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 08/14] qapi: Shorter " Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
` (5 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We want to share some clash detection code between enum values
and object type members. To assist with that, split off part
of QAPISchemaObjectTypeMember into a new base class
QAPISchemaMember that tracks name, owner, and common clash
detection code; while the former keeps the additional fields
for type and optional flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: new patch
---
scripts/qapi.py | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 085455b..2748464 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1018,28 +1018,18 @@ class QAPISchemaObjectType(QAPISchemaType):
self.members, self.variants)
-class QAPISchemaObjectTypeMember(object):
+class QAPISchemaMember(object):
role = 'member'
- def __init__(self, name, typ, optional):
+ def __init__(self, name):
assert isinstance(name, str)
- assert isinstance(typ, str)
- assert isinstance(optional, bool)
self.name = name
- self._type_name = typ
- self.type = None
- self.optional = optional
self.owner = None
def set_owner(self, name):
assert not self.owner
self.owner = name
- def check(self, schema):
- assert self.owner
- self.type = schema.lookup_type(self._type_name)
- assert self.type
-
def check_clash(self, info, seen):
cname = c_name(self.name)
if cname in seen:
@@ -1065,6 +1055,21 @@ class QAPISchemaObjectTypeMember(object):
return "'%s' %s" % (self.name, self._pretty_owner())
+class QAPISchemaObjectTypeMember(QAPISchemaMember):
+ def __init__(self, name, typ, optional):
+ QAPISchemaMember.__init__(self, name)
+ assert isinstance(typ, str)
+ assert isinstance(optional, bool)
+ self._type_name = typ
+ self.type = None
+ self.optional = optional
+
+ def check(self, schema):
+ assert self.owner
+ self.type = schema.lookup_type(self._type_name)
+ assert self.type
+
+
class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_member, variants):
# Flat unions pass tag_name but not tag_member.
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (8 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 09/14] qapi: Prepare new QAPISchemaMember base class Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-26 16:29 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity Eric Blake
` (4 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Rather than using just an array of strings, make enum.values be
an array of the new QAPISchemaMember type. Creating an enum
requires wrapping strings, and visiting an enum requires getting
at the name of each value. But using this type means we can
share the existing code for C name clash detection (although the
code is not yet active until a later commit removes the earlier
ad hoc parser checks). The ._pretty_owner() method needs to
learn about one more implicit type name: the generated enum
associated with a simple union.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13: new patch
---
scripts/qapi.py | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2748464..d1239c2 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -901,13 +901,17 @@ class QAPISchemaEnumType(QAPISchemaType):
def __init__(self, name, info, values, prefix):
QAPISchemaType.__init__(self, name, info)
for v in values:
- assert isinstance(v, str)
+ assert isinstance(v, QAPISchemaMember)
+ v.set_owner(name)
assert prefix is None or isinstance(prefix, str)
self.values = values
self.prefix = prefix
def check(self, schema):
- assert len(set(self.values)) == len(self.values)
+ # Check for collisions on the generated C enum values
+ seen = {}
+ for v in self.values:
+ v.check_clash(self.info, seen)
def is_implicit(self):
# See QAPISchema._make_implicit_enum_type()
@@ -917,15 +921,18 @@ class QAPISchemaEnumType(QAPISchemaType):
return c_name(self.name)
def c_null(self):
- return c_enum_const(self.name, (self.values + ['_MAX'])[0],
- self.prefix)
+ if self.values:
+ value = self.values[0].name
+ else:
+ value = '_MAX'
+ return c_enum_const(self.name, value, self.prefix)
def json_type(self):
return 'string'
def visit(self, visitor):
visitor.visit_enum_type(self.name, self.info,
- self.values, self.prefix)
+ [v.name for v in self.values], self.prefix)
class QAPISchemaArrayType(QAPISchemaType):
@@ -1049,6 +1056,8 @@ class QAPISchemaMember(object):
else:
assert owner.endswith('-wrapper')
return '(branch of %s)' % owner[:-8]
+ if owner.endswith('Kind'):
+ return '(branch of %s)' % owner[:-4]
return '(%s of %s)' % (self.role, owner)
def describe(self):
@@ -1094,12 +1103,13 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = seen[c_name(self.tag_name)]
assert self.tag_name == self.tag_member.name
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ tag_values = [v.name for v in self.tag_member.type.values]
for v in self.variants:
v.check(schema)
# Union names must match enum values; alternate names are
# checked separately. Use 'seen' to tell the two apart.
if seen:
- assert v.name in self.tag_member.type.values
+ assert v.name in tag_values
assert isinstance(v.type, QAPISchemaObjectType)
v.type.check(schema)
@@ -1257,15 +1267,16 @@ class QAPISchema(object):
self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
[], None)
self._def_entity(self.the_empty_object_type)
- self._def_entity(QAPISchemaEnumType('QType', None,
- ['none', 'qnull', 'qint',
- 'qstring', 'qdict', 'qlist',
- 'qfloat', 'qbool'],
+ qtype_values = [QAPISchemaMember(name) for name in
+ ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist',
+ 'qfloat', 'qbool']]
+ self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
'QTYPE'))
def _make_implicit_enum_type(self, name, info, values):
name = name + 'Kind' # Use namespace reserved by add_name()
- self._def_entity(QAPISchemaEnumType(name, info, values, None))
+ self._def_entity(QAPISchemaEnumType(
+ name, info, [QAPISchemaMember(v) for v in values], None))
return name
def _make_array_type(self, element_type, info):
@@ -1288,7 +1299,8 @@ class QAPISchema(object):
name = expr['enum']
data = expr['data']
prefix = expr.get('prefix')
- self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
+ self._def_entity(QAPISchemaEnumType(
+ name, info, [QAPISchemaMember(v) for v in data], prefix))
def _make_member(self, name, typ, info):
optional = False
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (9 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-26 16:46 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
` (3 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Every non-implicit entity is associated with an info dictionary,
but it is not easy to reverse-engineer the name of the top-most
entity associated with that info. Add a new info['name'] field
to track this information, as it will be handy in future commits
for better error messages.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi.py | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d1239c2..ff3fccb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1296,7 +1296,7 @@ class QAPISchema(object):
return name
def _def_enum_type(self, expr, info):
- name = expr['enum']
+ info['name'] = name = expr['enum']
data = expr['data']
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(
@@ -1317,7 +1317,7 @@ class QAPISchema(object):
for (key, value) in data.iteritems()]
def _def_struct_type(self, expr, info):
- name = expr['struct']
+ info['name'] = name = expr['struct']
base = expr.get('base')
data = expr['data']
self._def_entity(QAPISchemaObjectType(name, info, base,
@@ -1336,7 +1336,7 @@ class QAPISchema(object):
return QAPISchemaObjectTypeVariant(case, typ)
def _def_union_type(self, expr, info):
- name = expr['union']
+ info['name'] = name = expr['union']
data = expr['data']
base = expr.get('base')
tag_name = expr.get('discriminator')
@@ -1359,7 +1359,7 @@ class QAPISchema(object):
variants)))
def _def_alternate_type(self, expr, info):
- name = expr['alternate']
+ info['name'] = name = expr['alternate']
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
@@ -1371,7 +1371,7 @@ class QAPISchema(object):
variants)))
def _def_command(self, expr, info):
- name = expr['command']
+ info['name'] = name = expr['command']
data = expr.get('data')
rets = expr.get('returns')
gen = expr.get('gen', True)
@@ -1386,7 +1386,7 @@ class QAPISchema(object):
success_response))
def _def_event(self, expr, info):
- name = expr['event']
+ info['name'] = name = expr['event']
data = expr.get('data')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (10 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-27 9:03 ` Markus Armbruster
2015-11-27 9:42 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 13/14] qapi: Move duplicate collision checks to schema check() Eric Blake
` (2 subsequent siblings)
14 siblings, 2 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We document that members of enums and objects should be
'lower-case', although we were not enforcing it. We have to
whitelist a few pre-existing entities that violate the norms.
Add three new tests to expose the new error message, each of
which first uses the whitelisted name 'UuidInfo' to prove the
whitelist works, then triggers the failure.
Note that by adding this check, we have effectively forbidden
an entity with a case-insensitive clash of member names, for
any entity that is not on the whitelist (although there is
still the possibility to clash via '-' vs. '_').
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi.py | 19 +++++++++++++++++++
tests/Makefile | 3 +++
tests/qapi-schema/args-member-case.err | 1 +
tests/qapi-schema/args-member-case.exit | 1 +
tests/qapi-schema/args-member-case.json | 3 +++
tests/qapi-schema/args-member-case.out | 0
tests/qapi-schema/enum-member-case.err | 1 +
tests/qapi-schema/enum-member-case.exit | 1 +
tests/qapi-schema/enum-member-case.json | 3 +++
tests/qapi-schema/enum-member-case.out | 0
tests/qapi-schema/union-branch-case.err | 1 +
tests/qapi-schema/union-branch-case.exit | 1 +
tests/qapi-schema/union-branch-case.json | 3 +++
tests/qapi-schema/union-branch-case.out | 0
14 files changed, 37 insertions(+)
create mode 100644 tests/qapi-schema/args-member-case.err
create mode 100644 tests/qapi-schema/args-member-case.exit
create mode 100644 tests/qapi-schema/args-member-case.json
create mode 100644 tests/qapi-schema/args-member-case.out
create mode 100644 tests/qapi-schema/enum-member-case.err
create mode 100644 tests/qapi-schema/enum-member-case.exit
create mode 100644 tests/qapi-schema/enum-member-case.json
create mode 100644 tests/qapi-schema/enum-member-case.out
create mode 100644 tests/qapi-schema/union-branch-case.err
create mode 100644 tests/qapi-schema/union-branch-case.exit
create mode 100644 tests/qapi-schema/union-branch-case.json
create mode 100644 tests/qapi-schema/union-branch-case.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ff3fccb..00eb43e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -59,6 +59,21 @@ returns_whitelist = [
'guest-sync-delimited',
]
+# Whitelist of entities allowed to violate case conventions
+case_whitelist = [
+ # From QMP:
+ 'ACPISlotType',
+ 'CpuInfo',
+ 'CpuInfoBase',
+ 'CpuInfoMIPS',
+ 'CpuInfoTricore',
+ 'InputAxis',
+ 'InputButton',
+ 'QapiErrorClass',
+ 'UuidInfo',
+ 'X86CPURegister32',
+]
+
enum_types = []
struct_types = []
union_types = []
@@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
def check_clash(self, info, seen):
cname = c_name(self.name)
+ if cname.lower() != cname and info['name'] not in case_whitelist:
+ raise QAPIExprError(info,
+ "Member '%s' of '%s' should use lowercase"
+ % (self.name, info['name']))
if cname in seen:
raise QAPIExprError(info,
"%s collides with %s"
diff --git a/tests/Makefile b/tests/Makefile
index e377c70..ca386e9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -246,6 +246,7 @@ qapi-schema += args-array-unknown.json
qapi-schema += args-int.json
qapi-schema += args-invalid.json
qapi-schema += args-member-array-bad.json
+qapi-schema += args-member-case.json
qapi-schema += args-member-unknown.json
qapi-schema += args-name-clash.json
qapi-schema += args-union.json
@@ -267,6 +268,7 @@ qapi-schema += enum-bad-prefix.json
qapi-schema += enum-clash-member.json
qapi-schema += enum-dict-member.json
qapi-schema += enum-int-member.json
+qapi-schema += enum-member-case.json
qapi-schema += enum-missing-data.json
qapi-schema += enum-wrong-data.json
qapi-schema += escape-outside-string.json
@@ -341,6 +343,7 @@ qapi-schema += unclosed-string.json
qapi-schema += unicode-str.json
qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
+qapi-schema += union-branch-case.json
qapi-schema += union-clash-branches.json
qapi-schema += union-clash-data.json
qapi-schema += union-empty.json
diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err
new file mode 100644
index 0000000..7bace48
--- /dev/null
+++ b/tests/qapi-schema/args-member-case.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
diff --git a/tests/qapi-schema/args-member-case.exit b/tests/qapi-schema/args-member-case.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-member-case.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-member-case.json b/tests/qapi-schema/args-member-case.json
new file mode 100644
index 0000000..1bc823a
--- /dev/null
+++ b/tests/qapi-schema/args-member-case.json
@@ -0,0 +1,3 @@
+# Member names should be 'lower-case' unless the struct/command is whitelisted
+{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
+{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
diff --git a/tests/qapi-schema/args-member-case.out b/tests/qapi-schema/args-member-case.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err
new file mode 100644
index 0000000..e50b12a
--- /dev/null
+++ b/tests/qapi-schema/enum-member-case.err
@@ -0,0 +1 @@
+tests/qapi-schema/enum-member-case.json:3: Member 'Value' of 'Foo' should use lowercase
diff --git a/tests/qapi-schema/enum-member-case.exit b/tests/qapi-schema/enum-member-case.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/enum-member-case.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/enum-member-case.json b/tests/qapi-schema/enum-member-case.json
new file mode 100644
index 0000000..5101275
--- /dev/null
+++ b/tests/qapi-schema/enum-member-case.json
@@ -0,0 +1,3 @@
+# Member names should be 'lower-case' unless the enum is whitelisted
+{ 'enum': 'UuidInfo', 'data': [ 'Value' ] }
+{ 'enum': 'Foo', 'data': [ 'Value' ] }
diff --git a/tests/qapi-schema/enum-member-case.out b/tests/qapi-schema/enum-member-case.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err
new file mode 100644
index 0000000..6c6b740
--- /dev/null
+++ b/tests/qapi-schema/union-branch-case.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-branch-case.json:3: Member 'Branch' of 'Foo' should use lowercase
diff --git a/tests/qapi-schema/union-branch-case.exit b/tests/qapi-schema/union-branch-case.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-branch-case.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-branch-case.json b/tests/qapi-schema/union-branch-case.json
new file mode 100644
index 0000000..a5951f1
--- /dev/null
+++ b/tests/qapi-schema/union-branch-case.json
@@ -0,0 +1,3 @@
+# Branch names should be 'lower-case' unless the union is whitelisted
+{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
+{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
diff --git a/tests/qapi-schema/union-branch-case.out b/tests/qapi-schema/union-branch-case.out
new file mode 100644
index 0000000..e69de29
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 13/14] qapi: Move duplicate collision checks to schema check()
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (11 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
@ 2015-11-20 17:24 ` Eric Blake
2015-11-20 17:25 ` [Qemu-devel] [PATCH v13 14/14] qapi: Detect base class loops Eric Blake
2015-11-27 9:56 ` [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Markus Armbruster
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
With the recent commit 'qapi: Detect collisions in C member
names', we have two different locations for detecting clashes -
one at parse time, and another at QAPISchema*.check() time.
Remove all of the ad hoc parser checks, and delete associated
code (for example, the global check_member_clash() method is
no longer needed).
Testing this showed that the test union-bad-branch wasn't adding
much: union-clash-branches also exposes the error message when
branches collide, and we've recently fixed things to avoid an
implicit collision with max. Likewise, the error for
enum-clash-member changes to report our new detection of
upper case in a value name, unless we modify the test to use
all lower case.
The wording of several error messages has changed, but the
change is generally an improvement rather than a regression.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13 (no v12): merge two patches into one, fix enum-case-member
v11 (no v10): no change
v9: simplify on top of earlier check() improvements
v8: decide whether to inline members based on union vs. alternate,
not on flat vs. simple, and fix logic to avoid breaking
union-clash-data in the process; add comments; assumes
pull-qapi-2015-10-12 will go in without modifying commit ids
v7: comment improvements, retitle subject
v6: rebase to earlier testsuite improvements, fold in cleanup
of flat-union-clash-type
---
scripts/qapi.py | 51 +--------------------------
tests/Makefile | 1 -
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/enum-clash-member.json | 2 +-
tests/qapi-schema/flat-union-clash-member.err | 2 +-
tests/qapi-schema/struct-base-clash-deep.err | 2 +-
tests/qapi-schema/struct-base-clash.err | 2 +-
tests/qapi-schema/union-bad-branch.err | 1 -
tests/qapi-schema/union-bad-branch.exit | 1 -
tests/qapi-schema/union-bad-branch.json | 8 -----
tests/qapi-schema/union-bad-branch.out | 0
tests/qapi-schema/union-clash-branches.err | 2 +-
tests/qapi-schema/union-clash-branches.json | 2 +-
14 files changed, 9 insertions(+), 69 deletions(-)
delete mode 100644 tests/qapi-schema/union-bad-branch.err
delete mode 100644 tests/qapi-schema/union-bad-branch.exit
delete mode 100644 tests/qapi-schema/union-bad-branch.json
delete mode 100644 tests/qapi-schema/union-bad-branch.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 00eb43e..fdf6720 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -520,21 +520,6 @@ def check_type(expr_info, source, value, allow_array=False,
'enum'])
-def check_member_clash(expr_info, base_name, data, source=""):
- base = find_struct(base_name)
- assert base
- base_members = base['data']
- for key in data.keys():
- if key.startswith('*'):
- key = key[1:]
- if key in base_members or "*" + key in base_members:
- raise QAPIExprError(expr_info,
- "Member name '%s'%s clashes with base '%s'"
- % (key, source, base_name))
- if base.get('base'):
- check_member_clash(expr_info, base['base'], data, source)
-
-
def check_command(expr, expr_info):
name = expr['command']
@@ -564,7 +549,6 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = {}
# Two types of unions, determined by discriminator.
@@ -611,15 +595,9 @@ def check_union(expr, expr_info):
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
- # Each value must name a known type; furthermore, in flat unions,
- # branches must be a struct with no overlapping member names
+ # Each value must name a known type
check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
value, allow_array=not base, allow_metas=allow_metas)
- if base:
- branch_struct = find_struct(value)
- assert branch_struct
- check_member_clash(expr_info, base, branch_struct['data'],
- " of branch '%s'" % key)
# If the discriminator names an enum type, then all members
# of 'data' must also be members of the enum type.
@@ -630,34 +608,16 @@ def check_union(expr, expr_info):
"enum '%s'" %
(key, enum_define["enum_name"]))
- # Otherwise, check for conflicts in the generated enum
- else:
- c_key = camel_to_upper(key)
- if c_key in values:
- raise QAPIExprError(expr_info,
- "Union '%s' member '%s' clashes with '%s'"
- % (name, key, values[c_key]))
- values[c_key] = key
-
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
- values = {}
types_seen = {}
# Check every branch
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
- # Check for conflicts in the branch names
- c_key = c_name(key)
- if c_key in values:
- raise QAPIExprError(expr_info,
- "Alternate '%s' member '%s' clashes with '%s'"
- % (name, key, values[c_key]))
- values[c_key] = key
-
# Ensure alternates have no type conflicts.
check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
value,
@@ -676,7 +636,6 @@ def check_enum(expr, expr_info):
name = expr['enum']
members = expr.get('data')
prefix = expr.get('prefix')
- values = {}
if not isinstance(members, list):
raise QAPIExprError(expr_info,
@@ -687,12 +646,6 @@ def check_enum(expr, expr_info):
for member in members:
check_name(expr_info, "Member of enum '%s'" % name, member,
enum_member=True)
- key = camel_to_upper(member)
- if key in values:
- raise QAPIExprError(expr_info,
- "Enum '%s' member '%s' clashes with '%s'"
- % (name, member, values[key]))
- values[key] = member
def check_struct(expr, expr_info):
@@ -703,8 +656,6 @@ def check_struct(expr, expr_info):
allow_dict=True, allow_optional=True)
check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
allow_metas=['struct'])
- if expr.get('base'):
- check_member_clash(expr_info, expr['base'], expr['data'])
def check_keys(expr_elem, meta, required, optional=[]):
diff --git a/tests/Makefile b/tests/Makefile
index ca386e9..64cca48 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -341,7 +341,6 @@ qapi-schema += unclosed-list.json
qapi-schema += unclosed-object.json
qapi-schema += unclosed-string.json
qapi-schema += unicode-str.json
-qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
qapi-schema += union-branch-case.json
qapi-schema += union-clash-branches.json
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
index a475ab6..604d849 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/alternate-clash.json:7: 'a_b' (branch of Alt1) collides with 'a-b' (branch of Alt1)
diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
index 48bd136..5403c78 100644
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'
+tests/qapi-schema/enum-clash-member.json:2: 'one_two' (member of MyEnum) collides with 'one-two' (member of MyEnum)
diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json
index b7dc02a..b6928b8 100644
--- a/tests/qapi-schema/enum-clash-member.json
+++ b/tests/qapi-schema/enum-clash-member.json
@@ -1,2 +1,2 @@
# we reject enums where members will clash when mapped to C enum
-{ 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] }
+{ 'enum': 'MyEnum', 'data': [ 'one-two', 'one_two' ] }
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
index 2f0397a..2adf697 100644
--- a/tests/qapi-schema/flat-union-clash-member.err
+++ b/tests/qapi-schema/flat-union-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
+tests/qapi-schema/flat-union-clash-member.json:11: 'name' (member of Branch1) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
index f7a25a3..e2d7943 100644
--- a/tests/qapi-schema/struct-base-clash-deep.err
+++ b/tests/qapi-schema/struct-base-clash-deep.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
index 3a9f66b..c52f33d 100644
--- a/tests/qapi-schema/struct-base-clash.err
+++ b/tests/qapi-schema/struct-base-clash.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err
deleted file mode 100644
index 8822735..0000000
--- a/tests/qapi-schema/union-bad-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/union-bad-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json
deleted file mode 100644
index 913aa38..0000000
--- a/tests/qapi-schema/union-bad-branch.json
+++ /dev/null
@@ -1,8 +0,0 @@
-# we reject normal unions where branches would collide in C
-{ 'struct': 'One',
- 'data': { 'string': 'str' } }
-{ 'struct': 'Two',
- 'data': { 'number': 'int' } }
-{ 'union': 'MyUnion',
- 'data': { 'one': 'One',
- 'ONE': 'Two' } }
diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err
index 005c48d..e5b2113 100644
--- a/tests/qapi-schema/union-clash-branches.err
+++ b/tests/qapi-schema/union-clash-branches.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/union-clash-branches.json:4: 'a_b' (branch of TestUnion) collides with 'a-b' (branch of TestUnion)
diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json
index 31d135f..3bece8c 100644
--- a/tests/qapi-schema/union-clash-branches.json
+++ b/tests/qapi-schema/union-clash-branches.json
@@ -1,5 +1,5 @@
# Union branch name collision
# Reject a union that would result in a collision in generated C names (this
-# would try to generate two enum values 'TEST_UNION_KIND_A_B').
+# would try to generate two members 'a_b').
{ 'union': 'TestUnion',
'data': { 'a-b': 'int', 'a_b': 'str' } }
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v13 14/14] qapi: Detect base class loops
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (12 preceding siblings ...)
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 13/14] qapi: Move duplicate collision checks to schema check() Eric Blake
@ 2015-11-20 17:25 ` Eric Blake
2015-11-27 9:56 ` [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Markus Armbruster
14 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-20 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
It should be fairly obvious that qapi base classes need to
form an acyclic graph, since QMP cannot specify the same
key more than once, while base classes are included as flat
members alongside other members added by the child. But the
old check_member_clash() parser function was not prepared to
check for this, and entered an infinite recursion (at least
until Python gives up, complaining about nesting too deep).
Now that check_member_clash() has been recently removed,
attempts at self-inheritance trigger an assertion failure
introduced by commit ac88219a. The obvious fix is to turn
the assertion into a conditional.
This patch includes both the tests (base-cycle-direct and
base-cycle-indirect) and the fix, since the .err file output
for the unfixed case is not useful (particularly when it was
warning about unbounded recursion, as that limit may be
platform-specific).
We don't need to worry about cycles in flat unions (neither
the base type nor the type of a variant can be a union) nor
in alternates (alternate branches cannot themselves be an
alternate). But if we later allow a union type as a variant,
we will still be okay, as QAPISchemaObjectTypeVariants.check()
triggers the same QAPISchemaObjectType.check() that will
detect any loops.
Likewise, we need not worry about the case of diamond
inheritance where the same class is used for a flat union base
class and one of its variants; either both uses will introduce
a collision in trying to insert the same member name twice, or
the shared type is empty and changes nothing.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v13 (no v12): simplify error message, use cuter name 'Loopy' in
testsuite
v11 (no v10): rename base-cycle to base-cycle-indirect, and add
base-cycle-direct; touch up commit message
v9: no change
v8: improve commit message
v7: improve commit message
v6: rebase to earlier info changes
---
scripts/qapi.py | 4 +++-
tests/Makefile | 2 ++
tests/qapi-schema/base-cycle-direct.err | 1 +
tests/qapi-schema/base-cycle-direct.exit | 1 +
tests/qapi-schema/base-cycle-direct.json | 2 ++
tests/qapi-schema/base-cycle-direct.out | 0
tests/qapi-schema/base-cycle-indirect.err | 1 +
tests/qapi-schema/base-cycle-indirect.exit | 1 +
tests/qapi-schema/base-cycle-indirect.json | 3 +++
tests/qapi-schema/base-cycle-indirect.out | 0
10 files changed, 14 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/base-cycle-direct.err
create mode 100644 tests/qapi-schema/base-cycle-direct.exit
create mode 100644 tests/qapi-schema/base-cycle-direct.json
create mode 100644 tests/qapi-schema/base-cycle-direct.out
create mode 100644 tests/qapi-schema/base-cycle-indirect.err
create mode 100644 tests/qapi-schema/base-cycle-indirect.exit
create mode 100644 tests/qapi-schema/base-cycle-indirect.json
create mode 100644 tests/qapi-schema/base-cycle-indirect.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index fdf6720..4197b30 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -942,7 +942,9 @@ class QAPISchemaObjectType(QAPISchemaType):
self.members = None
def check(self, schema):
- assert self.members is not False # not running in cycles
+ if self.members is False: # check for cycles
+ raise QAPIExprError(self.info,
+ "Object %s contains itself" % self.name)
if self.members:
return
self.members = False # mark as being checked
diff --git a/tests/Makefile b/tests/Makefile
index 64cca48..8ccd11e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -257,6 +257,8 @@ qapi-schema += bad-ident.json
qapi-schema += bad-type-bool.json
qapi-schema += bad-type-dict.json
qapi-schema += bad-type-int.json
+qapi-schema += base-cycle-direct.json
+qapi-schema += base-cycle-indirect.json
qapi-schema += command-int.json
qapi-schema += comments.json
qapi-schema += double-data.json
diff --git a/tests/qapi-schema/base-cycle-direct.err b/tests/qapi-schema/base-cycle-direct.err
new file mode 100644
index 0000000..9c68f65
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-direct.err
@@ -0,0 +1 @@
+tests/qapi-schema/base-cycle-direct.json:2: Object Loopy contains itself
diff --git a/tests/qapi-schema/base-cycle-direct.exit b/tests/qapi-schema/base-cycle-direct.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-direct.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/base-cycle-direct.json b/tests/qapi-schema/base-cycle-direct.json
new file mode 100644
index 0000000..4fc66d0
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-direct.json
@@ -0,0 +1,2 @@
+# we reject a loop in base classes
+{ 'struct': 'Loopy', 'base': 'Loopy', 'data': {} }
diff --git a/tests/qapi-schema/base-cycle-direct.out b/tests/qapi-schema/base-cycle-direct.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/base-cycle-indirect.err b/tests/qapi-schema/base-cycle-indirect.err
new file mode 100644
index 0000000..fc92fe4
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-indirect.err
@@ -0,0 +1 @@
+tests/qapi-schema/base-cycle-indirect.json:2: Object Base1 contains itself
diff --git a/tests/qapi-schema/base-cycle-indirect.exit b/tests/qapi-schema/base-cycle-indirect.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-indirect.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/base-cycle-indirect.json b/tests/qapi-schema/base-cycle-indirect.json
new file mode 100644
index 0000000..2866772
--- /dev/null
+++ b/tests/qapi-schema/base-cycle-indirect.json
@@ -0,0 +1,3 @@
+# we reject a loop in base classes
+{ 'struct': 'Base1', 'base': 'Base2', 'data': {} }
+{ 'struct': 'Base2', 'base': 'Base1', 'data': {} }
diff --git a/tests/qapi-schema/base-cycle-indirect.out b/tests/qapi-schema/base-cycle-indirect.out
new file mode 100644
index 0000000..e69de29
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject Eric Blake
@ 2015-11-26 14:27 ` Markus Armbruster
2015-11-26 15:06 ` Markus Armbruster
1 sibling, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2015-11-26 14:27 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Luiz Capitulino
Eric Blake <eblake@redhat.com> writes:
> The QObject hierarchy is small enough, and unlikely to grow further
> (since we only use it to map to JSON and already cover all JSON
> types), that we can simplify things by not tracking a separate
> vtable, but just inline the refcnt element of the vtable QType
> directly into QObject, and track a separate array of destroy
> functions. We can drop qnull_destroy_obj() in the process.
>
> The remaining QObject subclasses must export their destructor.
>
> This also has the nice benefit of moving the typename 'QType'
> out of the way, so that the next patch can repurpose it for a
> nicer name for 'qtype_code'.
>
> The various objects are still the same size (so no change in cache
> line pressure), but now have less indirection (although I didn't
> bother benchmarking to see if there is a noticeable speedup, as
> we don't have hard evidence that this was in a performance hotspot
> in the first place).
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
[...]
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 4b96ed5..7e5b9b1 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -47,21 +47,20 @@ typedef enum {
> QTYPE_MAX,
> } qtype_code;
>
> -struct QObject;
> -
> -typedef struct QType {
> - qtype_code code;
> - void (*destroy)(struct QObject *);
> -} QType;
> -
> typedef struct QObject {
> - const QType *type;
> + qtype_code type;
> size_t refcnt;
> } QObject;
Future opportunity: uint32_t refcnt should be more than enough. Halves
the size to eight bytes.
> +typedef void (*QDestroy)(QObject *);
> +
Typedef used exactly once, in qobject.c. Let's drop it.
> /* Get the 'base' part of an object */
> #define QOBJECT(obj) (&(obj)->base)
>
> +/* High-level interface for qobject_init() */
> +#define QOBJECT_INIT(obj, type) \
> + qobject_init(QOBJECT(obj), type)
> +
Not really high-level; it's used only internally, and its users are the
high-level constructors.
I wouldn't mind dropping the macro.
> /* High-level interface for qobject_incref() */
> #define QINCREF(obj) \
> qobject_incref(QOBJECT(obj))
> @@ -71,9 +70,12 @@ typedef struct QObject {
> qobject_decref(obj ? QOBJECT(obj) : NULL)
>
> /* Initialize an object to default values */
> -#define QOBJECT_INIT(obj, qtype_type) \
> - obj->base.refcnt = 1; \
> - obj->base.type = qtype_type
> +static inline void qobject_init(QObject *obj, qtype_code type)
> +{
> + assert(type);
QTYPE_NONE < type && type < QTYPE_MAX would be tighter.
Aside: I dislike our use of _MAX as the last enum. _MAX should be the
largest *valid* value (see: INT_MAX), not one more.
> + obj->refcnt = 1;
> + obj->type = type;
> +}
>
> /**
> * qobject_incref(): Increment QObject's reference count
> @@ -85,6 +87,11 @@ static inline void qobject_incref(QObject *obj)
> }
>
> /**
> + * qobject_destroy(): Free resources used by the object
> + */
> +void qobject_destroy(QObject *obj);
> +
> +/**
> * qobject_decref(): Decrement QObject's reference count, deallocate
> * when it reaches zero
> */
> @@ -92,9 +99,7 @@ static inline void qobject_decref(QObject *obj)
> {
> assert(!obj || obj->refcnt);
> if (obj && --obj->refcnt == 0) {
> - assert(obj->type != NULL);
> - assert(obj->type->destroy != NULL);
> - obj->type->destroy(obj);
> + qobject_destroy(obj);
> }
> }
>
> @@ -103,8 +108,8 @@ static inline void qobject_decref(QObject *obj)
> */
> static inline qtype_code qobject_type(const QObject *obj)
> {
> - assert(obj->type != NULL);
> - return obj->type->code;
> + assert(obj->type);
Likewise.
> + return obj->type;
> }
>
> extern QObject qnull_;
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index 34675a7..df7df55 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -32,5 +32,6 @@ void qstring_append_int(QString *qstring, int64_t value);
> void qstring_append(QString *qstring, const char *str);
> void qstring_append_chr(QString *qstring, int c);
> QString *qobject_to_qstring(const QObject *obj);
> +void qstring_destroy_obj(QObject *obj);
>
> #endif /* QSTRING_H */
> diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
> index 0031e8b..bed5508 100644
> --- a/qobject/Makefile.objs
> +++ b/qobject/Makefile.objs
> @@ -1,2 +1,2 @@
> util-obj-y = qnull.o qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
> -util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
> +util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
> diff --git a/qobject/qbool.c b/qobject/qbool.c
> index bc6535f..895fd4d 100644
> --- a/qobject/qbool.c
> +++ b/qobject/qbool.c
> @@ -15,13 +15,6 @@
> #include "qapi/qmp/qobject.h"
> #include "qemu-common.h"
>
> -static void qbool_destroy_obj(QObject *obj);
> -
> -static const QType qbool_type = {
> - .code = QTYPE_QBOOL,
> - .destroy = qbool_destroy_obj,
> -};
> -
> /**
> * qbool_from_bool(): Create a new QBool from a bool
> *
> @@ -33,7 +26,7 @@ QBool *qbool_from_bool(bool value)
>
> qb = g_malloc(sizeof(*qb));
> qb->value = value;
> - QOBJECT_INIT(qb, &qbool_type);
> + QOBJECT_INIT(qb, QTYPE_QBOOL);
Good opportunity to clean up the order: init goes first.
>
> return qb;
> }
> @@ -61,7 +54,7 @@ QBool *qobject_to_qbool(const QObject *obj)
> * qbool_destroy_obj(): Free all memory allocated by a
> * QBool object
> */
> -static void qbool_destroy_obj(QObject *obj)
> +void qbool_destroy_obj(QObject *obj)
> {
> assert(obj != NULL);
> g_free(qobject_to_qbool(obj));
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 2d67bf1..dd3f1c2 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -19,13 +19,6 @@
> #include "qemu/queue.h"
> #include "qemu-common.h"
>
> -static void qdict_destroy_obj(QObject *obj);
> -
> -static const QType qdict_type = {
> - .code = QTYPE_QDICT,
> - .destroy = qdict_destroy_obj,
> -};
> -
> /**
> * qdict_new(): Create a new QDict
> *
> @@ -36,7 +29,7 @@ QDict *qdict_new(void)
> QDict *qdict;
>
> qdict = g_malloc0(sizeof(*qdict));
> - QOBJECT_INIT(qdict, &qdict_type);
> + QOBJECT_INIT(qdict, QTYPE_QDICT);
>
> return qdict;
> }
> @@ -441,7 +434,7 @@ void qdict_del(QDict *qdict, const char *key)
> /**
> * qdict_destroy_obj(): Free all the memory allocated by a QDict
> */
> -static void qdict_destroy_obj(QObject *obj)
> +void qdict_destroy_obj(QObject *obj)
> {
> int i;
> QDict *qdict;
> diff --git a/qobject/qfloat.c b/qobject/qfloat.c
> index c865163..1dffb54 100644
> --- a/qobject/qfloat.c
> +++ b/qobject/qfloat.c
> @@ -15,13 +15,6 @@
> #include "qapi/qmp/qobject.h"
> #include "qemu-common.h"
>
> -static void qfloat_destroy_obj(QObject *obj);
> -
> -static const QType qfloat_type = {
> - .code = QTYPE_QFLOAT,
> - .destroy = qfloat_destroy_obj,
> -};
> -
> /**
> * qfloat_from_int(): Create a new QFloat from a float
> *
> @@ -33,7 +26,7 @@ QFloat *qfloat_from_double(double value)
>
> qf = g_malloc(sizeof(*qf));
> qf->value = value;
> - QOBJECT_INIT(qf, &qfloat_type);
> + QOBJECT_INIT(qf, QTYPE_QFLOAT);
Likewise.
>
> return qf;
> }
> @@ -61,7 +54,7 @@ QFloat *qobject_to_qfloat(const QObject *obj)
> * qfloat_destroy_obj(): Free all memory allocated by a
> * QFloat object
> */
> -static void qfloat_destroy_obj(QObject *obj)
> +void qfloat_destroy_obj(QObject *obj)
> {
> assert(obj != NULL);
> g_free(qobject_to_qfloat(obj));
> diff --git a/qobject/qint.c b/qobject/qint.c
> index 999688e..112ee68 100644
> --- a/qobject/qint.c
> +++ b/qobject/qint.c
> @@ -14,13 +14,6 @@
> #include "qapi/qmp/qobject.h"
> #include "qemu-common.h"
>
> -static void qint_destroy_obj(QObject *obj);
> -
> -static const QType qint_type = {
> - .code = QTYPE_QINT,
> - .destroy = qint_destroy_obj,
> -};
> -
> /**
> * qint_from_int(): Create a new QInt from an int64_t
> *
> @@ -32,7 +25,7 @@ QInt *qint_from_int(int64_t value)
>
> qi = g_malloc(sizeof(*qi));
> qi->value = value;
> - QOBJECT_INIT(qi, &qint_type);
> + QOBJECT_INIT(qi, QTYPE_QINT);
Likewise.
>
> return qi;
> }
> @@ -60,7 +53,7 @@ QInt *qobject_to_qint(const QObject *obj)
> * qint_destroy_obj(): Free all memory allocated by a
> * QInt object
> */
> -static void qint_destroy_obj(QObject *obj)
> +void qint_destroy_obj(QObject *obj)
> {
> assert(obj != NULL);
> g_free(qobject_to_qint(obj));
> diff --git a/qobject/qlist.c b/qobject/qlist.c
> index 298003a..28be4f6 100644
> --- a/qobject/qlist.c
> +++ b/qobject/qlist.c
> @@ -15,13 +15,6 @@
> #include "qemu/queue.h"
> #include "qemu-common.h"
>
> -static void qlist_destroy_obj(QObject *obj);
> -
> -static const QType qlist_type = {
> - .code = QTYPE_QLIST,
> - .destroy = qlist_destroy_obj,
> -};
> -
> /**
> * qlist_new(): Create a new QList
> *
> @@ -33,7 +26,7 @@ QList *qlist_new(void)
>
> qlist = g_malloc(sizeof(*qlist));
> QTAILQ_INIT(&qlist->head);
> - QOBJECT_INIT(qlist, &qlist_type);
> + QOBJECT_INIT(qlist, QTYPE_QLIST);
>
> return qlist;
> }
> @@ -151,7 +144,7 @@ QList *qobject_to_qlist(const QObject *obj)
> /**
> * qlist_destroy_obj(): Free all the memory allocated by a QList
> */
> -static void qlist_destroy_obj(QObject *obj)
> +void qlist_destroy_obj(QObject *obj)
> {
> QList *qlist;
> QListEntry *entry, *next_entry;
> diff --git a/qobject/qnull.c b/qobject/qnull.c
> index 9873e26..5f7ba4d 100644
> --- a/qobject/qnull.c
> +++ b/qobject/qnull.c
> @@ -13,17 +13,7 @@
> #include "qemu-common.h"
> #include "qapi/qmp/qobject.h"
>
> -static void qnull_destroy_obj(QObject *obj)
> -{
> - assert(0);
> -}
> -
> -static const QType qnull_type = {
> - .code = QTYPE_QNULL,
> - .destroy = qnull_destroy_obj,
> -};
> -
> QObject qnull_ = {
> - .type = &qnull_type,
> + .type = QTYPE_QNULL,
> .refcnt = 1,
> };
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> new file mode 100644
> index 0000000..5325447
> --- /dev/null
> +++ b/qobject/qobject.c
> @@ -0,0 +1,34 @@
> +/*
> + * QObject
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1
> + * or later. See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include "qemu-common.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qfloat.h"
> +#include "qapi/qmp/qint.h"
> +#include "qapi/qmp/qlist.h"
> +#include "qapi/qmp/qstring.h"
> +
> +static QDestroy qdestroy[QTYPE_MAX] = {
> + [QTYPE_NONE] = NULL, /* No such object exists */
> + [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
I'd prefer to have the comments off to the right, like this:
+ [QTYPE_NONE] = NULL, /* No such object exists */
+ [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
> + [QTYPE_QINT] = qint_destroy_obj,
> + [QTYPE_QSTRING] = qstring_destroy_obj,
> + [QTYPE_QDICT] = qdict_destroy_obj,
> + [QTYPE_QLIST] = qlist_destroy_obj,
> + [QTYPE_QFLOAT] = qfloat_destroy_obj,
> + [QTYPE_QBOOL] = qbool_destroy_obj,
> +};
> +
> +void qobject_destroy(QObject *obj)
> +{
> + assert(!obj->refcnt);
> + assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
> + qdestroy[obj->type](obj);
> +}
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index cb72dfb..a2f5903 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -14,13 +14,6 @@
> #include "qapi/qmp/qstring.h"
> #include "qemu-common.h"
>
> -static void qstring_destroy_obj(QObject *obj);
> -
> -static const QType qstring_type = {
> - .code = QTYPE_QSTRING,
> - .destroy = qstring_destroy_obj,
> -};
> -
> /**
> * qstring_new(): Create a new empty QString
> *
> @@ -57,7 +50,7 @@ QString *qstring_from_substr(const char *str, int start, int end)
> memcpy(qstring->string, str + start, qstring->length);
> qstring->string[qstring->length] = 0;
>
> - QOBJECT_INIT(qstring, &qstring_type);
> + QOBJECT_INIT(qstring, QTYPE_QSTRING);
Another "init goes first".
>
> return qstring;
> }
> @@ -138,7 +131,7 @@ const char *qstring_get_str(const QString *qstring)
> * qstring_destroy_obj(): Free all memory allocated by a QString
> * object
> */
> -static void qstring_destroy_obj(QObject *obj)
> +void qstring_destroy_obj(QObject *obj)
> {
> QString *qs;
Patch is basically fine.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type Eric Blake
@ 2015-11-26 14:51 ` Markus Armbruster
2015-11-27 5:09 ` Eric Blake
0 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-11-26 14:51 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, qemu-devel, Luiz Capitulino
Eric Blake <eblake@redhat.com> writes:
> What's more meta than using qapi to define qapi? :)
>
> Convert QType into a full-fledged[*] builtin qapi enum type, so
> that a subsequent patch can then use it as the discriminator
> type of qapi alternate types. Fortunately, the judicious use of
> 'prefix' in the qapi definition avoids churn to the spelling of
> the enum constants.
>
> To avoid circular definitions, we have to flip the order of
> inclusion between "qobject.h" vs. "qapi-types.h". Back in commit
> 28770e0, we had the latter include the former, so that we could
> use 'QObject *' for our implementation of 'any'. But that usage
> also works with only a forward declaration, whereas the
> definition of QObject requires QType to be a complete type.
>
> [*] The type has to be builtin, rather than declared in
> qapi/common.json, because we want to use it for alternates even
> when common.json is not included. But since it is the first
> builtin enum type, we have to add special cases to qapi-types
> and qapi-visit to only emit definitions once, even when two
> qapi files are being compiled into the same binary (the way we
> already handled builtin list types like 'intList'). We may
> need to revisit how multiple qapi files share common types,
> but that's a project for another day.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v13: change title, use typedefs.h, rebase to cleaner QObject,
> rebase to Markus' typedefs.h re-sort
> v12: split out preparatory renames, retitle patch, use info rather
> than name comparison
> v11: new patch
> ---
> docs/qapi-code-gen.txt | 1 +
> include/qapi/qmp/qobject.h | 17 +++--------------
> include/qemu/typedefs.h | 1 +
> qobject/qobject.c | 4 ++--
> scripts/qapi-types.py | 15 +++++++++++----
> scripts/qapi-visit.py | 11 +++++++++--
> scripts/qapi.py | 6 ++++++
> tests/qapi-schema/alternate-empty.out | 2 ++
> tests/qapi-schema/comments.out | 2 ++
> tests/qapi-schema/empty.out | 2 ++
> tests/qapi-schema/event-case.out | 2 ++
> tests/qapi-schema/flat-union-empty.out | 2 ++
> tests/qapi-schema/ident-with-escape.out | 2 ++
> tests/qapi-schema/include-relpath.out | 2 ++
> tests/qapi-schema/include-repetition.out | 2 ++
> tests/qapi-schema/include-simple.out | 2 ++
> tests/qapi-schema/indented-expr.out | 2 ++
> tests/qapi-schema/qapi-schema-test.out | 2 ++
> tests/qapi-schema/union-clash-data.out | 2 ++
> tests/qapi-schema/union-empty.out | 2 ++
> 20 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 2becba9..79bf072 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -160,6 +160,7 @@ The following types are predefined, and map to C as follows:
> accepts size suffixes
> bool bool JSON true or false
> any QObject * any JSON value
> + QType QType JSON string matching enum QType values
>
>
> === Includes ===
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 525fb39..295aa76 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -34,23 +34,12 @@
>
> #include <stddef.h>
> #include <assert.h>
> +#include "qapi-types.h"
Needed for QType. Risk for circular inclusion. We're currently fine,
because generated qapi-types.h includes only "qemu/typedefs.h" (visible
below). Should we add a comment to qapi-types.py?
>
> -typedef enum {
> - QTYPE_NONE, /* sentinel value, no QObject has this type code */
> - QTYPE_QNULL,
> - QTYPE_QINT,
> - QTYPE_QSTRING,
> - QTYPE_QDICT,
> - QTYPE_QLIST,
> - QTYPE_QFLOAT,
> - QTYPE_QBOOL,
> - QTYPE_MAX,
> -} QType;
> -
> -typedef struct QObject {
> +struct QObject {
> QType type;
> size_t refcnt;
> -} QObject;
> +};
>
> typedef void (*QDestroy)(QObject *);
>
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 3eedcf4..78fe6e8 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -80,6 +80,7 @@ typedef struct QEMUSGList QEMUSGList;
> typedef struct QEMUSizedBuffer QEMUSizedBuffer;
> typedef struct QEMUTimer QEMUTimer;
> typedef struct QEMUTimerListGroup QEMUTimerListGroup;
> +typedef struct QObject QObject;
> typedef struct RAMBlock RAMBlock;
> typedef struct Range Range;
> typedef struct SerialState SerialState;
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> index 5325447..d0eb7f1 100644
> --- a/qobject/qobject.c
> +++ b/qobject/qobject.c
> @@ -15,7 +15,7 @@
> #include "qapi/qmp/qlist.h"
> #include "qapi/qmp/qstring.h"
>
> -static QDestroy qdestroy[QTYPE_MAX] = {
> +static QDestroy qdestroy[QTYPE__MAX] = {
> [QTYPE_NONE] = NULL, /* No such object exists */
> [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
> [QTYPE_QINT] = qint_destroy_obj,
> @@ -29,6 +29,6 @@ static QDestroy qdestroy[QTYPE_MAX] = {
> void qobject_destroy(QObject *obj)
> {
> assert(!obj->refcnt);
> - assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
> + assert(QTYPE_QNULL < obj->type && obj->type < QTYPE__MAX);
> qdestroy[obj->type](obj);
> }
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2f2f7df..82635b0 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -112,7 +112,7 @@ extern const int %(c_name)s_qtypes[];
> def gen_alternate_qtypes(name, variants):
> ret = mcgen('''
>
> -const int %(c_name)s_qtypes[QTYPE_MAX] = {
> +const int %(c_name)s_qtypes[QTYPE__MAX] = {
> ''',
> c_name=c_name(name))
>
> @@ -233,8 +233,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self.defn += gen_type_cleanup(name)
>
> def visit_enum_type(self, name, info, values, prefix):
> - self._fwdecl += gen_enum(name, values, prefix)
> - self._fwdefn += gen_enum_lookup(name, values, prefix)
> + # Special case for our lone builtin enum type
> + # TODO use something cleaner than existence of info
> + if not info:
> + self._btin += gen_enum(name, values, prefix)
> + if do_builtins:
> + self.defn += gen_enum_lookup(name, values, prefix)
> + else:
> + self._fwdecl += gen_enum(name, values, prefix)
> + self._fwdefn += gen_enum_lookup(name, values, prefix)
Odd: gen_enum_lookup() goes into .defn for built-ins, but ._fwdefn for
user-defineds. Makes me suspect it ._fwdefn isn't needed anymore. A
quick test compile is happy with .defn for both.
If we want to keep ._fwdefn for some reason, we should use for built-ins
as well.
>
> def visit_array_type(self, name, info, element_type):
> if isinstance(element_type, QAPISchemaBuiltinType):
> @@ -319,7 +326,7 @@ fdef.write(mcgen('''
> fdecl.write(mcgen('''
> #include <stdbool.h>
> #include <stdint.h>
> -#include "qapi/qmp/qobject.h"
> +#include "qemu/typedefs.h"
> '''))
>
Here you can see the #include generated into qapi-types.h.
> schema = QAPISchema(input_file)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 94cd113..7ceda18 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -347,8 +347,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> isinstance(entity, QAPISchemaObjectType))
>
> def visit_enum_type(self, name, info, values, prefix):
> - self.decl += gen_visit_decl(name, scalar=True)
> - self.defn += gen_visit_enum(name)
> + # Special case for our lone builtin enum type
> + # TODO use something cleaner than existence of info
> + if not info:
> + self._btin += gen_visit_decl(name, scalar=True)
> + if do_builtins:
> + self.defn += gen_visit_enum(name)
> + else:
> + self.decl += gen_visit_decl(name, scalar=True)
> + self.defn += gen_visit_enum(name)
>
> def visit_array_type(self, name, info, element_type):
> decl = gen_visit_decl(name)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 9fe9c95..3c8cb13 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -34,6 +34,7 @@ builtin_types = {
> 'uint64': 'QTYPE_QINT',
> 'size': 'QTYPE_QINT',
> 'any': None, # any QType possible, actually
> + 'QType': 'QTYPE_QSTRING',
> }
>
> # Whitelist of commands allowed to return a non-dictionary
> @@ -1243,6 +1244,11 @@ class QAPISchema(object):
> self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
> [], None)
> self._def_entity(self.the_empty_object_type)
> + self._def_entity(QAPISchemaEnumType('QType', None,
> + ['none', 'qnull', 'qint',
> + 'qstring', 'qdict', 'qlist',
> + 'qfloat', 'qbool'],
> + 'QTYPE'))
>
> def _make_implicit_enum_type(self, name, info, values):
> name = name + 'Kind' # Use namespace reserved by add_name()
[...]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject Eric Blake
2015-11-26 14:27 ` Markus Armbruster
@ 2015-11-26 15:06 ` Markus Armbruster
1 sibling, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2015-11-26 15:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Luiz Capitulino
Eric Blake <eblake@redhat.com> writes:
> The QObject hierarchy is small enough, and unlikely to grow further
> (since we only use it to map to JSON and already cover all JSON
> types), that we can simplify things by not tracking a separate
> vtable, but just inline the refcnt element of the vtable QType
> directly into QObject, and track a separate array of destroy
> functions. We can drop qnull_destroy_obj() in the process.
Make that something like "inline the code element of the vtable QType
directly into QObject (renamed to type), and".
>
> The remaining QObject subclasses must export their destructor.
>
> This also has the nice benefit of moving the typename 'QType'
> out of the way, so that the next patch can repurpose it for a
> nicer name for 'qtype_code'.
>
> The various objects are still the same size (so no change in cache
> line pressure), but now have less indirection (although I didn't
> bother benchmarking to see if there is a noticeable speedup, as
> we don't have hard evidence that this was in a performance hotspot
> in the first place).
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
@ 2015-11-26 16:29 ` Markus Armbruster
0 siblings, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2015-11-26 16:29 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Rather than using just an array of strings, make enum.values be
> an array of the new QAPISchemaMember type. Creating an enum
> requires wrapping strings, and visiting an enum requires getting
> at the name of each value. But using this type means we can
> share the existing code for C name clash detection (although the
> code is not yet active until a later commit removes the earlier
> ad hoc parser checks). The ._pretty_owner() method needs to
> learn about one more implicit type name: the generated enum
> associated with a simple union.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v13: new patch
> ---
> scripts/qapi.py | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2748464..d1239c2 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -901,13 +901,17 @@ class QAPISchemaEnumType(QAPISchemaType):
> def __init__(self, name, info, values, prefix):
> QAPISchemaType.__init__(self, name, info)
> for v in values:
> - assert isinstance(v, str)
> + assert isinstance(v, QAPISchemaMember)
> + v.set_owner(name)
> assert prefix is None or isinstance(prefix, str)
> self.values = values
> self.prefix = prefix
>
> def check(self, schema):
> - assert len(set(self.values)) == len(self.values)
> + # Check for collisions on the generated C enum values
> + seen = {}
> + for v in self.values:
> + v.check_clash(self.info, seen)
Is the comment worth its keep? We have similar loops elsewhere, without
such a comment.
>
> def is_implicit(self):
> # See QAPISchema._make_implicit_enum_type()
> @@ -917,15 +921,18 @@ class QAPISchemaEnumType(QAPISchemaType):
> return c_name(self.name)
>
> def c_null(self):
> - return c_enum_const(self.name, (self.values + ['_MAX'])[0],
> - self.prefix)
> + if self.values:
> + value = self.values[0].name
> + else:
> + value = '_MAX'
> + return c_enum_const(self.name, value, self.prefix)
Works. The alternative would be casting 0 to the enumeration type.
>
> def json_type(self):
> return 'string'
>
> def visit(self, visitor):
> visitor.visit_enum_type(self.name, self.info,
> - self.values, self.prefix)
> + [v.name for v in self.values], self.prefix)
This keeps the change local.
The alternative is changing visit_enum_type(). Precedence:
visit_object_type() takes an array of QAPISchemaObjectTypeMember. Could
also be done later if we find we want the consistency after all.
>
>
> class QAPISchemaArrayType(QAPISchemaType):
> @@ -1049,6 +1056,8 @@ class QAPISchemaMember(object):
> else:
> assert owner.endswith('-wrapper')
> return '(branch of %s)' % owner[:-8]
> + if owner.endswith('Kind'):
> + return '(branch of %s)' % owner[:-4]
> return '(%s of %s)' % (self.role, owner)
Let's add a comment pointing to _make_implicit_enum_type(), similar to
the existing one above that points to _make_implicit_object_type().
>
> def describe(self):
> @@ -1094,12 +1103,13 @@ class QAPISchemaObjectTypeVariants(object):
> self.tag_member = seen[c_name(self.tag_name)]
> assert self.tag_name == self.tag_member.name
> assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> + tag_values = [v.name for v in self.tag_member.type.values]
> for v in self.variants:
> v.check(schema)
> # Union names must match enum values; alternate names are
> # checked separately. Use 'seen' to tell the two apart.
> if seen:
> - assert v.name in self.tag_member.type.values
> + assert v.name in tag_values
Could search self.tag_member.type.values directly instead, but I can't
find an way to express it that can compete with your code in
readability.
> assert isinstance(v.type, QAPISchemaObjectType)
> v.type.check(schema)
>
> @@ -1257,15 +1267,16 @@ class QAPISchema(object):
> self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
> [], None)
> self._def_entity(self.the_empty_object_type)
> - self._def_entity(QAPISchemaEnumType('QType', None,
> - ['none', 'qnull', 'qint',
> - 'qstring', 'qdict', 'qlist',
> - 'qfloat', 'qbool'],
> + qtype_values = [QAPISchemaMember(name) for name in
> + ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist',
> + 'qfloat', 'qbool']]
Let's wrap after 'qdict'.
> + self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
> 'QTYPE'))
>
> def _make_implicit_enum_type(self, name, info, values):
> name = name + 'Kind' # Use namespace reserved by add_name()
> - self._def_entity(QAPISchemaEnumType(name, info, values, None))
> + self._def_entity(QAPISchemaEnumType(
> + name, info, [QAPISchemaMember(v) for v in values], None))
> return name
>
> def _make_array_type(self, element_type, info):
> @@ -1288,7 +1299,8 @@ class QAPISchema(object):
> name = expr['enum']
> data = expr['data']
> prefix = expr.get('prefix')
> - self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
> + self._def_entity(QAPISchemaEnumType(
> + name, info, [QAPISchemaMember(v) for v in data], prefix))
Third instance of the list comprehension mapping a list of names to a
list of QAPISchemaMember. Would a helper function make sense?
>
> def _make_member(self, name, typ, info):
> optional = False
I'm generally reluctant to add classes, but this one seems to pan out
nicely.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity Eric Blake
@ 2015-11-26 16:46 ` Markus Armbruster
0 siblings, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2015-11-26 16:46 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Every non-implicit entity is associated with an info dictionary,
> but it is not easy to reverse-engineer the name of the top-most
> entity associated with that info. Add a new info['name'] field
> to track this information, as it will be handy in future commits
> for better error messages.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> scripts/qapi.py | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index d1239c2..ff3fccb 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1296,7 +1296,7 @@ class QAPISchema(object):
> return name
>
> def _def_enum_type(self, expr, info):
> - name = expr['enum']
> + info['name'] = name = expr['enum']
> data = expr['data']
> prefix = expr.get('prefix')
> self._def_entity(QAPISchemaEnumType(
> @@ -1317,7 +1317,7 @@ class QAPISchema(object):
> for (key, value) in data.iteritems()]
>
> def _def_struct_type(self, expr, info):
> - name = expr['struct']
> + info['name'] = name = expr['struct']
> base = expr.get('base')
> data = expr['data']
> self._def_entity(QAPISchemaObjectType(name, info, base,
> @@ -1336,7 +1336,7 @@ class QAPISchema(object):
> return QAPISchemaObjectTypeVariant(case, typ)
>
> def _def_union_type(self, expr, info):
> - name = expr['union']
> + info['name'] = name = expr['union']
> data = expr['data']
> base = expr.get('base')
> tag_name = expr.get('discriminator')
> @@ -1359,7 +1359,7 @@ class QAPISchema(object):
> variants)))
>
> def _def_alternate_type(self, expr, info):
> - name = expr['alternate']
> + info['name'] = name = expr['alternate']
> data = expr['data']
> variants = [self._make_variant(key, value)
> for (key, value) in data.iteritems()]
> @@ -1371,7 +1371,7 @@ class QAPISchema(object):
> variants)))
>
> def _def_command(self, expr, info):
> - name = expr['command']
> + info['name'] = name = expr['command']
> data = expr.get('data')
> rets = expr.get('returns')
> gen = expr.get('gen', True)
> @@ -1386,7 +1386,7 @@ class QAPISchema(object):
> success_response))
>
> def _def_event(self, expr, info):
> - name = expr['event']
> + info['name'] = name = expr['event']
> data = expr.get('data')
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
There's no single place where we can do that?
info is created in QAPISchemaParser.__init__()'s loop:
expr_info = {'file': fname, 'line': self.line,
'parent': self.incl_info}
We obviously don't know the name there.
It's stored in QAPISchemaParser.exprs together with the expression
dictionary:
expr_elem = {'expr': expr,
'info': expr_info}
self.exprs.append(expr_elem)
QAPISchema.__init__() filters it through check_exprs()
self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
but check_exprs() should go away.
That leaves _def_exprs(). It currently can't, because the _def_FOO()
don't return anything, unlike the _make_BAR().
Your solution might be the simplest one... Let's see how much use the
later commits get out of it.
However, I'd prefer
name = info['name'] = expr['event']
because that makes it easier to see the definition of name, which is the
one that gets used further down in the functions.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type
2015-11-26 14:51 ` Markus Armbruster
@ 2015-11-27 5:09 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-11-27 5:09 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, qemu-devel, Luiz Capitulino
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
On 11/26/2015 07:51 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> What's more meta than using qapi to define qapi? :)
>>
>> Convert QType into a full-fledged[*] builtin qapi enum type, so
>> that a subsequent patch can then use it as the discriminator
>> type of qapi alternate types. Fortunately, the judicious use of
>> 'prefix' in the qapi definition avoids churn to the spelling of
>> the enum constants.
>>
>> #include <stddef.h>
>> #include <assert.h>
>> +#include "qapi-types.h"
>
> Needed for QType. Risk for circular inclusion. We're currently fine,
> because generated qapi-types.h includes only "qemu/typedefs.h" (visible
> below). Should we add a comment to qapi-types.py?
How about this:
@@ -323,6 +319,7 @@ fdef.write(mcgen('''
''',
prefix=prefix))
+# To avoid circular headers, use only typedefs.h here, not qobject.h
fdecl.write(mcgen('''
#include <stdbool.h>
#include <stdint.h>
-#include "qapi/qmp/qobject.h"
+#include "qemu/typedefs.h"
'''))
schema = QAPISchema(input_file)
>> def visit_enum_type(self, name, info, values, prefix):
>> - self._fwdecl += gen_enum(name, values, prefix)
>> - self._fwdefn += gen_enum_lookup(name, values, prefix)
>> + # Special case for our lone builtin enum type
>> + # TODO use something cleaner than existence of info
>> + if not info:
>> + self._btin += gen_enum(name, values, prefix)
>> + if do_builtins:
>> + self.defn += gen_enum_lookup(name, values, prefix)
>> + else:
>> + self._fwdecl += gen_enum(name, values, prefix)
>> + self._fwdefn += gen_enum_lookup(name, values, prefix)
>
> Odd: gen_enum_lookup() goes into .defn for built-ins, but ._fwdefn for
> user-defineds. Makes me suspect it ._fwdefn isn't needed anymore. A
> quick test compile is happy with .defn for both.
>
> If we want to keep ._fwdefn for some reason, we should use for built-ins
> as well.
We need it for gen_alternate_qtypes(). But that disappears in 4/14, so
I'll add a patch to clean it up.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
@ 2015-11-27 9:03 ` Markus Armbruster
2015-12-01 22:35 ` Eric Blake
2015-11-27 9:42 ` Markus Armbruster
1 sibling, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-11-27 9:03 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> We document that members of enums and objects should be
> 'lower-case', although we were not enforcing it. We have to
> whitelist a few pre-existing entities that violate the norms.
> Add three new tests to expose the new error message, each of
> which first uses the whitelisted name 'UuidInfo' to prove the
> whitelist works, then triggers the failure.
I guess a whitelist is the simplest solution that could possibly work.
Moreover, it matches the existing solution for allowing non-dictionary
returns. Drawback: they apply to all schemata. Good enough at least
for now.
> Note that by adding this check, we have effectively forbidden
> an entity with a case-insensitive clash of member names, for
> any entity that is not on the whitelist (although there is
> still the possibility to clash via '-' vs. '_').
Yes.
Still to be done: enforcing entity naming conventions.
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> scripts/qapi.py | 19 +++++++++++++++++++
> tests/Makefile | 3 +++
> tests/qapi-schema/args-member-case.err | 1 +
> tests/qapi-schema/args-member-case.exit | 1 +
> tests/qapi-schema/args-member-case.json | 3 +++
> tests/qapi-schema/args-member-case.out | 0
> tests/qapi-schema/enum-member-case.err | 1 +
> tests/qapi-schema/enum-member-case.exit | 1 +
> tests/qapi-schema/enum-member-case.json | 3 +++
> tests/qapi-schema/enum-member-case.out | 0
> tests/qapi-schema/union-branch-case.err | 1 +
> tests/qapi-schema/union-branch-case.exit | 1 +
> tests/qapi-schema/union-branch-case.json | 3 +++
> tests/qapi-schema/union-branch-case.out | 0
> 14 files changed, 37 insertions(+)
> create mode 100644 tests/qapi-schema/args-member-case.err
> create mode 100644 tests/qapi-schema/args-member-case.exit
> create mode 100644 tests/qapi-schema/args-member-case.json
> create mode 100644 tests/qapi-schema/args-member-case.out
> create mode 100644 tests/qapi-schema/enum-member-case.err
> create mode 100644 tests/qapi-schema/enum-member-case.exit
> create mode 100644 tests/qapi-schema/enum-member-case.json
> create mode 100644 tests/qapi-schema/enum-member-case.out
> create mode 100644 tests/qapi-schema/union-branch-case.err
> create mode 100644 tests/qapi-schema/union-branch-case.exit
> create mode 100644 tests/qapi-schema/union-branch-case.json
> create mode 100644 tests/qapi-schema/union-branch-case.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index ff3fccb..00eb43e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -59,6 +59,21 @@ returns_whitelist = [
> 'guest-sync-delimited',
> ]
>
> +# Whitelist of entities allowed to violate case conventions
> +case_whitelist = [
Double-checking for accuracy:
> + # From QMP:
> + 'ACPISlotType',
Because of enum member DIMM.
Visible in event ACPI_DEVICE_OST and command query-acpi-ospm-status,
both since 2.1.
> + 'CpuInfo',
Because of CpuInfoBase.
Visible in command query-cpus since 0.14.
> + 'CpuInfoBase',
Because of struct member CPU.
Visible since v1.0.
> + 'CpuInfoMIPS',
> + 'CpuInfoTricore',
Because of struct member PC.
Visible since v1.0 and v2.2.
> + 'InputAxis',
> + 'InputButton',
Because of all enum members,
Visible in x-input-send-event since 2.0. To be fixed when
x-input-send-event loses x-. TODO comment there?
> + 'QapiErrorClass',
Because of all enum members.
Visible in error replies since forever.
> + 'UuidInfo',
Because of struct member UUID.
Visible in command query-uuid since 0.14.
> + 'X86CPURegister32',
Because of all enum members.
*Not* visible in QMP, thus fixable. Fix or TODO comment, please.
> +]
> +
> enum_types = []
> struct_types = []
> union_types = []
> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
>
> def check_clash(self, info, seen):
> cname = c_name(self.name)
> + if cname.lower() != cname and info['name'] not in case_whitelist:
> + raise QAPIExprError(info,
> + "Member '%s' of '%s' should use lowercase"
> + % (self.name, info['name']))
> if cname in seen:
> raise QAPIExprError(info,
> "%s collides with %s"
> diff --git a/tests/Makefile b/tests/Makefile
> index e377c70..ca386e9 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -246,6 +246,7 @@ qapi-schema += args-array-unknown.json
> qapi-schema += args-int.json
> qapi-schema += args-invalid.json
> qapi-schema += args-member-array-bad.json
> +qapi-schema += args-member-case.json
> qapi-schema += args-member-unknown.json
> qapi-schema += args-name-clash.json
> qapi-schema += args-union.json
> @@ -267,6 +268,7 @@ qapi-schema += enum-bad-prefix.json
> qapi-schema += enum-clash-member.json
> qapi-schema += enum-dict-member.json
> qapi-schema += enum-int-member.json
> +qapi-schema += enum-member-case.json
> qapi-schema += enum-missing-data.json
> qapi-schema += enum-wrong-data.json
> qapi-schema += escape-outside-string.json
> @@ -341,6 +343,7 @@ qapi-schema += unclosed-string.json
> qapi-schema += unicode-str.json
> qapi-schema += union-bad-branch.json
> qapi-schema += union-base-no-discriminator.json
> +qapi-schema += union-branch-case.json
> qapi-schema += union-clash-branches.json
> qapi-schema += union-clash-data.json
> qapi-schema += union-empty.json
> diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err
> new file mode 100644
> index 0000000..7bace48
> --- /dev/null
> +++ b/tests/qapi-schema/args-member-case.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
> diff --git a/tests/qapi-schema/args-member-case.exit b/tests/qapi-schema/args-member-case.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/args-member-case.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/args-member-case.json b/tests/qapi-schema/args-member-case.json
> new file mode 100644
> index 0000000..1bc823a
> --- /dev/null
> +++ b/tests/qapi-schema/args-member-case.json
> @@ -0,0 +1,3 @@
> +# Member names should be 'lower-case' unless the struct/command is whitelisted
> +{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
> +{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
We normally put positive tests in qapi-schema-test.json, but I think
keeping this one here makes more sense.
[More of the same...]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
2015-11-27 9:03 ` Markus Armbruster
@ 2015-11-27 9:42 ` Markus Armbruster
2015-12-01 16:12 ` Eric Blake
1 sibling, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-11-27 9:42 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> We document that members of enums and objects should be
> 'lower-case', although we were not enforcing it. We have to
> whitelist a few pre-existing entities that violate the norms.
> Add three new tests to expose the new error message, each of
> which first uses the whitelisted name 'UuidInfo' to prove the
> whitelist works, then triggers the failure.
>
> Note that by adding this check, we have effectively forbidden
> an entity with a case-insensitive clash of member names, for
> any entity that is not on the whitelist (although there is
> still the possibility to clash via '-' vs. '_').
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
[...]
> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
>
> def check_clash(self, info, seen):
> cname = c_name(self.name)
> + if cname.lower() != cname and info['name'] not in case_whitelist:
> + raise QAPIExprError(info,
> + "Member '%s' of '%s' should use lowercase"
> + % (self.name, info['name']))
> if cname in seen:
> raise QAPIExprError(info,
> "%s collides with %s"
As far as I can tell, this is the only use of info['name'] in this
series.
Can you give an example where info['name'] != self.owner?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D)
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (13 preceding siblings ...)
2015-11-20 17:25 ` [Qemu-devel] [PATCH v13 14/14] qapi: Detect base class loops Eric Blake
@ 2015-11-27 9:56 ` Markus Armbruster
2015-12-01 16:28 ` Eric Blake
14 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-11-27 9:56 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> Pending prerequisites:
> + Markus' "typedefs: Put them back into alphabetical order"
> https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04417.html
> + Markus' qapi-next branch
> http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/qapi-next
>
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv13d
>
> and will soon be part of my branch with the rest of the v5 series, at:
> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
I'm not too fond of PATCH 11, but can't see a better way to do what it
does. However, I don't yet understand why doing it is useful. I
inquired about its use in PATCH 12, perhaps you can enlighten me there.
Other than that, just a few nitpicks. One more respin should take care
of them.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-11-27 9:42 ` Markus Armbruster
@ 2015-12-01 16:12 ` Eric Blake
2015-12-02 10:55 ` Markus Armbruster
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2015-12-01 16:12 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]
On 11/27/2015 02:42 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> We document that members of enums and objects should be
>> 'lower-case', although we were not enforcing it. We have to
>> whitelist a few pre-existing entities that violate the norms.
>> Add three new tests to expose the new error message, each of
>> which first uses the whitelisted name 'UuidInfo' to prove the
>> whitelist works, then triggers the failure.
>>
>> Note that by adding this check, we have effectively forbidden
>> an entity with a case-insensitive clash of member names, for
>> any entity that is not on the whitelist (although there is
>> still the possibility to clash via '-' vs. '_').
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> [...]
>> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
>>
>> def check_clash(self, info, seen):
>> cname = c_name(self.name)
>> + if cname.lower() != cname and info['name'] not in case_whitelist:
>> + raise QAPIExprError(info,
>> + "Member '%s' of '%s' should use lowercase"
>> + % (self.name, info['name']))
>> if cname in seen:
>> raise QAPIExprError(info,
>> "%s collides with %s"
>
> As far as I can tell, this is the only use of info['name'] in this
> series.
Yes, although I may find more uses for it later.
>
> Can you give an example where info['name'] != self.owner?
Sure; this triggers lots of debug lines before crashing[1]:
diff --git i/scripts/qapi.py w/scripts/qapi.py
index 6a77db4..ec59682 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -1054,6 +1054,8 @@ class QAPISchemaMember(object):
def check_clash(self, info, seen):
cname = c_name(self.name)
+ if info['name'] != self.owner:
+ print ' ** checking differs in %s, owner is %s' %
(info['name'], self.owner)
if cname.lower() != cname and info['name'] not in case_whitelist:
raise QAPIExprError(info,
"Member '%s' of '%s' should use lowercase"
The very first one is:
** checking differs in block_passwd, owner is :obj-block_passwd-arg
Remember, QAPISchemaMember.owner is the innermost (possibly-implicit)
type that owns the member, while info['name'] is the name of the
top-level entity that encloses the member. So the two are not always
equal. member._pretty_owner() converts from an implicit struct name
back to the top-level entity, but not directly (it is a human-readable
phrase, not the plain entity name).
Furthermore, look at CpuInfo's member 'CPU': there, we have two call
paths (one with info['name'] == 'CpuInfo', the other with it as
'CpuInfoBase') but both call paths would see only self.owner ==
'CpuInfoBase'. The whitelist covers both struct names. Perhaps
whitelisting only 'self.owner' names would be sufficient; but then the
whitelist would have to use implicit type names rather than entity names
from the .json file.
[1] The crash is "TypeError: 'NoneType' object has no attribute
'__getitem__'" at the point where QType is being tested. Normally,
QType is well-formed, so even though it is a builtin type and therefore
has info == None, the 'cname.lower() != cname' test never fails and we
short-circuit past an attempt to dereference None; but not so with my
temporary print hack.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D)
2015-11-27 9:56 ` [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Markus Armbruster
@ 2015-12-01 16:28 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-12-01 16:28 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]
On 11/27/2015 02:56 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Pending prerequisites:
>> + Markus' "typedefs: Put them back into alphabetical order"
>> https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04417.html
>> + Markus' qapi-next branch
>> http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/qapi-next
>>
>> Also available as a tag at this location:
>> git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv13d
>>
>> and will soon be part of my branch with the rest of the v5 series, at:
>> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
>
> I'm not too fond of PATCH 11, but can't see a better way to do what it
> does. However, I don't yet understand why doing it is useful. I
> inquired about its use in PATCH 12, perhaps you can enlighten me there.
Hopefully I've done just that in my reply on 12; but I will also try and
enhance the commit messages to call things out.
>
> Other than that, just a few nitpicks. One more respin should take care
> of them.
Coming up later today.
--
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] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-11-27 9:03 ` Markus Armbruster
@ 2015-12-01 22:35 ` Eric Blake
2015-12-02 8:20 ` Markus Armbruster
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2015-12-01 22:35 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]
On 11/27/2015 02:03 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> We document that members of enums and objects should be
>> 'lower-case', although we were not enforcing it. We have to
>> whitelist a few pre-existing entities that violate the norms.
>> Add three new tests to expose the new error message, each of
>> which first uses the whitelisted name 'UuidInfo' to prove the
>> whitelist works, then triggers the failure.
>
>> + 'X86CPURegister32',
>
> Because of all enum members.
>
> *Not* visible in QMP, thus fixable. Fix or TODO comment, please.
Not visible directly in QMP, but unfortunately visible in QOM. See
target-i386/cpu.c, where x86_reg_info_32[] references the all-caps
names, and where x86_cpu_get_feature_words() calls
visit_type_X86CPUFeatureWordInfoList() and thereby exposes
X86CPURegister32 to the caller through 'qom-get'.
I can certainly add comments, though.
>> +++ b/tests/qapi-schema/args-member-case.json
>> @@ -0,0 +1,3 @@
>> +# Member names should be 'lower-case' unless the struct/command is whitelisted
>> +{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
>> +{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
>
> We normally put positive tests in qapi-schema-test.json, but I think
> keeping this one here makes more sense.
The idea of a positive test prior to a negative test, in order to prove
that the .err file refers only to the line number of the negative test,
was copied from returns-whitelist.json. But maybe I can reuse the
'no-way-this-will-get-whitelisted' name from that test :)
--
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] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-01 22:35 ` Eric Blake
@ 2015-12-02 8:20 ` Markus Armbruster
2015-12-02 15:47 ` Eric Blake
0 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-12-02 8:20 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 11/27/2015 02:03 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> We document that members of enums and objects should be
>>> 'lower-case', although we were not enforcing it. We have to
>>> whitelist a few pre-existing entities that violate the norms.
>>> Add three new tests to expose the new error message, each of
>>> which first uses the whitelisted name 'UuidInfo' to prove the
>>> whitelist works, then triggers the failure.
>>
>
>>> + 'X86CPURegister32',
>>
>> Because of all enum members.
>>
>> *Not* visible in QMP, thus fixable. Fix or TODO comment, please.
>
> Not visible directly in QMP, but unfortunately visible in QOM. See
> target-i386/cpu.c, where x86_reg_info_32[] references the all-caps
> names, and where x86_cpu_get_feature_words() calls
> visit_type_X86CPUFeatureWordInfoList() and thereby exposes
> X86CPURegister32 to the caller through 'qom-get'.
>
> I can certainly add comments, though.
Hmm, this shows my use of "qapi-introspect.py -u" to find the externally
visible part of QAPI is flawed.
To make it work, we'd need QOM introspection, which we might want
anyway.
Additionally, a way to output just JSON rather than JSON encoded in C
would be nice.
>>> +++ b/tests/qapi-schema/args-member-case.json
>>> @@ -0,0 +1,3 @@
>>> +# Member names should be 'lower-case' unless the struct/command is
>>> whitelisted
>>> +{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
>>> +{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
>>
>> We normally put positive tests in qapi-schema-test.json, but I think
>> keeping this one here makes more sense.
>
> The idea of a positive test prior to a negative test, in order to prove
> that the .err file refers only to the line number of the negative test,
> was copied from returns-whitelist.json. But maybe I can reuse the
> 'no-way-this-will-get-whitelisted' name from that test :)
Yes, please.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-01 16:12 ` Eric Blake
@ 2015-12-02 10:55 ` Markus Armbruster
0 siblings, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2015-12-02 10:55 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 11/27/2015 02:42 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> We document that members of enums and objects should be
>>> 'lower-case', although we were not enforcing it. We have to
>>> whitelist a few pre-existing entities that violate the norms.
>>> Add three new tests to expose the new error message, each of
>>> which first uses the whitelisted name 'UuidInfo' to prove the
>>> whitelist works, then triggers the failure.
>>>
>>> Note that by adding this check, we have effectively forbidden
>>> an entity with a case-insensitive clash of member names, for
>>> any entity that is not on the whitelist (although there is
>>> still the possibility to clash via '-' vs. '_').
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> [...]
>>> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object):
>>>
>>> def check_clash(self, info, seen):
>>> cname = c_name(self.name)
>>> + if cname.lower() != cname and info['name'] not in case_whitelist:
>>> + raise QAPIExprError(info,
>>> + "Member '%s' of '%s' should use lowercase"
Hmm.
tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
'Foo' *does* use lowercase: 'o'. Easiest fix is "should not use
uppercase".
>>> + % (self.name, info['name']))
>>> if cname in seen:
>>> raise QAPIExprError(info,
>>> "%s collides with %s"
>>
>> As far as I can tell, this is the only use of info['name'] in this
>> series.
>
> Yes, although I may find more uses for it later.
>
>>
>> Can you give an example where info['name'] != self.owner?
>
> Sure; this triggers lots of debug lines before crashing[1]:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index 6a77db4..ec59682 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -1054,6 +1054,8 @@ class QAPISchemaMember(object):
>
> def check_clash(self, info, seen):
> cname = c_name(self.name)
> + if info['name'] != self.owner:
> + print ' ** checking differs in %s, owner is %s' % (info['name'], self.owner)
Crash is easy to avoid: if info and info['name'] != self.owner.
> if cname.lower() != cname and info['name'] not in case_whitelist:
> raise QAPIExprError(info,
> "Member '%s' of '%s' should use lowercase"
% (self.name, info['name']))
if cname in seen:
raise QAPIExprError(info,
"%s collides with %s"
% (self.describe(), seen[cname].describe()))
Two separate uses of info['name']:
a. Key for the whitelist
b. Error message
> The very first one is:
>
> ** checking differs in block_passwd, owner is :obj-block_passwd-arg
>
> Remember, QAPISchemaMember.owner is the innermost (possibly-implicit)
> type that owns the member, while info['name'] is the name of the
> top-level entity that encloses the member. So the two are not always
> equal. member._pretty_owner() converts from an implicit struct name
> back to the top-level entity, but not directly (it is a human-readable
> phrase, not the plain entity name).
>
> Furthermore, look at CpuInfo's member 'CPU': there, we have two call
> paths (one with info['name'] == 'CpuInfo', the other with it as
> 'CpuInfoBase') but both call paths would see only self.owner ==
> 'CpuInfoBase'. The whitelist covers both struct names. Perhaps
> whitelisting only 'self.owner' names would be sufficient; but then the
> whitelist would have to use implicit type names rather than entity names
> from the .json file.
With the crash avoided, I get 191 distinct lines. I can sort them into
just give buckets:
1. object vs. base
Example: in CpuInfo, owner is CpuInfoBase
self._pretty_owner() returns "(member of CpuInfoBase). Again, this
would do for error message use. For instance,
{ 'struct': 'Foo', 'data': { 'Memb': 'int' } }
would produce
foo.json:1: 'Memb' (member of Foo) should use lowercase
That leaves the whitelist key use. Using .owner as key would work,
as you already explained. The whitelist becomes slightly more
cumbersome, but also slightly tighter.
2. command / event FOO vs. :obj-FOO-arg
Example: in block_passwd, owner is :obj-block_passwd-arg
self._pretty_owner() returns '(parameter of FOO)'. This would do for
error message use, just like it does for the "collides with error
right below.
"%s should use lowercase" % self.describe()
yields something like
tests/qapi-schema/args-member-case.json:3: 'Arg' (parameter of Foo) should use lowercase
Again, .owner works as whitelist key. This is a case where we'd have
to use implicit type names. However, we don't actually have an
offender to cover.
3. flat union vs. branch
Example: in BlockdevOptions, owner is BlockdevOptionsArchipelago
self._pretty_owner() returns '(member of
BlockdevOptionsArchipelago)'. Error message using that is again
satisfactory:
tests/qapi-schema/union-branch-case.json:3: 'Branch' (branch of Foo) should use lowercase
Again, .owner works as whitelist key. Saves us whitelist entry
'CpuInfo', as you observed.
4. simple union vs. implicit tag enum
Example: in ChardevBackend, owner is ChardevBackendKind
self._pretty_owner() returns '(branch of ChardevBackend)'. Again,
this would do for error message use. For instance,
{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
would produce
foo.json:1: 'Branch' (branch of Foo) should use lowercase
Again, .owner would work as whitelist key, if we had offenders to
whitelist.
5. simple union FOO vs. member wrapper :obj-BAR-wrapper
Example: in ChardevBackend, owner is :obj-ChardevDummy-wrapper
self._pretty_owner() returns '(branch of BAR)'. This is actually
*wrong*: it's a branch of FOO, not a branch of BAR! We haven't
noticed until now, because we don't actually reach this code. We can
reach it only via .describe(), the only use of .describe() is
.check_clash(), and we check a simple union's implicit enum before
its members. Thus, a simple union member clash will always be found
and reported for the implicit enum, where the error message is
correct, and not for the member list, where it would be wrong.
The obvious knee jerk fix is to replace the incorrect code by an
assertion with a suitable comment.
Once we can actually reach it, we can consider *how* we reach it, and
what the appropriate description for a simple union's wrapped members
may be. There's hope it stays unreachable: implicit stuff like this
should not produce errors.
Bottom line for now: this case does not matter for the problem at
hand, namely enforcing "member names are in lower case".
> [1] The crash is "TypeError: 'NoneType' object has no attribute
> '__getitem__'" at the point where QType is being tested. Normally,
> QType is well-formed, so even though it is a builtin type and therefore
> has info == None, the 'cname.lower() != cname' test never fails and we
> short-circuit past an attempt to dereference None; but not so with my
> temporary print hack.
As an experiment, I used self.owner and dropped "qapi: Populate
info['name'] for each entity". I'll post the result as a reply to your
v14.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 8:20 ` Markus Armbruster
@ 2015-12-02 15:47 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-12-02 15:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 959 bytes --]
On 12/02/2015 01:20 AM, Markus Armbruster wrote:
> Hmm, this shows my use of "qapi-introspect.py -u" to find the externally
> visible part of QAPI is flawed.
>
> To make it work, we'd need QOM introspection, which we might want
> anyway.
>
> Additionally, a way to output just JSON rather than JSON encoded in C
> would be nice.
'qapi-introspect.py -u' is already for debugging purposes. Would it be
sufficient to have it do this in qmp-introspect.c?
const char qmp_schema_json[] = "["
"{\"arg-type\": \":obj-ACPI_DVICE_OST-arg\", \"meta-type\":
\"event\", \"name\": \"ACPI_DEVICE_OST\"}, "
...
";
/* The same, as straight JSON:
[
{"arg-type": ":obj-ACPI_DVICE_OST-arg", "meta-type": "event",
"name": "ACPI_DEVICE_OST"},
...
]
*/
If so, that's an easy patch (just visit things twice under -u mode).
--
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] 30+ messages in thread
end of thread, other threads:[~2015-12-02 15:47 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20 17:24 [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject Eric Blake
2015-11-26 14:27 ` Markus Armbruster
2015-11-26 15:06 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 02/14] qobject: Rename qtype_code to QType Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type Eric Blake
2015-11-26 14:51 ` Markus Armbruster
2015-11-27 5:09 ` Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 04/14] qapi: Simplify visiting of alternate types Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 05/14] qapi: Inline _make_implicit_tag() Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 06/14] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 07/14] qapi: Simplify visits of optional fields Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 08/14] qapi: Shorter " Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 09/14] qapi: Prepare new QAPISchemaMember base class Eric Blake
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
2015-11-26 16:29 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity Eric Blake
2015-11-26 16:46 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
2015-11-27 9:03 ` Markus Armbruster
2015-12-01 22:35 ` Eric Blake
2015-12-02 8:20 ` Markus Armbruster
2015-12-02 15:47 ` Eric Blake
2015-11-27 9:42 ` Markus Armbruster
2015-12-01 16:12 ` Eric Blake
2015-12-02 10:55 ` Markus Armbruster
2015-11-20 17:24 ` [Qemu-devel] [PATCH v13 13/14] qapi: Move duplicate collision checks to schema check() Eric Blake
2015-11-20 17:25 ` [Qemu-devel] [PATCH v13 14/14] qapi: Detect base class loops Eric Blake
2015-11-27 9:56 ` [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D) Markus Armbruster
2015-12-01 16:28 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.