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

[Marcel Apfelbaum]

On 09/09/2016 02:36 PM, Maxime Coquelin wrote:
> On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> > On Fri, 9 Sep 2016 14:02:17 +0300
> > Marcel Apfelbaum <address@hidden> 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.

Sounds good to me. Let's go with it and we'll re-think it if
we'll find other feature negotiation issues later.

Thanks,
Marcel


> Thanks,
> Maxime