[Qemu-devel] [PATCH 4/4] Add virtio disk identification support

[Qemu-devel] [PATCH 4/4] Add virtio disk identification support

[john cooper]

Return serial string to the guest application via
ioctl driver call.

Note this form of interface to the guest userland
was the consensus when the prior version using
the ATA_IDENTIFY came under dispute.

Signed-off-by: john cooper <john.cooper@redhat.com>
---

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cd66806..0954193 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -225,6 +225,21 @@ static int virtblk_ioctl(struct block_device *bdev, fmode_t mode,
 	struct gendisk *disk = bdev->bd_disk;
 	struct virtio_blk *vblk = disk->private_data;
 
+	if (cmd == 'VBID') {
+		void *usr_data = (void __user *)data;
+		char *id_str;
+		int err;
+
+		if (!(id_str = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL)))
+			err = -ENOMEM;
+		else if ((err = virtblk_get_id(disk, id_str)))
+			;
+		else if (copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES))
+			err = -EFAULT;
+		if (id_str)
+			kfree(id_str);
+		return err;
+	}
 	/*
 	 * Only allow the generic SCSI ioctls if the host can support it.
 	 */
-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 4/4] Add virtio disk identification support

[Rusty Russell]

On Thu, 25 Mar 2010 04:04:02 pm john cooper wrote:
> Return serial string to the guest application via
> ioctl driver call.

This is quite nice.  Minor nits:

> +	if (cmd == 'VBID') {
> +		void *usr_data = (void __user *)data;

void __user *usr_data;

> +		char *id_str;
> +		int err;
> +
> +		if (!(id_str = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL)))
> +			err = -ENOMEM;
> +		else if ((err = virtblk_get_id(disk, id_str)))
> +			;
> +		else if (copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES))
> +			err = -EFAULT;
> +		if (id_str)
> +			kfree(id_str);
> +		return err;
> +	}

We can't put the id_str on the stack?  Makes it even simpler :)

Cheers,
Rusty.

[Qemu-devel] Re: [PATCH 4/4] Add virtio disk identification support

[john cooper]

Rusty Russell wrote:
> On Thu, 25 Mar 2010 04:04:02 pm john cooper wrote:
>> Return serial string to the guest application via
>> ioctl driver call.
> 
> This is quite nice.  Minor nits:
> 
>> +	if (cmd == 'VBID') {
>> +		void *usr_data = (void __user *)data;
> 
> void __user *usr_data;
> 
>> +		char *id_str;
>> +		int err;
>> +
>> +		if (!(id_str = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL)))
>> +			err = -ENOMEM;
>> +		else if ((err = virtblk_get_id(disk, id_str)))
>> +			;
>> +		else if (copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES))
>> +			err = -EFAULT;
>> +		if (id_str)
>> +			kfree(id_str);
>> +		return err;
>> +	}
> 
> We can't put the id_str on the stack?  Makes it even simpler :)

At a VIRTIO_BLK_ID_BYTES of the current 20 bytes it seems
safe but there was discussion of extending it so I thought
to locate it in safer storage.

Note the above was intended as more of an example to
illustrate the mechanism.  Marc had proposed a /sys
style interface to retrieve a virtio id string which
is what motivated revisiting this issue.

Marc, if you don't foresee tying off that work relatively
soon where a /sys interface would be made available, I'll
rework the above to be a little more general.  The first
version of the S/N patch pulled a user buffer size from
the caller and limited the copy out to that length.

-john

-- 
john.cooper@redhat.com