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

[Markus Armbruster]

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?