Re: [PATCH v7 09/65] qapi: add type-infos generator

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> New QAPISchemaGenTypeInfoVisitor produces per-module
> qapi-type-infos-*.h/c files. Each file declares QAPITypeInfo constants
> pairing the QAPI type name with its masked introspection name.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  docs/devel/qapi-code-gen.rst |  92 ++++++++++++++++++++++++
>  meson.build                  |   1 +
>  scripts/qapi/backend.py      |   2 +
>  scripts/qapi/type_infos.py   | 163 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 258 insertions(+)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index 77f08bb68cac..1cc5338a210a 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -1250,6 +1250,9 @@ JSON array of strings.
>  To examine a type, start with a command or event using it, then follow
>  references by name.
>  
> +.. TODO document QOM introspection (qom-list, qom-list-properties,
> +   qom-get) and how it relates to QAPI type information.
> +
>  The SchemaInfo for a command has meta-type "command", and variant
>  members "arg-type", "ret-type" and "allow-oob".  On the wire, the
>  "arguments" member of a client's "execute" command must conform to the
> @@ -2099,3 +2102,92 @@ Example::
>      }));
>  
>      [Uninteresting stuff omitted...]
> +
> +
> +Code generated for type information
> +-----------------------------------
> +
> +Type information is generated for user-defined types and their array
> +types.  With option --builtins, it is also generated for the built-in
> +types.
> +
> +The following files are created:
> +
> + ``$(prefix)qapi-type-infos.c``
> +     A ``QAPITypeInfo`` instance for each type listed above, providing
> +     a mapping between the QAPI type name and the masked name used by
> +     introspection, along with optional enum lookup table and array
> +     type pointers.
> +
> + ``$(prefix)qapi-type-infos.h``
> +     Declarations for the above type info instances
> +
> +Each ``QAPITypeInfo`` struct has the following fields:
> +
> +``name``
> +    The QAPI type name (e.g. ``"UserDefOne"``).
> +
> +``masked_name``
> +    The masked name used in ``query-qmp-schema`` output.
> +
> +``lookup``
> +    For enum types, a pointer to the corresponding ``QEnumLookup``
> +    table.  ``NULL`` for non-enum types.
> +
> +``list``
> +    For types that have an array variant, a pointer to the array type's
> +    ``QAPITypeInfo``.  ``NULL`` when no array type exists.
> +
> +These type info instances are used by QOM property registration
> +functions (``object_property_add_qapi()``,
> +``object_class_property_add_qapi_enum()``, etc.) to associate each
> +property with its QAPI type.  The ``qom-list`` and
> +``device-list-properties`` QMP commands then expose the ``qapi-type``
> +field, giving management tools a formal type reference they can look
> +up in the introspection schema.
> +
> +Example::
> +
> +    $ cat qapi-generated/example-qapi-type-infos.h
> +    [Uninteresting stuff omitted...]
> +
> +    #ifndef EXAMPLE_QAPI_TYPE_INFOS_H
> +    #define EXAMPLE_QAPI_TYPE_INFOS_H
> +
> +    #include "qapi/qapi-builtin-type-infos.h"
> +
> +    extern const QAPITypeInfo UserDefOne_type_info;
> +
> +    extern const QAPITypeInfo UserDefOneList_type_info;
> +
> +    #endif /* EXAMPLE_QAPI_TYPE_INFOS_H */
> +    $ cat qapi-generated/example-qapi-type-infos.c
> +    [Uninteresting stuff omitted...]
> +
> +    const QAPITypeInfo UserDefOne_type_info = {
> +        .name = "UserDefOne",
> +        .masked_name = "1",

Code generated right now has "32".  Doesn't matter to readers, but using
generated code verbatim eases maintenance a bit.


> +        .list = &UserDefOneList_type_info,
> +    };
> +
> +    const QAPITypeInfo UserDefOneList_type_info = {
> +        .name = "UserDefOneList",
> +        .masked_name = "[1]",

Likewise.

> +    };
> +
> +    [Uninteresting stuff omitted...]
> +
> +For a modular QAPI schema (see section `Include directives`_), code for
> +each sub-module SUBDIR/SUBMODULE.json is actually generated into ::
> +
> + SUBDIR/$(prefix)qapi-type-infos-SUBMODULE.h
> + SUBDIR/$(prefix)qapi-type-infos-SUBMODULE.c
> +
> +If qapi-gen.py is run with option --builtins, additional files are
> +created:
> +
> + ``qapi-builtin-type-infos.h``
> +     Type info instances for built-in types
> +
> + ``qapi-builtin-type-infos.c``
> +     Definitions for the above type info instances
> diff --git a/meson.build b/meson.build
> index bbef9d9c2932..e51fb7a37740 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3510,6 +3510,7 @@ qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py',
>                       meson.current_source_dir() / 'scripts/qapi/parser.py',
>                       meson.current_source_dir() / 'scripts/qapi/schema.py',
>                       meson.current_source_dir() / 'scripts/qapi/source.py',
> +                     meson.current_source_dir() / 'scripts/qapi/type_infos.py',
>                       meson.current_source_dir() / 'scripts/qapi/types.py',
>                       meson.current_source_dir() / 'scripts/qapi/visit.py',
>                       meson.current_source_dir() / 'scripts/qapi-gen.py'
> diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
> index 49ae6ecdd33e..508ab2173539 100644
> --- a/scripts/qapi/backend.py
> +++ b/scripts/qapi/backend.py
> @@ -8,6 +8,7 @@
>  from .features import gen_features
>  from .introspect import gen_introspect
>  from .schema import QAPISchema
> +from .type_infos import gen_type_infos
>  from .types import gen_types
>  from .visit import gen_visit
>  
> @@ -63,3 +64,4 @@ def generate(self,
>          gen_commands(schema, output_dir, prefix, gen_tracing)
>          gen_events(schema, output_dir, prefix)
>          gen_introspect(schema, output_dir, prefix, unmask)
> +        gen_type_infos(schema, output_dir, prefix, builtins, unmask)
> diff --git a/scripts/qapi/type_infos.py b/scripts/qapi/type_infos.py
> new file mode 100644
> index 000000000000..b29e31d92640
> --- /dev/null
> +++ b/scripts/qapi/type_infos.py
> @@ -0,0 +1,163 @@
> +"""
> +QAPI type info generator
> +
> +SPDX-License-Identifier: GPL-2.0-or-later
> +"""
> +
> +from typing import List, Optional
> +
> +from .common import c_name, mcgen
> +from .gen import QAPISchemaModularCVisitor, ifcontext
> +from .schema import (
> +    QAPISchema,
> +    QAPISchemaAlternatives,
> +    QAPISchemaBranches,
> +    QAPISchemaEnumMember,
> +    QAPISchemaFeature,
> +    QAPISchemaIfCond,
> +    QAPISchemaObjectType,
> +    QAPISchemaObjectTypeMember,
> +    QAPISchemaType,
> +)
> +from .source import QAPISourceInfo
> +
> +
> +class QAPISchemaGenTypeInfoVisitor(QAPISchemaModularCVisitor):
> +
> +    def __init__(self, prefix: str, unmask: bool):
> +        super().__init__(
> +            prefix, 'qapi-type-infos',
> +            ' * Schema-defined QAPI type info',
> +            ' * Built-in QAPI type info', __doc__)
> +        self._unmask = unmask
> +        self._schema: Optional[QAPISchema] = None
> +
> +    def visit_begin(self, schema: QAPISchema) -> None:
> +        super().visit_begin(schema)
> +        self._schema = schema
> +
> +    def _begin_builtin_module(self) -> None:
> +        self._genc.preamble_add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-builtin-types.h"
> +#include "qapi/qapi-builtin-type-infos.h"
> +'''))
> +        self._genh.preamble_add(mcgen('''
> +#include "qapi/qapi-type-info.h"
> +'''))
> +
> +    def _begin_user_module(self, name: str) -> None:
> +        type_infos = self._module_basename('qapi-type-infos', name)
> +        types = self._module_basename('qapi-types', name)
> +        self._genc.preamble_add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "%(types)s.h"
> +#include "%(type_infos)s.h"
> +''',
> +                                      types=types,
> +                                      type_infos=type_infos))
> +        self._genh.preamble_add(mcgen('''
> +#include "qapi/qapi-builtin-type-infos.h"
> +'''))
> +
> +    def _has_list(self, name: str) -> bool:
> +        assert self._schema is not None
> +        return self._schema.lookup_type(name + 'List') is not None

