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

[KONRAD Frédéric]

On 05/12/2012 17:25, Peter Maydell wrote:
> 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).
Ok, I think you're right, I'll check.

> >       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.
>
> -- PMM
Yes, sure, I put it in comment to explain, why I deleted it.

Thanks,

Fred.