Re: [Qemu-devel] [PATCH 00/26] qapi: add #if pre-processor conditions to generated code

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Thu, Aug 17, 2017 at 3:55 PM, Markus Armbruster  wrote:
>> Marc-André Lureau  writes:
>>
>>> Hi,
>>>
>>> In order to clean-up some hacks in qapi (having to unregister commands
>>> at runtime), I proposed a "[PATCH v5 02/20] qapi.py: add a simple #ifdef 
>>> condition"
>>>
>>> (see http://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03106.html).
>>>
>>> However, we decided to drop that patch from the series and solve the
>>> problem later. The main issues were:
>>> - the syntax was awkward to the JSON schema and documentation
>>> - the evaluation of the condition was done in the qapi scripts, with
>>>   very limited capability
>>> - each target/config would need different generated files.
>>>
>>> Instead, it could defer the #if evaluation to the C-preprocessor.
>>>
>>> With this series, top-level qapi JSON entity can take 'if' keys:
>>>
>>> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>>>   'if': 'defined(TEST_IF_STRUCT)' }
>>>
>>> Members can be exploded as dictionnary with 'type'/'if' keys:
>>>
>>> { 'struct': 'TestIfStruct', 'data':
>>>   { 'foo': 'int',
>>> 'bar': { 'type': 'int', 'if': 'defined(TEST_IF_STRUCT_BAR)'} } }
>>>
>>> Enum values can be exploded as dictionnary with 'type'/'if' keys:
>>>
>>> { 'enum': 'TestIfEnum', 'data':
>>>   [ 'foo',
>>> { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ] }
>>>
>>> A good benefit from having conditional schema is that introspection
>>> will reflect more accurately the capability of the server. Another
>>> benefit is that it may help to remove some dead code when disabling a
>>> functionality.
>>
>> This is the main benefit.  Until we realize it, introspection remains
>> seriously hobbled.
>>
>> A few closing remarks.
>>
>> The general approach "generate the #if for the compiler to evaluate"
>> looks sound.
>>
>> I haven't been able to fully review how it's integrated into the QAPI
>> language and how the generators implement it.  I hope a bit of judicious
>> patch splitting will help me over the hump.
>
> I have done some patch splitting, that doubles the number of patches though ;)

A big pile of patches can look scary, but what really drags out review
is oversized non-trivial patches like PATCH 07.  I can take quick,
refreshing breaks much more easily between patches than within a big &
hairy one.

>> As so often, solving one problem makes other problems more visible.  In
>> this case, the problem that we generate a monolith, and pay for that
>> with compile time.  More of it once we compile the monolith per target.
>
> Indeed, it would be nice to improve that soon after.
>
>>
>>> Starting from patch "qapi: add conditions to VNC type/commands/events
>>> on the schema", the series demonstrate adding conditions, in order to
>>> remove qmp_unregister_commands_hack(). However, it feels like I
>>> cheated a little bit by using per-target NEED_CPU_H in the headers to
>>> avoid #define poison. The alternative could be to split the headers in
>>> common/target?
>>
>> Yup, we could really use a way to modularize the generated code.
>>
>> If your work leads us to ideas on how to crack the monolith, great.  If
>> not, we'll have to decide whether we can live with the compile time hit.
>> I'd rather not block your work on cracking the monolith.
>
> I think we could start by splitting the json schemas.

The way things work, QMP needs to be defined in a single schema.
Doesn't mean we have to generate monoliths from it.

>>> There are a lot more things we could make conditional in the QAPI
>>> schema, like pci/kvm/xen/numa/vde/slirp/posix/win32/vsock/lzo etc etc,
>>> however I am still evaluating the implication of such changes both
>>> externally and internally, for those interested, I can share my wip
>>> branch.
>>
>> You provide the infrastructure and useful examples of use.  Good enough,
>> no need to hunt down *all* uses right away.
>>
>
> I am sending a new version,
>
> thanks

Re: [Qemu-devel] [PATCH 00/26] qapi: add #if pre-processor conditions to generated code

[Marc-André Lureau]

Hi

