[Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Ryan Harper]

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@us.ibm.com>
Signed-off-by: john cooper <john.cooper@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

[Qemu-devel] [PATCH 2/2] Remove virtio_blk VBID ioctl

[Ryan Harper]

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@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

[Qemu-devel] Re: [PATCH 2/2] Remove virtio_blk VBID ioctl

[Rusty Russell]

On Sat, 19 Jun 2010 04:08:03 am Ryan Harper wrote:
> 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.

If John Cooper acks this, I'll push it to Linus immediately.

Unfortunately we offered this interface in 2.6.34, and we're now removing it.
That's unpleasant.

Thanks,
Rusty.
PS.  John should have been cc'd on these patches!

[Qemu-devel] Re: [PATCH 2/2] Remove virtio_blk VBID ioctl

[Ryan Harper]

* Rusty Russell <rusty@rustcorp.com.au> [2010-06-20 20:31]:
> On Sat, 19 Jun 2010 04:08:03 am Ryan Harper wrote:
> > 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.
> 
> If John Cooper acks this, I'll push it to Linus immediately.
> 
> Unfortunately we offered this interface in 2.6.34, and we're now removing it.
> That's unpleasant.

Yes; well.  There's a story as there always is.  John can tell it better
than I, but it goes something like:

John cooked up some patches, one of which was an example use of the
serial string including a VBID ioctl.  No one got around to doing a
sysfs interface and somehow the ioctl side in virtio-blk got picked up.

Working with what was available, I pushed some patches to linux-hotplug
to get this whole virtio-blk serial and disk/by-id symlinks working and
was met with:  why does a new kernel driver have an ioctl interface and
we don't want to collect additional single-use binaries in the udev
tree.

So now, the sysfs serial attribute patch and with it, no need and no
users of ioctl.

*whew*

> 
> Thanks,
> Rusty.
> PS.  John should have been cc'd on these patches!

He's cc'ed on the others, just not on this removal one.

I need to learn git-send-email better, I explicitly added him as --cc on
when sending; next time I'll look closer at the headers during
--dry-run.



-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

[Qemu-devel] Re: [PATCH 2/2] Remove virtio_blk VBID ioctl

[john cooper]

Rusty Russell wrote:
> On Sat, 19 Jun 2010 04:08:03 am Ryan Harper wrote:
>> 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.
> 
> If John Cooper acks this, I'll push it to Linus immediately.

Actually I'm the one who suggested removing it.
The code in question was only intended as example
usage of accessing the s/n data in the driver, for
the /sys interface under discussion back then.
That effort subsequently stalled and Ryan had
recently picked it up.  As such I believe this
overshadows the general need for an ioctl.  Even if
for some reason an ioctl would be justified going
forward, a more usage friendly form would be better.
So let's just drop it for now as the corresponding
qemu-side code hasn't been merged.
 
> Unfortunately we offered this interface in 2.6.34, and we're now removing it.
> That's unpleasant.

Indeed.  This entire effort, aside from being an exercise
in protracted agony, probably violates a Rube Goldberg
patent.

Thanks,

-john

-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Blue Swirl]

On Fri, Jun 18, 2010 at 6:38 PM, Ryan Harper <ryanh@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@us.ibm.com>
> Signed-off-by: john cooper <john.cooper@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
>
>
>

Re: [Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/19/2010 01:24 AM, Blue Swirl wrote:
>> +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;

I would guess you mean

   char *const di = d;

Quite different and doesn't elicit warnings from the compiler.

- -- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkwcotsACgkQ2ijCOnn/RHQLowCgqmoJCFjfh/ySP4/PQAWKmKJ9
rAwAn1O1L3hb2nzd7altEWT/PXg8oecx
=YkfO
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Blue Swirl]

On Sat, Jun 19, 2010 at 10:58 AM, Ulrich Drepper <drepper@redhat.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 06/19/2010 01:24 AM, Blue Swirl wrote:
>>> +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;
>
> I would guess you mean
>
>   char *const di = d;
>
> Quite different and doesn't elicit warnings from the compiler.

