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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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 <address@hidden>
>> ---
>>  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

You mean s->serial, don't you?

> 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.

I can't see that.

strncpy(dst, src, n) reads up to n characters or the terminating zero,
whatever comes first.

strncpy()'s second argument is zero-terminated.  "" trivially is.
s->serial is a malloc'ed, zero-terminated string.  It's set either by
qdev_prop_string's parse_string(), or two patch hunks down (some snipped
text restored for your convenience).  In both cases, the value is
freshly strdup'ed.  Therefore, strncpy() won't read beyond the buffer.

strncpy(dst, src, n) writes exactly n characters.

Its third argument is the minimum of the buffer size and
VIRTIO_BLK_ID_BYTES.

The old code works the same, only it uses sizeof(s->sn) instead of
VIRTIO_BLK_ID_BYTES, with sn defined as "char sn[BLOCK_SERIAL_STRLEN]".

VIRTIO_BLK_ID_BYTES is really part of the virtio protocol.  Its value
indeed happens to be the same as BLOCK_SERIAL_STRLEN, but I chose not to
rely on that.  Matches IDE and SCSI, only they use literals instead of
defines.

The kernel has it in include/linux/virtio_blk.h.  Our partial copy of
that header is hw/virtio-blk.h.  I added VIRTIO_BLK_ID_BYTES to that
copy (four patch hunks down).

>> @@ -531,7 +536,8 @@ static void virtio_blk_change_cb(void *opaque, int 
>> reason)
>>      }
>>  }
>>  
>> -VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
>> +VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
>> +                              char **serial)
>>  {
>>      VirtIOBlock *s;
>>      int cylinders, heads, secs;
>> @@ -547,6 +553,14 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, 
>> BlockConf *conf)
>>          return NULL;
>>      }
>>  
>> +    if (!*serial) {
>> +        /* try to fall back to value set with legacy -drive serial=... */
>> +        dinfo = drive_get_by_blockdev(conf->bs);
>> +        if (*dinfo->serial) {
>> +            *serial = strdup(dinfo->serial);
>> +        }
>> +    }
>> +
>>      s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
>>                                            sizeof(struct virtio_blk_config),
>>                                            sizeof(VirtIOBlock));
>> 
>> @@ -556,16 +570,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, 
>> BlockConf *conf)
>>      s->vdev.reset = virtio_blk_reset;
>>      s->bs = conf->bs;
>>      s->conf = conf;
>> +    s->serial = *serial;
>>      s->rq = NULL;
>>      s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
>>      bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>>  
>> -    /* NB: per existing s/n string convention the string is terminated
>> -     * by '\0' only when less than sizeof (s->sn)
>> -     */
>> -    dinfo = drive_get_by_blockdev(s->bs);
>> -    strncpy(s->sn, dinfo->serial, sizeof (s->sn));
>> -
>>      s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>>  
>>      qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
>> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
>> index fff46da..5645d2b 100644
>> --- a/hw/virtio-blk.h
>> +++ b/hw/virtio-blk.h
>> @@ -34,6 +34,8 @@
>>  #define VIRTIO_BLK_F_WCACHE     9       /* write cache enabled */
>>  #define VIRTIO_BLK_F_TOPOLOGY   10      /* Topology information is 
>> available */
>>  
>> +#define VIRTIO_BLK_ID_BYTES     20      /* ID string length */
>> +
>>  struct virtio_blk_config
>>  {
>>      uint64_t capacity;