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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> On Fri, 9 Sep 2016 14:02:17 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> I was thinking to keep the same function proposed by Maxime and change it to negate things:
>>
>> /*
>>   * A missing feature before all negotiations finished will still be missing at the end.
>>   */
>> bool virtio_test_backend_unsupported_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);
>> }
>>
>> We only check if the feature was not there from the start.
>
> I think you'll still end up with the same problem (overindicating
> unsupportedness), as you start out with an otherwise empty feature
> set, causing features with dependencies to be removed. I fear that
> anything starting with an incomplete feature list will have that
> problem.
>
> Maybe it would be better to limit this to the one bit we currently want
> to test (VERSION_1)? We know the semantics of that one. Less general,
> but also less headaches.

Yes, that could be the solution.
I agree that making it too generic might create confusion
with some features.

Thanks,
Maxime