| Message ID | 1447224690-9743-22-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > > --- > 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 <stddef.h> > #include <assert.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_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 <stdbool.h> > #include <stdint.h> > -#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]
On 11/11/2015 09:42 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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? It guarantees I found all affected instances. (Although I guess the rename could be split to a separate patch from making it builtin). It makes sure that if we later tighten rules about naming, we won't have to whitelist 'qtype_code' as an anomaly to our conventions. > > If we rename anyway, what about renaming to QType? Hmm, we burned that > on a struct we use only internally in qobject/. Oh well. Internal structs are often easy to rename. So if we want to avoid the need for 'prefix', I could certainly try to achieve that (move internal QType out of the way, then rename qtype_code to QType, then make QType the builtin). Looks like this one patch just became three :) > >> 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 I've got to quit coding late at night - my rate of typos increases :) >> +++ 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. > >> - >> -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. Oh, I see what you're saying. Since qapi-types.h now has a forward declaration of the QObject typedef, this could be changed to just struct QObject { ... }; >> +++ 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. Feels a bit hacky, since we just recently added is_implicit() to hide (and then change) the 'if not info' check on objects. Maybe an accessor is_builtin() makes more sense? But yes, same approach to both client files. >> -#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 > I favor dropping the second typedef. >> +++ 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? Yeah, probably. This array is only used by the ad hoc parser, and may disappear later as we move more into check(), but we should be consistent in the meantime. > >> # 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] I debated about hacking tests/qapi-schema/test-qapi.py to omit QTypeCode (the way we already omit builtin types and things like 'intList'), for less churn in the .out files. I can go either way, if you have a preference.
Eric Blake <eblake@redhat.com> writes: > On 11/11/2015 09:42 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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? > > It guarantees I found all affected instances. (Although I guess the > rename could be split to a separate patch from making it builtin). Well, you have to find them only because you rename, don't you? > It makes sure that if we later tighten rules about naming, we won't have > to whitelist 'qtype_code' as an anomaly to our conventions. Good point. >> If we rename anyway, what about renaming to QType? Hmm, we burned that >> on a struct we use only internally in qobject/. Oh well. > > Internal structs are often easy to rename. So if we want to avoid the > need for 'prefix', I could certainly try to achieve that (move internal > QType out of the way, then rename qtype_code to QType, then make QType > the builtin). Looks like this one patch just became three :) Not sure it's worth the bother; the patch is okay as it is. QType is overkill. Instead of typedef struct QType { qtype_code code; void (*destroy)(struct QObject *); } QType; typedef struct QObject { const QType *type; size_t refcnt; } QObject; we could simply have typedef struct QObject { QTypeCode type; size_t refcnt; } QObject; with an array mapping QTypeCode to destroy methods. We're not going to define additional types at run time. Perhaps such a change would be actually worth the bother. >>> 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 > > I've got to quit coding late at night - my rate of typos increases :) > >>> +++ 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. >> > >>> - >>> -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. > > Oh, I see what you're saying. Since qapi-types.h now has a forward > declaration of the QObject typedef, this could be changed to just > > struct QObject { > ... > }; > >>> +++ 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. > > Feels a bit hacky, since we just recently added is_implicit() to hide > (and then change) the 'if not info' check on objects. Maybe an accessor > is_builtin() makes more sense? But yes, same approach to both client files. QAPISchemaEntity methods like is_implicit() or a new is_builtin() can't work here, because we lack the entity. We have one in visit_needed(), and we use its is_implicit() to skip implicit object types. We could use entity.is_builtin() to skip (some) builtins, and handle them elsewhere, but that doesn't feel like an improvement over your code. Let's take a step back and reconsider how we do builtins. >> + 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): Linking generated code from multiple schemata that share names may fail, because multiple definitions of the same external symbol exist. Example: two schemata both define enum BadIdea. Both generate const char *BadIdea_lookup[] = { ... }, and we end up with two global symbols BadIdea_lookup. Solution: don't do that then. Easy enough, except *all* schemata share the builtin symbols! Solution: 1. For declarations, use ifdeffery to make the compiler ignore all but the first copy it encounters, 2. For definitions, make the programmer pick one schema to generate the definitions, and run qapi-types.py and qapi-visit.py with -b. In generator code, this looks like self._btin += ... declarations ... if do_builtins: self.defn += ... definitions ... instead of the normal self.decl += ... declarations ... self.defn += ... declarations ... (or the same with ._fwdecl, ._fwdefn, doesn't matter). This is why you need to know whether the enum is builtin in .visit_enum_type() above. The builtin definitions are emitted into a suitable #ifdef block by bracketing this code with an initial self._btin = guardstart('QAPI_TYPES_BUILTIN') and a final self._btin += guardend('QAPI_TYPES_BUILTIN') self.decl = self._btin + self.decl self._btin = None Here's an alternative solution that permits slightly code simpler generator code, and thus avoids the need to know: * Generate code for builtins exactly the same as for any other entities, i.e. get rid of self._btin and the ifdeffery. * If the program links just one generated schema, this just works. * If the program links multiple generated schemata, the programmer has to ensure their definitions get generated just once, and their declarations are available everywhere anyway. Straightforward method: - The programmer suppresses builtins *completely* for *all* schemata. The obvious way to suppress them is to filter them out in visit_needed(). - Instead, he generates them once for the *empty* schema, with a well-known --prefix. - Suppressing builtins generates a suitable #include for the well-known .h with the builtin declarations. - Additionally link the .c containing the builtin definitions. Alternatively, trade some ease-of-use for the single schema case for ease-of-use for the multiple schemata case and fewer cases: * The generators either generate for a schema, or they generate builtins. * When they generate builtins, they always use well-known file names. * When they generate for a schema, they always generate the #include for the well-known builtin .h. They never generate builtins. >>> -#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 >> > > I favor dropping the second typedef. Your choice. >>> +++ 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? > > Yeah, probably. This array is only used by the ad hoc parser, and may > disappear later as we move more into check(), but we should be > consistent in the meantime. > >> >>> # 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] > > I debated about hacking tests/qapi-schema/test-qapi.py to omit QTypeCode > (the way we already omit builtin types and things like 'intList'), for > less churn in the .out files. I can go either way, if you have a > preference. Omit them only if it's trivial. I guess it would be trivial if we adopted the alternative way to do builtins I sketched above.
On 11/12/2015 06:16 AM, Markus Armbruster wrote: > We have one in visit_needed(), and we use its is_implicit() to skip > implicit object types. We could use entity.is_builtin() to skip (some) > builtins, and handle them elsewhere, but that doesn't feel like an > improvement over your code. > > Let's take a step back and reconsider how we do builtins. > >>> + 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): > > Linking generated code from multiple schemata that share names may fail, > because multiple definitions of the same external symbol exist. > > Example: two schemata both define enum BadIdea. Both generate const > char *BadIdea_lookup[] = { ... }, and we end up with two global symbols > BadIdea_lookup. > > Solution: don't do that then. Easy enough, except *all* schemata share > the builtin symbols! Solution: [...] Yes, the existing solution is a bit ugly. We don't even need the secondary test-qapi-types.h to declare builtins, since it is relying on the top-level qapi-types.h to do it, so we are just wasting disk space. > Here's an alternative solution that permits slightly code simpler > generator code, and thus avoids the need to know: > > * Generate code for builtins exactly the same as for any other entities, > i.e. get rid of self._btin and the ifdeffery. > > * If the program links just one generated schema, this just works. > > * If the program links multiple generated schemata, the programmer has > to ensure their definitions get generated just once, and their > declarations are available everywhere anyway. Straightforward method: > > - The programmer suppresses builtins *completely* for *all* schemata. > The obvious way to suppress them is to filter them out in > visit_needed(). > > - Instead, he generates them once for the *empty* schema, with a > well-known --prefix. > > - Suppressing builtins generates a suitable #include for the > well-known .h with the builtin declarations. > > - Additionally link the .c containing the builtin definitions. Reasonably nice ideas. I'll play with them later, though, in the interest of getting the existing patch queue flushed first. > > Alternatively, trade some ease-of-use for the single schema case for > ease-of-use for the multiple schemata case and fewer cases: > > * The generators either generate for a schema, or they generate builtins. > > * When they generate builtins, they always use well-known file names. > > * When they generate for a schema, they always generate the #include for > the well-known builtin .h. They never generate builtins. I'd probably go with this variant (that is, set up the makefiles to run once with -b to generate builtins, and multiple times (one per .json) without -b to generate non-builtins.
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 === 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 <stddef.h> #include <assert.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_code; - -struct QObject; +#include "qapi-types.h" typedef struct QType { - qtype_code code; + QTypeCode code; void (*destroy)(struct QObject *); } QType; @@ -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': + 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 <stdbool.h> #include <stdint.h> -#include "qapi/qmp/qobject.h" + +typedef struct QObject QObject; ''')) 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 } # 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() diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out index 0f153b6..ab5a629 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 QTypeCode ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] + prefix QTYPE diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out index 9e2c656..59904ef 100644 --- a/tests/qapi-schema/comments.out +++ b/tests/qapi-schema/comments.out @@ -1,2 +1,4 @@ object :empty +enum QTypeCode ['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..42121b5 100644 --- a/tests/qapi-schema/empty.out +++ b/tests/qapi-schema/empty.out @@ -1 +1,3 @@ object :empty +enum QTypeCode ['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..5e4d10e 100644 --- a/tests/qapi-schema/event-case.out +++ b/tests/qapi-schema/event-case.out @@ -1,2 +1,4 @@ object :empty +enum QTypeCode ['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..2fd07b9 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 QTypeCode ['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..523eec9 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 QTypeCode ['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..59904ef 100644 --- a/tests/qapi-schema/include-relpath.out +++ b/tests/qapi-schema/include-relpath.out @@ -1,2 +1,4 @@ object :empty +enum QTypeCode ['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..59904ef 100644 --- a/tests/qapi-schema/include-repetition.out +++ b/tests/qapi-schema/include-repetition.out @@ -1,2 +1,4 @@ object :empty +enum QTypeCode ['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..59904ef 100644 --- a/tests/qapi-schema/include-simple.out +++ b/tests/qapi-schema/include-simple.out @@ -1,2 +1,4 @@ object :empty +enum QTypeCode ['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..b7e49fc 100644 --- a/tests/qapi-schema/indented-expr.out +++ b/tests/qapi-schema/indented-expr.out @@ -1,4 +1,6 @@ object :empty +enum QTypeCode ['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 786024e..d05fc37 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 QTypeCode ['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..e4a2932 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 QTypeCode ['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..5e7764c 100644 --- a/tests/qapi-schema/union-empty.out +++ b/tests/qapi-schema/union-empty.out @@ -1,4 +1,6 @@ object :empty +enum QTypeCode ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool'] + prefix QTYPE object Union member type: UnionKind optional=False enum UnionKind []
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. 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 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 <eblake@redhat.com> --- 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(-)