Patchwork [v9,03/10] virtio-blk: don't use pointer for configuration.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date March 15, 2013, 6:48 p.m.
Message ID <1363373338-5090-4-git-send-email-fred.konrad@greensocs.com>
Download mbox | patch
Permalink /patch/228141/
State New
Headers show

Comments

fred.konrad@greensocs.com - March 15, 2013, 6:48 p.m.
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(-)
Kevin Wolf - March 18, 2013, 8:59 a.m.
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
fred.konrad@greensocs.com - March 19, 2013, 1:38 p.m.
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
Kevin Wolf - March 19, 2013, 1:55 p.m.
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
>

Patch

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;