Message ID | 1276886283-1571-1-git-send-email-ryanh@us.ibm.com |
---|---|
State | New |
Headers | show |
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
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: