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? > 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? [...]