Linux Virtualization - Jun 2010 - [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

Create a new attribute for virtio-blk devices that will fetch the serial number
of the block device.  This attribute can be used by udev to create disk/by-id
symlinks for devices that don't have a UUID (filesystem) associated with
them.

ATA_IDENTIFY strings are special in that they can be up to 20 chars long
and aren't required to be NULL-terminated.  The buffer is also zero-padded
meaning that if the serial is 19 chars or less that we get a NULL terminated
string.  When copying this value into a string buffer, we must be careful to
copy up to the NULL (if it present) and only 20 if it is longer and not to
attempt to NULL terminate; this isn't needed.

Signed-off-by: Ryan Harper <ryanh at us.ibm.com>
Signed-off-by: john cooper <john.cooper at redhat.com>
---
 drivers/block/virtio_blk.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 258bc2a..f1ef26f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -281,6 +281,31 @@ static int index_to_minor(int index)
 	return index << PART_BITS;
 }
 
+/* Copy serial number from *s to *d.  Copy operation terminates on either
+ * encountering a nul in *s or after n bytes have been copied, whichever
+ * occurs first.  *d is not forcibly nul terminated.  Return # of bytes copied.
+ */
+static inline int serial_sysfs(char *d, char *s, int n)
+{
+	char *di = d;
+
+	while (*s && n--)
+		*d++ = *s++;
+	return d - di;
+}
+
+static ssize_t virtblk_serial_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	char id_str[VIRTIO_BLK_ID_BYTES];
+
+	if (IS_ERR(virtblk_get_id(disk, id_str)))
+		return 0;
+	return serial_sysfs(buf, id_str, min(VIRTIO_BLK_ID_BYTES, PAGE_SIZE));
+}
+DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -445,8 +470,15 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
 
 
 	add_disk(vblk->disk);
+	err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial);
+	if (err)
+		goto out_del_disk;
+
 	return 0;
 
+out_del_disk:
+	del_gendisk(vblk->disk);
+	blk_cleanup_queue(vblk->disk->queue);
 out_put_disk:
 	put_disk(vblk->disk);
 out_mempool:
-- 
1.6.3.3

With the availablility of a sysfs device attribute for examining disk serial
numbers the ioctl is no longer needed.  The user-space changes for this
aren't
upstream yet so we don't have any users to worry about.

Signed-off-by: Ryan Harper <ryanh at us.ibm.com>
---
 drivers/block/virtio_blk.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index f1ef26f..9e382dd 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -225,16 +225,6 @@ 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 == 0x56424944) { /* 'VBID' */
-		void __user *usr_data = (void __user *)data;
-		char id_str[VIRTIO_BLK_ID_BYTES];
-		int err;
-
-		err = virtblk_get_id(disk, id_str);
-		if (!err && copy_to_user(usr_data, id_str, VIRTIO_BLK_ID_BYTES))
-			err = -EFAULT;
-		return err;
-	}
 	/*
 	 * Only allow the generic SCSI ioctls if the host can support it.
 	 */
-- 
1.6.3.3

On Fri, Jun 18, 2010 at 6:38 PM, Ryan Harper <ryanh at us.ibm.com>
wrote:
> Create a new attribute for virtio-blk devices that will fetch the serial
number
> of the block device. ?This attribute can be used by udev to create
disk/by-id
> symlinks for devices that don't have a UUID (filesystem) associated
with them.
>
> ATA_IDENTIFY strings are special in that they can be up to 20 chars long
> and aren't required to be NULL-terminated. ?The buffer is also
zero-padded
> meaning that if the serial is 19 chars or less that we get a NULL
terminated
> string. ?When copying this value into a string buffer, we must be careful
to
> copy up to the NULL (if it present) and only 20 if it is longer and not to
> attempt to NULL terminate; this isn't needed.
>
> Signed-off-by: Ryan Harper <ryanh at us.ibm.com>
> Signed-off-by: john cooper <john.cooper at redhat.com>
> ---
> ?drivers/block/virtio_blk.c | ? 32 ++++++++++++++++++++++++++++++++
> ?1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 258bc2a..f1ef26f 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -281,6 +281,31 @@ static int index_to_minor(int index)
> ? ? ? ?return index << PART_BITS;
> ?}
>
> +/* Copy serial number from *s to *d. ?Copy operation terminates on either
> + * encountering a nul in *s or after n bytes have been copied, whichever
> + * occurs first. ?*d is not forcibly nul terminated. ?Return # of bytes
copied.
> + */
> +static inline int serial_sysfs(char *d, char *s, int n)
> +{
> + ? ? ? char *di = d;
I'd change this to:
static inline ssize_t serial_sysfs(char *d, const char *s, ssize_t n)
{
       const char *di = d;
> +
> + ? ? ? while (*s && n--)
> + ? ? ? ? ? ? ? *d++ = *s++;
> + ? ? ? return d - di;
> +}
> +
> +static ssize_t virtblk_serial_show(struct device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct device_attribute *attr, char *buf)
> +{
> + ? ? ? struct gendisk *disk = dev_to_disk(dev);
> + ? ? ? char id_str[VIRTIO_BLK_ID_BYTES];
> +
> + ? ? ? if (IS_ERR(virtblk_get_id(disk, id_str)))
> + ? ? ? ? ? ? ? return 0;
> + ? ? ? return serial_sysfs(buf, id_str, min(VIRTIO_BLK_ID_BYTES,
PAGE_SIZE));
> +}
> +DEVICE_ATTR(serial, S_IRUGO, virtblk_serial_show, NULL);
> +
> ?static int __devinit virtblk_probe(struct virtio_device *vdev)
> ?{
> ? ? ? ?struct virtio_blk *vblk;
> @@ -445,8 +470,15 @@ static int __devinit virtblk_probe(struct
virtio_device *vdev)
>
>
> ? ? ? ?add_disk(vblk->disk);
> + ? ? ? err = device_create_file(disk_to_dev(vblk->disk),
&dev_attr_serial);
> + ? ? ? if (err)
> + ? ? ? ? ? ? ? goto out_del_disk;
> +
> ? ? ? ?return 0;
>
> +out_del_disk:
> + ? ? ? del_gendisk(vblk->disk);
> + ? ? ? blk_cleanup_queue(vblk->disk->queue);
> ?out_put_disk:
> ? ? ? ?put_disk(vblk->disk);
> ?out_mempool:
> --
> 1.6.3.3
>
>
>

On Sat, 19 Jun 2010 04:08:02 am Ryan Harper wrote:
> Create a new attribute for virtio-blk devices that will fetch the serial
number
> of the block device.  This attribute can be used by udev to create
disk/by-id
> symlinks for devices that don't have a UUID (filesystem) associated
with them.
> 
> ATA_IDENTIFY strings are special in that they can be up to 20 chars long
> and aren't required to be NULL-terminated.  The buffer is also
zero-padded
> meaning that if the serial is 19 chars or less that we get a NULL
terminated
> string.  When copying this value into a string buffer, we must be careful
to
> copy up to the NULL (if it present) and only 20 if it is longer and not to
> attempt to NULL terminate; this isn't needed.
> 
> Signed-off-by: Ryan Harper <ryanh at us.ibm.com>
> Signed-off-by: john cooper <john.cooper at redhat.com>
> ---
>  drivers/block/virtio_blk.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 258bc2a..f1ef26f 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -281,6 +281,31 @@ static int index_to_minor(int index)
>  	return index << PART_BITS;
>  }
>  
> +/* Copy serial number from *s to *d.  Copy operation terminates on either
> + * encountering a nul in *s or after n bytes have been copied, whichever
> + * occurs first.  *d is not forcibly nul terminated.  Return # of bytes
copied.
> + */
> +static inline int serial_sysfs(char *d, char *s, int n)
> +{
> +	char *di = d;
> +
> +	while (*s && n--)
> +		*d++ = *s++;
> +	return d - di;
> +}
> +
> +static ssize_t virtblk_serial_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct gendisk *disk = dev_to_disk(dev);
> +	char id_str[VIRTIO_BLK_ID_BYTES];
> +
> +	if (IS_ERR(virtblk_get_id(disk, id_str)))
> +		return 0;
0?  Really?  That doesn't seem very informative.
> +	return serial_sysfs(buf, id_str, min(VIRTIO_BLK_ID_BYTES, PAGE_SIZE));
How about something like this:

	BUILD_BUG_ON(PAGE_SIZE < VIRTIO_BLK_ID_BYTES + 1);

	/* id_str is not necessarily nul-terminated! */
	buf[VIRTIO_BLK_ID_BYTES] = '\0';
	return virtblk_get_id(disk, buf);

Thanks,
Rusty.

On Fri, Jun 18, 2010 at 01:38:02PM -0500, Ryan Harper
wrote:
> Create a new attribute for virtio-blk devices that will fetch the serial
number
> of the block device.  This attribute can be used by udev to create
disk/by-id
> symlinks for devices that don't have a UUID (filesystem) associated
with them.
> 
> ATA_IDENTIFY strings are special in that they can be up to 20 chars long
> and aren't required to be NULL-terminated.  The buffer is also
zero-padded
> meaning that if the serial is 19 chars or less that we get a NULL
terminated
> string.  When copying this value into a string buffer, we must be careful
to
> copy up to the NULL (if it present) and only 20 if it is longer and not to
> attempt to NULL terminate; this isn't needed.
Why is this virtio-blk specific?  In a later mail you mention you want
to use it for udev.  So please export this from scsi/libata as well and
we have one proper interface that we can use for all devices.