| Message ID | 20240807093406.40360-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] qapi: Generate QAPI files using qapi/ for generated header paths | expand |
On Wed, Aug 07, 2024 at 11:34:06AM +0200, Philippe Mathieu-Daudé wrote: > QAPI script generates headers under the qapi/ directory, > so use this prefix in generated header paths, keeping > all QAPI under the qapi/ namespace. Err that's not always the case is it ? For the main QMP schema, it generates under $BUILDDIR/qapi/, but there are other schemas such as QGA, which is generated under $BUILDDIR/qga/ It is confusing that we have both shared stuff and QMP schema only stuff under the same location. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Aug 07, 2024 at 11:34:06AM +0200, Philippe Mathieu-Daudé wrote: >> QAPI script generates headers under the qapi/ directory, >> so use this prefix in generated header paths, keeping >> all QAPI under the qapi/ namespace. > > Err that's not always the case is it ? Yup. We generate into the current directory by default, but you can set another one with --output-dir. > For the main QMP schema, it generates under $BUILDDIR/qapi/, > but there are other schemas such as QGA, which is generated > under $BUILDDIR/qga/ I think we could prepend the argument of --output-dir instead. > It is confusing that we have both shared stuff and QMP schema > only stuff under the same location. Which stuff in which location?
On Wed, Aug 07, 2024 at 12:50:26PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Aug 07, 2024 at 11:34:06AM +0200, Philippe Mathieu-Daudé wrote: > >> QAPI script generates headers under the qapi/ directory, > >> so use this prefix in generated header paths, keeping > >> all QAPI under the qapi/ namespace. > > > > Err that's not always the case is it ? > > Yup. We generate into the current directory by default, but you can set > another one with --output-dir. > > > For the main QMP schema, it generates under $BUILDDIR/qapi/, > > but there are other schemas such as QGA, which is generated > > under $BUILDDIR/qga/ > > I think we could prepend the argument of --output-dir instead. > > > It is confusing that we have both shared stuff and QMP schema > > only stuff under the same location. > > Which stuff in which location? There are multiple directories with 'qapi' in their name - $SRC/include/qapi - all generic stuff for any consumer of QAPI - $SRC/qapi - impl of generic stuff from $SRC/include/qapi, but also the QMP schema for machine emulator - $BUILD/qapi - generated code for QMP schema for machine emulator I find it confusing that we have both generic QAPI code and the main machine emulator QMP schema in directories sharing the same 'qapi' name. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Aug 07, 2024 at 12:50:26PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: [...] >> > It is confusing that we have both shared stuff and QMP schema >> > only stuff under the same location. >> >> Which stuff in which location? > > There are multiple directories with 'qapi' in their name > > - $SRC/include/qapi - all generic stuff for any consumer of QAPI > - $SRC/qapi - impl of generic stuff from $SRC/include/qapi, but > also the QMP schema for machine emulator > - $BUILD/qapi - generated code for QMP schema for machine emulator - scripts/qapi - the generator code > > I find it confusing that we have both generic QAPI code and the main > machine emulator QMP schema in directories sharing the same 'qapi' > name. Got it. Lack of separation between generic C infrastructure and specific schema hasn't really annoyed me. Possibly because the two are, for better or worse, joined at the hip. Except for the use of "qapi:" in commit message titles; there I've at times felt a slight urge to distinguish between schema work, C infrastructure work, and generator work. Of course, other people's confusion trumps my non-annoyance.
On Wed, Aug 07, 2024 at 01:21:25PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Aug 07, 2024 at 12:50:26PM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > > [...] > > >> > It is confusing that we have both shared stuff and QMP schema > >> > only stuff under the same location. > >> > >> Which stuff in which location? > > > > There are multiple directories with 'qapi' in their name > > > > - $SRC/include/qapi - all generic stuff for any consumer of QAPI > > - $SRC/qapi - impl of generic stuff from $SRC/include/qapi, but > > also the QMP schema for machine emulator > > - $BUILD/qapi - generated code for QMP schema for machine emulator > > - scripts/qapi - the generator code > > > > > I find it confusing that we have both generic QAPI code and the main > > machine emulator QMP schema in directories sharing the same 'qapi' > > name. > > Got it. > > Lack of separation between generic C infrastructure and specific schema > hasn't really annoyed me. Possibly because the two are, for better or > worse, joined at the hip. Except for the use of "qapi:" in commit > message titles; there I've at times felt a slight urge to distinguish > between schema work, C infrastructure work, and generator work. > > Of course, other people's confusion trumps my non-annoyance. When we first introduced the QAPI/QMP schema for system emulator of course it was fine, since we didn't have QGA usage. Now days we have a dedicate $SRCDIR/system directory for the system emulators, so I wonder if its worth putting the system emulator schemas in there instead ? Caveat is that the QSD also uses some of this schema. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Aug 07, 2024 at 01:21:25PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Wed, Aug 07, 2024 at 12:50:26PM +0200, Markus Armbruster wrote: >> >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> [...] >> >> >> > It is confusing that we have both shared stuff and QMP schema >> >> > only stuff under the same location. >> >> >> >> Which stuff in which location? >> > >> > There are multiple directories with 'qapi' in their name >> > >> > - $SRC/include/qapi - all generic stuff for any consumer of QAPI >> > - $SRC/qapi - impl of generic stuff from $SRC/include/qapi, but >> > also the QMP schema for machine emulator >> > - $BUILD/qapi - generated code for QMP schema for machine emulator >> >> - scripts/qapi - the generator code >> >> > >> > I find it confusing that we have both generic QAPI code and the main >> > machine emulator QMP schema in directories sharing the same 'qapi' >> > name. >> >> Got it. >> >> Lack of separation between generic C infrastructure and specific schema >> hasn't really annoyed me. Possibly because the two are, for better or >> worse, joined at the hip. Except for the use of "qapi:" in commit >> message titles; there I've at times felt a slight urge to distinguish >> between schema work, C infrastructure work, and generator work. >> >> Of course, other people's confusion trumps my non-annoyance. > > When we first introduced the QAPI/QMP schema for system emulator of > course it was fine, since we didn't have QGA usage. Actually, qga's QAPI schema (commit e3d4d25206a) predates the main QAPI schema (commit e3193601c84) by a few weeks. > Now days we have > a dedicate $SRCDIR/system directory for the system emulators, so I > wonder if its worth putting the system emulator schemas in there > instead ? Caveat is that the QSD also uses some of this schema. Another caveat is that much QAPI code, both infrastructure and generated, has bled into programs other than qemu-system-FOO. $ gdb -batch -ex "info sources" bld/qemu-i386 | tr ',' '\012' | sed -n '/qapi/s,^.*/qemu/,,p' bld/qapi/qapi-types-machine-common.h bld/qapi/qapi-types-machine.h include/qapi/util.h bld/qapi/qapi-builtin-types.h bld/qapi/qapi-types-error.h bld/qapi/qapi-types-common.h bld/qapi/qapi-types-run-state.h include/qapi/error.h bld/qapi/qapi-visit-machine.h include/qapi/visitor.h bld/qapi/qapi-builtin-visit.h bld/qapi/qapi-types-replay.h include/qapi/qmp/qobject.h include/qapi/qmp/qlist.h include/qapi/qmp/qdict.h bld/qapi/qapi-events-qdev.h include/qapi/qmp/qbool.h include/qapi/qmp/qnum.h include/qapi/qmp/qstring.h include/qapi/forward-visitor.h include/qapi/string-output-visitor.h include/qapi/string-input-visitor.h include/qapi/qobject-input-visitor.h bld/qapi/qapi-types-authz.h bld/qapi/qapi-types-crypto.h bld/qapi/qapi-types-sockets.h bld/qapi/qapi-types-block-core.h bld/qapi/qapi-types-qom.h include/qapi/qmp/qjson.h bld/qapi/qapi-visit-qom.h include/qapi/qobject-output-visitor.h bld/qapi/qapi-builtin-visit.c bld/qapi/qapi-types-common.c bld/qapi/qapi-visit-common.h include/qapi/dealloc-visitor.h bld/qapi/qapi-visit-common.c bld/qapi/qapi-visit-machine.c bld/qapi/qapi-visit-machine-common.h bld/qapi/qapi-types-qom.c bld/qapi/qapi-visit-qom.c bld/qapi/qapi-visit-crypto.h bld/qapi/qapi-visit-block-core.h bld/qapi/qapi-visit-authz.h bld/qapi/qapi-visit-sockets.h bld/qapi/qapi-visit-sockets.c bld/qapi/qapi-events-qdev.c bld/qapi/qapi-emit-events.h bld/qapi/qapi-types-qdev.h bld/qapi/qapi-visit-qdev.h include/qapi/compat-policy.h include/qapi/qmp-event.h qapi/qapi-dealloc-visitor.c include/qapi/qmp/qnull.h include/qapi/visitor-impl.h bld/qapi/qapi-types-compat.h qapi/qapi-forward-visitor.c qapi/qapi-util.c qapi/qapi-visit-core.c bld/trace/trace-qapi.h qapi/trace-events bld/trace/trace-dtrace-qapi.h qapi/qobject-input-visitor.c qapi/qobject-output-visitor.c qapi/string-input-visitor.c qapi/string-output-visitor.c qapi/qmp-dispatch.c include/qapi/qmp/dispatch.h qapi/qmp-event.c qapi/qmp-registry.c include/qapi/qmp/json-parser.h include/qapi/qmp/json-writer.h bld/qapi/qapi-builtin-types.c bld/qapi/qapi-visit-authz.c bld/qapi/qapi-visit-block-core.c bld/qapi/qapi-types-job.h bld/qapi/qapi-visit-job.h bld/qapi/qapi-visit-crypto.c bld/qapi/qapi-types-error.c bld/qapi/qapi-visit-job.c bld/qapi/qapi-visit-machine-common.c bld/qapi/qapi-types-machine.c bld/qapi/qapi-types-sockets.c bld/qapi/qapi-visit-qdev.c bld/trace/trace-qapi.c bld/qapi/qapi-types-authz.c bld/qapi/qapi-types-block-core.c bld/qapi/qapi-types-crypto.c bld/qapi/qapi-types-job.c bld/qapi/qapi-types-machine-common.c
On 7/8/24 14:01, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Wed, Aug 07, 2024 at 01:21:25PM +0200, Markus Armbruster wrote: >>> Daniel P. Berrangé <berrange@redhat.com> writes: >>> >>>> On Wed, Aug 07, 2024 at 12:50:26PM +0200, Markus Armbruster wrote: >>>>> Daniel P. Berrangé <berrange@redhat.com> writes: >>> >>> [...] >>> >>>>>> It is confusing that we have both shared stuff and QMP schema >>>>>> only stuff under the same location. >>>>> >>>>> Which stuff in which location? >>>> >>>> There are multiple directories with 'qapi' in their name >>>> >>>> - $SRC/include/qapi - all generic stuff for any consumer of QAPI >>>> - $SRC/qapi - impl of generic stuff from $SRC/include/qapi, but >>>> also the QMP schema for machine emulator >>>> - $BUILD/qapi - generated code for QMP schema for machine emulator >>> >>> - scripts/qapi - the generator code >>> >>>> >>>> I find it confusing that we have both generic QAPI code and the main >>>> machine emulator QMP schema in directories sharing the same 'qapi' >>>> name. >>> >>> Got it. >>> >>> Lack of separation between generic C infrastructure and specific schema >>> hasn't really annoyed me. Possibly because the two are, for better or >>> worse, joined at the hip. Except for the use of "qapi:" in commit >>> message titles; there I've at times felt a slight urge to distinguish >>> between schema work, C infrastructure work, and generator work. >>> >>> Of course, other people's confusion trumps my non-annoyance. >> >> When we first introduced the QAPI/QMP schema for system emulator of >> course it was fine, since we didn't have QGA usage. > > Actually, qga's QAPI schema (commit e3d4d25206a) predates the main QAPI > schema (commit e3193601c84) by a few weeks. > >> Now days we have >> a dedicate $SRCDIR/system directory for the system emulators, so I >> wonder if its worth putting the system emulator schemas in there >> instead ? Caveat is that the QSD also uses some of this schema. > > Another caveat is that much QAPI code, both infrastructure and > generated, has bled into programs other than qemu-system-FOO. qapi/qapi-types-foo.h is OK since we to share libcommon.a. I'm not sure about qapi/qapi-visit-foo.h, maybe due to qtests? There was also some code pulled by QOM properties, althougth not used. > $ gdb -batch -ex "info sources" bld/qemu-i386 | tr ',' '\012' | sed -n '/qapi/s,^.*/qemu/,,p' > bld/qapi/qapi-types-machine-common.h > bld/qapi/qapi-types-machine.h > include/qapi/util.h > bld/qapi/qapi-builtin-types.h > bld/qapi/qapi-types-error.h > bld/qapi/qapi-types-common.h > bld/qapi/qapi-types-run-state.h > include/qapi/error.h > bld/qapi/qapi-visit-machine.h > include/qapi/visitor.h > bld/qapi/qapi-builtin-visit.h > bld/qapi/qapi-types-replay.h > include/qapi/qmp/qobject.h > include/qapi/qmp/qlist.h > include/qapi/qmp/qdict.h > bld/qapi/qapi-events-qdev.h > include/qapi/qmp/qbool.h > include/qapi/qmp/qnum.h > include/qapi/qmp/qstring.h > include/qapi/forward-visitor.h > include/qapi/string-output-visitor.h > include/qapi/string-input-visitor.h > include/qapi/qobject-input-visitor.h > bld/qapi/qapi-types-authz.h > bld/qapi/qapi-types-crypto.h > bld/qapi/qapi-types-sockets.h > bld/qapi/qapi-types-block-core.h > bld/qapi/qapi-types-qom.h > include/qapi/qmp/qjson.h > bld/qapi/qapi-visit-qom.h > include/qapi/qobject-output-visitor.h > bld/qapi/qapi-builtin-visit.c > bld/qapi/qapi-types-common.c > bld/qapi/qapi-visit-common.h > include/qapi/dealloc-visitor.h > bld/qapi/qapi-visit-common.c > bld/qapi/qapi-visit-machine.c > bld/qapi/qapi-visit-machine-common.h > bld/qapi/qapi-types-qom.c > bld/qapi/qapi-visit-qom.c > bld/qapi/qapi-visit-crypto.h > bld/qapi/qapi-visit-block-core.h > bld/qapi/qapi-visit-authz.h > bld/qapi/qapi-visit-sockets.h > bld/qapi/qapi-visit-sockets.c > bld/qapi/qapi-events-qdev.c > bld/qapi/qapi-emit-events.h > bld/qapi/qapi-types-qdev.h > bld/qapi/qapi-visit-qdev.h > include/qapi/compat-policy.h > include/qapi/qmp-event.h > qapi/qapi-dealloc-visitor.c > include/qapi/qmp/qnull.h > include/qapi/visitor-impl.h > bld/qapi/qapi-types-compat.h > qapi/qapi-forward-visitor.c > qapi/qapi-util.c > qapi/qapi-visit-core.c > bld/trace/trace-qapi.h > qapi/trace-events > bld/trace/trace-dtrace-qapi.h > qapi/qobject-input-visitor.c > qapi/qobject-output-visitor.c > qapi/string-input-visitor.c > qapi/string-output-visitor.c > qapi/qmp-dispatch.c > include/qapi/qmp/dispatch.h > qapi/qmp-event.c > qapi/qmp-registry.c > include/qapi/qmp/json-parser.h > include/qapi/qmp/json-writer.h > bld/qapi/qapi-builtin-types.c > bld/qapi/qapi-visit-authz.c > bld/qapi/qapi-visit-block-core.c > bld/qapi/qapi-types-job.h > bld/qapi/qapi-visit-job.h > bld/qapi/qapi-visit-crypto.c > bld/qapi/qapi-types-error.c > bld/qapi/qapi-visit-job.c > bld/qapi/qapi-visit-machine-common.c > bld/qapi/qapi-types-machine.c > bld/qapi/qapi-types-sockets.c > bld/qapi/qapi-visit-qdev.c > bld/trace/trace-qapi.c > bld/qapi/qapi-types-authz.c > bld/qapi/qapi-types-block-core.c > bld/qapi/qapi-types-crypto.c > bld/qapi/qapi-types-job.c > bld/qapi/qapi-types-machine-common.c > At least there are no qapi/qapi-commands-foo.[ch]!
Markus Armbruster <armbru@redhat.com> writes: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Wed, Aug 07, 2024 at 11:34:06AM +0200, Philippe Mathieu-Daudé wrote: >>> QAPI script generates headers under the qapi/ directory, >>> so use this prefix in generated header paths, keeping >>> all QAPI under the qapi/ namespace. >> >> Err that's not always the case is it ? > > Yup. We generate into the current directory by default, but you can set > another one with --output-dir. > >> For the main QMP schema, it generates under $BUILDDIR/qapi/, >> but there are other schemas such as QGA, which is generated >> under $BUILDDIR/qga/ > > I think we could prepend the argument of --output-dir instead. I'm willing to give this a try after my vacation. You may have to remind me... [...]
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 79951a841f..07fbd21334 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -323,8 +323,8 @@ def _begin_user_module(self, name: str) -> None: #include "qapi/qmp/qdict.h" #include "qapi/dealloc-visitor.h" #include "qapi/error.h" -#include "%(visit)s.h" -#include "%(commands)s.h" +#include "qapi/%(visit)s.h" +#include "qapi/%(commands)s.h" ''', commands=commands, visit=visit)) @@ -338,7 +338,7 @@ def _begin_user_module(self, name: str) -> None: # match .underscorify() in trace/meson.build self._genh.add(mcgen(''' -#include "%(types)s.h" +#include "qapi/%(types)s.h" ''', types=types)) @@ -353,8 +353,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 "%(prefix)sqapi-commands.h" -#include "%(prefix)sqapi-init-commands.h" +#include "qapi/%(prefix)sqapi-commands.h" +#include "qapi/%(prefix)sqapi-init-commands.h" void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds) { diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index d1f639981a..f3803c8afe 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -189,9 +189,9 @@ def _begin_user_module(self, name: str) -> None: visit = self._module_basename('qapi-visit', name) self._genc.add(mcgen(''' #include "qemu/osdep.h" -#include "%(prefix)sqapi-emit-events.h" -#include "%(events)s.h" -#include "%(visit)s.h" +#include "qapi/%(prefix)sqapi-emit-events.h" +#include "qapi/%(events)s.h" +#include "qapi/%(visit)s.h" #include "qapi/compat-policy.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" @@ -201,7 +201,7 @@ def _begin_user_module(self, name: str) -> None: prefix=self._prefix)) self._genh.add(mcgen(''' #include "qapi/util.h" -#include "%(types)s.h" +#include "qapi/%(types)s.h" ''', types=types)) @@ -209,7 +209,7 @@ def visit_end(self) -> None: self._add_module('./emit', ' * QAPI Events emission') self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" -#include "%(prefix)sqapi-emit-events.h" +#include "qapi/%(prefix)sqapi-emit-events.h" ''', prefix=self._prefix)) self._genh.preamble_add(mcgen(''' diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 6a8abe0041..d24bd7ba08 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -363,6 +363,6 @@ def visit_include(self, name: str, info: Optional[QAPISourceInfo]) -> None: relname = os.path.relpath(self._module_filename(self._what, name), os.path.dirname(self._genh.fname)) self._genh.preamble_add(mcgen(''' -#include "%(relname)s.h" +#include "qapi/%(relname)s.h" ''', relname=relname)) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index ac14b20f30..7b74db11ed 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -182,7 +182,7 @@ def __init__(self, prefix: str, unmask: bool): self._name_map: Dict[str, str] = {} self._genc.add(mcgen(''' #include "qemu/osdep.h" -#include "%(prefix)sqapi-introspect.h" +#include "qapi/%(prefix)sqapi-introspect.h" ''', prefix=prefix)) diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 0dd0b00ada..19c8c9c18c 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -306,8 +306,8 @@ def _begin_user_module(self, name: str) -> None: self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "qapi/dealloc-visitor.h" -#include "%(types)s.h" -#include "%(visit)s.h" +#include "qapi/%(types)s.h" +#include "qapi/%(visit)s.h" ''', types=types, visit=visit)) self._genh.preamble_add(mcgen(''' diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 12f92e429f..06d89504ce 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -359,12 +359,12 @@ def _begin_user_module(self, name: str) -> None: self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "qapi/error.h" -#include "%(visit)s.h" +#include "qapi/%(visit)s.h" ''', visit=visit)) self._genh.preamble_add(mcgen(''' #include "qapi/qapi-builtin-visit.h" -#include "%(types)s.h" +#include "qapi/%(types)s.h" ''', types=types))
QAPI script generates headers under the qapi/ directory, so use this prefix in generated header paths, keeping all QAPI under the qapi/ namespace. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- Ideally I'd also remove "CPPFLAGS+=-I qapi" in this patch but I couldn't figure out where meson adds it... --- scripts/qapi/commands.py | 10 +++++----- scripts/qapi/events.py | 10 +++++----- scripts/qapi/gen.py | 2 +- scripts/qapi/introspect.py | 2 +- scripts/qapi/types.py | 4 ++-- scripts/qapi/visit.py | 4 ++-- 6 files changed, 16 insertions(+), 16 deletions(-)