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="">
>> > 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:
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:
> 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.