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, <fred.kon...@greensocs.com> wrote:
>>>> From: KONRAD Frederic <fred.kon...@greensocs.com>
>>>>
>>>> Create virtio-blk which extends virtio-device, so it can be
>>>> connected on
>>>> virtio-bus.
>>>>
>>>> Signed-off-by: KONRAD Frederic <fred.kon...@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);
>
> ?
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
