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

[Kevin Wolf <kwolf@redhat.com>]

Am 04.07.2011 13:29, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> 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 <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
> 
> 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.

You're right, sorry for the noise.

What confused me is that I didn't expect some limit in the protocol (and
if any, then certainly not 20), so I started making wild guesses what
this might be used for, and reading strncpy as memcpy because it made
more sense with the guesses...

I should just stop reviewing patches early in the morning. :-)

Applied the patch to the block branch.

Kevin