Re: [Qemu-devel] [PATCH] virtio-blk: Turn drive serial into a qdev property

[Kevin Wolf <kwolf@redhat.com>]

Am 20.06.2011 11:35, schrieb Markus Armbruster:
> It needs to be a qdev property, because it belongs to the drive's
> guest part.  Precedence: commit a0fef654 and 6ced55a5.
> 
> Bonus: info qtree now shows the serial number.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390-virtio-bus.c |    4 +++-
>  hw/s390-virtio-bus.h |    1 +
>  hw/virtio-blk.c      |   29 +++++++++++++++++++----------
>  hw/virtio-blk.h      |    2 ++
>  hw/virtio-pci.c      |    4 +++-
>  hw/virtio-pci.h      |    1 +
>  hw/virtio.h          |    3 ++-
>  7 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index d4a12f7..2bf4821 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev)
>  {
>      VirtIODevice *vdev;
>  
> -    vdev = virtio_blk_init((DeviceState *)dev, &dev->block);
> +    vdev = virtio_blk_init((DeviceState *)dev, &dev->block,
> +                           &dev->block_serial);
>      if (!vdev) {
>          return -1;
>      }
> @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = {
>      .qdev.size = sizeof(VirtIOS390Device),
>      .qdev.props = (Property[]) {
>          DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block),
> +        DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial),
>          DEFINE_PROP_END_OF_LIST(),
>      },
>  };
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index 0c412d0..f1bece7 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device {
>      uint8_t feat_len;
>      VirtIODevice *vdev;
>      BlockConf block;
> +    char *block_serial;
>      NICConf nic;
>      uint32_t host_features;
>      virtio_serial_conf serial;
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 91e0394..6471ac8 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -28,8 +28,8 @@ typedef struct VirtIOBlock
>      void *rq;
>      QEMUBH *bh;
>      BlockConf *conf;
> +    char *serial;
>      unsigned short sector_mask;
> -    char sn[BLOCK_SERIAL_STRLEN];
>      DeviceState *qdev;
>  } VirtIOBlock;
>  
> @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>      } else if (type & VIRTIO_BLK_T_GET_ID) {
>          VirtIOBlock *s = req->dev;
>  
> -        memcpy(req->elem.in_sg[0].iov_base, s->sn,
> -               MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn)));
> +        /*
> +         * NB: per existing s/n string convention the string is
> +         * terminated by '\0' only when shorter than buffer.
> +         */
> +        strncpy(req->elem.in_sg[0].iov_base,
> +                s->serial ? s->serial : "",
> +                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));

Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here.

s->string either is dinfo->serial, in which case it happens to be the
same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a
qdev property, in which case the string just has the length it has. Or
it's the empty string. So I think in two of three cases you're
potentially reading beyond the end of the buffer.

Kevin