Re: [Qemu-devel] Re: [PATCH RFC] virtio: add features qdev property

[Michael S. Tsirkin]

On Mon, Dec 14, 2009 at 02:30:19PM +0100, Markus Armbruster wrote:
> Gerd Hoffmann <address@hidden> writes:
> 
> > On 12/14/09 12:10, Michael S. Tsirkin wrote:
> >> On Mon, Dec 14, 2009 at 11:24:41AM +0100, Gerd Hoffmann wrote:
> >> for block:
> >>      if (strcmp(s->serial_str, "0"))
> >>          features |= 1<<  VIRTIO_BLK_F_IDENTIFY;
> >>
> >>      if (bdrv_is_read_only(s->bs))
> >>          features |= 1<<  VIRTIO_BLK_F_RO;
> >
> > Sure you want these be configurable?
> >
> >> Also, I'd like these things to be saved in bits and not add a ton
> >> of fields in device. Ideas how to do this?
> >
> > I guess you only want disable features?

It's not an easy quesiton.
If we do it as disable bits, then we get incompatible
machines when running on different hosts.
Would it be better to fail instead?

> > You could have a bitmap property then, which accepts names for the
> > bits. It would need a table like this ...
> >
> >    char *bitnames[] = {
> >     [ VIRTIO_BLK_F_IDENTIFY ] = "blk-identify",
> >     [ VIRTIO_BLK_F_RO       [ = "blk-ro",
> >     [ ... ]
> >    };
> >
> > Then the property parser would accepts strings such as 'bit1|bit2' and
> > you can have
> >
> >   -device 'virtio-blk-pci,disable=blk-identify|ring-indirect'
> >
> > The driver will just do 'vdev->host_features &= ~disable'.
> 
> Use of '|' in option argument syntax is user-hostile, because it
> requires quoting in the shell.  What about '+'?

I am guessing that '|' parsing is already there,
but yes + would be a nice touch.

-- 
MST