Re: [Qemu-devel] [PATCH v13 03/14] qapi: Convert QType into QAPI built-in enum type

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> What's more meta than using qapi to define qapi? :)
>
> Convert QType into a full-fledged[*] builtin qapi enum type, so
> that a subsequent patch can then use it as the discriminator
> type of qapi alternate types.  Fortunately, the judicious use of
> 'prefix' in the qapi definition avoids churn to the spelling of
> the enum constants.
>
> To avoid circular definitions, we have to flip the order of
> inclusion between "qobject.h" vs. "qapi-types.h".  Back in commit
> 28770e0, we had the latter include the former, so that we could
> use 'QObject *' for our implementation of 'any'.  But that usage
> also works with only a forward declaration, whereas the
> definition of QObject requires QType to be a complete type.
>
> [*] The type has to be builtin, rather than declared in
> qapi/common.json, because we want to use it for alternates even
> when common.json is not included. But since it is the first
> builtin enum type, we have to add special cases to qapi-types
> and qapi-visit to only emit definitions once, even when two
> qapi files are being compiled into the same binary (the way we
> already handled builtin list types like 'intList').  We may
> need to revisit how multiple qapi files share common types,
> but that's a project for another day.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v13: change title, use typedefs.h, rebase to cleaner QObject,
> rebase to Markus' typedefs.h re-sort
> v12: split out preparatory renames, retitle patch, use info rather
> than name comparison
> v11: new patch
> ---
>  docs/qapi-code-gen.txt                   |  1 +
>  include/qapi/qmp/qobject.h               | 17 +++--------------
>  include/qemu/typedefs.h                  |  1 +
>  qobject/qobject.c                        |  4 ++--
>  scripts/qapi-types.py                    | 15 +++++++++++----
>  scripts/qapi-visit.py                    | 11 +++++++++--
>  scripts/qapi.py                          |  6 ++++++
>  tests/qapi-schema/alternate-empty.out    |  2 ++
>  tests/qapi-schema/comments.out           |  2 ++
>  tests/qapi-schema/empty.out              |  2 ++
>  tests/qapi-schema/event-case.out         |  2 ++
>  tests/qapi-schema/flat-union-empty.out   |  2 ++
>  tests/qapi-schema/ident-with-escape.out  |  2 ++
>  tests/qapi-schema/include-relpath.out    |  2 ++
>  tests/qapi-schema/include-repetition.out |  2 ++
>  tests/qapi-schema/include-simple.out     |  2 ++
>  tests/qapi-schema/indented-expr.out      |  2 ++
>  tests/qapi-schema/qapi-schema-test.out   |  2 ++
>  tests/qapi-schema/union-clash-data.out   |  2 ++
>  tests/qapi-schema/union-empty.out        |  2 ++
>  20 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 2becba9..79bf072 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -160,6 +160,7 @@ The following types are predefined, and map to C as 
> follows:
>                         accepts size suffixes
>    bool      bool       JSON true or false
>    any       QObject *  any JSON value
> +  QType     QType      JSON string matching enum QType values
>
>
>  === Includes ===
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 525fb39..295aa76 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -34,23 +34,12 @@
>
>  #include <stddef.h>
>  #include <assert.h>
> +#include "qapi-types.h"

Needed for QType.  Risk for circular inclusion.  We're currently fine,
because generated qapi-types.h includes only "qemu/typedefs.h" (visible
below).  Should we add a comment to qapi-types.py?