I had actually confused the types of d and s. So either your version
or the original for this line.

The return statement assumes that ptrdiff_t is equal to ssize_t (or
int in the original version) but I guess that's OK.

[Qemu-devel] Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Rusty Russell]

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@us.ibm.com>
> Signed-off-by: john cooper <john.cooper@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.

[Qemu-devel] Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[john cooper]

Rusty Russell wrote:
> 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@us.ibm.com>
>> Signed-off-by: john cooper <john.cooper@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.

Propagating a prospective error from virtblk_get_id() should
be possible.  Unsure if doing so is more useful from the
user's perspective compared to just a nul id string.

>> +	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);

Agreed, that's a better wrench in the gearworks.
Note padding buf[] by 1 isn't necessary as indicated
below.

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

The /sys file is rendered according to the length
returned from this function and the trailing nul
is not interpreted in this context.  In fact if a
nul is added and included in the byte count of the
string it will appear in the /sys file.

Thanks,

-john


-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Ryan Harper]

* john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
> Rusty Russell wrote:
> > 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@us.ibm.com>
> >> Signed-off-by: john cooper <john.cooper@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.
> 
> Propagating a prospective error from virtblk_get_id() should
> be possible.  Unsure if doing so is more useful from the
> user's perspective compared to just a nul id string.

I'm not sure we can do any thing else here; maybe printk a warning?

Documentation/filesystems/sysfs.txt says that showing attributes should
always return the number of chars put into the buffer; so when there is
an error; zero is the right value to return since we're not filling the
buffer.
> 
> >> +	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);
> 
> Agreed, that's a better wrench in the gearworks.
> Note padding buf[] by 1 isn't necessary as indicated
> below.

Yep; that's a good one to take.

> 
> > 	/* id_str is not necessarily nul-terminated! */
> > 	buf[VIRTIO_BLK_ID_BYTES] = '\0';
> > 	return virtblk_get_id(disk, buf);
> 
> The /sys file is rendered according to the length
> returned from this function and the trailing nul
> is not interpreted in this context.  In fact if a
> nul is added and included in the byte count of the
> string it will appear in the /sys file.

Yeah; I like the simplicity; but we do need to know how long the string
is so we can return that value. 

> 
> Thanks,
> 
> -john
> 
> 
> -- 
> john.cooper@redhat.com

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

[Qemu-devel] Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[john cooper]

Ryan Harper wrote:
> * john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
>> Rusty Russell wrote:
>>> 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@us.ibm.com>
>>>> Signed-off-by: john cooper <john.cooper@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.
>> Propagating a prospective error from virtblk_get_id() should
>> be possible.  Unsure if doing so is more useful from the
>> user's perspective compared to just a nul id string.
> 
> I'm not sure we can do any thing else here; maybe printk a warning?
> 
> Documentation/filesystems/sysfs.txt says that showing attributes should
> always return the number of chars put into the buffer; so when there is
> an error; zero is the right value to return since we're not filling the
> buffer.

So we return a nul string in the case the qemu user
didn't specify an id string and also in the case a
legacy qemu doesn't support retrieval of an id string.
Not too much difference and if needed going forward the
error return can be elaborated.

>>> 	/* id_str is not necessarily nul-terminated! */
>>> 	buf[VIRTIO_BLK_ID_BYTES] = '\0';
>>> 	return virtblk_get_id(disk, buf);
>> The /sys file is rendered according to the length
>> returned from this function and the trailing nul
>> is not interpreted in this context.  In fact if a
>> nul is added and included in the byte count of the
>> string it will appear in the /sys file.
> 
> Yeah; I like the simplicity; but we do need to know how long the string
> is so we can return that value. 

Which we're getting from serial_sysfs() without
having to accommodate an unused nul.  I'd hazard the
primary reason the sysfs calling code keys off a
return of byte count vs. traversing the string itself
is due to the called function almost always having the
byte count available.

-john

-- 
john.cooper@redhat.com

