This allows us to include multiple QAPI schemas in the same file.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
scripts/qapi/commands.py | 7 ++++---
scripts/qapi/events.py | 3 ++-
scripts/qapi/gen.py | 6 +++---
scripts/qapi/types.py | 5 +++--
scripts/qapi/visit.py | 5 +++--
5 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index d629d2d97e..81134de6c0 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -280,7 +280,8 @@ def gen_register_command(name: str,
success_response: bool,
allow_oob: bool,
allow_preconfig: bool,
- coroutine: bool) -> str:
+ coroutine: bool,
+ prefix: str) -> str:
options = []
if not success_response:
@@ -298,7 +299,7 @@ def gen_register_command(name: str,
''',
name=name, c_name=c_name(name),
opts=' | '.join(options) or 0,
- feats=gen_features(features))
+ feats=gen_features(prefix, features))
return ret
@@ -407,7 +408,7 @@ def visit_command(self,
with ifcontext(ifcond, self._genh, self._genc):
self._genc.add(gen_register_command(
name, features, success_response, allow_oob,
- allow_preconfig, coroutine))
+ allow_preconfig, coroutine, self._prefix))
def gen_commands(schema: QAPISchema,
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index d1f639981a..8a489e559a 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -218,7 +218,8 @@ def visit_end(self) -> None:
self._genh.add(gen_enum(self._event_enum_name,
self._event_enum_members))
self._genc.add(gen_enum_lookup(self._event_enum_name,
- self._event_enum_members))
+ self._event_enum_members,
+ self._prefix))
self._genh.add(mcgen('''
void %(event_emit)s(%(event_enum)s event, QDict *qdict);
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 036977d989..399a0ae62d 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -41,9 +41,9 @@
from .source import QAPISourceInfo
-def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
- features = [f"1u << {c_enum_const('QAPI_FEATURE', feat.name)}"
- for feat in features if feat.is_special()]
+def gen_features(prefix, features: Sequence[QAPISchemaFeature]) -> str:
+ features = [f"1u << {c_enum_const(prefix + 'QAPI_FEATURE', feat.name)}"
+ for feat in features]
return ' | '.join(features) or '0'
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index ade6b7a3d7..b2d26c2ea8 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -43,6 +43,7 @@
def gen_enum_lookup(name: str,
members: List[QAPISchemaEnumMember],
+ modprefix: str,
prefix: Optional[str] = None) -> str:
max_index = c_enum_const(name, '_MAX', prefix)
feats = ''
@@ -61,7 +62,7 @@ def gen_enum_lookup(name: str,
index=index, name=memb.name)
ret += memb.ifcond.gen_endif()
- features = gen_features(memb.features)
+ features = gen_features(modprefix, memb.features)
if features != '0':
feats += mcgen('''
[%(index)s] = %(features)s,
@@ -331,7 +332,7 @@ def visit_enum_type(self,
prefix: Optional[str]) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.preamble_add(gen_enum(name, members, prefix))
- self._genc.add(gen_enum_lookup(name, members, prefix))
+ self._genc.add(gen_enum_lookup(name, members, self._prefix, prefix))
def visit_array_type(self,
name: str,
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 8dbf4ef1c3..883d720e36 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -62,6 +62,7 @@ def gen_visit_members_decl(name: str) -> str:
def gen_visit_object_members(name: str,
+ prefix: str,
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
branches: Optional[QAPISchemaBranches]) -> str:
@@ -103,7 +104,7 @@ def gen_visit_object_members(name: str,
''',
name=memb.name, has=has)
indent.increase()
- features = gen_features(memb.features)
+ features = gen_features(prefix, memb.features)
if features != '0':
ret += mcgen('''
if (visit_policy_reject(v, "%(name)s", %(features)s, errp)) {
@@ -402,7 +403,7 @@ def visit_object_type(self,
return
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_visit_members_decl(name))
- self._genc.add(gen_visit_object_members(name, base,
+ self._genc.add(gen_visit_object_members(name, self._prefix, base,
members, branches))
# TODO Worth changing the visitor signature, so we could
# directly use rather than repeat type.is_implicit()?
--
2.45.2
Daniel P. Berrangé <berrange@redhat.com> writes:
> This allows us to include multiple QAPI schemas in the same file.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This commit prepends an optional prefix to generated uses of
QAPI_FEATURE_{DEPRECATED,UNSTABLE}.
It touches neither the handwritten definition in include/qapi/util.h,
nor the handwritten uses in qapi/qapi-util.c and
qapi/qobject-output-visitor.c.
Code generated with a prefix will not compile, unless it uses no
features. No biggie, because:
The next commit replaces the handwritten definition (no prefix) by a
generated one (with optional prefix).
Now the handwritten code compiles only because it includes an enum
definition generated without a prefix, namely the main schema's.
Note that the handwritten code continues to work even when you use it
together with some other schema, but only because all the generated
enums assign the same numeric value to the enumeration constants
_DEPRECATED and _UNSTABLE.
While that seems fairly unlikely to break accidentally, it still feels
unnecessarily dirty.
What about generating an enum like this:
typedef enum {
QAPI_FEATURE_DEPRECATED = QAPI_DEPRECATED,
QAPI_FEATURE_UNSTABLE = QAPI_UNSTABLE,
...
} QapiFeature;
where QAPI_DEPRECATED and QAPI_UNSTABLE are defined in qapi/util.h, like
they are before this series.
We can discuss renaming them to QAPI_SPECIAL_FEATURE_DEPRECATED and
_UNSTABLE if you like.
Code referring to special features, i.e. all references before this
series, use the definitions from qapi/util.h.
Code referring to arbitrary, possibly non-special features, i.e. the
references new in this series, use the generated definitions from
qapi/qapi-features.h.
Thoughts?
Daniel P. Berrangé <berrange@redhat.com> writes: > This allows us to include multiple QAPI schemas in the same file. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> I figure you had reason to simultaneously include headers generated for multiple schemas. Do tell :)
On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > This allows us to include multiple QAPI schemas in the same file.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> I figure you had reason to simultaneously include headers generated for
> multiple schemas. Do tell :)
I didn't want to have this patch, but the unit tests do this :-(
[2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
In file included from tests/test-qapi-types-sub-sub-module.h:17,
from tests/test-qapi-visit-sub-sub-module.h:17,
from tests/test-qapi-commands-sub-sub-module.c:19:
tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
16 | QAPI_FEATURE_DEPRECATED,
| ^~~~~~~~~~~~~~~~~~~~~~~
In file included from ./qapi/qapi-types-error.h:17,
from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
from tests/test-qapi-commands-sub-sub-module.c:14:
./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
16 | QAPI_FEATURE_DEPRECATED,
| ^~~~~~~~~~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:167: run-ninja] Error 1
make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
make: *** [GNUmakefile:6: build] Error 2
I would be nice to eliminate that, but some parts of the test appear
to pull in more of the system emulator code which forces in the main
qapi schema
n file included from ./qapi/qapi-types-block-core.h:17,
from /var/home/berrange/src/virt/qemu/include/block/block-common.h:27,
from /var/home/berrange/src/virt/qemu/include/block/block-global-state.h:27,
from /var/home/berrange/src/virt/qemu/include/block/block.h:27,
from /var/home/berrange/src/virt/qemu/include/monitor/monitor.h:4,
from /var/home/berrange/src/virt/qemu/include/qapi/qmp/dispatch.h:17,
from tests/test-qapi-init-commands.h:16,
from tests/test-qapi-init-commands.c:15:
./qapi/qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
16 | QAPI_FEATURE_DEPRECATED,
| ^~~~~~~~~~~~~~~~~~~~~~~
In file included from tests/include/../test-qapi-types-sub-sub-module.h:17,
from tests/include/../test-qapi-commands-sub-sub-module.h:16,
from tests/include/test-qapi-commands-sub-module.h:16,
from tests/test-qapi-commands.h:16,
from tests/test-qapi-init-commands.c:14:
tests/include/../test-qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
16 | QAPI_FEATURE_DEPRECATED,
| ^~~~~~~~~~~~~~~~~~~~~~~
tests/test-qapi-init-commands.c: In function ‘test_qmp_init_marshal’:
tests/test-qapi-init-commands.c:64:71: error: ‘TEST_QAPI_FEATURE_DEPRECATED’ undeclared (first use in this function); did you mean ‘QAPI_FEATURE_DEPRECATED’?
64 | qmp_marshal_test_command_features1, 0, 1u << TEST_QAPI_FEATURE_DEPRECATED);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
| QAPI_FEATURE_DEPRECATED
tests/test-qapi-init-commands.c:64:71: note: each undeclared identifier is reported only once for each function it appears in
Maybe the test needs to be split into two ? One test exclusively testing
with the tests/qapi-schema/qapi-schema-test.json, and one test exclusively
with the main QMP QAPI schema ?
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > This allows us to include multiple QAPI schemas in the same file. >> > >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >> >> I figure you had reason to simultaneously include headers generated for >> multiple schemas. Do tell :) > > I didn't want to have this patch, but the unit tests do this :-( > > [2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o > FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o > cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c > In file included from tests/test-qapi-types-sub-sub-module.h:17, > from tests/test-qapi-visit-sub-sub-module.h:17, > from tests/test-qapi-commands-sub-sub-module.c:19: > tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’ > 16 | QAPI_FEATURE_DEPRECATED, > | ^~~~~~~~~~~~~~~~~~~~~~~ > In file included from ./qapi/qapi-types-error.h:17, > from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275, > from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16, > from tests/test-qapi-commands-sub-sub-module.c:14: > ./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’ > 16 | QAPI_FEATURE_DEPRECATED, > | ^~~~~~~~~~~~~~~~~~~~~~~ > ninja: build stopped: subcommand failed. > make[1]: *** [Makefile:167: run-ninja] Error 1 > make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build' > make: *** [GNUmakefile:6: build] Error 2 Compiles for me with PATCH 5/6 taken out. What am I doing wrong? > I would be nice to eliminate that, but some parts of the test appear > to pull in more of the system emulator code which forces in the main > qapi schema > > n file included from ./qapi/qapi-types-block-core.h:17, > from /var/home/berrange/src/virt/qemu/include/block/block-common.h:27, > from /var/home/berrange/src/virt/qemu/include/block/block-global-state.h:27, > from /var/home/berrange/src/virt/qemu/include/block/block.h:27, > from /var/home/berrange/src/virt/qemu/include/monitor/monitor.h:4, > from /var/home/berrange/src/virt/qemu/include/qapi/qmp/dispatch.h:17, > from tests/test-qapi-init-commands.h:16, > from tests/test-qapi-init-commands.c:15: > ./qapi/qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’ > 16 | QAPI_FEATURE_DEPRECATED, > | ^~~~~~~~~~~~~~~~~~~~~~~ > In file included from tests/include/../test-qapi-types-sub-sub-module.h:17, > from tests/include/../test-qapi-commands-sub-sub-module.h:16, > from tests/include/test-qapi-commands-sub-module.h:16, > from tests/test-qapi-commands.h:16, > from tests/test-qapi-init-commands.c:14: > tests/include/../test-qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’ > 16 | QAPI_FEATURE_DEPRECATED, > | ^~~~~~~~~~~~~~~~~~~~~~~ > tests/test-qapi-init-commands.c: In function ‘test_qmp_init_marshal’: > tests/test-qapi-init-commands.c:64:71: error: ‘TEST_QAPI_FEATURE_DEPRECATED’ undeclared (first use in this function); did you mean ‘QAPI_FEATURE_DEPRECATED’? > 64 | qmp_marshal_test_command_features1, 0, 1u << TEST_QAPI_FEATURE_DEPRECATED); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | QAPI_FEATURE_DEPRECATED > tests/test-qapi-init-commands.c:64:71: note: each undeclared identifier is reported only once for each function it appears in > > > Maybe the test needs to be split into two ? One test exclusively testing > with the tests/qapi-schema/qapi-schema-test.json, and one test exclusively > with the main QMP QAPI schema ? Which test is it?
On Mon, Aug 05, 2024 at 03:11:12PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > This allows us to include multiple QAPI schemas in the same file.
> >> >
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >>
> >> I figure you had reason to simultaneously include headers generated for
> >> multiple schemas. Do tell :)
> >
> > I didn't want to have this patch, but the unit tests do this :-(
> >
> > [2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
> > FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
> > cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
> > In file included from tests/test-qapi-types-sub-sub-module.h:17,
> > from tests/test-qapi-visit-sub-sub-module.h:17,
> > from tests/test-qapi-commands-sub-sub-module.c:19:
> > tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
> > 16 | QAPI_FEATURE_DEPRECATED,
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > In file included from ./qapi/qapi-types-error.h:17,
> > from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
> > from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
> > from tests/test-qapi-commands-sub-sub-module.c:14:
> > ./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
> > 16 | QAPI_FEATURE_DEPRECATED,
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > ninja: build stopped: subcommand failed.
> > make[1]: *** [Makefile:167: run-ninja] Error 1
> > make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
> > make: *** [GNUmakefile:6: build] Error 2
>
> Compiles for me with PATCH 5/6 taken out. What am I doing wrong?
The bit in patch 6 which generates the enum still has the prefix:
+ self._genh.add("typedef enum {\n")
+ for name in features:
+ self._genh.add(f" {c_enum_const(self._prefix + 'QAPI_FEATURE', name)},\n")
+
+ self._genh.add("} " + c_name(self._prefix + 'QapiFeature') + ";\n")
>
> > I would be nice to eliminate that, but some parts of the test appear
> > to pull in more of the system emulator code which forces in the main
> > qapi schema
> >
> > n file included from ./qapi/qapi-types-block-core.h:17,
> > from /var/home/berrange/src/virt/qemu/include/block/block-common.h:27,
> > from /var/home/berrange/src/virt/qemu/include/block/block-global-state.h:27,
> > from /var/home/berrange/src/virt/qemu/include/block/block.h:27,
> > from /var/home/berrange/src/virt/qemu/include/monitor/monitor.h:4,
> > from /var/home/berrange/src/virt/qemu/include/qapi/qmp/dispatch.h:17,
> > from tests/test-qapi-init-commands.h:16,
> > from tests/test-qapi-init-commands.c:15:
> > ./qapi/qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
> > 16 | QAPI_FEATURE_DEPRECATED,
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > In file included from tests/include/../test-qapi-types-sub-sub-module.h:17,
> > from tests/include/../test-qapi-commands-sub-sub-module.h:16,
> > from tests/include/test-qapi-commands-sub-module.h:16,
> > from tests/test-qapi-commands.h:16,
> > from tests/test-qapi-init-commands.c:14:
> > tests/include/../test-qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
> > 16 | QAPI_FEATURE_DEPRECATED,
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > tests/test-qapi-init-commands.c: In function ‘test_qmp_init_marshal’:
> > tests/test-qapi-init-commands.c:64:71: error: ‘TEST_QAPI_FEATURE_DEPRECATED’ undeclared (first use in this function); did you mean ‘QAPI_FEATURE_DEPRECATED’?
> > 64 | qmp_marshal_test_command_features1, 0, 1u << TEST_QAPI_FEATURE_DEPRECATED);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > | QAPI_FEATURE_DEPRECATED
> > tests/test-qapi-init-commands.c:64:71: note: each undeclared identifier is reported only once for each function it appears in
> >
> >
> > Maybe the test needs to be split into two ? One test exclusively testing
> > with the tests/qapi-schema/qapi-schema-test.json, and one test exclusively
> > with the main QMP QAPI schema ?
>
> Which test is it?
I'm unclear which is using this - its just failing with plain 'make' for
me.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Mon, Aug 05, 2024 at 03:11:12PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >>
>> >> > This allows us to include multiple QAPI schemas in the same file.
>> >> >
>> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> >>
>> >> I figure you had reason to simultaneously include headers generated for
>> >> multiple schemas. Do tell :)
>> >
>> > I didn't want to have this patch, but the unit tests do this :-(
>> >
>> > [2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
>> > FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
>> > cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
>> > In file included from tests/test-qapi-types-sub-sub-module.h:17,
>> > from tests/test-qapi-visit-sub-sub-module.h:17,
>> > from tests/test-qapi-commands-sub-sub-module.c:19:
>> > tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
>> > 16 | QAPI_FEATURE_DEPRECATED,
>> > | ^~~~~~~~~~~~~~~~~~~~~~~
>> > In file included from ./qapi/qapi-types-error.h:17,
>> > from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
>> > from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
>> > from tests/test-qapi-commands-sub-sub-module.c:14:
>> > ./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
>> > 16 | QAPI_FEATURE_DEPRECATED,
>> > | ^~~~~~~~~~~~~~~~~~~~~~~
>> > ninja: build stopped: subcommand failed.
>> > make[1]: *** [Makefile:167: run-ninja] Error 1
>> > make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
>> > make: *** [GNUmakefile:6: build] Error 2
>>
>> Compiles for me with PATCH 5/6 taken out. What am I doing wrong?
>
> The bit in patch 6 which generates the enum still has the prefix:
>
> + self._genh.add("typedef enum {\n")
> + for name in features:
> + self._genh.add(f" {c_enum_const(self._prefix + 'QAPI_FEATURE', name)},\n")
> +
> + self._genh.add("} " + c_name(self._prefix + 'QapiFeature') + ";\n")
Alright, I got it to fail with the appended patch. I'll have a closer
look. Thanks!
[...]
diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
index 9b77be6310..1eb0c8a8ac 100644
--- a/scripts/qapi/features.py
+++ b/scripts/qapi/features.py
@@ -55,9 +55,9 @@ def visit_end(self) -> None:
self._genh.add("typedef enum {\n")
for name in features:
- self._genh.add(f" {c_enum_const(self._prefix + 'QAPI_FEATURE', name)},\n")
+ self._genh.add(f" {c_enum_const('QAPI_FEATURE', name)},\n")
- self._genh.add("} " + c_name(self._prefix + 'QapiFeature') + ";\n")
+ self._genh.add("} " + c_name('QapiFeature') + ";\n")
def _record(self, features: List[QAPISchemaFeature]):
for f in features:
It's not just tests. QAPI-related headers have deteriorated, and pull in too much. I'll try to clean this up. Thanks!
Markus Armbruster <armbru@redhat.com> writes: > It's not just tests. QAPI-related headers have deteriorated, and pull > in too much. I'll try to clean this up. Thanks! While some cleanup is certainly possible and probably useful, I don't think I can completely solve the problem that way. Not counting tests, we have two or three QAPI schemas, depending on how you count: * QEMU's main schema qapi/qapi-schema.json This schema defines qemu-system-FOO's QMP (plus types used for QOM properties and such, but that's detail we can ignore here). * The storage daemon's schema storage-daemon/qapi/qapi-schema.json This is a strict subset the main schema, not a schema on its own. qemu-storage-daemon wants to provide a subset of qemu-system-FOO's QMP. We get this as follows. qapi/qapi-schema.json contains nothing but include directives. storage-daemon/qapi/qapi-schema.json contains nothing but a subset of the same include directives. The exact same code gets generated for these modules both times. We always use the copy generated for the main schema's modules, and ignore the one generated for the storage daemon. Different code gets generated for command & event registration, and QMP introspection. The storage daemon uses the code generated for its subset. This is a bit of a hack. * The guest agent's schema qga/qapi-schema.json This schema defines qemu-ga's QMP. Each program generally uses at most one QAPI schema. Easy. The exception is the guest agent. It might have initially used just its own QAPI schema as well, but today, it also uses parts of the main schema. That's because the main schema has grown tentacles into common code. Example: qemu-ga with -m vsock-listen prefixes its -p argument with "vsock:" and parses the result with socket_parse(). While we can quibble about the details here, sharing vsock address syntax and parser between guest agent and QEMU proper makes sense. The address structure / abstract syntax is defined in the main schema's qapi/sockets.json. The address parser is declared in qemu/sockets.h, and pulls in the main schema's qapi/qapi-types-sockets.h. Trying to limit qemu-ga to just its own schema would make no sense. We *want* to share things defined in a (shared) QAPI schema. Note that while qemu-ga C code pulls in (generated) main schema stuff, its QAPI schema so far does not. However, reuse of utility types like SocketAddress or OnOffAuto from the main schema might become interesting some day. The QAPI generator supports more than one schema per program, although kind of half-heartedly. There's -p to set a prefix, which is prepended to filenames and symbols. But there's no systematic effort to do the latter. We slap it to symbols whenever we run into a collision. Example: your patch slaps it to the generated QapiFeature enum and its members. Example: we don't slap it to command handlers. If two schemas both define command 'frobnicate', we generate a prototype qmp_frobnicate() for both, and expect the handwritten code to supply a function suitable for both. We can continue slapping the prefix to symbols whenever we run into a collision, i.e. go with your patch. Feels like the pragmatic choice to not block your series. But could we do better in the long run? Back when we created the guest agent schema, QAPI schemas were monolithic: a single schema file, generating a single qapi-types.h and so forth. Today, a schema can be modular, i.e. split into multiple modules, each generating its own qapi-types-MODULE.h and so forth. We exploit this to have a single schema both for qemu-system-FOO's QMP and qemu-storage-daemon's QMP, albeit in a hacky way. What if QAPI provided cleaner means for generating multiple QMPs from a single schema? Then we could have a single schema, and no worries about collisions. I'm not asking you to try this. It's just an idea I wrote up so I don't forget it again.
© 2016 - 2024 Red Hat, Inc.