From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40387 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OKQgG-0008OE-Ii for qemu-devel@nongnu.org; Fri, 04 Jun 2010 02:47:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OKQgF-0005PS-Fp for qemu-devel@nongnu.org; Fri, 04 Jun 2010 02:47:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45732) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OKQgF-0005PB-2k for qemu-devel@nongnu.org; Fri, 04 Jun 2010 02:46:59 -0400 Message-ID: <4C089E11.1050102@redhat.com> Date: Fri, 04 Jun 2010 02:32:49 -0400 From: john cooper MIME-Version: 1.0 References: <4BAAF573.5000109@redhat.com> <4C07FDE3.3020905@codemonkey.ws> In-Reply-To: <4C07FDE3.3020905@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 1/4] Add virtio disk identification support List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: john.cooper@redhat.com, Rusty Russell , Marc Haber , qemu-devel@nongnu.org Anthony Liguori wrote: > On 03/25/2010 12:32 AM, john cooper wrote: >> Add virtio-blk device id (s/n) support via virtio request. >> Remove artifacts of pci and ATA_IDENTIFY implementation >> relative to prior versions. >> >> Signed-off-by: john cooper >> --- >> >> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >> index 9915840..358b0af 100644 >> --- a/hw/virtio-blk.c >> +++ b/hw/virtio-blk.c >> @@ -19,6 +19,8 @@ >> # include >> #endif >> >> +#define min(a,b) ((a)< (b) ? (a) : (b)) >> > > We already have MIN(). > >> + >> typedef struct VirtIOBlock >> { >> VirtIODevice vdev; >> @@ -28,6 +30,7 @@ typedef struct VirtIOBlock >> QEMUBH *bh; >> BlockConf *conf; >> unsigned short sector_mask; >> + char sn[BLOCK_SERIAL_STRLEN]; >> } VirtIOBlock; >> >> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >> @@ -317,6 +320,12 @@ static void >> virtio_blk_handle_request(VirtIOBlockReq *req, >> virtio_blk_handle_flush(req); >> } else if (req->out->type& VIRTIO_BLK_T_SCSI_CMD) { >> virtio_blk_handle_scsi(req); >> + } else if (req->out->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))); >> + virtio_blk_req_complete(req, VIRTIO_BLK_S_OK); >> } else if (req->out->type& VIRTIO_BLK_T_OUT) { >> qemu_iovec_init_external(&req->qiov,&req->elem.out_sg[1], >> req->elem.out_num - 1); >> @@ -496,6 +505,8 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, >> BlockConf *conf) >> bdrv_guess_geometry(s->bs,&cylinders,&heads,&secs); >> bdrv_set_geometry_hint(s->bs, cylinders, heads, secs); >> >> + strncpy(s->sn, drive_get_serial(s->bs), sizeof (s->sn)); >> + >> > > Friends don't let friends use strncpy(). > > This actually will result in a non-NULL terminated string if > drive_get_serial() returns a string larger than s->sn. Use snprintf() > instead. That actually is the desired behavior here as a serial string is of BLOCK_SERIAL_STRLEN bytes length maximum and not assured to be nul terminated (legacy ATA convention). snprintf() would cause us to lose the last string character in the case the full BLOCK_SERIAL_STRLEN bytes were in use. There are existing storage allocations of BLOCK_SERIAL_STRLEN + 1 in some cases but this appears as an internal convenience and is not part of the serial string data. -john -- john.cooper@redhat.com