From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:43593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qdi5O-0007ng-W8 for qemu-devel@nongnu.org; Mon, 04 Jul 2011 08:17:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QdhvW-0002Jn-Jm for qemu-devel@nongnu.org; Mon, 04 Jul 2011 08:07:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26611) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QdhvT-0002IN-2j for qemu-devel@nongnu.org; Mon, 04 Jul 2011 08:06:55 -0400 Message-ID: <4E11AD89.7000909@redhat.com> Date: Mon, 04 Jul 2011 14:09:45 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1308562518-21322-1-git-send-email-armbru@redhat.com> <4E0D7CDC.50104@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-blk: Turn drive serial into a qdev property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, agraf@suse.de Am 04.07.2011 13:29, schrieb Markus Armbruster: > Kevin Wolf 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 >>> --- >>> 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