[PATCH] qapi: Make some ObjectTypes depend on the build settings

[PATCH] qapi: Make some ObjectTypes depend on the build settings

[Thomas Huth]

Some of the ObjectType entries already depend on CONFIG_* switches.
Some others also only make sense with certain configurations, but
are currently always listed in the ObjectType enum. Let's make them
depend on the correpsonding CONFIG_* switches, too, so that upper
layers (like libvirt) have a better way to determine which features
are available in QEMU.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qapi/qom.json | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index a25616bc7a..78b60433a9 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -777,7 +777,8 @@
     'authz-pam',
     'authz-simple',
     'can-bus',
-    'can-host-socketcan',
+    { 'name': 'can-host-socketcan',
+      'if': 'CONFIG_LINUX' },
     'colo-compare',
     'cryptodev-backend',
     'cryptodev-backend-builtin',
@@ -791,20 +792,24 @@
     'filter-replay',
     'filter-rewriter',
     'input-barrier',
-    'input-linux',
+    { 'name': 'input-linux',
+      'if': 'CONFIG_LINUX' },
     'iothread',
     'memory-backend-file',
     { 'name': 'memory-backend-memfd',
       'if': 'CONFIG_LINUX' },
     'memory-backend-ram',
     'pef-guest',
-    'pr-manager-helper',
+    { 'name': 'pr-manager-helper',
+      'if': 'CONFIG_LINUX' },
     'qtest',
     'rng-builtin',
     'rng-egd',
-    'rng-random',
+    { 'name': 'rng-random',
+      'if': 'CONFIG_POSIX' },
     'secret',
-    'secret_keyring',
+    { 'name': 'secret_keyring',
+      'if': 'CONFIG_SECRET_KEYRING' },
     'sev-guest',
     's390-pv-guest',
     'throttle-group',
@@ -835,7 +840,8 @@
       'authz-listfile':             'AuthZListFileProperties',
       'authz-pam':                  'AuthZPAMProperties',
       'authz-simple':               'AuthZSimpleProperties',
-      'can-host-socketcan':         'CanHostSocketcanProperties',
+      'can-host-socketcan':         { 'type': 'CanHostSocketcanProperties',
+                                      'if': 'CONFIG_LINUX' },
       'colo-compare':               'ColoCompareProperties',
       'cryptodev-backend':          'CryptodevBackendProperties',
       'cryptodev-backend-builtin':  'CryptodevBackendProperties',
@@ -849,19 +855,23 @@
       'filter-replay':              'NetfilterProperties',
       'filter-rewriter':            'FilterRewriterProperties',
       'input-barrier':              'InputBarrierProperties',
-      'input-linux':                'InputLinuxProperties',
+      'input-linux':                { 'type': 'InputLinuxProperties',
+                                      'if': 'CONFIG_LINUX' },
       'iothread':                   'IothreadProperties',
       'memory-backend-file':        'MemoryBackendFileProperties',
       'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
                                       'if': 'CONFIG_LINUX' },
       'memory-backend-ram':         'MemoryBackendProperties',
-      'pr-manager-helper':          'PrManagerHelperProperties',
+      'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
+                                      'if': 'CONFIG_LINUX' },
       'qtest':                      'QtestProperties',
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
-      'rng-random':                 'RngRandomProperties',
+      'rng-random':                 { 'type': 'RngRandomProperties',
+                                      'if': 'CONFIG_POSIX' },
       'secret':                     'SecretProperties',
-      'secret_keyring':             'SecretKeyringProperties',
+      'secret_keyring':             { 'type': 'SecretKeyringProperties',
+                                      'if': 'CONFIG_SECRET_KEYRING' },
       'sev-guest':                  'SevGuestProperties',
       'throttle-group':             'ThrottleGroupProperties',
       'tls-creds-anon':             'TlsCredsAnonProperties',
-- 
2.27.0

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Philippe Mathieu-Daudé]

