Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Markus Armbruster <armbru@redhat.com>]

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

> Hi
>
>
> On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Replace hard-coded "qemu/osdep.h" include with a qapi-gen option to
>> > specify the headers to include. This will allow to substitute QEMU
>> > osdep.h with glib.h for example, for projects with different
>> > global headers.
>> >
>> > For historical reasons, we can keep the default as "qemu/osdep.h".
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> I wish we'd all agree on "config.h" (conventional with autoconf).  But
>> we don't.
>>
>> Precedence for tweaking generated code with command line options: -p.
>>
>> I suspect that we'll always specify the same -p and -i for a given
>> schema.  To me, that suggests they should both go into the schema
>> instead.  Observation, not demand.
>>
>> > ---
>> >  scripts/qapi/commands.py   | 15 ++++++++++-----
>> >  scripts/qapi/events.py     | 17 +++++++++++------
>> >  scripts/qapi/gen.py        | 17 +++++++++++++++++
>> >  scripts/qapi/introspect.py | 11 +++++++----
>> >  scripts/qapi/main.py       | 17 +++++++++++------
>> >  scripts/qapi/types.py      | 17 +++++++++++------
>> >  scripts/qapi/visit.py      | 17 +++++++++++------
>> >  7 files changed, 78 insertions(+), 33 deletions(-)
>> >
>> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> > index 38ca38a7b9dd..781491b6390d 100644
>> > --- a/scripts/qapi/commands.py
>> > +++ b/scripts/qapi/commands.py
>> > @@ -294,9 +294,9 @@ def gen_register_command(name: str,
>> >
>> >
>> >  class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>> > -    def __init__(self, prefix: str, gen_tracing: bool):
>> > +    def __init__(self, prefix: str, include: List[str], gen_tracing: bool):
>>
>> Ignorant question: why ist this List[str], not str?  Do multiple options
>> accumulate into a list?
>>
>> Alright, I'm back from further down: looks like they do.
>>
>> >          super().__init__(
>> > -            prefix, 'qapi-commands',
>> > +            prefix, include, 'qapi-commands',
>> >              ' * Schema-defined QAPI/QMP commands', None, __doc__,
>> >              gen_tracing=gen_tracing)
>> >          self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
>> > @@ -308,7 +308,8 @@ def _begin_user_module(self, name: str) -> None:
>> >          types = self._module_basename('qapi-types', name)
>> >          visit = self._module_basename('qapi-visit', name)
>> >          self._genc.add(mcgen('''
>> > -#include "qemu/osdep.h"
>> > +%(include)s
>> > +
>> >  #include "qapi/compat-policy.h"
>> >  #include "qapi/visitor.h"
>> >  #include "qapi/qmp/qdict.h"
>> > @@ -318,6 +319,7 @@ def _begin_user_module(self, name: str) -> None:
>> >  #include "%(commands)s.h"
>> >
>> >  ''',
>> > +                             include=self.genc_include(),
>> >                               commands=commands, visit=visit))
>> >
>> >          if self._gen_tracing and commands != 'qapi-commands':
>> > @@ -344,7 +346,8 @@ def visit_begin(self, schema: QAPISchema) -> None:
>> >  ''',
>> >                               c_prefix=c_name(self._prefix, protect=False)))
>> >          self._genc.add(mcgen('''
>> > -#include "qemu/osdep.h"
>> > +%(include)s
>> > +
>> >  #include "%(prefix)sqapi-commands.h"
>> >  #include "%(prefix)sqapi-init-commands.h"
>> >
>> > @@ -353,6 +356,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
>> >      QTAILQ_INIT(cmds);
>> >
>> >  ''',
>> > +                             include=self.genc_include(),
>> >                               prefix=self._prefix,
>> >                               c_prefix=c_name(self._prefix, protect=False)))
>> >
>> > @@ -404,7 +408,8 @@ def visit_command(self,
>> >  def gen_commands(schema: QAPISchema,
>> >                   output_dir: str,
>> >                   prefix: str,
>> > +                 include: List[str],
>> >                   gen_tracing: bool) -> None:
>> > -    vis = QAPISchemaGenCommandVisitor(prefix, gen_tracing)
>> > +    vis = QAPISchemaGenCommandVisitor(prefix, include, gen_tracing)
>> >      schema.visit(vis)
>> >      vis.write(output_dir)
>> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> > index 27b44c49f5e9..6e677d11d2e0 100644
>> > --- a/scripts/qapi/events.py
>> > +++ b/scripts/qapi/events.py
>> > @@ -175,9 +175,9 @@ def gen_event_send(name: str,
>> >
>> >  class QAPISchemaGenEventVisitor(QAPISchemaModularCVisitor):
>> >
>> > -    def __init__(self, prefix: str):
>> > +    def __init__(self, prefix: str, include: List[str]):
>> >          super().__init__(
>> > -            prefix, 'qapi-events',
>> > +            prefix, include, 'qapi-events',
>> >              ' * Schema-defined QAPI/QMP events', None, __doc__)
>> >          self._event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>> >          self._event_enum_members: List[QAPISchemaEnumMember] = []
>> > @@ -188,7 +188,8 @@ def _begin_user_module(self, name: str) -> None:
>> >          types = self._module_basename('qapi-types', name)
>> >          visit = self._module_basename('qapi-visit', name)
>> >          self._genc.add(mcgen('''
>> > -#include "qemu/osdep.h"
>> > +%(include)s
>> > +
>> >  #include "%(prefix)sqapi-emit-events.h"
>> >  #include "%(events)s.h"
>> >  #include "%(visit)s.h"
>> > @@ -198,6 +199,7 @@ def _begin_user_module(self, name: str) -> None:
>> >  #include "qapi/qmp-event.h"
>> >
>> >  ''',
>> > +                             include=self.genc_include(),
>> >                               events=events, visit=visit,
>> >                               prefix=self._prefix))
>> >          self._genh.add(mcgen('''
>> > @@ -209,9 +211,11 @@ def _begin_user_module(self, name: str) -> None:
>> >      def visit_end(self) -> None:
>> >          self._add_module('./emit', ' * QAPI Events emission')
>> >          self._genc.preamble_add(mcgen('''
>> > -#include "qemu/osdep.h"
>> > +%(include)s
>> > +
>> >  #include "%(prefix)sqapi-emit-events.h"
>> >  ''',
>> > +                                      include=self.genc_include(),
>> >                                        prefix=self._prefix))
>> >          self._genh.preamble_add(mcgen('''
>> >  #include "qapi/util.h"
>> > @@ -246,7 +250,8 @@ def visit_event(self,
>> >
>> >  def gen_events(schema: QAPISchema,
>> >                 output_dir: str,
>> > -               prefix: str) -> None:
>> > -    vis = QAPISchemaGenEventVisitor(prefix)
>> > +               prefix: str,
>> > +               include: List[str]) -> None:
>> > +    vis = QAPISchemaGenEventVisitor(prefix, include)
>> >      schema.visit(vis)
>> >      vis.write(output_dir)
>> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> > index 113b49134de4..54a70a5ff516 100644
>> > --- a/scripts/qapi/gen.py
>> > +++ b/scripts/qapi/gen.py
>> > @@ -17,6 +17,7 @@
>> >  from typing import (
>> >      Dict,
>> >      Iterator,
>> > +    List,
>> >      Optional,
>> >      Sequence,
>> >      Tuple,
>> > @@ -45,6 +46,12 @@ def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
>> >      return ' | '.join(special_features) or '0'
>> >
>> >
>> > +def genc_include(include: List[str]) -> str:
>> > +    return '\n'.join(['#include ' +
>> > +                      (f'"{inc}"' if inc[0] not in ('<', '"') else inc)
>> > +                      for inc in include])
>>
>> This maps a list of file names / #include arguments to C code including
>> them, mapping file names to #include arguments by enclosing them in "".
>>
>> Option arguments of the form <foo.h> and "foo.h" need to be quoted in
>> the shell.  The latter can be done without quoting as foo.h.
>>
>> Somewhat funky wedding of generality with convenience.
>>
>> > +
>> > +
>> >  class QAPIGen:
>> >      def __init__(self, fname: str):
>> >          self.fname = fname
>> > @@ -228,16 +235,21 @@ def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) -> Iterator[None]:
>> >  class QAPISchemaMonolithicCVisitor(QAPISchemaVisitor):
>> >      def __init__(self,
>> >                   prefix: str,
>> > +                 include: List[str],
>> >                   what: str,
>> >                   blurb: str,
>> >                   pydoc: str):
>> >          self._prefix = prefix
>> > +        self._include = include
>> >          self._what = what
>> >          self._genc = QAPIGenC(self._prefix + self._what + '.c',
>> >                                blurb, pydoc)
>> >          self._genh = QAPIGenH(self._prefix + self._what + '.h',
>> >                                blurb, pydoc)
>> >
>> > +    def genc_include(self) -> str:
>> > +        return genc_include(self._include)
>> > +
>> >      def write(self, output_dir: str) -> None:
>> >          self._genc.write(output_dir)
>> >          self._genh.write(output_dir)
>> > @@ -246,12 +258,14 @@ def write(self, output_dir: str) -> None:
>> >  class QAPISchemaModularCVisitor(QAPISchemaVisitor):
>> >      def __init__(self,
>> >                   prefix: str,
>> > +                 include: List[str],
>> >                   what: str,
>> >                   user_blurb: str,
>> >                   builtin_blurb: Optional[str],
>> >                   pydoc: str,
>> >                   gen_tracing: bool = False):
>> >          self._prefix = prefix
>> > +        self._include = include
>> >          self._what = what
>> >          self._user_blurb = user_blurb
>> >          self._builtin_blurb = builtin_blurb
>> > @@ -262,6 +276,9 @@ def __init__(self,
>> >          self._main_module: Optional[str] = None
>> >          self._gen_tracing = gen_tracing
>> >
>> > +    def genc_include(self) -> str:
>> > +        return genc_include(self._include)
>> > +
>> >      @property
>> >      def _genc(self) -> QAPIGenC:
>> >          assert self._current_module is not None
>> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> > index 67c7d89aae00..d965d1769447 100644
>> > --- a/scripts/qapi/introspect.py
>> > +++ b/scripts/qapi/introspect.py
>> > @@ -170,9 +170,9 @@ def to_c_string(string: str) -> str:
>> >
>> >  class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>> >
>> > -    def __init__(self, prefix: str, unmask: bool):
>> > +    def __init__(self, prefix: str, include: List[str], unmask: bool):
>> >          super().__init__(
>> > -            prefix, 'qapi-introspect',
>> > +            prefix, include, 'qapi-introspect',
>> >              ' * QAPI/QMP schema introspection', __doc__)
>> >          self._unmask = unmask
>> >          self._schema: Optional[QAPISchema] = None
>> > @@ -180,10 +180,12 @@ def __init__(self, prefix: str, unmask: bool):
>> >          self._used_types: List[QAPISchemaType] = []
>> >          self._name_map: Dict[str, str] = {}
>> >          self._genc.add(mcgen('''
>> > -#include "qemu/osdep.h"
>> > +%(include)s
>> > +
>> >  #include "%(prefix)sqapi-introspect.h"
>> >
>> >  ''',
>> > +                             include=self.genc_include(),
>> >                               prefix=prefix))
>> >
>> >      def visit_begin(self, schema: QAPISchema) -> None:
>> > @@ -384,7 +386,8 @@ def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>> >
>> >
>> >  def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
>> > +                   include: List[str],
>> >                     opt_unmask: bool) -> None:
>> > -    vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
>> > +    vis = QAPISchemaGenIntrospectVisitor(prefix, include, opt_unmask)
>> >      schema.visit(vis)
>> >      vis.write(output_dir)
>> > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> > index fc216a53d32a..eba98cb9ace2 100644
>> > --- a/scripts/qapi/main.py
>> > +++ b/scripts/qapi/main.py
>> > @@ -9,7 +9,7 @@
>> >
>> >  import argparse
>> >  import sys
>> > -from typing import Optional
>> > +from typing import List, Optional
>> >
>> >  from .commands import gen_commands
>> >  from .common import must_match
>> > @@ -31,6 +31,7 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
>> >  def generate(schema_file: str,
>> >               output_dir: str,
>> >               prefix: str,
>> > +             include: List[str],
>> >               unmask: bool = False,
>> >               builtins: bool = False,
>> >               gen_tracing: bool = False) -> None:
>> > @@ -48,11 +49,11 @@ def generate(schema_file: str,
>> >      assert invalid_prefix_char(prefix) is None
>> >
>> >      schema = QAPISchema(schema_file)
>> > -    gen_types(schema, output_dir, prefix, builtins)
>> > -    gen_visit(schema, output_dir, prefix, builtins)
>> > -    gen_commands(schema, output_dir, prefix, gen_tracing)
>> > -    gen_events(schema, output_dir, prefix)
>> > -    gen_introspect(schema, output_dir, prefix, unmask)
>> > +    gen_types(schema, output_dir, prefix, include, builtins)
>> > +    gen_visit(schema, output_dir, prefix, include, builtins)
>> > +    gen_commands(schema, output_dir, prefix, include, gen_tracing)
>> > +    gen_events(schema, output_dir, prefix, include)
>> > +    gen_introspect(schema, output_dir, prefix, include, unmask)
>> >
>> >
>> >  def main() -> int:
>> > @@ -75,6 +76,9 @@ def main() -> int:
>> >      parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>> >                          dest='unmask',
>> >                          help="expose non-ABI names in introspection")
>> > +    parser.add_argument('-i', '--include', nargs='*',
>> > +                        default=['qemu/osdep.h'],
>> > +                        help="top-level include headers")
>>
>> The option name --include doesn't really tell me what it is about.  Is
>> it an include path for schema files?  Or is it about including something
>> in generated C?  Where in generated C?
>>
>> The help text provides clues: "headers" suggests .h, and "top-level"
>> suggests somewhere top-like.
>>
>> In fact, it's about inserting C code at the beginning of generated .c
>> files.  For the uses we have in mind, the C code is a single #include.
>> The patch implements any number of #includes.
>>
>> More general and arguably less funky: a way to insert arbitrary C code.
>>
>> Except arbitrary C code on the command line is unwieldy.  Better kept it
>> in the schema.  Pragma?
>>
>> Thoughts?
>
> Pragmas are global currently. This doesn't scale well, as we would
> like to split the schemas. I have a following patch that will allow me
> to split/merge the pragmas. This is not optimal either, I would rather
> remove/replace them (using annotations).

Now I'm curious.  Can you sketch what you have in mind?

> Imho, global tweaking of compilation is better done from the command line.

The command line is fine for straightforward configuration.  It's not
suitable for injecting code.

Fine: cc -c, which tells the compiler to work in a certain way.

Still fine: cc -DFOO, which effectively prepends '#define FOO 1" to the
.c.

No longer fine: a hypothetical option to prepend arbitrary C code.  Even
if it was occasionally useful.

Now watch this:

    $ python qapi-gen.py -o t qapi/qapi-schema.json -i '"abc.h"
    #define FOO'

    $ head -n 16 t/qapi-types.c
    /* AUTOMATICALLY GENERATED, DO NOT MODIFY */

    /*
     * Schema-defined QAPI types
     *
     * Copyright IBM, Corp. 2011
     * Copyright (c) 2013-2018 Red Hat Inc.
     *
     * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
     * See the COPYING.LIB file in the top-level directory.
     */

    #include "abc.h"
    #define FOO

    #include "qapi/dealloc-visitor.h"

Sure, nobody of sane mind would ever do this.  The fact remains that
we're doing something on the command line that should not be done there.

Your -i enables code injection because it takes either a file name or a
#include argument.  Can we dumb it down to just file name?

[...]