Re: [Qemu-block] [Qemu-devel] [PATCH v11 21/28] qapi: Convert qtype_code into qapi enum type

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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]