On 9/28/21 18:02, Thomas Huth wrote:
> Some of the ObjectType entries already depend on CONFIG_* switches.
> Some others also only make sense with certain configurations, but
> are currently always listed in the ObjectType enum. Let's make them
> depend on the correpsonding CONFIG_* switches, too, so that upper
> layers (like libvirt) have a better way to determine which features
> are available in QEMU.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qapi/qom.json | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/qapi/qom.json b/qapi/qom.json
> index a25616bc7a..78b60433a9 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -777,7 +777,8 @@
>      'authz-pam',
>      'authz-simple',
>      'can-bus',
> -    'can-host-socketcan',
> +    { 'name': 'can-host-socketcan',
> +      'if': 'CONFIG_LINUX' },
>      'colo-compare',
>      'cryptodev-backend',
>      'cryptodev-backend-builtin',
> @@ -791,20 +792,24 @@
>      'filter-replay',
>      'filter-rewriter',
>      'input-barrier',
> -    'input-linux',
> +    { 'name': 'input-linux',
> +      'if': 'CONFIG_LINUX' },
>      'iothread',
>      'memory-backend-file',
>      { 'name': 'memory-backend-memfd',
>        'if': 'CONFIG_LINUX' },
>      'memory-backend-ram',
>      'pef-guest',
> -    'pr-manager-helper',
> +    { 'name': 'pr-manager-helper',
> +      'if': 'CONFIG_LINUX' },
>      'qtest',
>      'rng-builtin',
>      'rng-egd',
> -    'rng-random',
> +    { 'name': 'rng-random',
> +      'if': 'CONFIG_POSIX' },
>      'secret',
> -    'secret_keyring',
> +    { 'name': 'secret_keyring',
> +      'if': 'CONFIG_SECRET_KEYRING' },
>      'sev-guest',
>      's390-pv-guest',
>      'throttle-group',
> @@ -835,7 +840,8 @@
>        'authz-listfile':             'AuthZListFileProperties',
>        'authz-pam':                  'AuthZPAMProperties',
>        'authz-simple':               'AuthZSimpleProperties',
> -      'can-host-socketcan':         'CanHostSocketcanProperties',
> +      'can-host-socketcan':         { 'type': 'CanHostSocketcanProperties',
> +                                      'if': 'CONFIG_LINUX' },
>        'colo-compare':               'ColoCompareProperties',
>        'cryptodev-backend':          'CryptodevBackendProperties',
>        'cryptodev-backend-builtin':  'CryptodevBackendProperties',
> @@ -849,19 +855,23 @@
>        'filter-replay':              'NetfilterProperties',
>        'filter-rewriter':            'FilterRewriterProperties',
>        'input-barrier':              'InputBarrierProperties',
> -      'input-linux':                'InputLinuxProperties',
> +      'input-linux':                { 'type': 'InputLinuxProperties',
> +                                      'if': 'CONFIG_LINUX' },
>        'iothread':                   'IothreadProperties',
>        'memory-backend-file':        'MemoryBackendFileProperties',
>        'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
>                                        'if': 'CONFIG_LINUX' },
>        'memory-backend-ram':         'MemoryBackendProperties',
> -      'pr-manager-helper':          'PrManagerHelperProperties',
> +      'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
> +                                      'if': 'CONFIG_LINUX' },
>        'qtest':                      'QtestProperties',
>        'rng-builtin':                'RngProperties',
>        'rng-egd':                    'RngEgdProperties',
> -      'rng-random':                 'RngRandomProperties',
> +      'rng-random':                 { 'type': 'RngRandomProperties',
> +                                      'if': 'CONFIG_POSIX' },
>        'secret':                     'SecretProperties',
> -      'secret_keyring':             'SecretKeyringProperties',
> +      'secret_keyring':             { 'type': 'SecretKeyringProperties',
> +                                      'if': 'CONFIG_SECRET_KEYRING' },
>        'sev-guest':                  'SevGuestProperties',
>        'throttle-group':             'ThrottleGroupProperties',
>        'tls-creds-anon':             'TlsCredsAnonProperties',
> 

I quickly opened qapi/qom.json and spotted another one:

--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -870,3 +870,4 @@
       'tls-cipher-suites':          'TlsCredsProperties',
-      'x-remote-object':            'RemoteObjectProperties'
+      'x-remote-object':            { 'type': 'RemoteObjectProperties',
+                                      'if': 'CONFIG_MULTIPROCESS' },
   } }

While your change is correct, this isn't maintainable long term.
Not sure how we could improve that :/ But having to handle similar
information in 3 different places (configure, meson.build, qapi json)
is error prone. Thoughts?

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Thomas Huth]

