Re: [Qemu-devel] [PATCH v9 03/10] virtio-blk: don't use pointer for configuration.

[KONRAD Frédéric]

On 18/03/2013 09:59, Kevin Wolf wrote:
> Am 15.03.2013 um 19:48 hat address@hidden geschrieben:
> > From: KONRAD Frederic <address@hidden>
> >
> > The configuration field must not be a pointer as it will be used for virtio-blk
> > properties. So *blk is replaced by blk in VirtIOBlock structure.
> >
> > Signed-off-by: KONRAD Frederic <address@hidden>
> > Reviewed-by: Peter Maydell <address@hidden>
> > ---
> >   hw/virtio-blk.c | 8 ++++----
> >   hw/virtio-blk.h | 2 +-
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > index 6714b01..908c316 100644
> > --- a/hw/virtio-blk.c
> > +++ b/hw/virtio-blk.c
> > @@ -151,7 +151,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
> >        */
> >       req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
> >   
> > -    if (!req->dev->blk->scsi) {
> > +    if (!req->dev->blk.scsi) {
> >           status = VIRTIO_BLK_S_UNSUPP;
> >           goto fail;
> >       }
> > @@ -371,7 +371,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
> >            * terminated by '\0' only when shorter than buffer.
> >            */
> >           strncpy(req->elem.in_sg[0].iov_base,
> > -                s->blk->serial ? s->blk->serial : "",
> > +                s->blk.serial ? s->blk.serial : "",
> >                   MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
> >           virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> >           g_free(req);
> > @@ -534,7 +534,7 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, 
> > uint32_t features)
> >       features |= (1 << VIRTIO_BLK_F_BLK_SIZE);
> >       features |= (1 << VIRTIO_BLK_F_SCSI);
> >   
> > -    if (s->blk->config_wce) {
> > +    if (s->blk.config_wce) {
> >           features |= (1 << VIRTIO_BLK_F_CONFIG_WCE);
> >       }
> >       if (bdrv_enable_write_cache(s->bs))
> > @@ -650,7 +650,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, 
> > VirtIOBlkConf *blk)
> >       s->vdev.reset = virtio_blk_reset;
> >       s->bs = blk->conf.bs;
> >       s->conf = &blk->conf;
> > -    s->blk = blk;
> > +    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
> Why not simply s->blk = *blk?
>
> The reason why copying this works is that blk is read-only after
> initialisation. We also get an additional reference to blk->serial, but
> we know that it can only go away after this device has been destroyed
> (the same assumption was necessary for the s->blk pointer in the old
> code).

You mean this copying (s->blk = *blk) ?

> Is my understanding of this correct?
>
> Kevin