* [Qemu-devel] [PATCH v14 01/15] qobject: Simplify QObject
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 02/15] qobject: Rename qtype_code to QType Eric Blake
` (14 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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 code element of the vtable QType
directly into QObject (renamed to type), 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).
A future patch could drop the refcnt size to 32 bits for a smaller
struct on 64-bit architectures, if desired (we have limits on the
largest JSON that we are willing to parse, and will probably never
need to take full advantage of a 64-bit refcnt).
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v14: drop QDestroy typedef and QOBJECT_INIT macro, move init right
after malloc, tighter bounds checks in asserts, improve commit message
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 | 31 +++++++++++++++----------------
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, 69 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..550ba40 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -47,15 +47,8 @@ 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;
@@ -71,9 +64,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(QTYPE_NONE < type && type < QTYPE_MAX);
+ obj->refcnt = 1;
+ obj->type = type;
+}
/**
* qobject_incref(): Increment QObject's reference count
@@ -85,6 +81,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 +93,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 +102,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(QTYPE_NONE < obj->type && obj->type < QTYPE_MAX);
+ 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..856c743 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
*
@@ -32,8 +25,8 @@ QBool *qbool_from_bool(bool value)
QBool *qb;
qb = g_malloc(sizeof(*qb));
+ qobject_init(QOBJECT(qb), QTYPE_QBOOL);
qb->value = value;
- QOBJECT_INIT(qb, &qbool_type);
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..eeade15 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(QOBJECT(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..87d89a7 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
*
@@ -32,8 +25,8 @@ QFloat *qfloat_from_double(double value)
QFloat *qf;
qf = g_malloc(sizeof(*qf));
+ qobject_init(QOBJECT(qf), QTYPE_QFLOAT);
qf->value = value;
- QOBJECT_INIT(qf, &qfloat_type);
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..7cba9ad 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
*
@@ -31,8 +24,8 @@ QInt *qint_from_int(int64_t value)
QInt *qi;
qi = g_malloc(sizeof(*qi));
+ qobject_init(QOBJECT(qi), QTYPE_QINT);
qi->value = value;
- QOBJECT_INIT(qi, &qint_type);
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..3c045ae 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
*
@@ -32,8 +25,8 @@ QList *qlist_new(void)
QList *qlist;
qlist = g_malloc(sizeof(*qlist));
+ qobject_init(QOBJECT(qlist), QTYPE_QLIST);
QTAILQ_INIT(&qlist->head);
- QOBJECT_INIT(qlist, &qlist_type);
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..1df315a
--- /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 void (*qdestroy[QTYPE_MAX])(QObject *) = {
+ [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..f44c5c4 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
*
@@ -49,6 +42,7 @@ QString *qstring_from_substr(const char *str, int start, int end)
QString *qstring;
qstring = g_malloc(sizeof(*qstring));
+ qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
qstring->length = end - start + 1;
qstring->capacity = qstring->length;
@@ -57,7 +51,6 @@ 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);
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] 28+ messages in thread* [Qemu-devel] [PATCH v14 02/15] qobject: Rename qtype_code to QType
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 01/15] qobject: Simplify QObject Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 03/15] qapi: Convert QType into QAPI built-in enum type Eric Blake
` (13 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: rebase to context changes
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 267f147..c0e877e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -588,7 +588,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]: ";
@@ -606,7 +606,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 550ba40..c0f1e99 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;
@@ -64,7 +64,7 @@ typedef struct 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(QTYPE_NONE < type && type < QTYPE_MAX);
obj->refcnt = 1;
@@ -100,7 +100,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(QTYPE_NONE < obj->type && obj->type < QTYPE_MAX);
return obj->type;
diff --git a/qobject/qdict.c b/qobject/qdict.c
index eeade15..19df837 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] 28+ messages in thread* [Qemu-devel] [PATCH v14 03/15] qapi: Convert QType into QAPI built-in enum type
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 01/15] qobject: Simplify QObject Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 02/15] qobject: Rename qtype_code to QType Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 04/15] qapi: Simplify visiting of alternate types Eric Blake
` (12 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: rebase to earlier changes, add comment
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 | 21 +++++----------------
include/qemu/typedefs.h | 1 +
qobject/qobject.c | 4 ++--
scripts/qapi-types.py | 16 ++++++++++++----
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, 62 insertions(+), 24 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 c0f1e99..74459ae 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;
+};
/* Get the 'base' part of an object */
#define QOBJECT(obj) (&(obj)->base)
@@ -66,7 +55,7 @@ typedef struct QObject {
/* Initialize an object to default values */
static inline void qobject_init(QObject *obj, QType type)
{
- assert(QTYPE_NONE < type && type < QTYPE_MAX);
+ assert(QTYPE_NONE < type && type < QTYPE__MAX);
obj->refcnt = 1;
obj->type = type;
}
@@ -102,7 +91,7 @@ static inline void qobject_decref(QObject *obj)
*/
static inline QType qobject_type(const QObject *obj)
{
- assert(QTYPE_NONE < obj->type && obj->type < QTYPE_MAX);
+ assert(QTYPE_NONE < obj->type && obj->type < QTYPE__MAX);
return obj->type;
}
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 1df315a..a3ef14e 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -15,7 +15,7 @@
#include "qapi/qmp/qlist.h"
#include "qapi/qmp/qstring.h"
-static void (*qdestroy[QTYPE_MAX])(QObject *) = {
+static void (*qdestroy[QTYPE__MAX])(QObject *) = {
[QTYPE_NONE] = NULL, /* No such object exists */
[QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
[QTYPE_QINT] = qint_destroy_obj,
@@ -29,6 +29,6 @@ static void (*qdestroy[QTYPE_MAX])(QObject *) = {
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..2071846 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):
@@ -316,10 +323,11 @@ 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)
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] 28+ messages in thread* [Qemu-devel] [PATCH v14 04/15] qapi: Simplify visiting of alternate types
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (2 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 03/15] qapi: Convert QType into QAPI built-in enum type Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 05/15] qapi-types: Drop unnedeed ._fwdefn Eric Blake
` (11 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: typo fix
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 2071846..84ec858 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..4797d6e 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 * invalidly 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] 28+ messages in thread* [Qemu-devel] [PATCH v14 05/15] qapi-types: Drop unnedeed ._fwdefn
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (3 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 04/15] qapi: Simplify visiting of alternate types Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 06/15] qapi: Inline _make_implicit_tag() Eric Blake
` (10 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Previously, the generated code in qapi-types.c initialized all
enum lookup tables first, prior to any other definitions. But
there are no topological sorting requirements that mandate this
layout, so we can drop the QAPISchemaGenTypeVisitor._fwdefn
field and just generate all definitions in visitation order.
The generated code shows some churn due to reordering, but it
is still fairly straightforward to follow (all the deletions
occur in one hunk, and all the deleted lines are re-inserted
in the same order later in the same files, just spread across
multiple insertion points).
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v14: new patch
---
scripts/qapi-types.py | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 84ec858..0d86269 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -168,21 +168,17 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.decl = None
self.defn = None
self._fwdecl = None
- self._fwdefn = None
self._btin = None
def visit_begin(self, schema):
self.decl = ''
self.defn = ''
self._fwdecl = ''
- self._fwdefn = ''
self._btin = guardstart('QAPI_TYPES_BUILTIN')
def visit_end(self):
self.decl = self._fwdecl + self.decl
self._fwdecl = None
- self.defn = self._fwdefn + self.defn
- self._fwdefn = None
# To avoid header dependency hell, we always generate
# declarations for built-in types in our header files and
# simply guard them. See also do_builtins (command line
@@ -209,7 +205,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.defn += gen_enum_lookup(name, values, prefix)
else:
self._fwdecl += gen_enum(name, values, prefix)
- self._fwdefn += gen_enum_lookup(name, values, prefix)
+ self.defn += gen_enum_lookup(name, values, prefix)
def visit_array_type(self, name, info, element_type):
if isinstance(element_type, QAPISchemaBuiltinType):
--
2.4.3
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v14 06/15] qapi: Inline _make_implicit_tag()
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (4 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 05/15] qapi-types: Drop unnedeed ._fwdefn Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 07/15] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
` (9 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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] 28+ messages in thread* [Qemu-devel] [PATCH v14 07/15] qapi: Fix alternates that accept 'number' but not 'int'
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (5 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 06/15] qapi: Inline _make_implicit_tag() Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 08/15] qapi: Simplify visits of optional fields Eric Blake
` (8 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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 4797d6e..b93690b 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] 28+ messages in thread* [Qemu-devel] [PATCH v14 08/15] qapi: Simplify visits of optional fields
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (6 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 07/15] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 09/15] qapi: Shorter " Eric Blake
` (7 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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] 28+ messages in thread* [Qemu-devel] [PATCH v14 09/15] qapi: Shorter visits of optional fields
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (7 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 08/15] qapi: Simplify visits of optional fields Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 10/15] qapi: Prepare new QAPISchemaMember base class Eric Blake
` (6 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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] 28+ messages in thread* [Qemu-devel] [PATCH v14 10/15] qapi: Prepare new QAPISchemaMember base class
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (8 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 09/15] qapi: Shorter " Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 11/15] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
` (5 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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] 28+ messages in thread* [Qemu-devel] [PATCH v14 11/15] qapi: Track enum values by QAPISchemaMember, not string
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (9 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 10/15] qapi: Prepare new QAPISchemaMember base class Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 15:52 ` Markus Armbruster
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 12/15] qapi: Populate info['name'] for each entity Eric Blake
` (4 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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, and add a helper
member_names() method to get back at the original list of names.
Likewise, creating an enum requires wrapping strings, via a new
QAPISchema._make_enum_members() method. The benefit of wrapping
enum members in a QAPISchemaMember Python object is that we now
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).
In a related change, the QAPISchemaMember._pretty_owner() method
needs to learn about one more implicit type name: the generated
enum associated with a simple union.
In the interest of keeping the changes of this patch local to one
file, the visitor interface still passes just a list of names
rather than the full list of QAPISchemaMember instances. We may
want to revisit this in the future, if the consistency with
visit_object_type() is worth it.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v14: Add .member_names() and ._make_enum_members(), cross-reference
comments, improve commit message
v13: new patch
---
scripts/qapi.py | 37 ++++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2748464..8fad7c8 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -901,13 +901,16 @@ 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)
+ seen = {}
+ for v in self.values:
+ v.check_clash(self.info, seen)
def is_implicit(self):
# See QAPISchema._make_implicit_enum_type()
@@ -916,8 +919,11 @@ class QAPISchemaEnumType(QAPISchemaType):
def c_type(self, is_param=False):
return c_name(self.name)
+ def member_names(self):
+ return [v.name for v in self.values]
+
def c_null(self):
- return c_enum_const(self.name, (self.values + ['_MAX'])[0],
+ return c_enum_const(self.name, (self.member_names() + ['_MAX'])[0],
self.prefix)
def json_type(self):
@@ -925,7 +931,7 @@ class QAPISchemaEnumType(QAPISchemaType):
def visit(self, visitor):
visitor.visit_enum_type(self.name, self.info,
- self.values, self.prefix)
+ self.member_names(), self.prefix)
class QAPISchemaArrayType(QAPISchemaType):
@@ -1049,6 +1055,9 @@ class QAPISchemaMember(object):
else:
assert owner.endswith('-wrapper')
return '(branch of %s)' % owner[:-8]
+ if owner.endswith('Kind'):
+ # See QAPISchema._make_implicit_enum_type()
+ return '(branch of %s)' % owner[:-4]
return '(%s of %s)' % (self.role, owner)
def describe(self):
@@ -1099,7 +1108,7 @@ class QAPISchemaObjectTypeVariants(object):
# 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 self.tag_member.type.member_names()
assert isinstance(v.type, QAPISchemaObjectType)
v.type.check(schema)
@@ -1257,15 +1266,20 @@ 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 = self._make_enum_members('none', 'qnull', 'qint',
+ 'qstring', 'qdict', 'qlist',
+ 'qfloat', 'qbool')
+ self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
'QTYPE'))
+ def _make_enum_members(self, *values):
+ return [QAPISchemaMember(v) for v in values]
+
def _make_implicit_enum_type(self, name, info, values):
+ # See also QAPISchemaObjectTypeMember._pretty_owner()
name = name + 'Kind' # Use namespace reserved by add_name()
- self._def_entity(QAPISchemaEnumType(name, info, values, None))
+ self._def_entity(QAPISchemaEnumType(
+ name, info, self._make_enum_members(*values), None))
return name
def _make_array_type(self, element_type, info):
@@ -1288,7 +1302,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, self._make_enum_members(*data), prefix))
def _make_member(self, name, typ, info):
optional = False
--
2.4.3
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 11/15] qapi: Track enum values by QAPISchemaMember, not string
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 11/15] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
@ 2015-12-02 15:52 ` Markus Armbruster
2015-12-02 16:09 ` [Qemu-devel] [PATCH] fixup! " Eric Blake
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2015-12-02 15:52 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, and add a helper
> member_names() method to get back at the original list of names.
> Likewise, creating an enum requires wrapping strings, via a new
> QAPISchema._make_enum_members() method. The benefit of wrapping
> enum members in a QAPISchemaMember Python object is that we now
> 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).
>
> In a related change, the QAPISchemaMember._pretty_owner() method
> needs to learn about one more implicit type name: the generated
> enum associated with a simple union.
>
> In the interest of keeping the changes of this patch local to one
> file, the visitor interface still passes just a list of names
> rather than the full list of QAPISchemaMember instances. We may
> want to revisit this in the future, if the consistency with
> visit_object_type() is worth it.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v14: Add .member_names() and ._make_enum_members(), cross-reference
> comments, improve commit message
> v13: new patch
> ---
> scripts/qapi.py | 37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2748464..8fad7c8 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -901,13 +901,16 @@ 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)
> + seen = {}
> + for v in self.values:
> + v.check_clash(self.info, seen)
>
> def is_implicit(self):
> # See QAPISchema._make_implicit_enum_type()
> @@ -916,8 +919,11 @@ class QAPISchemaEnumType(QAPISchemaType):
> def c_type(self, is_param=False):
> return c_name(self.name)
>
> + def member_names(self):
> + return [v.name for v in self.values]
> +
> def c_null(self):
> - return c_enum_const(self.name, (self.values + ['_MAX'])[0],
> + return c_enum_const(self.name, (self.member_names() + ['_MAX'])[0],
> self.prefix)
>
> def json_type(self):
> @@ -925,7 +931,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>
> def visit(self, visitor):
> visitor.visit_enum_type(self.name, self.info,
> - self.values, self.prefix)
> + self.member_names(), self.prefix)
>
>
> class QAPISchemaArrayType(QAPISchemaType):
> @@ -1049,6 +1055,9 @@ class QAPISchemaMember(object):
> else:
> assert owner.endswith('-wrapper')
> return '(branch of %s)' % owner[:-8]
> + if owner.endswith('Kind'):
> + # See QAPISchema._make_implicit_enum_type()
> + return '(branch of %s)' % owner[:-4]
> return '(%s of %s)' % (self.role, owner)
>
> def describe(self):
> @@ -1099,7 +1108,7 @@ class QAPISchemaObjectTypeVariants(object):
> # 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 self.tag_member.type.member_names()
> assert isinstance(v.type, QAPISchemaObjectType)
> v.type.check(schema)
>
> @@ -1257,15 +1266,20 @@ 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 = self._make_enum_members('none', 'qnull', 'qint',
> + 'qstring', 'qdict', 'qlist',
> + 'qfloat', 'qbool')
> + self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
> 'QTYPE'))
>
> + def _make_enum_members(self, *values):
> + return [QAPISchemaMember(v) for v in values]
Sure a var-positional parameter makes sense here? Two of three callers
want to pass a list and have to splice it with a *expression. Only the
call above actually profits from the var-positional, and it could easily
keep passing a list.
> +
> def _make_implicit_enum_type(self, name, info, values):
> + # See also QAPISchemaObjectTypeMember._pretty_owner()
> name = name + 'Kind' # Use namespace reserved by add_name()
> - self._def_entity(QAPISchemaEnumType(name, info, values, None))
> + self._def_entity(QAPISchemaEnumType(
> + name, info, self._make_enum_members(*values), None))
> return name
>
> def _make_array_type(self, element_type, info):
> @@ -1288,7 +1302,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, self._make_enum_members(*data), prefix))
>
> def _make_member(self, name, typ, info):
> optional = False
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH] fixup! qapi: Track enum values by QAPISchemaMember, not string
2015-12-02 15:52 ` Markus Armbruster
@ 2015-12-02 16:09 ` Eric Blake
0 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 16:09 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, Michael Roth
Rather than making 2/3 callers explode a list just to have it
be reconstructed, instead fix the remaining caller to wrap its
arguments in a list.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Markus was right
scripts/qapi.py | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index bebd8c7..c107c4b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1241,20 +1241,20 @@ class QAPISchema(object):
self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None,
[], None)
self._def_entity(self.the_empty_object_type)
- qtype_values = self._make_enum_members('none', 'qnull', 'qint',
- 'qstring', 'qdict', 'qlist',
- 'qfloat', 'qbool')
+ qtype_values = self._make_enum_members(['none', 'qnull', 'qint',
+ 'qstring', 'qdict', 'qlist',
+ 'qfloat', 'qbool'])
self._def_entity(QAPISchemaEnumType('QType', None, qtype_values,
'QTYPE'))
- def _make_enum_members(self, *values):
+ def _make_enum_members(self, values):
return [QAPISchemaMember(v) for v in values]
def _make_implicit_enum_type(self, name, info, values):
# See also QAPISchemaObjectTypeMember._pretty_owner()
name = name + 'Kind' # Use namespace reserved by add_name()
self._def_entity(QAPISchemaEnumType(
- name, info, self._make_enum_members(*values), None))
+ name, info, self._make_enum_members(values), None))
return name
def _make_array_type(self, element_type, info):
@@ -1278,7 +1278,7 @@ class QAPISchema(object):
data = expr['data']
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(
- name, info, self._make_enum_members(*data), prefix))
+ name, info, self._make_enum_members(data), prefix))
def _make_member(self, name, typ, info):
optional = False
--
2.4.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v14 12/15] qapi: Populate info['name'] for each entity
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (10 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 11/15] qapi: Track enum values by QAPISchemaMember, not string Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
` (3 subsequent siblings)
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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'.
For example, inside QAPISchemaMember.check_clash(), the value
of self.owner may be the implicit ':obj-block_passwd-arg', while
we want to report any problems on behalf of the top-level
'block_passwd' command entity. And while we have a
._pretty_owner() that maps from implicit names back to a human
readable phrase, that is more than just a plain top-level entity
name. What's more, the .check_clash() method may be called for
the same member object more than once, with different caller
'info' objects (such as a base and derived object); if a clash
is only introduced in the derived class, reporting the error
on behalf of the base class named in member.owner seems wrong.
Add a new info['name'] field to track the information we need;
it will be handy for better error messages in future commits.
Unfortunately, there is no one good place to add the mapping:
at the point 'info' is created in QAPISchemaParser.__init__(),
we don't know the name. Info is then stored into
QAPISchemaParser.exprs, which then then gets fed to
QAPISchema.__init__() via the global check_exprs(), but we want
check_exprs() to go away. And QAPISchema._def_exprs() sees
every entity, except that the various _def_FOO() helpers don't
return anything. So we have to modify all of the _def_FOO()
methods.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v14: rearrange assignment, improve commit message
v13: new patch
---
scripts/qapi.py | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8fad7c8..7de405c 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1299,7 +1299,7 @@ class QAPISchema(object):
return name
def _def_enum_type(self, expr, info):
- name = expr['enum']
+ name = info['name'] = expr['enum']
data = expr['data']
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(
@@ -1320,7 +1320,7 @@ class QAPISchema(object):
for (key, value) in data.iteritems()]
def _def_struct_type(self, expr, info):
- name = expr['struct']
+ name = info['name'] = expr['struct']
base = expr.get('base')
data = expr['data']
self._def_entity(QAPISchemaObjectType(name, info, base,
@@ -1339,7 +1339,7 @@ class QAPISchema(object):
return QAPISchemaObjectTypeVariant(case, typ)
def _def_union_type(self, expr, info):
- name = expr['union']
+ name = info['name'] = expr['union']
data = expr['data']
base = expr.get('base')
tag_name = expr.get('discriminator')
@@ -1362,7 +1362,7 @@ class QAPISchema(object):
variants)))
def _def_alternate_type(self, expr, info):
- name = expr['alternate']
+ name = info['name'] = expr['alternate']
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
@@ -1374,7 +1374,7 @@ class QAPISchema(object):
variants)))
def _def_command(self, expr, info):
- name = expr['command']
+ name = info['name'] = expr['command']
data = expr.get('data')
rets = expr.get('returns')
gen = expr.get('gen', True)
@@ -1389,7 +1389,7 @@ class QAPISchema(object):
success_response))
def _def_event(self, expr, info):
- name = expr['event']
+ name = info['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] 28+ messages in thread* [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (11 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 12/15] qapi: Populate info['name'] for each entity Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 11:51 ` Markus Armbruster
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 14/15] qapi: Move duplicate collision checks to schema check() Eric Blake
` (2 subsequent siblings)
15 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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 (this is the same
pattern used in the existing returns-whitelist.json test).
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. '_').
Not done here: a future patch should also add naming convention
support and whitelist exceptions for command, event, and type
names.
The additions to QAPISchemaMember.check_clash() check whether
info['name'] is in the whitelist (the top-most entity name at
the point 'info' tracks), rather than self.owner (the type,
possibly implicit, that directly owns the member), because it
is easier to maintain the whitelist by the names actually in
the user's .json file, rather than worrying about the names
of implicit types.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v14: improved commit message, add comments justifying each
whitelist member
v13: new patch
---
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 7de405c..3fc1ff5 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', # DIMM, visible through query-acpi-ospm-status
+ 'CpuInfo', # CPU, PC, visible through query-cpu
+ 'CpuInfoBase', # CPU, visible through query-cpu
+ 'CpuInfoMIPS', # PC, visible through query-cpu
+ 'CpuInfoTricore', # PC, visible through query-cpu
+ 'InputAxis', # TODO: drop when x-input-send-event is fixed
+ 'InputButton', # TODO: drop when x-input-send-event is fixed
+ 'QapiErrorClass', # all members, visible through errors
+ 'UuidInfo', # UUID, visible through query-uuid
+ 'X86CPURegister32', # all members, visible indirectly through qom-get
+]
+
enum_types = []
struct_types = []
union_types = []
@@ -1038,6 +1053,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 5d83fc6..c3fd3c1 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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
@ 2015-12-02 11:51 ` Markus Armbruster
2015-12-02 13:41 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Markus Armbruster @ 2015-12-02 11:51 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
This is the fixup I mentioned in the v13 thread. The "Unreachable and
not implemented" hunk should probably be its own patch.
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6d38d7c..870e476 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -63,7 +63,6 @@ returns_whitelist = [
case_whitelist = [
# From QMP:
'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
- 'CpuInfo', # CPU, PC, visible through query-cpu
'CpuInfoBase', # CPU, visible through query-cpu
'CpuInfoMIPS', # PC, visible through query-cpu
'CpuInfoTricore', # PC, visible through query-cpu
@@ -1053,10 +1052,9 @@ 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:
+ if cname.lower() != cname and self.owner not in case_whitelist:
raise QAPIExprError(info,
- "Member '%s' of '%s' should use lowercase"
- % (self.name, info['name']))
+ "%s should not use uppercase" % self.describe())
if cname in seen:
raise QAPIExprError(info,
"%s collides with %s"
@@ -1073,7 +1071,8 @@ class QAPISchemaMember(object):
return '(parameter of %s)' % owner[:-4]
else:
assert owner.endswith('-wrapper')
- return '(branch of %s)' % owner[:-8]
+ # Unreachable and not implemented
+ assert False
if owner.endswith('Kind'):
# See QAPISchema._make_implicit_enum_type()
return '(branch of %s)' % owner[:-4]
diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err
index 7bace48..44c31ea 100644
--- a/tests/qapi-schema/args-member-case.err
+++ b/tests/qapi-schema/args-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
+tests/qapi-schema/args-member-case.json:3: 'Arg' (parameter of Foo) should not use uppercase
diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err
index e50b12a..a1d67c6 100644
--- a/tests/qapi-schema/enum-member-case.err
+++ b/tests/qapi-schema/enum-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/enum-member-case.json:3: Member 'Value' of 'Foo' should use lowercase
+tests/qapi-schema/enum-member-case.json:3: 'Value' (member of Foo) should not use uppercase
diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err
index 6c6b740..0b4c1b5 100644
--- a/tests/qapi-schema/union-branch-case.err
+++ b/tests/qapi-schema/union-branch-case.err
@@ -1 +1 @@
-tests/qapi-schema/union-branch-case.json:3: Member 'Branch' of 'Foo' should use lowercase
+tests/qapi-schema/union-branch-case.json:3: 'Branch' (branch of Foo) should not use uppercase
--
2.4.3
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 11:51 ` Markus Armbruster
@ 2015-12-02 13:41 ` Eric Blake
2015-12-02 15:19 ` Eric Blake
2015-12-02 16:48 ` Markus Armbruster
2015-12-02 14:11 ` Eric Blake
2015-12-02 15:30 ` [Qemu-devel] [PATCH] fixup! " Eric Blake
2 siblings, 2 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 13:41 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]
On 12/02/2015 04:51 AM, Markus Armbruster wrote:
> This is the fixup I mentioned in the v13 thread. The "Unreachable and
> not implemented" hunk should probably be its own patch.
In fact, that hunk...
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6d38d7c..870e476 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1073,7 +1071,8 @@ class QAPISchemaMember(object):
> return '(parameter of %s)' % owner[:-4]
> else:
> assert owner.endswith('-wrapper')
> - return '(branch of %s)' % owner[:-8]
> + # Unreachable and not implemented
> + assert False
> if owner.endswith('Kind'):
> # See QAPISchema._make_implicit_enum_type()
> return '(branch of %s)' % owner[:-4]
...should probably just be squashed directly into commit 8f3a05b on your
current qapi-next branch, since it hasn't landed upstream yet.
Your fixup looks sane, and eliminates the need for 12/15. So I'm fine
if you'd like to make that change when updating qapi-next.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 13:41 ` Eric Blake
@ 2015-12-02 15:19 ` Eric Blake
2015-12-02 17:19 ` Markus Armbruster
2015-12-02 16:48 ` Markus Armbruster
1 sibling, 1 reply; 28+ messages in thread
From: Eric Blake @ 2015-12-02 15:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3373 bytes --]
On 12/02/2015 06:41 AM, Eric Blake wrote:
> On 12/02/2015 04:51 AM, Markus Armbruster wrote:
>> This is the fixup I mentioned in the v13 thread. The "Unreachable and
>> not implemented" hunk should probably be its own patch.
>
> In fact, that hunk...
>
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 6d38d7c..870e476 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>
>> @@ -1073,7 +1071,8 @@ class QAPISchemaMember(object):
>> return '(parameter of %s)' % owner[:-4]
>> else:
>> assert owner.endswith('-wrapper')
>> - return '(branch of %s)' % owner[:-8]
>> + # Unreachable and not implemented
>> + assert False
>> if owner.endswith('Kind'):
>> # See QAPISchema._make_implicit_enum_type()
>> return '(branch of %s)' % owner[:-4]
>
> ...should probably just be squashed directly into commit 8f3a05b on your
> current qapi-next branch, since it hasn't landed upstream yet.
>
> Your fixup looks sane, and eliminates the need for 12/15. So I'm fine
> if you'd like to make that change when updating qapi-next.
> Reviewed-by: Eric Blake <eblake@redhat.com>
Scratch that.
With your patch, the positive tests no longer work in isolation. You
were getting lucky that things sorted such that 'Foo' was checked for
correctness prior to 'UuidInfo'; but if you comment out the 'Foo'
declaration, or rename from 'Foo' to something else that hashes after
'UuidInfo', then args-member-case and union-branch-case start reporting
failures about UuidInfo (and only enum-member-case honors the
whitelist). That's because your change to qapi.py would require the
whitelist to contain ':obj-UuidInfo-args' and 'UuidInfoKind',
respectively (with my approach of info['name'], the whitelist containing
'UuidInfo' was sufficient).
Maybe we need to modify qapi.py as follows:
diff --git i/scripts/qapi.py w/scripts/qapi.py
index 04c4c8d..1325da1 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -71,6 +71,10 @@ case_whitelist = [
'QapiErrorClass', # all members, visible through errors
'UuidInfo', # UUID, visible through query-uuid
'X86CPURegister32', # all members, visible indirectly through
qom-get
+
+ # For use in the testsuite
+ ':obj-x-UuidInfo-arg', # args-member-case
+ 'x-UuidInfoList', # union-branch-case
]
enum_types = []
index 1bc823a..193eb66 100644
--- i/tests/qapi-schema/args-member-case.json
+++ w/tests/qapi-schema/args-member-case.json
@@ -1,3 +1,3 @@
# Member names should be 'lower-case' unless the struct/command is
whitelisted
-{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
+{ 'command': 'x-UuidInfo', 'data': { 'Arg': 'int' } }
{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
index a5951f1..4f0988a 100644
--- i/tests/qapi-schema/union-branch-case.json
+++ w/tests/qapi-schema/union-branch-case.json
@@ -1,3 +1,3 @@
# Branch names should be 'lower-case' unless the union is whitelisted
-{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
+{ 'union': 'x-UuidInfo', 'data': { 'Branch': 'int' } }
{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
--
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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 15:19 ` Eric Blake
@ 2015-12-02 17:19 ` Markus Armbruster
2015-12-02 19:43 ` Eric Blake
0 siblings, 1 reply; 28+ messages in thread
From: Markus Armbruster @ 2015-12-02 17:19 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 12/02/2015 06:41 AM, Eric Blake wrote:
>> On 12/02/2015 04:51 AM, Markus Armbruster wrote:
>>> This is the fixup I mentioned in the v13 thread. The "Unreachable and
>>> not implemented" hunk should probably be its own patch.
>>
>> In fact, that hunk...
>>
>>>
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 6d38d7c..870e476 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>
>>> @@ -1073,7 +1071,8 @@ class QAPISchemaMember(object):
>>> return '(parameter of %s)' % owner[:-4]
>>> else:
>>> assert owner.endswith('-wrapper')
>>> - return '(branch of %s)' % owner[:-8]
>>> + # Unreachable and not implemented
>>> + assert False
>>> if owner.endswith('Kind'):
>>> # See QAPISchema._make_implicit_enum_type()
>>> return '(branch of %s)' % owner[:-4]
>>
>> ...should probably just be squashed directly into commit 8f3a05b on your
>> current qapi-next branch, since it hasn't landed upstream yet.
>>
>> Your fixup looks sane, and eliminates the need for 12/15. So I'm fine
>> if you'd like to make that change when updating qapi-next.
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Scratch that.
>
> With your patch, the positive tests no longer work in isolation. You
> were getting lucky that things sorted such that 'Foo' was checked for
> correctness prior to 'UuidInfo'; but if you comment out the 'Foo'
> declaration, or rename from 'Foo' to something else that hashes after
> 'UuidInfo', then args-member-case and union-branch-case start reporting
> failures about UuidInfo (and only enum-member-case honors the
> whitelist).
You're right.
> That's because your change to qapi.py would require the
> whitelist to contain ':obj-UuidInfo-args' and 'UuidInfoKind',
> respectively (with my approach of info['name'], the whitelist containing
> 'UuidInfo' was sufficient).
>
> Maybe we need to modify qapi.py as follows:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index 04c4c8d..1325da1 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -71,6 +71,10 @@ case_whitelist = [
> 'QapiErrorClass', # all members, visible through errors
> 'UuidInfo', # UUID, visible through query-uuid
> 'X86CPURegister32', # all members, visible indirectly through
> qom-get
> +
> + # For use in the testsuite
> + ':obj-x-UuidInfo-arg', # args-member-case
> + 'x-UuidInfoList', # union-branch-case
Corrected to 'x-UuidInfoKind' in your actual fixup patch.
> ]
>
> enum_types = []
>
> index 1bc823a..193eb66 100644
> --- i/tests/qapi-schema/args-member-case.json
> +++ w/tests/qapi-schema/args-member-case.json
> @@ -1,3 +1,3 @@
> # Member names should be 'lower-case' unless the struct/command is
> whitelisted
> -{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
> +{ 'command': 'x-UuidInfo', 'data': { 'Arg': 'int' } }
Will fail as soon as we enforce the command naming convention. To avoid
that, we need to add a suitable name to the whitelist.
However, we don't actually have any command parameters to whitelist.
Why bother testing it then?
> { 'command': 'Foo', 'data': { 'Arg': 'int' } }
> index a5951f1..4f0988a 100644
> --- i/tests/qapi-schema/union-branch-case.json
> +++ w/tests/qapi-schema/union-branch-case.json
> @@ -1,3 +1,3 @@
> # Branch names should be 'lower-case' unless the union is whitelisted
> -{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
> +{ 'union': 'x-UuidInfo', 'data': { 'Branch': 'int' } }
> { 'union': 'Foo', 'data': { 'Branch': 'int' } }
Likewise, we don't actually have any simple union branches to whitelist.
Why test?
Appending my current fixup.
>From 44f07a40c8b9b5d1f24833028b5dacde1fd50c80 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Wed, 2 Dec 2015 17:41:55 +0100
Subject: [PATCH] fixup! qapi: Enforce (or whitelist) case conventions on qapi
members
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi.py | 6 ++----
tests/qapi-schema/args-member-case.err | 2 +-
tests/qapi-schema/args-member-case.json | 3 +--
tests/qapi-schema/enum-member-case.err | 2 +-
tests/qapi-schema/enum-member-case.json | 4 ++--
tests/qapi-schema/union-branch-case.err | 2 +-
tests/qapi-schema/union-branch-case.json | 3 +--
7 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0aed6d4..74a561a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -63,7 +63,6 @@ returns_whitelist = [
case_whitelist = [
# From QMP:
'ACPISlotType', # DIMM, visible through query-acpi-ospm-status
- 'CpuInfo', # CPU, PC, visible through query-cpu
'CpuInfoBase', # CPU, visible through query-cpu
'CpuInfoMIPS', # PC, visible through query-cpu
'CpuInfoTricore', # PC, visible through query-cpu
@@ -1053,10 +1052,9 @@ 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:
+ if cname.lower() != cname and self.owner not in case_whitelist:
raise QAPIExprError(info,
- "Member '%s' of '%s' should use lowercase"
- % (self.name, info['name']))
+ "%s should not use uppercase" % self.describe())
if cname in seen:
raise QAPIExprError(info,
"%s collides with %s"
diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err
index 7bace48..19c4426 100644
--- a/tests/qapi-schema/args-member-case.err
+++ b/tests/qapi-schema/args-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
+tests/qapi-schema/args-member-case.json:2: 'Arg' (parameter of no-way-this-will-get-whitelisted) should not use uppercase
diff --git a/tests/qapi-schema/args-member-case.json b/tests/qapi-schema/args-member-case.json
index 1bc823a..93439be 100644
--- a/tests/qapi-schema/args-member-case.json
+++ b/tests/qapi-schema/args-member-case.json
@@ -1,3 +1,2 @@
# Member names should be 'lower-case' unless the struct/command is whitelisted
-{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
-{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
+{ 'command': 'no-way-this-will-get-whitelisted', 'data': { 'Arg': 'int' } }
diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err
index e50b12a..b652e9a 100644
--- a/tests/qapi-schema/enum-member-case.err
+++ b/tests/qapi-schema/enum-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/enum-member-case.json:3: Member 'Value' of 'Foo' should use lowercase
+tests/qapi-schema/enum-member-case.json:3: 'Value' (member of NoWayThisWillGetWhitelisted) should not use uppercase
diff --git a/tests/qapi-schema/enum-member-case.json b/tests/qapi-schema/enum-member-case.json
index 5101275..2096b35 100644
--- a/tests/qapi-schema/enum-member-case.json
+++ b/tests/qapi-schema/enum-member-case.json
@@ -1,3 +1,3 @@
# Member names should be 'lower-case' unless the enum is whitelisted
-{ 'enum': 'UuidInfo', 'data': [ 'Value' ] }
-{ 'enum': 'Foo', 'data': [ 'Value' ] }
+{ 'enum': 'UuidInfo', 'data': [ 'Value' ] } # UuidInfo is whitelisted
+{ 'enum': 'NoWayThisWillGetWhitelisted', 'data': [ 'Value' ] }
diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err
index 6c6b740..1152190 100644
--- a/tests/qapi-schema/union-branch-case.err
+++ b/tests/qapi-schema/union-branch-case.err
@@ -1 +1 @@
-tests/qapi-schema/union-branch-case.json:3: Member 'Branch' of 'Foo' should use lowercase
+tests/qapi-schema/union-branch-case.json:2: 'Branch' (branch of NoWayThisWillGetWhitelisted) should not use uppercase
diff --git a/tests/qapi-schema/union-branch-case.json b/tests/qapi-schema/union-branch-case.json
index a5951f1..e6565dc 100644
--- a/tests/qapi-schema/union-branch-case.json
+++ b/tests/qapi-schema/union-branch-case.json
@@ -1,3 +1,2 @@
# Branch names should be 'lower-case' unless the union is whitelisted
-{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
-{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
+{ 'union': 'NoWayThisWillGetWhitelisted', 'data': { 'Branch': 'int' } }
--
2.4.3
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 17:19 ` Markus Armbruster
@ 2015-12-02 19:43 ` Eric Blake
2015-12-02 20:27 ` Markus Armbruster
0 siblings, 1 reply; 28+ messages in thread
From: Eric Blake @ 2015-12-02 19:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]
On 12/02/2015 10:19 AM, Markus Armbruster wrote:
>> With your patch, the positive tests no longer work in isolation. You
>> were getting lucky that things sorted such that 'Foo' was checked for
>> correctness prior to 'UuidInfo'; but if you comment out the 'Foo'
>> declaration, or rename from 'Foo' to something else that hashes after
>> 'UuidInfo', then args-member-case and union-branch-case start reporting
>> failures about UuidInfo (and only enum-member-case honors the
>> whitelist).
>
> You're right.
>
>> +++ w/tests/qapi-schema/args-member-case.json
>> @@ -1,3 +1,3 @@
>> # Member names should be 'lower-case' unless the struct/command is
>> whitelisted
>> -{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
>> +{ 'command': 'x-UuidInfo', 'data': { 'Arg': 'int' } }
>
> Will fail as soon as we enforce the command naming convention. To avoid
> that, we need to add a suitable name to the whitelist.
>
> However, we don't actually have any command parameters to whitelist.
> Why bother testing it then?
Good point.
>
>>From 44f07a40c8b9b5d1f24833028b5dacde1fd50c80 Mon Sep 17 00:00:00 2001
> From: Markus Armbruster <armbru@redhat.com>
> Date: Wed, 2 Dec 2015 17:41:55 +0100
> Subject: [PATCH] fixup! qapi: Enforce (or whitelist) case conventions on qapi
> members
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/qapi.py | 6 ++----
> tests/qapi-schema/args-member-case.err | 2 +-
> tests/qapi-schema/args-member-case.json | 3 +--
> tests/qapi-schema/enum-member-case.err | 2 +-
> tests/qapi-schema/enum-member-case.json | 4 ++--
> tests/qapi-schema/union-branch-case.err | 2 +-
> tests/qapi-schema/union-branch-case.json | 3 +--
> 7 files changed, 9 insertions(+), 13 deletions(-)
Looks good. Commit 6c1eb345 on your qapi-not-next branch has this
already squashed in, but lacking your S-o-b. But that only affects the
commit message, so I'll go ahead and base my future patch submissions on
top of the current contents of qapi-not-next (it will be interesting to
see how many patches land from various trees right after 2.6 opens 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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 19:43 ` Eric Blake
@ 2015-12-02 20:27 ` Markus Armbruster
0 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2015-12-02 20:27 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 12/02/2015 10:19 AM, Markus Armbruster wrote:
>
>>> With your patch, the positive tests no longer work in isolation. You
>>> were getting lucky that things sorted such that 'Foo' was checked for
>>> correctness prior to 'UuidInfo'; but if you comment out the 'Foo'
>>> declaration, or rename from 'Foo' to something else that hashes after
>>> 'UuidInfo', then args-member-case and union-branch-case start reporting
>>> failures about UuidInfo (and only enum-member-case honors the
>>> whitelist).
>>
>> You're right.
>>
>
>>> +++ w/tests/qapi-schema/args-member-case.json
>>> @@ -1,3 +1,3 @@
>>> # Member names should be 'lower-case' unless the struct/command is
>>> whitelisted
>>> -{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
>>> +{ 'command': 'x-UuidInfo', 'data': { 'Arg': 'int' } }
>>
>> Will fail as soon as we enforce the command naming convention. To avoid
>> that, we need to add a suitable name to the whitelist.
>>
>> However, we don't actually have any command parameters to whitelist.
>> Why bother testing it then?
>
> Good point.
>
>
>>
>>>From 44f07a40c8b9b5d1f24833028b5dacde1fd50c80 Mon Sep 17 00:00:00 2001
>> From: Markus Armbruster <armbru@redhat.com>
>> Date: Wed, 2 Dec 2015 17:41:55 +0100
>> Subject: [PATCH] fixup! qapi: Enforce (or whitelist) case conventions on qapi
>> members
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> scripts/qapi.py | 6 ++----
>> tests/qapi-schema/args-member-case.err | 2 +-
>> tests/qapi-schema/args-member-case.json | 3 +--
>> tests/qapi-schema/enum-member-case.err | 2 +-
>> tests/qapi-schema/enum-member-case.json | 4 ++--
>> tests/qapi-schema/union-branch-case.err | 2 +-
>> tests/qapi-schema/union-branch-case.json | 3 +--
>> 7 files changed, 9 insertions(+), 13 deletions(-)
>
> Looks good. Commit 6c1eb345 on your qapi-not-next branch has this
> already squashed in, but lacking your S-o-b. But that only affects the
> commit message, so I'll go ahead and base my future patch submissions on
> top of the current contents of qapi-not-next (it will be interesting to
> see how many patches land from various trees right after 2.6 opens up...)
I intended to push with a separate fixup commit; looks like a last
minute rebase to tweak commit messages squashed it in. Anyway, I fixed
up the commit message and pushed again. Thanks!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 13:41 ` Eric Blake
2015-12-02 15:19 ` Eric Blake
@ 2015-12-02 16:48 ` Markus Armbruster
1 sibling, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2015-12-02 16:48 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 12/02/2015 04:51 AM, Markus Armbruster wrote:
>> This is the fixup I mentioned in the v13 thread. The "Unreachable and
>> not implemented" hunk should probably be its own patch.
>
> In fact, that hunk...
>
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 6d38d7c..870e476 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>
>> @@ -1073,7 +1071,8 @@ class QAPISchemaMember(object):
>> return '(parameter of %s)' % owner[:-4]
>> else:
>> assert owner.endswith('-wrapper')
>> - return '(branch of %s)' % owner[:-8]
>> + # Unreachable and not implemented
>> + assert False
>> if owner.endswith('Kind'):
>> # See QAPISchema._make_implicit_enum_type()
>> return '(branch of %s)' % owner[:-4]
>
> ...should probably just be squashed directly into commit 8f3a05b on your
> current qapi-next branch, since it hasn't landed upstream yet.
Good idea.
> Your fixup looks sane, and eliminates the need for 12/15. So I'm fine
Yes, let's set that patch aside for now. We can bring it back if we
find a need.
> if you'd like to make that change when updating qapi-next.
> Reviewed-by: Eric Blake <eblake@redhat.com>
Checkout out your later fixups now.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 11:51 ` Markus Armbruster
2015-12-02 13:41 ` Eric Blake
@ 2015-12-02 14:11 ` Eric Blake
2015-12-02 15:30 ` [Qemu-devel] [PATCH] fixup! " Eric Blake
2 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 14:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]
On 12/02/2015 04:51 AM, Markus Armbruster wrote:
> This is the fixup I mentioned in the v13 thread. The "Unreachable and
> not implemented" hunk should probably be its own patch.
>
> +++ b/tests/qapi-schema/args-member-case.err
> @@ -1 +1 @@
> -tests/qapi-schema/args-member-case.json:3: Member 'Arg' of 'Foo' should use lowercase
> +tests/qapi-schema/args-member-case.json:3: 'Arg' (parameter of Foo) should not use uppercase
Oh, and I forgot to use the name 'no-way-this-will-get-whitelisted' (or
NoWayThisWillGetWhitelisted) as suggested in v13, if you want to also
change naming.
> diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err
> index e50b12a..a1d67c6 100644
> --- a/tests/qapi-schema/enum-member-case.err
> +++ b/tests/qapi-schema/enum-member-case.err
> @@ -1 +1 @@
> -tests/qapi-schema/enum-member-case.json:3: Member 'Value' of 'Foo' should use lowercase
> +tests/qapi-schema/enum-member-case.json:3: 'Value' (member of Foo) should not use uppercase
> diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err
> index 6c6b740..0b4c1b5 100644
> --- a/tests/qapi-schema/union-branch-case.err
> +++ b/tests/qapi-schema/union-branch-case.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-branch-case.json:3: Member 'Branch' of 'Foo' should use lowercase
> +tests/qapi-schema/union-branch-case.json:3: 'Branch' (branch of Foo) should not use uppercase
>
--
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] 28+ messages in thread
* [Qemu-devel] [PATCH] fixup! qapi: Enforce (or whitelist) case conventions on qapi members
2015-12-02 11:51 ` Markus Armbruster
2015-12-02 13:41 ` Eric Blake
2015-12-02 14:11 ` Eric Blake
@ 2015-12-02 15:30 ` Eric Blake
2 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 15:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, Michael Roth
Whitelist must now include implicit names, and update the testsuite
to use names that match returns-whitelist
Signed-off-by: Eric Blake <eblake@redhat.com>
---
To be applied on top of Markus' fixup, if we indeed want to drop
info['name'] and allow implicit names in the whitelist.
scripts/qapi.py | 4 ++++
tests/qapi-schema/args-member-case.err | 2 +-
tests/qapi-schema/args-member-case.json | 5 +++--
tests/qapi-schema/enum-member-case.err | 2 +-
tests/qapi-schema/enum-member-case.json | 3 ++-
tests/qapi-schema/union-branch-case.err | 2 +-
tests/qapi-schema/union-branch-case.json | 5 +++--
7 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 131638a..bebd8c7 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -71,6 +71,10 @@ case_whitelist = [
'QapiErrorClass', # all members, visible through errors
'UuidInfo', # UUID, visible through query-uuid
'X86CPURegister32', # all members, visible indirectly through qom-get
+
+ # For use in the testsuite
+ ':obj-x-UuidInfo-arg', # args-member-case
+ 'x-UuidInfoKind', # union-branch-case
]
enum_types = []
diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err
index 44c31ea..aeab66d 100644
--- a/tests/qapi-schema/args-member-case.err
+++ b/tests/qapi-schema/args-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/args-member-case.json:3: 'Arg' (parameter of Foo) should not use uppercase
+tests/qapi-schema/args-member-case.json:4: 'Arg' (parameter of no-way-this-will-get-whitelisted) should not use uppercase
diff --git a/tests/qapi-schema/args-member-case.json b/tests/qapi-schema/args-member-case.json
index 1bc823a..c5cd07f 100644
--- a/tests/qapi-schema/args-member-case.json
+++ b/tests/qapi-schema/args-member-case.json
@@ -1,3 +1,4 @@
# Member names should be 'lower-case' unless the struct/command is whitelisted
-{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
-{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
+{ 'command': 'x-UuidInfo', 'data': { 'Arg': 'int' } }
+
+{ 'command': 'no-way-this-will-get-whitelisted', 'data': { 'Arg': 'int' } }
diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err
index a1d67c6..3c67a3a 100644
--- a/tests/qapi-schema/enum-member-case.err
+++ b/tests/qapi-schema/enum-member-case.err
@@ -1 +1 @@
-tests/qapi-schema/enum-member-case.json:3: 'Value' (member of Foo) should not use uppercase
+tests/qapi-schema/enum-member-case.json:4: 'Value' (member of NoWayThisWillGetWhitelisted) should not use uppercase
diff --git a/tests/qapi-schema/enum-member-case.json b/tests/qapi-schema/enum-member-case.json
index 5101275..cdede72 100644
--- a/tests/qapi-schema/enum-member-case.json
+++ b/tests/qapi-schema/enum-member-case.json
@@ -1,3 +1,4 @@
# Member names should be 'lower-case' unless the enum is whitelisted
{ 'enum': 'UuidInfo', 'data': [ 'Value' ] }
-{ 'enum': 'Foo', 'data': [ 'Value' ] }
+
+{ 'enum': 'NoWayThisWillGetWhitelisted', 'data': [ 'Value' ] }
diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err
index 0b4c1b5..34b13db 100644
--- a/tests/qapi-schema/union-branch-case.err
+++ b/tests/qapi-schema/union-branch-case.err
@@ -1 +1 @@
-tests/qapi-schema/union-branch-case.json:3: 'Branch' (branch of Foo) should not use uppercase
+tests/qapi-schema/union-branch-case.json:4: 'Branch' (branch of NoWayThisWillGetWhitelisted) should not use uppercase
diff --git a/tests/qapi-schema/union-branch-case.json b/tests/qapi-schema/union-branch-case.json
index a5951f1..880c173 100644
--- a/tests/qapi-schema/union-branch-case.json
+++ b/tests/qapi-schema/union-branch-case.json
@@ -1,3 +1,4 @@
# Branch names should be 'lower-case' unless the union is whitelisted
-{ 'union': 'UuidInfo', 'data': { 'Branch': 'int' } }
-{ 'union': 'Foo', 'data': { 'Branch': 'int' } }
+{ 'union': 'x-UuidInfo', 'data': { 'Branch': 'int' } }
+
+{ 'union': 'NoWayThisWillGetWhitelisted', 'data': { 'Branch': 'int' } }
--
2.4.3
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v14 14/15] qapi: Move duplicate collision checks to schema check()
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (12 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 13/15] qapi: Enforce (or whitelist) case conventions on qapi members Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 15/15] qapi: Detect base class loops Eric Blake
2015-12-02 17:57 ` [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Markus Armbruster
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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 3fc1ff5..0a616fc 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 c3fd3c1..25bd6b4 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] 28+ messages in thread* [Qemu-devel] [PATCH v14 15/15] qapi: Detect base class loops
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (13 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 14/15] qapi: Move duplicate collision checks to schema check() Eric Blake
@ 2015-12-02 5:20 ` Eric Blake
2015-12-02 17:57 ` [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Markus Armbruster
15 siblings, 0 replies; 28+ messages in thread
From: Eric Blake @ 2015-12-02 5:20 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>
---
v14: no change
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 0a616fc..56c8fb1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -941,7 +941,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 25bd6b4..477cc85 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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D)
2015-12-02 5:20 [Qemu-devel] [PATCH v14 00/15] qapi member collision (post-introspection cleanups, subset D) Eric Blake
` (14 preceding siblings ...)
2015-12-02 5:20 ` [Qemu-devel] [PATCH v14 15/15] qapi: Detect base class loops Eric Blake
@ 2015-12-02 17:57 ` Markus Armbruster
15 siblings, 0 replies; 28+ messages in thread
From: Markus Armbruster @ 2015-12-02 17:57 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
With PATCH 12 dropped and 13 fixed up, I'm happy with this series.
Pushed to qapi-not-next, with the fixup still separate for now.
^ permalink raw reply [flat|nested] 28+ messages in thread