On 28/09/2021 19.39, Philippe Mathieu-Daudé wrote:
> On 9/28/21 18:02, Thomas Huth wrote:
>> Some of the ObjectType entries already depend on CONFIG_* switches.
>> Some others also only make sense with certain configurations, but
>> are currently always listed in the ObjectType enum. Let's make them
>> depend on the correpsonding CONFIG_* switches, too, so that upper
>> layers (like libvirt) have a better way to determine which features
>> are available in QEMU.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   qapi/qom.json | 30 ++++++++++++++++++++----------
>>   1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index a25616bc7a..78b60433a9 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -777,7 +777,8 @@
>>       'authz-pam',
>>       'authz-simple',
>>       'can-bus',
>> -    'can-host-socketcan',
>> +    { 'name': 'can-host-socketcan',
>> +      'if': 'CONFIG_LINUX' },
>>       'colo-compare',
>>       'cryptodev-backend',
>>       'cryptodev-backend-builtin',
>> @@ -791,20 +792,24 @@
>>       'filter-replay',
>>       'filter-rewriter',
>>       'input-barrier',
>> -    'input-linux',
>> +    { 'name': 'input-linux',
>> +      'if': 'CONFIG_LINUX' },
>>       'iothread',
>>       'memory-backend-file',
>>       { 'name': 'memory-backend-memfd',
>>         'if': 'CONFIG_LINUX' },
>>       'memory-backend-ram',
>>       'pef-guest',
>> -    'pr-manager-helper',
>> +    { 'name': 'pr-manager-helper',
>> +      'if': 'CONFIG_LINUX' },
>>       'qtest',
>>       'rng-builtin',
>>       'rng-egd',
>> -    'rng-random',
>> +    { 'name': 'rng-random',
>> +      'if': 'CONFIG_POSIX' },
>>       'secret',
>> -    'secret_keyring',
>> +    { 'name': 'secret_keyring',
>> +      'if': 'CONFIG_SECRET_KEYRING' },
>>       'sev-guest',
>>       's390-pv-guest',
>>       'throttle-group',
>> @@ -835,7 +840,8 @@
>>         'authz-listfile':             'AuthZListFileProperties',
>>         'authz-pam':                  'AuthZPAMProperties',
>>         'authz-simple':               'AuthZSimpleProperties',
>> -      'can-host-socketcan':         'CanHostSocketcanProperties',
>> +      'can-host-socketcan':         { 'type': 'CanHostSocketcanProperties',
>> +                                      'if': 'CONFIG_LINUX' },
>>         'colo-compare':               'ColoCompareProperties',
>>         'cryptodev-backend':          'CryptodevBackendProperties',
>>         'cryptodev-backend-builtin':  'CryptodevBackendProperties',
>> @@ -849,19 +855,23 @@
>>         'filter-replay':              'NetfilterProperties',
>>         'filter-rewriter':            'FilterRewriterProperties',
>>         'input-barrier':              'InputBarrierProperties',
>> -      'input-linux':                'InputLinuxProperties',
>> +      'input-linux':                { 'type': 'InputLinuxProperties',
>> +                                      'if': 'CONFIG_LINUX' },
>>         'iothread':                   'IothreadProperties',
>>         'memory-backend-file':        'MemoryBackendFileProperties',
>>         'memory-backend-memfd':       { 'type': 'MemoryBackendMemfdProperties',
>>                                         'if': 'CONFIG_LINUX' },
>>         'memory-backend-ram':         'MemoryBackendProperties',
>> -      'pr-manager-helper':          'PrManagerHelperProperties',
>> +      'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
>> +                                      'if': 'CONFIG_LINUX' },
>>         'qtest':                      'QtestProperties',
>>         'rng-builtin':                'RngProperties',
>>         'rng-egd':                    'RngEgdProperties',
>> -      'rng-random':                 'RngRandomProperties',
>> +      'rng-random':                 { 'type': 'RngRandomProperties',
>> +                                      'if': 'CONFIG_POSIX' },
>>         'secret':                     'SecretProperties',
>> -      'secret_keyring':             'SecretKeyringProperties',
>> +      'secret_keyring':             { 'type': 'SecretKeyringProperties',
>> +                                      'if': 'CONFIG_SECRET_KEYRING' },
>>         'sev-guest':                  'SevGuestProperties',
>>         'throttle-group':             'ThrottleGroupProperties',
>>         'tls-creds-anon':             'TlsCredsAnonProperties',
>>
> 
> I quickly opened qapi/qom.json and spotted another one:
> 
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -870,3 +870,4 @@
>         'tls-cipher-suites':          'TlsCredsProperties',
> -      'x-remote-object':            'RemoteObjectProperties'
> +      'x-remote-object':            { 'type': 'RemoteObjectProperties',
> +                                      'if': 'CONFIG_MULTIPROCESS' },
>     } }

