| Message ID | 1363373338-5090-4-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
Am 15.03.2013 um 19:48 hat fred.konrad@greensocs.com geschrieben: > From: KONRAD Frederic <fred.konrad@greensocs.com> > > 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 <fred.konrad@greensocs.com> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > --- > 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). Is my understanding of this correct? Kevin
On 18/03/2013 09:59, Kevin Wolf wrote: > Am 15.03.2013 um 19:48 hat fred.konrad@greensocs.com geschrieben: >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> >> 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 <fred.konrad@greensocs.com> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> 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
Am 19.03.2013 um 14:38 hat KONRAD Frédéric geschrieben: > On 18/03/2013 09:59, Kevin Wolf wrote: > >Am 15.03.2013 um 19:48 hat fred.konrad@greensocs.com geschrieben: > >>From: KONRAD Frederic <fred.konrad@greensocs.com> > >> > >>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 <fred.konrad@greensocs.com> > >>Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > >>--- > >> 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) ? Or the memcpy() in your code, which should be equivalent. Kevin > >Is my understanding of this correct? > > > >Kevin >
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)); s->rq = NULL; s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h index 19ec569..b704d50 100644 --- a/hw/virtio-blk.h +++ b/hw/virtio-blk.h @@ -118,7 +118,7 @@ typedef struct VirtIOBlock { void *rq; QEMUBH *bh; BlockConf *conf; - VirtIOBlkConf *blk; + VirtIOBlkConf blk; unsigned short sector_mask; DeviceState *qdev; VMChangeStateEntry *change;