[Qemu-devel] Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Rusty Russell]

On Tue, 22 Jun 2010 02:13:21 am Ryan Harper wrote:
> * john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
> > Rusty Russell wrote:
> > > 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@us.ibm.com>
> > >> Signed-off-by: john cooper <john.cooper@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.
> > 
> > Propagating a prospective error from virtblk_get_id() should
> > be possible.  Unsure if doing so is more useful from the
> > user's perspective compared to just a nul id string.
> 
> I'm not sure we can do any thing else here; maybe printk a warning?
> 
> Documentation/filesystems/sysfs.txt says that showing attributes should
> always return the number of chars put into the buffer; so when there is
> an error; zero is the right value to return since we're not filling the
> buffer.

Ideally, the file shouldn't be set up if we don't have an ID.  But we never
did add a feature bit for this :(

At a glance, we'll get -EIO if the host doesn't support it (or any other
transport error).  -ENOMEM if we run out of memory.

printk is dumb, but it's nice to differentiate "host didn't supply one" vs
"something went wrong".  How about return 0 on -EIO?  Whatever is easiest
for udev is best here.

> > > 	/* id_str is not necessarily nul-terminated! */
> > > 	buf[VIRTIO_BLK_ID_BYTES] = '\0';
> > > 	return virtblk_get_id(disk, buf);
> > 
> > The /sys file is rendered according to the length
> > returned from this function and the trailing nul
> > is not interpreted in this context.  In fact if a
> > nul is added and included in the byte count of the
> > string it will appear in the /sys file.
> 
> Yeah; I like the simplicity; but we do need to know how long the string
> is so we can return that value. 

So we're looking at something like:

	/* id_str is not necessarily nul-terminated! */
	buf[VIRTIO_BLK_ID_BYTES] = '\0';
	err = virtblk_get_id(disk, buf);
	if (!err)
		return strlen(buf);
	if (err == -EIO) /* Unsupported?  Make it empty. */	
		return 0;
	return err;

Then, please *test*!

Thanks,
Rusty.

[Qemu-devel] Re: [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[john cooper]

Rusty Russell wrote:
> On Tue, 22 Jun 2010 02:13:21 am Ryan Harper wrote:
>> * john cooper <john.cooper@redhat.com> [2010-06-21 01:11]:
>>> Rusty Russell wrote:
>>>> 	/* id_str is not necessarily nul-terminated! */
>>>> 	buf[VIRTIO_BLK_ID_BYTES] = '\0';
>>>> 	return virtblk_get_id(disk, buf);
>>> The /sys file is rendered according to the length
>>> returned from this function and the trailing nul
>>> is not interpreted in this context.  In fact if a
>>> nul is added and included in the byte count of the
>>> string it will appear in the /sys file.
>> Yeah; I like the simplicity; but we do need to know how long the string
>> is so we can return that value. 
> 
> So we're looking at something like:
> 
> 	/* id_str is not necessarily nul-terminated! */
> 	buf[VIRTIO_BLK_ID_BYTES] = '\0';
> 	err = virtblk_get_id(disk, buf);
> 	if (!err)
> 		return strlen(buf);
> 	if (err == -EIO) /* Unsupported?  Make it empty. */	
> 		return 0;
> 	return err;

In my haste reading your prior mail, I'd glossed over
the fact you were copying direct to the sysfs buf.  So
in retrospect that (and the above) do make sense.

Thanks,

-john

-- 
john.cooper@redhat.com

Re: [Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Christoph Hellwig]

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.

Re: [Qemu-devel] [PATCH 1/2] Add 'serial' attribute to virtio-blk devices

[Ryan Harper]

* Christoph Hellwig <hch@lst.de> [2010-06-21 07:46]:
> 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.

ATA and SCSI devices are already supported via ata_id and scsi_id
commands included in udev.  Qemu implements the drive serial part for
them and udev creates proper disk/by-id links.  This patch is about
filling the gap for virtio-blk devices which cannot work with ata_id and
scsi_id.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com