No, CONFIG_MULTIPROCESS is in config-poison.h (it's target specific), so 
that won't work here. We can only use the CONFIG switches from config-host.h 
here.

> While your change is correct, this isn't maintainable long term.
> Not sure how we could improve that :/ But having to handle similar
> information in 3 different places (configure, meson.build, qapi json)
> is error prone. Thoughts?

The current situation is just that bad since we didn't have these 'if:' 
statements in the past. For future code, I think we just have to be more 
careful during code review...
(and for configure vs. meson.build the answer is clear: Move more stuff from 
the configure script into meson.build, so that configure finally is only a 
stupid simple wrapper script)

  Thomas

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 28/09/2021 19.39, Philippe Mathieu-Daudé wrote:
>> I quickly opened qapi/qom.json and spotted another one:
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -870,3 +870,4 @@
>>         'tls-cipher-suites':          'TlsCredsProperties',
>> -      'x-remote-object':            'RemoteObjectProperties'
>> +      'x-remote-object':            { 'type': 'RemoteObjectProperties',
>> +                                      'if': 'CONFIG_MULTIPROCESS' },
>>     } }
>
> No, CONFIG_MULTIPROCESS is in config-poison.h (it's target specific),
> so that won't work here. We can only use the CONFIG switches from
> config-host.h here.

Target-specific conditions are available in qapi/*-target.json only.

The generated qapi-*-target.h are only usable from target-specific
code.

Moving stuff to qapi/*-target.json may necessitate compiling much more
code per target.  Care is advised.

>> While your change is correct, this isn't maintainable long term.
>> Not sure how we could improve that :/ But having to handle similar
>> information in 3 different places (configure, meson.build, qapi json)
>> is error prone. Thoughts?
>
> The current situation is just that bad since we didn't have these
> 'if:' statements in the past. For future code, I think we just have to
> be more careful during code review...

In my opinion, use of CONFIG_FOO in QAPI schemata is no worse than using
them in C type definitions.

In both cases, we have a choice: compile out stuff this build doesn't
need with compile-time conditionals, or leave it in unused.

In C, we sometimes have to compile out stuff, say when it depends on
headers we don't have.

In QAPI, we sometimes want to compile out stuff to make introspection
more useful.  This can be a killer argument.

> (and for configure vs. meson.build the answer is clear: Move more
> stuff from the configure script into meson.build, so that configure
> finally is only a stupid simple wrapper script)
>
>  Thomas

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Thomas Huth]

On 29/09/2021 13.41, Markus Armbruster wrote:
[...]
> In my opinion, use of CONFIG_FOO in QAPI schemata is no worse than using
> them in C type definitions.
> 
> In both cases, we have a choice: compile out stuff this build doesn't
> need with compile-time conditionals, or leave it in unused.
> 
> In C, we sometimes have to compile out stuff, say when it depends on
> headers we don't have.
> 
> In QAPI, we sometimes want to compile out stuff to make introspection
> more useful.  This can be a killer argument.

So what's your opinion on this patch here? Good to go? Needs a rework? Or 
should I simply forget about it?

  Thomas

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Some of the ObjectType entries already depend on CONFIG_* switches.
> Some others also only make sense with certain configurations, but
> are currently always listed in the ObjectType enum. Let's make them
> depend on the correpsonding CONFIG_* switches, too, so that upper
> layers (like libvirt) have a better way to determine which features
> are available in QEMU.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

All these look good to me.  I didn't look for more.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Markus Armbruster]

Paolo, do you have something for QOM queued up already?  If not, I'm
happy to take this through my tree.

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Paolo Bonzini]

On 08/10/21 14:01, Markus Armbruster wrote:
> Paolo, do you have something for QOM queued up already?  If not, I'm
> happy to take this through my tree.
> 

I don't but I have enough stuff that I'll be sending a pull request 
shortly.  So, queued, and while at it I also made memory-backend-epc 
depend on CONFIG_LINUX (more strictly it depends on CONFIG_SGX, which 
depends on CONFIG_KVM, which depends on CONFIG_LINUX; but the other two 
are target-dependent so we have to do with CONFIG_LINUX).

Paolo

Re: [PATCH] qapi: Make some ObjectTypes depend on the build settings

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 08/10/21 14:01, Markus Armbruster wrote:
>> Paolo, do you have something for QOM queued up already?  If not, I'm
>> happy to take this through my tree.
>> 
>
> I don't but I have enough stuff that I'll be sending a pull request
> shortly.  So, queued, and while at it I also made memory-backend-epc 
> depend on CONFIG_LINUX (more strictly it depends on CONFIG_SGX, which
> depends on CONFIG_KVM, which depends on CONFIG_LINUX; but the other
> two are target-dependent so we have to do with CONFIG_LINUX).

Thanks!

Could you throw in this one?

    Subject: [PATCH] monitor: Tidy up find_device_state()
    Date: Thu, 16 Sep 2021 13:17:07 +0200
    Message-Id: <20210916111707.84999-1-armbru@redhat.com>