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

[Andreas Färber]

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.
You can then rename vdev to parent_obj as a check that you caught all users.

Regards,
Andreas

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