>
> -typedef enum {
> -    QTYPE_NONE,    /* sentinel value, no QObject has this type code */
> -    QTYPE_QNULL,
> -    QTYPE_QINT,
> -    QTYPE_QSTRING,
> -    QTYPE_QDICT,
> -    QTYPE_QLIST,
> -    QTYPE_QFLOAT,
> -    QTYPE_QBOOL,
> -    QTYPE_MAX,
> -} QType;
> -
> -typedef struct QObject {
> +struct QObject {
>      QType type;
>      size_t refcnt;
> -} QObject;
> +};
>
>  typedef void (*QDestroy)(QObject *);
>
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 3eedcf4..78fe6e8 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -80,6 +80,7 @@ typedef struct QEMUSGList QEMUSGList;
>  typedef struct QEMUSizedBuffer QEMUSizedBuffer;
>  typedef struct QEMUTimer QEMUTimer;
>  typedef struct QEMUTimerListGroup QEMUTimerListGroup;
> +typedef struct QObject QObject;
>  typedef struct RAMBlock RAMBlock;
>  typedef struct Range Range;
>  typedef struct SerialState SerialState;
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> index 5325447..d0eb7f1 100644
> --- a/qobject/qobject.c
> +++ b/qobject/qobject.c
> @@ -15,7 +15,7 @@
>  #include "qapi/qmp/qlist.h"
>  #include "qapi/qmp/qstring.h"
>
> -static QDestroy qdestroy[QTYPE_MAX] = {
> +static QDestroy qdestroy[QTYPE__MAX] = {
>      [QTYPE_NONE] = NULL, /* No such object exists */
>      [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
>      [QTYPE_QINT] = qint_destroy_obj,
> @@ -29,6 +29,6 @@ static QDestroy qdestroy[QTYPE_MAX] = {
>  void qobject_destroy(QObject *obj)
>  {
>      assert(!obj->refcnt);
> -    assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
> +    assert(QTYPE_QNULL < obj->type && obj->type < QTYPE__MAX);
>      qdestroy[obj->type](obj);
>  }
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2f2f7df..82635b0 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -112,7 +112,7 @@ extern const int %(c_name)s_qtypes[];
>  def gen_alternate_qtypes(name, variants):
>      ret = mcgen('''
>
> -const int %(c_name)s_qtypes[QTYPE_MAX] = {
> +const int %(c_name)s_qtypes[QTYPE__MAX] = {
>  ''',
>                  c_name=c_name(name))
>
> @@ -233,8 +233,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self.defn += gen_type_cleanup(name)
>
>      def visit_enum_type(self, name, info, values, prefix):
> -        self._fwdecl += gen_enum(name, values, prefix)
> -        self._fwdefn += gen_enum_lookup(name, values, prefix)
> +        # Special case for our lone builtin enum type
> +        # TODO use something cleaner than existence of info
> +        if not info:
> +            self._btin += gen_enum(name, values, prefix)
> +            if do_builtins:
> +                self.defn += gen_enum_lookup(name, values, prefix)
> +        else:
> +            self._fwdecl += gen_enum(name, values, prefix)
> +            self._fwdefn += gen_enum_lookup(name, values, prefix)

Odd: gen_enum_lookup() goes into .defn for built-ins, but ._fwdefn for
user-defineds.  Makes me suspect it ._fwdefn isn't needed anymore.  A
quick test compile is happy with .defn for both.

If we want to keep ._fwdefn for some reason, we should use for built-ins
as well.

>
>      def visit_array_type(self, name, info, element_type):
>          if isinstance(element_type, QAPISchemaBuiltinType):
> @@ -319,7 +326,7 @@ fdef.write(mcgen('''
>  fdecl.write(mcgen('''
>  #include <stdbool.h>
>  #include <stdint.h>
> -#include "qapi/qmp/qobject.h"
> +#include "qemu/typedefs.h"
>  '''))
>

Here you can see the #include generated into qapi-types.h.

>  schema = QAPISchema(input_file)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 94cd113..7ceda18 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -347,8 +347,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>                      isinstance(entity, QAPISchemaObjectType))
>
>      def visit_enum_type(self, name, info, values, prefix):
> -        self.decl += gen_visit_decl(name, scalar=True)
> -        self.defn += gen_visit_enum(name)
> +        # Special case for our lone builtin enum type
> +        # TODO use something cleaner than existence of info
> +        if not info:
> +            self._btin += gen_visit_decl(name, scalar=True)
> +            if do_builtins:
> +                self.defn += gen_visit_enum(name)
> +        else:
> +            self.decl += gen_visit_decl(name, scalar=True)
> +            self.defn += gen_visit_enum(name)
>
>      def visit_array_type(self, name, info, element_type):
>          decl = gen_visit_decl(name)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 9fe9c95..3c8cb13 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -34,6 +34,7 @@ builtin_types = {
>      'uint64':   'QTYPE_QINT',
>      'size':     'QTYPE_QINT',
>      'any':      None,           # any QType possible, actually
> +    'QType':    'QTYPE_QSTRING',
>  }
>
>  # Whitelist of commands allowed to return a non-dictionary
> @@ -1243,6 +1244,11 @@ class QAPISchema(object):
>          self.the_empty_object_type = QAPISchemaObjectType(':empty', None, 
> None,
>                                                            [], None)
>          self._def_entity(self.the_empty_object_type)
> +        self._def_entity(QAPISchemaEnumType('QType', None,
> +                                            ['none', 'qnull', 'qint',
> +                                             'qstring', 'qdict', 'qlist',
> +                                             'qfloat', 'qbool'],
> +                                            'QTYPE'))
>
>      def _make_implicit_enum_type(self, name, info, values):
>          name = name + 'Kind'   # Use namespace reserved by add_name()
[...]