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.

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

Markus Armbruster <armbru@redhat.com> writes:

> 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.

PATCH 01-02 make sense on their own; queued.

Hi

On Tue, Dec 18, 2018 at 10:26 PM Markus Armbruster <armbru@redhat.com> 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

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Dec 18, 2018 at 10:26 PM Markus Armbruster <armbru@redhat.com> 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?

Yes, but...

> I am actually a bit surprised there are no poisoined define errors.
> Possibly because the top-level header is rarely included.

... next to nothing includes the top-level header:

    $ git-grep -E 'include.*"(qapi/)?qapi-[^-]*.h'
    monitor.c:#include "qapi/qapi-commands.h"

Here we actually need all commands, complete with their
target-dependence.

    monitor.c:#include "qapi/qapi-introspect.h"
    tests/test-qobject-input-visitor.c:#include "qapi/qapi-introspect.h"

qapi-introspect.[ch] are monolithic.

    ui/cocoa.m:#include "qapi/qapi-commands.h"

This is just lazy; we should include just the qapi-commands-FOO we
actually need.  I'll ask the maintainer for help with cleaning this up.

Including top-level headers has been a bad idea ever since we generate
modular headers (commit 252dc3105fc), because it leads to "touch the
QAPI schema, have some coffee while the world is recompiled".

Adding target-dependent conditionals makes this bad idea impractical in
target-independent code.  Feature!

> 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.

Please do :)