Hi
On Tue, Aug 2, 2022 at 5:28 PM Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
> > Hi
> >
> >
> > On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <arm...@redhat.com>
> wrote:
> >>
> >> marcandre.lur...@redhat.com writes:
> >>
> >> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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?
>
I simply made the pragma lists additive:
https://gitlab.com/marcandre.lureau/qemu/-/commit/1861964a317c2e74bea2d1f86944625e00df777f
I didn't think much about replacing pragmas with extra annotations. But it
could be for ex moving some pragmas to the declarations.
From:
{ 'pragma': {
# Command names containing '_'
'command-name-exceptions': [
'add_client',
...
{ 'command': 'add_client',
'data': { ... } }
To:
{ 'command': {
'name': 'add_client',
# Command name containing '_'
'name-exception': true },
'data': { ... } }
Or eventually to the comment:
# @add_client: (name-exception):
> > 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?
>
>
I think that can work too. Users can include a header that itself includes
extra headers in different ways, if needed.
thanks
--
Marc-André Lureau
