| Message ID | 1440679281-13234-17-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 27, 2015 at 02:41:14PM +0200, Hannes Reinecke wrote: > Add a function scsi_vpd_lun_id() to return a unique device > identifcation based on the designation descriptors of > VPD page 0x83. > > As devices might implement several descriptors the order > of preference is: > - NAA IEE Registered Extended > - EUI-64 based 16-byte > - EUI-64 based 12-byte > - NAA IEEE Registered > - NAA IEEE Extended > A SCSI name string descriptor is preferred to all of them > if the identification is longer than 16 bytes. > > The returned unique device identification will be formatted > as a SCSI Name string to avoid clashes between different > designator types. Looks good in general, but I wonder if it might make sense to switch to kasprintf and let the caller free the buffer? Otherwise: Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/01/2015 12:22 PM, Christoph Hellwig wrote: > On Thu, Aug 27, 2015 at 02:41:14PM +0200, Hannes Reinecke wrote: >> Add a function scsi_vpd_lun_id() to return a unique device >> identifcation based on the designation descriptors of >> VPD page 0x83. >> >> As devices might implement several descriptors the order >> of preference is: >> - NAA IEE Registered Extended >> - EUI-64 based 16-byte >> - EUI-64 based 12-byte >> - NAA IEEE Registered >> - NAA IEEE Extended >> A SCSI name string descriptor is preferred to all of them >> if the identification is longer than 16 bytes. >> >> The returned unique device identification will be formatted >> as a SCSI Name string to avoid clashes between different >> designator types. > > Looks good in general, but I wonder if it might make sense to switch > to kasprintf and let the caller free the buffer? > I really am no friend of caller-needs-to-free-up-buffer thingies. Allocations and deallocations should be done in the same context for better tracking (and debugging), IMO. > Otherwise: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Cheers, Hannes
On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote: > Add a function scsi_vpd_lun_id() to return a unique device > identifcation based on the designation descriptors of > VPD page 0x83. > > As devices might implement several descriptors the order > of preference is: > - NAA IEE Registered Extended > - EUI-64 based 16-byte > - EUI-64 based 12-byte > - NAA IEEE Registered > - NAA IEEE Extended > A SCSI name string descriptor is preferred to all of them > if the identification is longer than 16 bytes. > > The returned unique device identification will be formatted > as a SCSI Name string to avoid clashes between different > designator types. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/scsi_lib.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ > include/scsi/scsi_device.h | 1 + > 2 files changed, 122 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 2a864b8..48d6ff6 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -3145,3 +3145,124 @@ void sdev_enable_disk_events(struct scsi_device *sdev) > atomic_dec(&sdev->disk_events_disable_depth); > } > EXPORT_SYMBOL(sdev_enable_disk_events); > + > +/* > + * scsi_vpd_lun_id - return a unique device identification > + * @sdev: SCSI device > + * @id: buffer for the identification > + * @id_len: length of the buffer > + * > + * Copies a unique device identification into @id based > + * on the information in the VPD page 0x83 of the device. > + * The string will be formatted as a SCSI name string. > + * > + * Returns the length of the identification or error on failure. > + */ > +int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len) > +{ > + u8 cur_id_type = 0xff; > + u8 cur_id_size = 0; > + unsigned char *d, *cur_id_str; > + int id_size = -EAGAIN; > + > + /* > + * Look for the correct descriptor. > + * Order of preference for lun descriptor: > + * - SCSI name string > + * - NAA IEEE Registered Extended > + * - EUI-64 based 16-byte > + * - EUI-64 based 12-byte > + * - NAA IEEE Registered > + * - NAA IEEE Extended > + * as longer descriptors reduce the likelyhood > + * of identification clashes. > + */ > + > + /* The id string must be at least 20 bytes + terminating NULL byte */ > + if (id_len < 21) > + return -EINVAL; > + > + memset(id, 0, id_len); > + d = sdev->vpd_pg83 + 4; > + while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) { > + /* Skip designators not referring to the LUN */ > + if ((d[1] & 0x30) != 0x00) > + goto next_desig; > + > + switch (d[1] & 0xf) { > + case 0x2: > + /* EUI-64 */ > + if (cur_id_size > d[3]) > + break; > + /* Prefer NAA IEEE Registered Extended */ > + if (cur_id_type == 0x3 && > + cur_id_size == d[3]) > + break; > + cur_id_size = d[3]; > + cur_id_str = d + 4; > + cur_id_type = d[1] & 0xf; > + switch (cur_id_size) { > + case 8: > + id_size = snprintf(id, id_len, > + "eui.%8phN", > + cur_id_str); > + break; > + case 12: > + id_size = snprintf(id, id_len, > + "eui.%12phN", > + cur_id_str); > + break; > + case 16: > + id_size = snprintf(id, id_len, > + "eui.%16phN", > + cur_id_str); > + break; > + default: > + cur_id_size = 0; > + break; > + } > + break; > + case 0x3: > + /* NAA */ > + if (cur_id_size > d[3]) > + break; > + cur_id_size = d[3]; > + cur_id_str = d + 4; > + cur_id_type = d[1] & 0xf; > + switch (cur_id_size) { > + case 8: > + id_size = snprintf(id, id_len, > + "naa.%8phN", > + cur_id_str); > + break; > + case 16: > + id_size = snprintf(id, id_len, > + "naa.%16phN", > + cur_id_str); > + break; > + default: > + cur_id_size = 0; > + break; > + } > + break; > + case 0x8: > + /* SCSI name string */ > + if (cur_id_size + 4 > d[3]) > + break; > + cur_id_size = d[3]; > + cur_id_str = d + 4; > + cur_id_type = d[1] & 0xf; > + if (cur_id_size >= id_len) > + cur_id_size = id_len - 1; > + memcpy(id, cur_id_str, cur_id_size); > + break; > + default: > + break; > + } > +next_desig: > + d += d[3] + 4; > + } > + > + return id_size; > +} > +EXPORT_SYMBOL(scsi_vpd_lun_id); > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index b6b1dc4..595a9be 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -413,6 +413,7 @@ static inline int scsi_execute_req(struct scsi_device *sdev, > } > extern void sdev_disable_disk_events(struct scsi_device *sdev); > extern void sdev_enable_disk_events(struct scsi_device *sdev); > +extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t); > > #ifdef CONFIG_PM > extern int scsi_autopm_get_device(struct scsi_device *); scsi_vpd_lun_id() is an exported function, but does not check if sdev->vpd_pg83 != NULL (i.e. no VPD 83 info), and we will crash if it is NULL. Also, why return -EAGAIN? What will be different next time? Maybe a different errno would be better, like -ENOENT or -ENODATA, I'm not sure. In the SCSI name string case, if the name has to be truncated to fit the supplied buffer, scsi_vpd_lun_id() does not ensure that the last byte is zero, it relies on the caller to have done this, (which alua_check_vpd() does not do). I'm not sure if we should truncate the SCSI name string here or return an error here, as a truncated name will likely not be unique. Reviewed-by: Ewan D. Milne <emilne@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/22/2015 09:17 PM, Ewan Milne wrote: [ .. ] > > scsi_vpd_lun_id() is an exported function, but does not check if sdev->vpd_pg83 != NULL > (i.e. no VPD 83 info), and we will crash if it is NULL. Also, why return -EAGAIN? > What will be different next time? Maybe a different errno would be better, like > -ENOENT or -ENODATA, I'm not sure. > > In the SCSI name string case, if the name has to be truncated to fit the supplied buffer, > scsi_vpd_lun_id() does not ensure that the last byte is zero, it relies on the caller to > have done this, (which alua_check_vpd() does not do). I'm not sure if we should truncate > the SCSI name string here or return an error here, as a truncated name will likely not > be unique. > I've now updated the description, and decreased the priority if the SCSI name descriptor is truncated. With that any other descriptor will have preference if the SCSI name descriptor is truncated. Cheers, Hannes
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 2a864b8..48d6ff6 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -3145,3 +3145,124 @@ void sdev_enable_disk_events(struct scsi_device *sdev) atomic_dec(&sdev->disk_events_disable_depth); } EXPORT_SYMBOL(sdev_enable_disk_events); + +/* + * scsi_vpd_lun_id - return a unique device identification + * @sdev: SCSI device + * @id: buffer for the identification + * @id_len: length of the buffer + * + * Copies a unique device identification into @id based + * on the information in the VPD page 0x83 of the device. + * The string will be formatted as a SCSI name string. + * + * Returns the length of the identification or error on failure. + */ +int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len) +{ + u8 cur_id_type = 0xff; + u8 cur_id_size = 0; + unsigned char *d, *cur_id_str; + int id_size = -EAGAIN; + + /* + * Look for the correct descriptor. + * Order of preference for lun descriptor: + * - SCSI name string + * - NAA IEEE Registered Extended + * - EUI-64 based 16-byte + * - EUI-64 based 12-byte + * - NAA IEEE Registered + * - NAA IEEE Extended + * as longer descriptors reduce the likelyhood + * of identification clashes. + */ + + /* The id string must be at least 20 bytes + terminating NULL byte */ + if (id_len < 21) + return -EINVAL; + + memset(id, 0, id_len); + d = sdev->vpd_pg83 + 4; + while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) { + /* Skip designators not referring to the LUN */ + if ((d[1] & 0x30) != 0x00) + goto next_desig; + + switch (d[1] & 0xf) { + case 0x2: + /* EUI-64 */ + if (cur_id_size > d[3]) + break; + /* Prefer NAA IEEE Registered Extended */ + if (cur_id_type == 0x3 && + cur_id_size == d[3]) + break; + cur_id_size = d[3]; + cur_id_str = d + 4; + cur_id_type = d[1] & 0xf; + switch (cur_id_size) { + case 8: + id_size = snprintf(id, id_len, + "eui.%8phN", + cur_id_str); + break; + case 12: + id_size = snprintf(id, id_len, + "eui.%12phN", + cur_id_str); + break; + case 16: + id_size = snprintf(id, id_len, + "eui.%16phN", + cur_id_str); + break; + default: + cur_id_size = 0; + break; + } + break; + case 0x3: + /* NAA */ + if (cur_id_size > d[3]) + break; + cur_id_size = d[3]; + cur_id_str = d + 4; + cur_id_type = d[1] & 0xf; + switch (cur_id_size) { + case 8: + id_size = snprintf(id, id_len, + "naa.%8phN", + cur_id_str); + break; + case 16: + id_size = snprintf(id, id_len, + "naa.%16phN", + cur_id_str); + break; + default: + cur_id_size = 0; + break; + } + break; + case 0x8: + /* SCSI name string */ + if (cur_id_size + 4 > d[3]) + break; + cur_id_size = d[3]; + cur_id_str = d + 4; + cur_id_type = d[1] & 0xf; + if (cur_id_size >= id_len) + cur_id_size = id_len - 1; + memcpy(id, cur_id_str, cur_id_size); + break; + default: + break; + } +next_desig: + d += d[3] + 4; + } + + return id_size; +} +EXPORT_SYMBOL(scsi_vpd_lun_id); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index b6b1dc4..595a9be 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -413,6 +413,7 @@ static inline int scsi_execute_req(struct scsi_device *sdev, } extern void sdev_disable_disk_events(struct scsi_device *sdev); extern void sdev_enable_disk_events(struct scsi_device *sdev); +extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t); #ifdef CONFIG_PM extern int scsi_autopm_get_device(struct scsi_device *);
Add a function scsi_vpd_lun_id() to return a unique device identifcation based on the designation descriptors of VPD page 0x83. As devices might implement several descriptors the order of preference is: - NAA IEE Registered Extended - EUI-64 based 16-byte - EUI-64 based 12-byte - NAA IEEE Registered - NAA IEEE Extended A SCSI name string descriptor is preferred to all of them if the identification is longer than 16 bytes. The returned unique device identification will be formatted as a SCSI Name string to avoid clashes between different designator types. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/scsi_lib.c | 121 +++++++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 1 + 2 files changed, 122 insertions(+)