From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39946) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwYTz-0004NF-Pg for qemu-devel@nongnu.org; Wed, 11 Nov 2015 11:42:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwYTx-00077Z-O9 for qemu-devel@nongnu.org; Wed, 11 Nov 2015 11:42:51 -0500 From: Markus Armbruster References: <1447224690-9743-1-git-send-email-eblake@redhat.com> <1447224690-9743-22-git-send-email-eblake@redhat.com> Date: Wed, 11 Nov 2015 17:42:37 +0100 In-Reply-To: <1447224690-9743-22-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 10 Nov 2015 23:51:23 -0700") Message-ID: <8761188p36.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v11 21/28] qapi: Convert qtype_code into qapi enum type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Michael Roth , qemu-devel@nongnu.org, "open list:Block layer core" , Luiz Capitulino Eric Blake writes: > What's more meta than using qapi to define qapi? :) > > Convert qtype_code 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. Doing so is easiest when renaming > it to qapi conventions, as QTypeCode. Out of curiosity: why does the rename make the conversion easier? If we rename anyway, what about renaming to QType? Hmm, we burned that on a struct we use only internally in qobject/. Oh well. > Fortunately, there are not > many places in the tree that were actually spelling the type name > out, and the judicious use of 'prefix' in the qapi defintion 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 QType requires QTypeCode 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 > > --- > v11: new patch > --- > block/qapi.c | 4 ++-- > docs/qapi-code-gen.txt | 1 + > include/hw/qdev-core.h | 2 +- > include/qapi/qmp/qobject.h | 19 +++---------------- > qobject/qdict.c | 2 +- > scripts/qapi-types.py | 13 ++++++++++--- > scripts/qapi-visit.py | 10 ++++++++-- > scripts/qapi.py | 7 ++++++- > tests/qapi-schema/alternate-empty.out | 2 ++ > tests/qapi-schema/comments.out | 2 ++ > tests/qapi-schema/empty.out | 2 ++ > tests/qapi-schema/event-case.out | 2 ++ > tests/qapi-schema/flat-union-empty.out | 2 ++ > tests/qapi-schema/ident-with-escape.out | 2 ++ > tests/qapi-schema/include-relpath.out | 2 ++ > tests/qapi-schema/include-repetition.out | 2 ++ > tests/qapi-schema/include-simple.out | 2 ++ > tests/qapi-schema/indented-expr.out | 2 ++ > tests/qapi-schema/qapi-schema-test.out | 2 ++ > tests/qapi-schema/union-clash-data.out | 2 ++ > tests/qapi-schema/union-empty.out | 2 ++ > 21 files changed, 58 insertions(+), 26 deletions(-) > > diff --git a/block/qapi.c b/block/qapi.c > index ec0f513..4211f11 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -539,7 +539,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); > + QTypeCode type = qobject_type(entry->value); > bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); > const char *format = composite ? "%*s[%i]:\n" : "%*s[%i]: "; > > @@ -557,7 +557,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); > + QTypeCode 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/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index 54a6a7b..35301c5 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -163,6 +163,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 > + QTypeCode QTypeCode JSON string of enum QTypeCode values QTypeCode is currently used only internally, so the JSON values don't matter. I don't expect that to change. However, we either enforce internal use somehow, or document the JSON values. Documenting them is easier. In short, your patch is fine. > > > === Includes === > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 8057aed..8e7df8e 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -239,7 +239,7 @@ struct Property { > PropertyInfo *info; > int offset; > uint8_t bitnr; > - qtype_code qtype; > + QTypeCode qtype; > int64_t defval; > int arrayoffset; > PropertyInfo *arrayinfo; > diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h > index 4b96ed5..8d6322b 100644 > --- a/include/qapi/qmp/qobject.h > +++ b/include/qapi/qmp/qobject.h > @@ -34,23 +34,10 @@ > > #include > #include > - > -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_code; > - > -struct QObject; > +#include "qapi-types.h" > > typedef struct QType { > - qtype_code code; > + QTypeCode code; > void (*destroy)(struct QObject *); > } QType; > typedef struct QObject { const QType *type; size_t refcnt; } QObject; Note: typedef name QObject still defined here. > @@ -101,7 +88,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 QTypeCode qobject_type(const QObject *obj) > { > assert(obj->type != NULL); > return obj->type->code; > diff --git a/qobject/qdict.c b/qobject/qdict.c > index 2d67bf1..92915f4 100644 > --- a/qobject/qdict.c > +++ b/qobject/qdict.c > @@ -185,7 +185,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) > + QTypeCode type) > { > QObject *obj; > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 2f2f7df..93e905a 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -233,8 +233,14 @@ 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 > + if name == 'QTypeCode': Would "if not info" work? Same in qapi-visit.py below. > + self._btin += gen_enum(name, values, prefix) > + if do_builtins: > + self.defn += gen_enum_lookup(name, values, prefix) > + else: > + self._fwdecl += gen_enum(name, values, prefix) > + self._fwdefn += gen_enum_lookup(name, values, prefix) > > def visit_array_type(self, name, info, element_type): > if isinstance(element_type, QAPISchemaBuiltinType): > @@ -319,7 +325,8 @@ fdef.write(mcgen(''' > fdecl.write(mcgen(''' > #include > #include > -#include "qapi/qmp/qobject.h" > + > +typedef struct QObject QObject; Typedef name QObject now also defined here. GCC accepts this silently without -Wpedantic, but other compilers might not. Whether we care for such compilers or not, defining things in exactly one place is neater. Possible fixes: * Drop the typedef from qobject.h * Don't add it to qapi-types.h, and use struct QObject there > ''')) > > schema = QAPISchema(input_file) > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 94cd113..6f0b4e1 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -347,8 +347,14 @@ 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 > + if name == 'QTypeCode': > + 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 ed7a32b..d4ef08e 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 QTypeCode possible, actually > } > Should we list QTypeCode here? > # Whitelist of commands allowed to return a non-dictionary > @@ -1243,6 +1243,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('QTypeCode', 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() [Trivial changes to expected test output snipped]