Re: [Qemu-devel] Re: [PATCH 1/4] Add virtio disk identification support
Am 03.06.2010 21:09, schrieb Anthony Liguori:
> 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.
Isn't this what we have pstrcpy for?
Kevin
[Qemu-devel] Re: [PATCH 1/4] Add virtio disk identification support
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.coo...@redhat.com
[Qemu-devel] Re: [PATCH 1/4] Add virtio disk identification support
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.
Regards,
Anthony Liguori
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 7a7ece3..fff46da 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -59,6 +59,9 @@ struct virtio_blk_config
/* Flush the volatile write cache */
#define VIRTIO_BLK_T_FLUSH 4
+/* return the device ID string */
+#define VIRTIO_BLK_T_GET_ID 8
+
/* Barrier before this op. */
#define VIRTIO_BLK_T_BARRIER0x8000
