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