diff mbox

lsscsi: Fix truncation of 128-bit wwn

Message ID 20170317043840.26560-1-vaibhav@linux.vnet.ibm.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Vaibhav Jain March 17, 2017, 4:38 a.m. UTC 
Currently with '--wwn' flag, 128-bit wwns gets truncated and their
last 3 hex-digits missing. Below is a comparison of wwn reported by
lsscsi compared to wwn info at /dev/disk/by-id directory.

% lsscsi -w 0:0:5:0
[0:0:5:0]    disk    0x60050764008181941000000000000  /dev/sdad

% ls -l /dev/disk/by-id/wwn-*
lrwxrwxrwx. 1 root root 10 Oct 19 01:08 /dev/disk/by-id/wwn-0x600507640081819410000000000001b1 -> ../../sdad

To fix this, the patch increases the size of member wwn of struct
disk_wwn_node_entry to 35 chars to accommodate the extra '0x' prefix and
null terminator. Also the size of the buffer wwn_str thats used to
output wwn to the std-out is increased to match the corresponding
member of disk_wwn_node_entry.

Link: https://bugs.launchpad.net/ubuntu/+source/lsscsi/+bug/1636467
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1387263

Cc: Jon Grimm <jon.grimm@canonical.com>
Cc: Vipin K Parashar <vipin@linux.vnet.ibm.com>
Cc: Ping Tian Han <pthan@cn.ibm.com>
Cc: Gris Ge <fge@redhat.com>
Reported-by: Ping Tian Han <pthan@cn.ibm.com>
Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Reviewed-by: Gris Ge <fge@redhat.com>
---
 src/lsscsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Gris Ge March 17, 2017, 10:28 a.m. UTC | #1 
