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

["KONRAD Frédéric" <fred.konrad@greensocs.com>]

On 12/03/2013 15:28, Peter Maydell wrote:
> On 12 March 2013 09:22,  <fred.konrad@greensocs.com> 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