Re: [PATCH v2 03/53] qapi: add type-infos generator

Marc-André Lureau <[email protected]> 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: Marc-André Lureau <[email protected]>
> ---
>  docs/devel/qapi-code-gen.rst |  90 +++++++++++++++++++++
>  meson.build                  |   1 +
>  scripts/qapi/type_infos.py   | 182 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index 3a632b4a648..bb2e8d9c675 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -2103,3 +2103,93 @@ Example::
>      }));
>  
>      [Uninteresting stuff omitted...]
> +
> +
> +Code generated for type information
> +-----------------------------------
> +
> +The following files are created:
> +
> + ``$(prefix)qapi-type-infos.c``
> +     A ``QAPITypeInfo`` instance for each schema-defined type, providing
> +     a mapping between the C type name and the schema name used by
> +     introspection, along with optional enum lookup table and list-element
> +     pointers.
> +
> + ``$(prefix)qapi-type-infos.h``
> +     Declarations for the above type info instances
> +
> +Each ``QAPITypeInfo`` struct has the following fields:
> +
> +``name``
> +    The C identifier of the type (e.g. ``"UserDefOne"``).

Isn't this the name used in the QAPI schema source?

> +
> +``schema_name``
> +    The masked name used in ``query-qmp-schema`` output, or ``NULL``
> +    for built-in types whose schema name equals their C name.  This
> +    allows management tools to cross-reference a QOM property's
> +    ``qapi-type`` against the introspection schema.

I believe it can also be null when query-qmp-schema omits the type.
More on that below.

> +
> +``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 list type's
> +    ``QAPITypeInfo``.  ``NULL`` when no list type exists.

Confusing mix of "array" and "list" in the same sentence.  Suggest "the
array type's".

QAPI can't decide whether to call the thing an array (like JSON) or a
list (like the C implementation).  Unfortunate.
docs/devel/qapi-code-gen.rst generally sticks to array, but there are a
few lapses.

> +
> +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 schema type.

You're using "schema type" like you do above, i.e. for the masked type
name.  Correct?

>                                       The ``qom-list`` and
> +``device-list-properties`` QMP commands then expose the ``qapi-type``
> +field, giving management tools a formal type reference into the
> +introspection schema.

As far as I understand, we connect a property of QAPI type to the type's
QAPITypeInfo, the commands you mentioned then use it to emit the masked
type name, so that management applications can use that to find a
property's type in the output of query-qmp-schema.

Problem (already discussed elsewhere): only works when the type is used
in QMP.  If it isn't, we still have QAPITypeInfo, but its .schema_name
is null.  This needs to be documented as a known shortcoming.

Other uses of QAPITypeInfo may emerge in the future.  We might also
replace it by something that supports additional uses; see my ramblings
in review of PATCH 01.  In this series, however, connecting QOM
properties to their masked QAPI type name is their only use.  I
understand we emit QAPITypeInfo for any QAPI type that could be used as
QOM property type.  Dead data unless it is actually used as QOM property
type.  This is acceptable; we already generate similarly dead visitors.
But it's worth mentioning in the commit message, I think.

> +
> +Implicit types (names starting with ``q_``) are skipped.

This isn't quite correct.  Array types are also implicitly defined, and
their names don't start with 'q_'.

Possible fixes:

