diff mbox series

[5/6] qapi: apply schema prefix to QAPI feature enum constants

Message ID 20240801175913.669013-6-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi: generalize special features | expand

Commit Message

Daniel P. Berrangé Aug. 1, 2024, 5:59 p.m. UTC
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(-)

Comments

Markus Armbruster Aug. 5, 2024, 12:22 p.m. UTC | #1
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 :)
Daniel P. Berrangé Aug. 5, 2024, 12:33 p.m. UTC | #2
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
Markus Armbruster Aug. 5, 2024, 1:11 p.m. UTC | #3
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?
Daniel P. Berrangé Aug. 5, 2024, 1:24 p.m. UTC | #4
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
Markus Armbruster Aug. 5, 2024, 1:54 p.m. UTC | #5
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:
Markus Armbruster Aug. 5, 2024, 2:59 p.m. UTC | #6
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 Aug. 8, 2024, 11:48 a.m. UTC | #7
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?
diff mbox series

Patch

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()?