Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Marcel Apfelbaum <marcel@redhat.com>]

On 09/09/2016 01:33 PM, Cornelia Huck wrote:
> On Fri,  9 Sep 2016 12:14:31 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>> This patch adds virtio_test_backend_feature() function to
>> enable checking a backend feature before the negociation
>> takes place.
>>
>> It may be used, for example, to check whether the backend
>> supports VIRTIO_F_VERSION_1 before enabling modern
>> capabilities.
>>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  hw/virtio/virtio.c         | 14 ++++++++++++++
>>  include/hw/virtio/virtio.h |  2 ++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 74c085c..7ab91a1 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>  }
>>
>> +bool virtio_test_backend_feature(VirtIODevice *vdev,
>> +                                 unsigned int fbit, Error **errp)
>> +{
>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +    uint64_t feature;
>> +
>> +    virtio_add_feature(&feature, fbit);
>> +
>> +    assert(k->get_features != NULL);
>> +    feature = k->get_features(vdev, feature, errp);
>> +
>> +    return virtio_has_feature(feature, fbit);
>> +}
>> +
>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>  {
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>
> What happens if you want to test for features that depend upon each
> other? The backend may support your feature, but it may withdraw the
> feature bit if a dependency is not fullfilled.
>
> You'll probably want to run validation on the whole feature set; but
> that is hard if you're too early in the setup process.
>

While I agree with the feature dependency issue , would the negation be ok?
What I mean is: if the backend does not support feature X, no
matter what the depending features are, it will still not support it after the negotiation.

Changing the function to virtio_backend_unsupported_feature(x) would be better?

Thanks,
Marcel