[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@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(-)

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

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

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.

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.

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

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.

* 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

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

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.

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