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

[Andreas Färber]

Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
> 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,  <address@hidden> wrote:
>>>> From: KONRAD Frederic <address@hidden>
>>>>
>>>> Create virtio-blk which extends virtio-device, so it can be
>>>> connected on
>>>> virtio-bus.
>>>>
>>>> Signed-off-by: KONRAD Frederic <address@hidden>
>>>> ---
>>>>   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);
> 
> ?

Sorry. I expected to see the macros above the typedef above, but in the
header is even better! :) VIRTIO_BLOCK vs. VIRTIO_BLK is just a style
question.

Further, I missed on brief sight that the to_* function was commented
out, thought it was still being used. Didn't find enough time to review
the series fully yet.

> 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.

Don't understand that part due to lack of virtio knowledge...
Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with
this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should
only be necessary as a command line option alias for backwards
compatibility, no? Are you saying you can't make this switch and
refactoring for virtio-blk *only*?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg