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

[Thomas Huth <thuth@redhat.com>]

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