On Thu, Aug 17, 2017 at 3:55 PM, Markus Armbruster  wrote:
> Marc-André Lureau  writes:
>
>> Hi,
>>
>> In order to clean-up some hacks in qapi (having to unregister commands
>> at runtime), I proposed a "[PATCH v5 02/20] qapi.py: add a simple #ifdef 
>> condition"
>>
>> (see http://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03106.html).
>>
>> However, we decided to drop that patch from the series and solve the
>> problem later. The main issues were:
>> - the syntax was awkward to the JSON schema and documentation
>> - the evaluation of the condition was done in the qapi scripts, with
>>   very limited capability
>> - each target/config would need different generated files.
>>
>> Instead, it could defer the #if evaluation to the C-preprocessor.
>>
>> With this series, top-level qapi JSON entity can take 'if' keys:
>>
>> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>>   'if': 'defined(TEST_IF_STRUCT)' }
>>
>> Members can be exploded as dictionnary with 'type'/'if' keys:
>>
>> { 'struct': 'TestIfStruct', 'data':
>>   { 'foo': 'int',
>> 'bar': { 'type': 'int', 'if': 'defined(TEST_IF_STRUCT_BAR)'} } }
>>
>> Enum values can be exploded as dictionnary with 'type'/'if' keys:
>>
>> { 'enum': 'TestIfEnum', 'data':
>>   [ 'foo',
>> { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ] }
>>
>> A good benefit from having conditional schema is that introspection
>> will reflect more accurately the capability of the server. Another
>> benefit is that it may help to remove some dead code when disabling a
>> functionality.
>
> This is the main benefit.  Until we realize it, introspection remains
> seriously hobbled.
>
> A few closing remarks.
>
> The general approach "generate the #if for the compiler to evaluate"
> looks sound.
>
> I haven't been able to fully review how it's integrated into the QAPI
> language and how the generators implement it.  I hope a bit of judicious
> patch splitting will help me over the hump.

I have done some patch splitting, that doubles the number of patches though ;)

>
> As so often, solving one problem makes other problems more visible.  In
> this case, the problem that we generate a monolith, and pay for that
> with compile time.  More of it once we compile the monolith per target.

Indeed, it would be nice to improve that soon after.

>
>> Starting from patch "qapi: add conditions to VNC type/commands/events
>> on the schema", the series demonstrate adding conditions, in order to
>> remove qmp_unregister_commands_hack(). However, it feels like I
>> cheated a little bit by using per-target NEED_CPU_H in the headers to
>> avoid #define poison. The alternative could be to split the headers in
>> common/target?
>
> Yup, we could really use a way to modularize the generated code.
>
> If your work leads us to ideas on how to crack the monolith, great.  If
> not, we'll have to decide whether we can live with the compile time hit.
> I'd rather not block your work on cracking the monolith.

I think we could start by splitting the json schemas.

>
>> There are a lot more things we could make conditional in the QAPI
>> schema, like pci/kvm/xen/numa/vde/slirp/posix/win32/vsock/lzo etc etc,
>> however I am still evaluating the implication of such changes both
>> externally and internally, for those interested, I can share my wip
>> branch.
>
> You provide the infrastructure and useful examples of use.  Good enough,
> no need to hunt down *all* uses right away.
>

