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:42, Peter Maydell wrote:
> On 12 March 2013 14:37, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>> 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?
> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
> ones that are expected to be used by other code, right? So you can
> define them with commas (and name them something so it's obvious
> they're not intended for wider use as property array elements),
> and then just make sure your public-facing DEFINE_VIRTIO_BLK_PROPERTIES
> doesn't end with a comma. (You can do that by putting the macros
> that expand to maybe-comma-or-not at the front, not the end.)
>
> -- PMM
ok, I can put a comment which say not to use them?

Thanks,

Fred