Re: [Qemu-devel] [RFC PATCH v2 00/15] qapi: add #if pre-processor conditions to generated code (part 3)

[Marc-André Lureau]

Hi

On Tue, Dec 18, 2018 at 10:26 PM Markus Armbruster <address@hidden> wrote:
>
> Marc-André posted v1 that relies on a QAPI schema language extension
> 'top-unit' to permit splitting the generated code.  Here is his cover
> letter:
>
>     The thrid and last part (of "[PATCH v2 00/54] qapi: add #if
>     pre-processor conditions to generated code") is about adding schema
>     conditions based on the target.
>
>     For now, the qapi code is compiled in common objects (common to all
>     targets). This makes it impossible to add #if TARGET_ARM for example.
>
>     The patch "RFC: qapi: learn to split the schema by 'top-unit'"
>     proposes to split the schema by "top-unit", so that generated code can
>     be built either in common objects or per-target. That patch is a bit
>     rough, I would like to get some feedback about the approach before
>     trying to improve it. The following patches demonstrate usage of
>     target-based #if conditions, and getting rid of the
>     qmp_unregister_command() hack.
>
> We already have a way to split generated code: QAPI modules.  I took
> the liberty to rework Marc-André's series to use a target module
> instead.  Less code, no change to the QAPI language.
>
> One of the patches (marked WIP) is a total hack.  Fixable, but I want
> to get this out for discussion first.

The approach you propose includes -target.h header in the top headers,
effectively making all the qapi code target-dependent, no?
I am actually a bit surprised there are no poisoined define errors.
Possibly because the top-level header is rarely included.

By contrast, my approach has the advantage of a clean split between
target and non-target dependent code, which I would feel more
confident about.

That's the reason why I promptly discarded the QAPI modules approach
without having second thoughts at least. Now you force me to
reconsider it though.

>
> Marc-André Lureau (9):
>   build-sys: move qmp-introspect per target
>   qapi: make rtc-reset-reinjection and SEV depend on TARGET_I386
>   qapi: make s390 commands depend on TARGET_S390X
>   target.json: add a note about query-cpu* not being s390x-specific
>   qapi: make query-gic-capabilities depend on TARGET_ARM
>   qapi: make query-cpu-model-expansion depend on s390 or x86
>   qapi: make query-cpu-definitions depend on specific targets
>   qapi: remove qmp_unregister_command()
>   qapi: move RTC_CHANGE to the target schema
>
> Markus Armbruster (6):
>   qapi: Belatedly update docs for commit 9c2f56e9f9d
>   qapi: Eliminate indirection through qmp_event_get_func_emit()
>   qapi: Generate QAPIEvent stuff into separate files WIP
>   qapi: New module target.json
>   Revert "qapi-events: add 'if' condition to implicit event enum"
>   qmp: Deprecate query-events in favor of query-qmp-schema
>
>  Makefile                                |   1 +
>  Makefile.objs                           |  22 +-
>  Makefile.target                         |  12 +
>  docs/devel/qapi-code-gen.txt            |  12 +-
>  hw/ppc/spapr_rtc.c                      |   2 +-
>  hw/s390x/s390-skeys.c                   |   2 +-
>  hw/timer/mc146818rtc.c                  |   4 +-
>  include/qapi/qmp-event.h                |   6 -
>  include/qapi/qmp/dispatch.h             |   1 -
>  include/sysemu/arch_init.h              |  11 -
>  monitor.c                               |  92 +----
>  qapi/misc.json                          | 485 +---------------------
>  qapi/qapi-schema.json                   |   1 +
>  qapi/qmp-event.c                        |  12 -
>  qapi/qmp-registry.c                     |   8 -
>  qapi/target.json                        | 514 ++++++++++++++++++++++++
>  qemu-deprecated.texi                    |   5 +
>  qmp.c                                   |  26 --
>  scripts/qapi/events.py                  |  51 ++-
>  stubs/Makefile.objs                     |   4 -
>  stubs/arch-query-cpu-def.c              |  11 -
>  stubs/arch-query-cpu-model-baseline.c   |  13 -
>  stubs/arch-query-cpu-model-comparison.c |  13 -
>  stubs/arch-query-cpu-model-expansion.c  |  13 -
>  stubs/monitor.c                         |   5 +
>  target/arm/helper.c                     |   3 +-
>  target/arm/monitor.c                    |   2 +-
>  target/i386/cpu.c                       |   7 +-
>  target/i386/sev_i386.h                  |   2 +-
>  target/ppc/translate_init.inc.c         |   3 +-
>  target/s390x/cpu_models.c               |   9 +-
>  tests/Makefile.include                  |   4 +-
>  tests/test-qmp-event.c                  |   7 +-
>  tests/test-qobject-input-visitor.c      |   1 -
>  ui/vnc.c                                |   3 +-
>  35 files changed, 623 insertions(+), 744 deletions(-)
>  create mode 100644 qapi/target.json
>  delete mode 100644 stubs/arch-query-cpu-def.c
>  delete mode 100644 stubs/arch-query-cpu-model-baseline.c
>  delete mode 100644 stubs/arch-query-cpu-model-comparison.c
>  delete mode 100644 stubs/arch-query-cpu-model-expansion.c
>
> --
> 2.17.2
>
>


-- 
Marc-André Lureau