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