Re: [Qemu-devel] [PATCH v6 2/8] virtio-blk: add the virtio-blk device.

[KONRAD Frédéric]

On 12/03/2013 15:28, Peter Maydell wrote:
> On 12 March 2013 09:22,  <address@hidden> wrote:
> >   /* The ID for virtio_block */
> > @@ -130,4 +134,28 @@ typedef struct VirtIOBlock {
> >   #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
> >           DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
> >
> > +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> > +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)                          \
> > +        DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
> > +#else
> > +#define DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
> > +#endif /* CONFIG_VIRTIO_BLK_DATA_PLANE */
> > +
> > +#ifdef __linux__
> > +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
> > +        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
> > +#else
> > +#define DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)
> > +#endif /* __linux__ */
> > +
> > +#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
> > +        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
> > +        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
> > +        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
> > +        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
> > +        DEFINE_VIRTIO_BLK_SCSI_PROPERTY(_state, _field)                       \
> > +        DEFINE_DATA_PLANE_PROPERTIES(_state, _field)
> > +
> > +void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
> > +
> >   #endif
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index 39c1966..9ed0228 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -1084,19 +1084,10 @@ static void virtio_rng_exit_pci(PCIDevice *pci_dev)
> >
> >   static Property virtio_blk_properties[] = {
> >       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> > -    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
> > -    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
> > -    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
> > -#ifdef __linux__
> > -    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
> > -#endif
> > -    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0, true),
> >       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, 
> > VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> > -#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> > -    DEFINE_PROP_BIT("x-data-plane", VirtIOPCIProxy, blk.data_plane, 0, false),
> > -#endif
> >       DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> >       DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
> > +    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOPCIProxy, blk)
> >       DEFINE_PROP_END_OF_LIST(),
> You need to tweak your macro definitions so that the user can
> put a comma after DEFINE_VIRTIO_BLK_PROPERTIES() [compare
> DEFINE_BLOCK_PROPERTIES and DEFINE_BLOCK_CHS_PROPERTIES].
> Otherwise it's going to get confusing and somebody's going
> to add one without noticing that that gets you an extra
> empty properties array element.
Do you have any idea for that?

It's a little tricky with the two conditions 
CONFIG_VIRTIO_BLK_DATA_PLANE and __linux__,

I can't have #define with an #ifdef inside..

Do you see what I mean?
> >   };
> >
> > --
> > 1.7.11.7
> -- PMM