Relies on QAPISchema._make_array_type()'s

           name = element_type + 'List'    # reserved by check_defn_name_str()

Suggest to point to ._make_array_type() with a comment.

There's another name + 'List' below, at [*].  One comment here could do,
though.

> +
> +    def _masked_name(self, name: str) -> str:
> +        assert self._schema is not None
> +        return self._schema.definition_introspection_name(name, self._unmask)
> +
> +    def _gen_type_info(self, name: str,
> +                       ifcond: Optional[QAPISchemaIfCond] = None,
> +                       with_lookup: bool = False,
> +                       with_list: bool = False) -> None:
> +        c_id = c_name(name + '_type_info')
> +        masked_name_str = '"%s"' % self._masked_name(name)
> +        with ifcontext(ifcond or QAPISchemaIfCond(),
> +                       self._genh, self._genc):
> +            self._genh.add(mcgen('''
> +
> +extern const QAPITypeInfo %(c_id)s;
> +''',
> +                                 c_id=c_id))
> +            self._genc.add(mcgen('''
> +
> +const QAPITypeInfo %(c_id)s = {
> +    .name = "%(name)s",
> +    .masked_name = %(masked_name)s,
> +''',
> +                                 c_id=c_id, name=name,
> +                                 masked_name=masked_name_str))