On Fri, Mar 17, 2017 at 10:08:40AM +0530, Vaibhav Jain wrote:
> ---
>  src/lsscsi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lsscsi.c b/src/lsscsi.c
> index 974b3f1..f20adcf 100644
> --- a/src/lsscsi.c
> +++ b/src/lsscsi.c
> @@ -210,8 +210,8 @@ struct dev_node_list {
>  static struct dev_node_list* dev_node_listhead = NULL;
>  
>  struct disk_wwn_node_entry {
> -       char wwn[32];
> -       char disk_bname[12];
> +	char wwn[35]; /* '0x' + wwn<128-bit> + <null-terminator> */
Please consider to define a constant instead of using a magic number
everywhere. Like:
#define _DISK_WWN_MAX_LEN           35
/* ^ WWN here is extracted from /dev/disk/by-id/wwn-<WWN> which is
 *   created by udev 60-persistent-storage.rules using ID_WWN_WITH_EXTENSION.
 *   The udev ID_WWN_WITH_EXTENSION is the combination of char wwn[17] and
 *   char wwn_vendor_extension[17] from struct scsi_id_device.
 */
> +	char disk_bname[12];
Please use space instead of \t for indention.
>  };
>  
>  #define DISK_WWN_NODE_LIST_ENTRIES 16
> @@ -2939,14 +2939,14 @@ one_sdev_entry(const char * dir_name, const char * devname,
>                  }
>                  if (wd[0]) {
>                          char dev_node[LMAX_NAME] = "";
> -                        char wwn_str[34];
> +			char wwn_str[35];
Please use space instead of \t for indention.
>                          enum dev_type typ;
>  
>                          typ = (FT_BLOCK == non_sg.ft) ? BLK_DEV : CHR_DEV;
>                          if (get_wwn) {
>                                  if ((BLK_DEV == typ) &&
>                                      get_disk_wwn(wd, wwn_str, sizeof(wwn_str)))
> -                                        printf("%-30s  ", wwn_str);
> +					printf("%-34s  ", wwn_str);
Please use constant instead of magic number:
    printf("%-*s  ", _DISK_WWN_MAX_LEN - 1, wwn_str);
>                                  else
>                                          printf("                          "
>                                                 "      ");
> -- 
> 2.9.3
>
Vaibhav Jain March 24, 2017, 4:18 a.m. UTC | #2 
Hi Gris,

Thanks for reviewing the patch. I have posted a v2 patch addressing your
review comments. Can you please take a look.

Patch-Link:
https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg59720.html

Gris Ge <fge@redhat.com> writes:

> On Fri, Mar 17, 2017 at 10:08:40AM +0530, Vaibhav Jain wrote:
>> ---
>>  src/lsscsi.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/lsscsi.c b/src/lsscsi.c
>> index 974b3f1..f20adcf 100644
>> --- a/src/lsscsi.c
>> +++ b/src/lsscsi.c
>> @@ -210,8 +210,8 @@ struct dev_node_list {
>>  static struct dev_node_list* dev_node_listhead = NULL;
>>  
>>  struct disk_wwn_node_entry {
>> -       char wwn[32];
>> -       char disk_bname[12];
>> +	char wwn[35]; /* '0x' + wwn<128-bit> + <null-terminator> */
> Please consider to define a constant instead of using a magic number
> everywhere. Like:
> #define _DISK_WWN_MAX_LEN           35
> /* ^ WWN here is extracted from /dev/disk/by-id/wwn-<WWN> which is
>  *   created by udev 60-persistent-storage.rules using ID_WWN_WITH_EXTENSION.
>  *   The udev ID_WWN_WITH_EXTENSION is the combination of char wwn[17] and
>  *   char wwn_vendor_extension[17] from struct scsi_id_device.
>  */
>> +	char disk_bname[12];
> Please use space instead of \t for indention.
>>  };
>>  
>>  #define DISK_WWN_NODE_LIST_ENTRIES 16
>> @@ -2939,14 +2939,14 @@ one_sdev_entry(const char * dir_name, const char * devname,
>>                  }
>>                  if (wd[0]) {
>>                          char dev_node[LMAX_NAME] = "";
>> -                        char wwn_str[34];
>> +			char wwn_str[35];
> Please use space instead of \t for indention.
>>                          enum dev_type typ;
>>  
>>                          typ = (FT_BLOCK == non_sg.ft) ? BLK_DEV : CHR_DEV;
>>                          if (get_wwn) {
>>                                  if ((BLK_DEV == typ) &&
>>                                      get_disk_wwn(wd, wwn_str, sizeof(wwn_str)))
>> -                                        printf("%-30s  ", wwn_str);
>> +					printf("%-34s  ", wwn_str);
> Please use constant instead of magic number:
>     printf("%-*s  ", _DISK_WWN_MAX_LEN - 1, wwn_str);
>>                                  else
>>                                          printf("                          "
>>                                                 "      ");
>> -- 
>> 2.9.3
>> 
>
> -- 
> Gris Ge
diff mbox

Patch

diff --git a/src/lsscsi.c b/src/lsscsi.c
index 974b3f1..f20adcf 100644
--- a/src/lsscsi.c
+++ b/src/lsscsi.c
@@ -210,8 +210,8 @@  struct dev_node_list {
 static struct dev_node_list* dev_node_listhead = NULL;
 
 struct disk_wwn_node_entry {
-       char wwn[32];
-       char disk_bname[12];
+	char wwn[35]; /* '0x' + wwn<128-bit> + <null-terminator> */
+	char disk_bname[12];
 };
 
 #define DISK_WWN_NODE_LIST_ENTRIES 16
@@ -2939,14 +2939,14 @@  one_sdev_entry(const char * dir_name, const char * devname,
                 }
                 if (wd[0]) {
                         char dev_node[LMAX_NAME] = "";
-                        char wwn_str[34];
+			char wwn_str[35];
                         enum dev_type typ;
 
                         typ = (FT_BLOCK == non_sg.ft) ? BLK_DEV : CHR_DEV;
                         if (get_wwn) {
                                 if ((BLK_DEV == typ) &&
                                     get_disk_wwn(wd, wwn_str, sizeof(wwn_str)))
-                                        printf("%-30s  ", wwn_str);
+					printf("%-34s  ", wwn_str);
                                 else
                                         printf("                          "
                                                "      ");