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

[Thomas Huth <thuth@redhat.com>]

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