Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

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

On 05/12/2012 18:22, Andreas Färber wrote:
> Am 05.12.2012 17:25, schrieb Peter Maydell:
>> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> Create virtio-blk which extends virtio-device, so it can be connected on
>>> virtio-bus.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>>   hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>   hw/virtio-blk.h |   4 ++
>>>   2 files changed, 150 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>> index e25cc96..ee1ea8b 100644
>>> --- a/hw/virtio-blk.c
>>> +++ b/hw/virtio-blk.c
>>> @@ -21,24 +21,42 @@
>>>   #ifdef __linux__
>>>   # include <scsi/sg.h>
>>>   #endif
>>> +#include "virtio-bus.h"
>>>
>>> +/* Take this structure as our device structure. */
>>>   typedef struct VirtIOBlock
>>>   {
>>> +    /*
>>> +     * Adding parent_obj breaks to_virtio_blk cast function,
>>> +     * and virtio_blk_init.
>>> +     */
>>> +    DeviceState parent_obj;
>>> +    /*
>>> +     * We don't need that anymore, as we'll use QOM cast to get the
>>> +     * VirtIODevice. Just temporary keep it, for not breaking functionality.
>>> +     */
>>>       VirtIODevice vdev;
>> This doesn't make sense. After your previous patch, VirtIODevice
>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
>> so there's nothing to do here. Adding this parent_obj field
>> here is just breaking things (it would make the VirtIOBlock
>> into a direct child of DeviceState, which isn't what we want).
>>
>>>       BlockDriverState *bs;
>>>       VirtQueue *vq;
>>>       void *rq;
>>>       QEMUBH *bh;
>>>       BlockConf *conf;
>>> -    VirtIOBlkConf *blk;
>>> +    /*
>>> +     * We can't use pointer with properties.
>>> +     */
>>> +    VirtIOBlkConf blk;
>>>       unsigned short sector_mask;
>>>       DeviceState *qdev;
>>>   } VirtIOBlock;
>>>
>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>> -{
>>> -    return (VirtIOBlock *)vdev;
>>> -}
>>> +/*
>>> + * Use the QOM cast, so we don't need that anymore.
>>> + *
>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>> + * {
>>> + *     return (VirtIOBlock *)vdev;
>>> + * }
>>> + */
>> If we don't need it, just delete it.
> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by
> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with
> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on.
Yes, that's why I comment to_virtio_blk().

Isn't what I made in this patch with :

+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
+

and

-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);

?


I agree with that, but, there is an issue :
The refactored VirtIOBlk is a device and seems to work, but the device 
which use this VirtIOBlock
(eg virtio-blk-pci) are just allocating a structure ( in 
virtio_common_init ).

That's why this patch is breaking virtio-blk-pci.
>
> Regards,
> Andreas
>

Thanks,

Fred.