* Drop the sentence (only if we believe it's not needed)

* Drop the parenthesis, add "other than arrays" or something like that.

* Positive instead of negative: only user-defined and built-in types.
  This could perhaps be worked into the text under "The following files
  are created".  May want to add why: only these may be used as property
  types.

> +
> +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",
> +        .schema_name = "1",
> +        .list = &UserDefOneList_type_info,
> +    };
> +
> +    const QAPITypeInfo UserDefOneList_type_info = {
> +        .name = "UserDefOneList",
> +        .schema_name = "[1]",
> +    };
> +
> +    [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 19e123423b5..f135eddad12 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3487,6 +3487,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/type_infos.py b/scripts/qapi/type_infos.py
> new file mode 100644
> index 00000000000..dc7e2bace9e
> --- /dev/null
> +++ b/scripts/qapi/type_infos.py

Please use '-' instead of '_' in filenames for local consistency.

> @@ -0,0 +1,182 @@
> +"""
> +QAPI type info generator
> +
> +SPDX-License-Identifier: GPL-2.0-or-later
> +"""
> +
> +from typing import (
> +    Dict,
> +    List,
> +    Optional,
> +    Set,
> +)
> +
> +from .common import c_name, mcgen
> +from .gen import QAPISchemaModularCVisitor, ifcontext
> +from .schema import (
> +    QAPISchema,
> +    QAPISchemaAlternatives,
> +    QAPISchemaBranches,
> +    QAPISchemaEnumMember,
> +    QAPISchemaFeature,
> +    QAPISchemaIfCond,
> +    QAPISchemaObjectType,
> +    QAPISchemaObjectTypeMember,
> +    QAPISchemaType,
> +    QAPISchemaVisitor,
> +)
> +from .source import QAPISourceInfo
> +
> +
> +class _ArrayTypeCollector(QAPISchemaVisitor):
> +    def __init__(self) -> None:
> +        self.list_types: Set[str] = set()
> +
> +    def visit_array_type(self,
> +                         name: str,
> +                         info: Optional[QAPISourceInfo],
> +                         ifcond: QAPISchemaIfCond,
> +                         element_type: QAPISchemaType) -> None:
> +        self.list_types.add(name)
> +
> +
> +class QAPISchemaGenTypeInfoVisitor(QAPISchemaModularCVisitor):
> +
> +    def __init__(self, prefix: str, name_map: Dict[str, str],
> +                 list_types: Set[str]):
> +        super().__init__(
> +            prefix, 'qapi-type-infos',
> +            ' * Schema-defined QAPI type info',
> +            ' * Built-in QAPI type info', __doc__)
> +        self._name_map = name_map
> +        self._list_types = list_types
> +
> +    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 _gen_type_info(self, name: str,
> +                       ifcond: Optional[QAPISchemaIfCond] = None,
> +                       with_lookup: bool = False,
> +                       with_list: bool = False,
> +                       schema_name: Optional[str] = None) -> None:
> +        c_id = c_name(name + '_type_info')
> +        lookup = ''
> +        if with_lookup:
> +            lookup = mcgen('''
> +    .lookup = &%(c_name)s_lookup,
> +''',
> +                           c_name=c_name(name))
> +        list_ref = ''
> +        if with_list:
> +            list_ref = mcgen('''
> +    .list = &%(list_id)s,
> +''',
> +                             list_id=c_name(name + 'List_type_info'))
> +        if schema_name is None:
> +            masked = self._name_map.get(name)
> +            schema_name = '"%s"' % masked if masked is not None else 'NULL'
> +        else:
> +            schema_name = '"%s"' % schema_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",
> +    .schema_name = %(schema_name)s,
> +%(lookup)s%(list_ref)s};

This substitutes values of mcgen() into mcgen().  That's not how it
wants to be used.  Please ...

> +''',
> +                                 c_id=c_id, name=name,
> +                                 schema_name=schema_name,
> +                                 lookup=lookup,
> +                                 list_ref=list_ref))

... try something like

               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",
       .schema_name = %(schema_name)s,
   ''',
                                    c_id=c_id, name=name,
                                    schema_name=schema_name))

               if with_lookup:
                   self._genc.add(mcgen('''
       .lookup = &%(c_name)s_lookup,
   ''',
                              c_name=c_name(name))

and so forth.

> +
> +    def _has_list(self, name: str) -> bool:
> +        return name + 'List' in self._list_types

Simpler, I think:

           return self._schema.lookup_type(name + 'List')

where self._schema is set in .visit_begin().

> +
> +    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_object_type(self,
> +                          name: str,
> +                          info: Optional[QAPISourceInfo],
> +                          ifcond: QAPISchemaIfCond,
> +                          features: List[QAPISchemaFeature],
> +                          base: Optional[QAPISchemaObjectType],
> +                          members: List[QAPISchemaObjectTypeMember],
> +                          branches: Optional[QAPISchemaBranches]) -> None:
> +        if name.startswith('q_'):

Duplicates QAPISchemaObjectType.is_implicit().  Similar code in types.py
and visit.py.  Not this patch's fault; moving on.

> +            return
> +        self._gen_type_info(name, ifcond,
> +                            with_list=self._has_list(name))
> +
> +    def visit_array_type(self,
> +                         name: str,
> +                         info: Optional[QAPISourceInfo],
> +                         ifcond: QAPISchemaIfCond,
> +                         element_type: QAPISchemaType) -> None:
> +        elem_schema = self._name_map.get(element_type.name,
> +                                         element_type.name)
> +        self._gen_type_info(name, ifcond,
> +                            schema_name='[' + elem_schema + ']')
> +
> +    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))
> +
> +
> +def gen_type_infos(schema: QAPISchema,
> +                   output_dir: str,
> +                   prefix: str,
> +                   opt_builtins: bool,
> +                   name_map: Dict[str, str]) -> None:
> +    collector = _ArrayTypeCollector()
> +    schema.visit(collector)
> +    vis = QAPISchemaGenTypeInfoVisitor(prefix, name_map,
> +                                       collector.list_types)
> +    schema.visit(vis)
> +    vis.write(output_dir, opt_builtins)