Re: [PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code

[Thomas Huth]

On 15/04/2021 09.44, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
> > On 14/04/2021 15.55, Markus Armbruster wrote:
> > > Thomas Huth <thuth@redhat.com> writes:
> > >
> > > > The ObjectType enum and ObjectOptions are included from qapi-types-qom.h
> > > > into common code. We should not use target-specific config switches like
> > > > CONFIG_VIRTIO_CRYPTO here, since this is not defined in common code and
> > > > thus the enum will look differently between common and target specific
> > > > code. For this case, it's hopefully enough to check for CONFIG_VHOST_CRYPTO
> > > > only (which is a host specific config switch, i.e. it's the same on all
> > > > targets).
> > >
> > > Drawback: introspection now claims cryptodev-vhost-user is among the
> > > values of qom-type, which is a lie when !defined(CONFIG_VIRTIO_CRYPTO).
> > >
> > > Is this the first lie about QOM object types?
> > >
> > > Do we care?
> >
> > I don't think we really care, since there are other entries in the list
> > which are obviously only available on certain targets or configurations, but
> > not fenced with "if"s, e.g. s390-pv-guest, input-linux or rng-random.
>
> So introspection already flawed, and adding another instance doesn't
> really make it worse.
>
> > Or do you see a special problem with cryptodev-vhost-user here?
>
> No, only the general problem that query-qmp-schema can't reliably tell
> us what QOM types are available.
>
> I see no need to revert the patch.
>
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >    qapi/qom.json | 4 ++--
> > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/qapi/qom.json b/qapi/qom.json
> > > > index db5ac419b1..cd0e76d564 100644
> > > > --- a/qapi/qom.json
> > > > +++ b/qapi/qom.json
> > > > @@ -752,7 +752,7 @@
> > > >        'cryptodev-backend',
> > > >        'cryptodev-backend-builtin',
> > > >        { 'name': 'cryptodev-vhost-user',
> > > > -      'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
> > > > +      'if': 'defined(CONFIG_VHOST_CRYPTO)' },
> > > >        'dbus-vmstate',
> > > >        'filter-buffer',
> > > >        'filter-dump',
> > > > @@ -809,7 +809,7 @@
> > > >          'cryptodev-backend':          'CryptodevBackendProperties',
> > > >          'cryptodev-backend-builtin':  'CryptodevBackendProperties',
> > > >          'cryptodev-vhost-user':       { 'type': 'CryptodevVhostUserProperties',
> > > > -                                      'if': 'defined(CONFIG_VIRTIO_CRYPTO) && 
> > > > defined(CONFIG_VHOST_CRYPTO)' },
> > > > +                                      'if': 'defined(CONFIG_VHOST_CRYPTO)' },
> > > >          'dbus-vmstate':               'DBusVMStateProperties',
> > > >          'filter-buffer':              'FilterBufferProperties',
> > > >          'filter-dump':                'FilterDumpProperties',
> > >
> > > Could CryptodevVhostUserProperties be conditional, too?
> >
> > That's certainly a question for the QOM experts here...
>
> Here's the expert's method to find out: slap on the conditional,
> compile with all targets enabled, see whether any of them explode.
>
> Mind to try?

Yes. I've currently got plenty of other stuff to do. So I'd prefer if you 
could give it a try instead.

 Thomas