Simpler and clearer, I think:

       .masked_name = "%(masked_name)s",
                                    ...
                                    masked_name= self._masked_name(name))

> +            if with_lookup:
> +                self._genc.add(mcgen('''
> +    .lookup = &%(c_name)s_lookup,
> +''',
> +                                     c_name=c_name(name)))
> +            if with_list:
> +                self._genc.add(mcgen('''
> +    .list = &%(list_id)s,
> +''',
> +                                     list_id=c_name(name + 'List_type_info')))

[*]

> +            self._genc.add(mcgen('''
> +};
> +'''))

Three variations of c_name(name maybe with suffix) in this function:

    c_name(name + '_type_info')
    c_name(name)
    c_name(name + 'List_type_info')

Let's put just

    cname = c_name(name)

at the beginning, and add the suffix within the template like we do in
events.py, types.py, and visit.py, i.e. like this:

               self._genh.add(mcgen('''

   extern const QAPITypeInfo %(cname)s_type_info;
   ''',
                                    cname=cname))

> +
> +    def visit_builtin_type(self,
> +                           name: str,
> +                           info: Optional[QAPISourceInfo],
> +                           json_type: str) -> None:
> +        self._gen_type_info(name, with_list=self._has_list(name))
> +
> +    def visit_enum_type(self,
> +                        name: str,
> +                        info: Optional[QAPISourceInfo],
> +                        ifcond: QAPISchemaIfCond,
> +                        features: List[QAPISchemaFeature],
> +                        members: List[QAPISchemaEnumMember],
> +                        prefix: Optional[str]) -> None:
> +        self._gen_type_info(name, ifcond,
> +                            with_lookup=True,
> +                            with_list=self._has_list(name))
> +
> +    def visit_array_type(self,
> +                         name: str,
> +                         info: Optional[QAPISourceInfo],
> +                         ifcond: QAPISchemaIfCond,
> +                         element_type: QAPISchemaType) -> None:
> +        self._gen_type_info(name, ifcond)
> +
> +    def visit_object_type(self,
> +                          name: str,
> +                          info: Optional[QAPISourceInfo],
> +                          ifcond: QAPISchemaIfCond,
> +                          features: List[QAPISchemaFeature],
> +                          base: Optional[QAPISchemaObjectType],
> +                          members: List[QAPISchemaObjectTypeMember],
> +                          branches: Optional[QAPISchemaBranches]) -> None:
> +        assert self._schema is not None
> +        typ = self._schema.lookup_type(name)
> +        assert typ
> +        if typ.is_implicit():
> +            return
> +        self._gen_type_info(name, ifcond,
> +                            with_list=self._has_list(name))
> +
> +    def visit_alternate_type(self,
> +                             name: str,
> +                             info: Optional[QAPISourceInfo],
> +                             ifcond: QAPISchemaIfCond,
> +                             features: List[QAPISchemaFeature],
> +                             alternatives: QAPISchemaAlternatives) -> None:
> +        self._gen_type_info(name, ifcond,
> +                            with_list=self._has_list(name))

All but one caller of ._gen_type_info() pass
with_list=self._has_list(name).  The exception is visit_array_type(),
where we let with_list default to False.  Since .has_list(name) would
surely be False there, we could do away with with_list and check
._has_list() in ._gen_type_info().

The array business complicates review of this patch and PATCH 07 a bit.
It isn't actually needed until PATCH 59.  I'd like you to delay it until
then to get it out of the way here:

* Create QAPITypeInfo without member @list in PATCH 07.

* Don't generate member @list in PATCH 09.

* Do both right before PATCH 59.

Might also enable quicker merging of the bulk of this series.

> +
> +
> +def gen_type_infos(schema: QAPISchema,
> +                   output_dir: str,
> +                   prefix: str,
> +                   opt_builtins: bool,
> +                   opt_unmask: bool) -> None:
> +    vis = QAPISchemaGenTypeInfoVisitor(prefix, opt_unmask)
> +    schema.visit(vis)
> +    vis.write(output_dir, opt_builtins)