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@xxxxxxxxxxxxxxx/msg59720.html Gris Ge <fge@xxxxxxxxxx> 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 -- Vaibhav Jain <vaibhav@xxxxxxxxxxxxxxxxxx> Linux Technology Center, IBM India Pvt. Ltd.