I am sending a new version,

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 00/26] qapi: add #if pre-processor conditions to generated code

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi,
>
> In order to clean-up some hacks in qapi (having to unregister commands
> at runtime), I proposed a "[PATCH v5 02/20] qapi.py: add a simple #ifdef 
> condition"
>
> (see http://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03106.html).
>
> However, we decided to drop that patch from the series and solve the
> problem later. The main issues were:
> - the syntax was awkward to the JSON schema and documentation
> - the evaluation of the condition was done in the qapi scripts, with
>   very limited capability
> - each target/config would need different generated files.
>
> Instead, it could defer the #if evaluation to the C-preprocessor.
>
> With this series, top-level qapi JSON entity can take 'if' keys:
>
> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>   'if': 'defined(TEST_IF_STRUCT)' }
>
> Members can be exploded as dictionnary with 'type'/'if' keys:
>
> { 'struct': 'TestIfStruct', 'data':
>   { 'foo': 'int',
> 'bar': { 'type': 'int', 'if': 'defined(TEST_IF_STRUCT_BAR)'} } }
>
> Enum values can be exploded as dictionnary with 'type'/'if' keys:
>
> { 'enum': 'TestIfEnum', 'data':
>   [ 'foo',
> { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ] }
>
> A good benefit from having conditional schema is that introspection
> will reflect more accurately the capability of the server. Another
> benefit is that it may help to remove some dead code when disabling a
> functionality.

This is the main benefit.  Until we realize it, introspection remains
seriously hobbled.

A few closing remarks.

The general approach "generate the #if for the compiler to evaluate"
looks sound.

I haven't been able to fully review how it's integrated into the QAPI
language and how the generators implement it.  I hope a bit of judicious
patch splitting will help me over the hump.

As so often, solving one problem makes other problems more visible.  In
this case, the problem that we generate a monolith, and pay for that
with compile time.  More of it once we compile the monolith per target.

> Starting from patch "qapi: add conditions to VNC type/commands/events
> on the schema", the series demonstrate adding conditions, in order to
> remove qmp_unregister_commands_hack(). However, it feels like I
> cheated a little bit by using per-target NEED_CPU_H in the headers to
> avoid #define poison. The alternative could be to split the headers in
> common/target?

Yup, we could really use a way to modularize the generated code.

If your work leads us to ideas on how to crack the monolith, great.  If
not, we'll have to decide whether we can live with the compile time hit.
I'd rather not block your work on cracking the monolith.

> There are a lot more things we could make conditional in the QAPI
> schema, like pci/kvm/xen/numa/vde/slirp/posix/win32/vsock/lzo etc etc,
> however I am still evaluating the implication of such changes both
> externally and internally, for those interested, I can share my wip
> branch.

You provide the infrastructure and useful examples of use.  Good enough,
no need to hunt down *all* uses right away.

[Qemu-devel] [PATCH 00/26] qapi: add #if pre-processor conditions to generated code

[Marc-André Lureau]

Hi,

In order to clean-up some hacks in qapi (having to unregister commands
at runtime), I proposed a "[PATCH v5 02/20] qapi.py: add a simple #ifdef 
condition"

(see http://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03106.html).

However, we decided to drop that patch from the series and solve the
problem later. The main issues were:
- the syntax was awkward to the JSON schema and documentation
- the evaluation of the condition was done in the qapi scripts, with
  very limited capability
- each target/config would need different generated files.

Instead, it could defer the #if evaluation to the C-preprocessor.

With this series, top-level qapi JSON entity can take 'if' keys:

{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
  'if': 'defined(TEST_IF_STRUCT)' }

Members can be exploded as dictionnary with 'type'/'if' keys:

{ 'struct': 'TestIfStruct', 'data':
  { 'foo': 'int',
'bar': { 'type': 'int', 'if': 'defined(TEST_IF_STRUCT_BAR)'} } }

Enum values can be exploded as dictionnary with 'type'/'if' keys:

{ 'enum': 'TestIfEnum', 'data':
  [ 'foo',
{ 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ] }

A good benefit from having conditional schema is that introspection
will reflect more accurately the capability of the server. Another
benefit is that it may help to remove some dead code when disabling a
functionality.

Starting from patch "qapi: add conditions to VNC type/commands/events
on the schema", the series demonstrate adding conditions, in order to
remove qmp_unregister_commands_hack(). However, it feels like I
cheated a little bit by using per-target NEED_CPU_H in the headers to
avoid #define poison. The alternative could be to split the headers in
common/target?

There are a lot more things we could make conditional in the QAPI
schema, like pci/kvm/xen/numa/vde/slirp/posix/win32/vsock/lzo etc etc,
however I am still evaluating the implication of such changes both
externally and internally, for those interested, I can share my wip
branch.

Comments welcome,

Marc-André Lureau (26):
  qapi: fix type_seen key error
  qobject: replace dump_qobject() by qobject_to_string()
  qboject: add literal qobject type
  qapi: generate a literal qobject for introspection
  visitor: pass size of strings array to enum visitor
  qapi2texi: minor python code simplification
  qapi: add 'if' condition on top-level schema elements
  qapi: add 'if' condition on enum member values
  qapi: add 'if' condition on struct member
  qapi: add 'if' condition on union variant
  qapi: add 'if' condition on alternate variant
  qapi2texi: add 'If:' section to generated documentation
  qapi2texi: add 'If:' condition to enum values
  qapi2texi: add 'If:' condition to struct members
  qapi2texi: add condition to variants
  qapi: add conditions to VNC type/commands/events on the schema
  qapi: add conditions to SPICE type/commands/events on the schema
  qapi: add conditions to REPLICATION type/commands on the schema
  build-sys: move qapi variables in qapi.mak
  tests/qmp-test: add query-qmp-schema test
  build-sys: make qemu qapi objects per-target
  qapi: make rtc-reset-reinjection depend on TARGET_I386
  qapi: make s390 commands depend on TARGET_S390X
  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-schema.json |  98 ++
 qapi/event.json  |  21 +-
 include/qapi/visitor.h   |   3 +-
 scripts/qapi.py  | 232 ++-
 scripts/qapi-commands.py |   4 +-
 scripts/qapi-event.py|   4 +-
 scripts/qapi-introspect.py   | 116 +++-
 scripts/qapi-types.py|  55 --
 scripts/qapi-visit.py|  29 ++-
 scripts/qapi2texi.py |  49 ++---
 include/hw/qdev-core.h   |   1 +
 include/qapi/qmp/qdict.h |   2 +
 include/qapi/qmp/qlist.h |   2 +
 include/qapi/qmp/qlit.h  |  53 ++
 include/qapi/qmp/qobject.h   |   7 +
 include/qom/object.h |   4 +
 include/sysemu/arch_init.h   |  11 --
 qapi/qapi-visit-core.c   |  23 +--
 backends/hostmem.c   |   1 +
 block/qapi.c |  90 +
 crypto/secret.c  |   1 +
 crypto/tlscreds.c|   1 +
 hmp.c|  14 +-
 hw/core/qdev-properties.c|  11 +-
 migration/colo.c |  14 +-
 monitor.c|  70 +--
 net/filter.c