nvme-cli wdc plugin: Add get pfail dump command.
Subsystems (2)
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
---
This patch implements the function to retreive a WDC device's pfail crash dump.
wdc-nvme.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++++------------ wdc-nvme.h | 1 + 2 files changed, 164 insertions(+), 37 deletions(-)
wdc-nvme.c +163 -37
@@ -72,6 +72,9 @@#define WDC_NVME_CAP_DIAG_SUBCMD 0x00#define WDC_NVME_CAP_DIAG_CMD 0x00 +#define WDC_NVME_CRASH_DUMP_TYPE 1 +#define WDC_NVME_PFAIL_DUMP_TYPE 2 +/* Crash dump */ #define WDC_NVME_CRASH_DUMP_SIZE_OPCODE WDC_NVME_CAP_DIAG_CMD_OPCODE#define WDC_NVME_CRASH_DUMP_SIZE_DATA_LEN WDC_NVME_LOG_SIZE_DATA_LEN
@@ -83,6 +86,16 @@#define WDC_NVME_CRASH_DUMP_CMD 0x20#define WDC_NVME_CRASH_DUMP_SUBCMD 0x04 +/* PFail Crash dump */ +#define WDC_NVME_PF_CRASH_DUMP_SIZE_DATA_LEN WDC_NVME_LOG_SIZE_HDR_LEN +#define WDC_NVME_PF_CRASH_DUMP_SIZE_NDT 0x02 +#define WDC_NVME_PF_CRASH_DUMP_SIZE_CMD 0x20 +#define WDC_NVME_PF_CRASH_DUMP_SIZE_SUBCMD 0x05 + +#define WDC_NVME_PF_CRASH_DUMP_OPCODE WDC_NVME_CAP_DIAG_CMD_OPCODE +#define WDC_NVME_PF_CRASH_DUMP_CMD 0x20 +#define WDC_NVME_PF_CRASH_DUMP_SUBCMD 0x06 +/* Drive Log */ #define WDC_NVME_DRIVE_LOG_SIZE_OPCODE WDC_NVME_CAP_DIAG_CMD_OPCODE#define WDC_NVME_DRIVE_LOG_SIZE_DATA_LEN WDC_NVME_LOG_SIZE_DATA_LEN
@@ -110,9 +123,10 @@#define WDC_NVME_PURGE_STATE_PWR_CYC_PURGE 0x04/* Clear dumps */ -#define WDC_NVME_CLEAR_DUMP_OPCODE 0xFF +#define WDC_NVME_CLEAR_DUMP_OPCODE 0xFF#define WDC_NVME_CLEAR_CRASH_DUMP_CMD 0x03#define WDC_NVME_CLEAR_CRASH_DUMP_SUBCMD 0x05 +#define WDC_NVME_CLEAR_PF_CRASH_DUMP_SUBCMD 0x06/* Additional Smart Log */ #define WDC_ADD_LOG_BUF_LEN 0x4000
@@ -294,10 +308,10 @@ static int wdc_get_serial_name(int fd, char *file, size_t len, char *suffix);static int wdc_create_log_file (char *file,__u8 *drive_log_data,__u32 drive_log_length);static int wdc_do_clear_dump (int fd,__u8 opcode,__u32 cdw12); -static int wdc_do_dump (int fd,__u32 opcode,__u32 data_len,__u32 cdw10, -__u32 cdw12,__u32 dump_length,char *file); -static int wdc_do_crash_dump (int fd,char *file); -static int wdc_crash_dump (int fd,char *file); +static int wdc_do_dump (int fd,__u32 opcode,__u32 data_len, +__u32 cdw12,char *file,__u32 xfer_size); +static int wdc_do_crash_dump (int fd,char *file,int type); +static int wdc_crash_dump (int fd,char *file,int type);static int wdc_get_crash_dump (int argc,char **argv,struct command *command,struct plugin *plugin);static int wdc_do_drive_log (int fd,char *file);
@@ -766,29 +780,59 @@ static __u32 wdc_dump_length_e6(int fd, __u32 opcode, __u32 cdw10, __u32 cdw12,return ret; } -static int wdc_do_dump (int fd,__u32 opcode,__u32 data_len,__u32 cdw10, -__u32 cdw12,__u32 dump_length,char *file) +static int wdc_do_dump (int fd,__u32 opcode,__u32 data_len, +__u32 cdw12,char *file,__u32 xfer_size) { -int ret; +int ret =0 ;__u8 *dump_data; +__u32 curr_data_offset, curr_data_len; +int i;struct nvme_admin_cmd admin_cmd; +__u32 dump_length = data_len; dump_data = (__u8 *)malloc (sizeof (__u8) * dump_length);if (dump_data ==NULL ) { -fprintf (stderr,"ERROR : malloc : %s\n" ,strerror (errno)); +fprintf (stderr,"%s: ERROR : malloc : %s\n" , __func__,strerror (errno));return -1 ; }memset (dump_data,0 ,sizeof (__u8) * dump_length);memset (&admin_cmd,0 ,sizeof (struct nvme_admin_cmd )); + curr_data_offset =0 ; + curr_data_len = xfer_size; + i =0 ; + admin_cmd.opcode = opcode; admin_cmd.addr = (__u64 )(uintptr_t )dump_data; - admin_cmd.data_len = data_len; - admin_cmd.cdw10 = cdw10; + admin_cmd.data_len = curr_data_len; + admin_cmd.cdw10 = curr_data_len >>2 ; admin_cmd.cdw12 = cdw12; + admin_cmd.cdw13 = curr_data_offset; + +while (curr_data_offset < data_len) { + ret =nvme_submit_passthru (fd,NVME_IOCTL_ADMIN_CMD , &admin_cmd); +if (ret !=0 ) { +fprintf (stderr,"%s: ERROR : WDC : NVMe Status:%s(%x)\n" , + __func__,nvme_status_to_string (ret), ret); +fprintf (stderr,"%s: ERROR : WDC : Get chunk %d, size = 0x%x, offset = 0x%x, addr = 0x%lx\n" , + __func__, i, admin_cmd.data_len, curr_data_offset, (long unsigned int )admin_cmd.addr); +break ; + } + +if ((curr_data_offset + xfer_size) <= data_len) + curr_data_len = xfer_size; +else + curr_data_len = data_len - curr_data_offset;// last transfer + + curr_data_offset += curr_data_len; + admin_cmd.addr = (__u64 )(uintptr_t )dump_data + (__u64 )curr_data_offset; + admin_cmd.data_len = curr_data_len; + admin_cmd.cdw10 = curr_data_len >>2 ; + admin_cmd.cdw13 = curr_data_offset >>2 ; + i++; + } - ret =nvme_submit_passthru (fd,NVME_IOCTL_ADMIN_CMD , &admin_cmd); -fprintf (stderr,"NVMe Status:%s(%x)\n" ,nvme_status_to_string (ret), ret);if (ret ==0 ) { +fprintf (stderr,"%s: NVMe Status:%s(%x)\n" , __func__,nvme_status_to_string (ret), ret); ret =wdc_create_log_file (file, dump_data, dump_length); }free (dump_data);
@@ -1023,47 +1067,96 @@ static int wdc_internal_fw_log(int argc, char **argv, struct command *command, } } -static int wdc_do_crash_dump (int fd,char *file) +static int wdc_do_crash_dump (int fd,char *file,int type) {int ret;__u32 crash_dump_length; -__u8 opcode =WDC_NVME_CLEAR_DUMP_OPCODE ; -__u32 cdw12 = ((WDC_NVME_CLEAR_CRASH_DUMP_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | +__u32 opcode; +__u32 cdw12; +__u32 cdw10_size; +__u32 cdw12_size; +__u32 cdw12_clear; + +if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) { +/* set parms to get the PFAIL Crash Dump */ + opcode =WDC_NVME_PF_CRASH_DUMP_OPCODE ; + cdw10_size =WDC_NVME_PF_CRASH_DUMP_SIZE_NDT ; + cdw12_size = ((WDC_NVME_PF_CRASH_DUMP_SIZE_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | +WDC_NVME_PF_CRASH_DUMP_SIZE_CMD ); + + cdw12 = (WDC_NVME_PF_CRASH_DUMP_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | +WDC_NVME_PF_CRASH_DUMP_CMD ; + + cdw12_clear = ((WDC_NVME_CLEAR_PF_CRASH_DUMP_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | +WDC_NVME_CLEAR_CRASH_DUMP_CMD ); + + }else { +/* set parms to get the Crash Dump */ + opcode =WDC_NVME_CRASH_DUMP_OPCODE ; + cdw10_size =WDC_NVME_CRASH_DUMP_SIZE_NDT ; + cdw12_size = ((WDC_NVME_CRASH_DUMP_SIZE_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | +WDC_NVME_CRASH_DUMP_SIZE_CMD ); + + cdw12 = (WDC_NVME_CRASH_DUMP_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | +WDC_NVME_CRASH_DUMP_CMD ; + + cdw12_clear = ((WDC_NVME_CLEAR_CRASH_DUMP_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) |WDC_NVME_CLEAR_CRASH_DUMP_CMD ); + } + + ret =wdc_dump_length (fd, + opcode, + cdw10_size, + cdw12_size, + &crash_dump_length); - ret =wdc_dump_length (fd,WDC_NVME_CRASH_DUMP_SIZE_OPCODE , -WDC_NVME_CRASH_DUMP_SIZE_NDT , - ((WDC_NVME_CRASH_DUMP_SIZE_SUBCMD << -WDC_NVME_SUBCMD_SHIFT ) |WDC_NVME_CRASH_DUMP_SIZE_CMD ), - &crash_dump_length);if (ret ==-1 ) { +if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) +fprintf (stderr,"INFO : WDC: Pfail dump get size failed\n" ); +else +fprintf (stderr,"INFO : WDC: Crash dump get size failed\n" ); +return -1 ; } +if (crash_dump_length ==0 ) { -fprintf (stderr,"INFO : WDC: Crash dump is empty\n" ); +if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) +fprintf (stderr,"INFO : WDC: Pfail dump is empty\n" ); +else +fprintf (stderr,"INFO : WDC: Crash dump is empty\n" ); }else { - ret =wdc_do_dump (fd,WDC_NVME_CRASH_DUMP_OPCODE , crash_dump_length, - crash_dump_length, - (WDC_NVME_CRASH_DUMP_SUBCMD <<WDC_NVME_SUBCMD_SHIFT ) | -WDC_NVME_CRASH_DUMP_CMD , crash_dump_length, file); + ret =wdc_do_dump (fd, + opcode, + crash_dump_length, + cdw12, + file, + crash_dump_length); +if (ret ==0 ) - ret =wdc_do_clear_dump (fd, opcode, cdw12); + ret =wdc_do_clear_dump (fd,WDC_NVME_CLEAR_DUMP_OPCODE , cdw12_clear); }return ret; } -static int wdc_crash_dump (int fd,char *file) +static int wdc_crash_dump (int fd,char *file,int type) {char f[PATH_MAX ] = {0 }; +char dump_type[PATH_MAX ] = {0 };if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); } -if (wdc_get_serial_name (fd, f,PATH_MAX ,"crash_dump" ) ==-1 ) { + +if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) +strncpy (dump_type,"_pfail_dump" ,11 ); +else +strncpy (dump_type,"_crash_dump" ,11 ); + +if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" );return -1 ; - } -return wdc_do_crash_dump (fd, f); + } +return wdc_do_crash_dump (fd, f, type); }static int wdc_do_drive_log (int fd,char *file)
@@ -1149,7 +1242,6 @@ static int wdc_get_crash_dump(int argc, char **argv, struct command *command,const char *desc ="Get Crash Dump." ;const char *file ="Output file pathname." ;int fd; -int ret;struct config {char *file; };
@@ -1168,12 +1260,46 @@ static int wdc_get_crash_dump(int argc, char **argv, struct command *command,if (fd <0 )return fd; -wdc_check_device (fd); - ret =wdc_crash_dump (fd, cfg.file); -if (ret !=0 ) { -fprintf (stderr,"ERROR : WDC : failed to read crash dump\n" ); +if (wdc_check_device_match (fd,WDC_NVME_VID ,WDC_NVME_SN100_DEV_ID ) || +wdc_check_device_match (fd,WDC_NVME_VID ,WDC_NVME_SN200_DEV_ID ) ) { +return wdc_crash_dump (fd, cfg.file,WDC_NVME_CRASH_DUMP_TYPE ); + }else { +fprintf (stderr,"ERROR : WDC: unsupported device for get-crash-dump command\n" ); +return -1 ; + } +} + +static int wdc_get_pfail_dump (int argc,char **argv,struct command *command, +struct plugin *plugin) +{ +char *desc ="Get Pfail Crash Dump." ; +char *file ="Output file pathname." ; +int fd; +struct config { +char *file; + }; + +struct config cfg = { + .file =NULL , + }; + +const struct argconfig_commandline_options command_line_options[] = { + {"output-file" ,'o' ,"FILE" ,CFG_STRING , &cfg.file, required_argument, file}, + {NULL ,'\0' ,NULL ,CFG_NONE ,NULL , no_argument, desc}, + {NULL } + }; + + fd =parse_and_open (argc, argv, desc, command_line_options,NULL ,0 ); +if (fd <0 ) +return fd; + +if (wdc_check_device_match (fd,WDC_NVME_VID ,WDC_NVME_SN100_DEV_ID ) || +wdc_check_device_match (fd,WDC_NVME_VID ,WDC_NVME_SN200_DEV_ID ) ) { +return wdc_crash_dump (fd, cfg.file,WDC_NVME_PFAIL_DUMP_TYPE ); + }else { +fprintf (stderr,"ERROR : WDC: unsupported device for get-pfail-dump command\n" ); +return -1 ; } -return ret; }static void wdc_do_id_ctrl (__u8 *vs,struct json_object *root)
wdc-nvme.h +1 -0
@@ -11,6 +11,7 @@ PLUGIN(NAME("wdc", "Western Digital vendor specific extensions"), ENTRY("cap-diag", "WDC Capture-Diagnostics", wdc_cap_diag) ENTRY("drive-log", "WDC Drive Log", wdc_drive_log) ENTRY("get-crash-dump", "WDC Crash Dump", wdc_get_crash_dump) + ENTRY("get-pfail-dump", "WDC Pfail Dump", wdc_get_pfail_dump) ENTRY("id-ctrl", "WDC identify controller", wdc_id_ctrl) ENTRY("purge", "WDC Purge", wdc_purge)ENTRY ("purge-monitor" ,"WDC Purge Monitor" , wdc_purge_monitor)
Signature · 1 line
1.8.3.1
PATCH 2/2 nvme-cli wdc plugin: Add documentation for pfail dump.
--- Documentation/nvme-wdc-get-pfail-dump.txt | 54 +++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/nvme-wdc-get-pfail-dump.txt
Documentation/nvme-wdc-get-pfail-dump.txt new file +54 -0
@@ -0,0 +1,54 @@ +nvme-wdc-get-pfail-dump(1) +========================== + +NAME +---- +nvme-wdc-get-pfail-dump - Retrieve WDC device's pfail crash dump. + +SYNOPSIS +-------- +[verse] +'nvme wdc get-pfail-dump' <device> [--output-file=<FILE>, -o <FILE>] + +DESCRIPTION +----------- +For the NVMe device given, sends the WDC vendor unique pfail crash dump +request and saves the result to file. In current implementation pfail crash +dump is captured if it is present. On success it will save the dump in file +with appropriate suffix. Note that this command will clear the available +dump from the device on success. + +The <device> parameter is mandatory NVMe character device (ex: /dev/nvme0). + +This will only work on WDC devices supporting this feature. +Results for any other device are undefined. + +OPTIONS +------- +-o <FILE>:: +--output-file=<FILE>:: + Output file; defaults to device serial number followed by "pfail_dump" suffix + +EXAMPLES +-------- +* Gets the pfail crash dump from the device and saves to default file in current directory +(e.g. STM00019F3F9_pfail_dump.bin): ++ +------------ +# nvme wdc get-pfail-dump /dev/nvme0 +------------ +* Gets the pfail crash dump from the device and saves to defined file in current directory +(e.g. testSTM00019F3F9_pfail_dump.bin): ++ +------------ +# nvme wdc get-pfail-dump /dev/nvme0 -o test +------------ +* Gets the pfail crash dump from the device and saves to defined file with pathname (e.g. /tmp/testSTM00019F3F9_pfail_dump.bin): ++ +------------ +# nvme wdc get-pfail-dump /dev/nvme0 -o /tmp/test +------------ + +NVME +---- +Part of the nvme-user suite
Signature · 1 line
1.8.3.1
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
In reply to Dong HoQuoted 2 lines · Dong Ho
---
This patch implements the function to retreive a WDC device's pfail crash dump.
Thanks, both applied.
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
The patched version fails to build on Fedora 28 (gcc 8.1.1) due to a warning.
Here is the message:
cc -D_GNU_SOURCE -D__CHECK_ENDIAN__ -O2 -g -Wall -Werror -std=gnu99 -DLIBUUID -DNVME_VERSION='"1.6.96.g352d"' -c wdc-nvme.c wdc-nvme.c: In function ?wdc_crash_dump?:
wdc-nvme.c:1151:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation]
strncpy(dump_type, "_pfail_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
wdc-nvme.c:1153:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation]
strncpy(dump_type, "_crash_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [Makefile:46: wdc-nvme.o] Error 1
Quoted 11 lines · Keith Busch
Quoted 2 lines · Dong Ho
---
This patch implements the function to retreive a WDC device's pfail crash dump.Thanks, both applied.
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Quoted 17 lines · Eyal BenDavid
The patched version fails to build on Fedora 28 (gcc 8.1.1) due to a warning.
Here is the message:
cc -D_GNU_SOURCE -D__CHECK_ENDIAN__ -O2 -g -Wall -Werror -std=gnu99 -DLIBUUID -DNVME_VERSION='"1.6.96.g352d"' -c
wdc-nvme.cwdc-nvme.c: In function ?wdc_crash_dump?:
wdc-nvme.c:1151:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation]strncpy(dump_type, "_pfail_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ wdc-nvme.c:1153:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation] strncpy(dump_type, "_crash_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Quoted 3 lines
cc1: all warnings being treated as errors
make: *** [Makefile:46: wdc-nvme.o] Error 1
On Fri, Oct 19, 2018@8:21 PM Keith Busch <[email protected]> wrote:
Thanks for the notice. The pattern used in that patch is a bit odd as you'd normally supply the size of the destination to strncpy rather than the source. I'll fix that up.
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Maybe they want to copy exactly 11 bytes regardless of the zero byte in the end If so, memcpy is the function to use.
Quoted 26 lines · Keith Busch
Quoted 17 lines · Eyal BenDavid
The patched version fails to build on Fedora 28 (gcc 8.1.1) due to a warning.
Here is the message:
cc -D_GNU_SOURCE -D__CHECK_ENDIAN__ -O2 -g -Wall -Werror -std=gnu99 -DLIBUUID -DNVME_VERSION='"1.6.96.g352d"' -c
wdc-nvme.cwdc-nvme.c: In function ?wdc_crash_dump?:
wdc-nvme.c:1151:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation]strncpy(dump_type, "_pfail_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ wdc-nvme.c:1153:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation] strncpy(dump_type, "_crash_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Quoted 3 lines
cc1: all warnings being treated as errors
make: *** [Makefile:46: wdc-nvme.o] Error 1
On Fri, Oct 19, 2018@8:21 PM Keith Busch <[email protected]> wrote:Thanks for the notice. The pattern used in that patch is a bit odd as you'd normally supply the size of the destination to strncpy rather than the source. I'll fix that up.
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Yea, the intent is to copy 11 bytes into the string. The zero byte/space is not needed so memcpy is probably the better solution.
Jeff Lien
-----Original Message-----
From: Eyal BenDavid [mailto:[email protected]]
Sent: Wednesday, October 24, 2018 11:29 AM
To: keith.busch at intel.com
Cc: Keith Busch <keith.busch at linux.intel.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Jeffrey Lien <Jeff.Lien at wdc.com>
Subject: Re: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Maybe they want to copy exactly 11 bytes regardless of the zero byte in the end If so, memcpy is the function to use.
Quoted 26 lines · Keith Busch
Quoted 17 lines · Eyal BenDavid
The patched version fails to build on Fedora 28 (gcc 8.1.1) due to a warning.
Here is the message:
cc -D_GNU_SOURCE -D__CHECK_ENDIAN__ -O2 -g -Wall -Werror -std=gnu99 -DLIBUUID -DNVME_VERSION='"1.6.96.g352d"' -c
wdc-nvme.cwdc-nvme.c: In function ?wdc_crash_dump?:
wdc-nvme.c:1151:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation]strncpy(dump_type, "_pfail_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ wdc-nvme.c:1153:3: error: ?strncpy? output truncated before terminating nul copying 11 bytes from a string of the same length [-Werror=stringop-truncation] strncpy(dump_type, "_crash_dump", 11); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Quoted 3 lines
cc1: all warnings being treated as errors
make: *** [Makefile:46: wdc-nvme.o] Error 1 On Fri, Oct 19, 2018 at 8:21 PM Keith Busch <[email protected]> wrote:Thanks for the notice. The pattern used in that patch is a bit odd as you'd normally supply the size of the destination to strncpy rather than the source. I'll fix that up.
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Quoted 1 line · Jeffrey Lien
Yea, the intent is to copy 11 bytes into the string. The zero byte/space is not needed so memcpy is probably the better solution.
No, look again. The string terminator most certainly is needed the way you're using it. It happened to work out since you've initialized the temporary character array to all 0's, but there was no reason to even allocate 4k stack space to hold a copy of that string instead of using the pointing a 'const char *' directly to the literal.
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Hi Jeff,
-static int wdc_crash_dump(int fd, char *file) +static int wdc_crash_dump(int fd, char *file, int type)
{
char f[PATH_MAX] = {0};+ char dump_type[PATH_MAX] = {0};
Please follow the Keith's comment, "dump_type" should not use the PATH_MAX as this predefined macro is used to hold the pathname for the file, so we need to change the dump_type.
Either allocate a memory for dump_type or defined a macro as it is suffix we don't have to copy string literal.
?On 10/24/18, 2:54 PM, "Linux-nvme on behalf of Keith Busch" <[email protected] on behalf of [email protected]> wrote:
On Wed, Oct 24, 2018@09:42:28PM +0000, Jeffrey Lien wrote:
> Yea, the intent is to copy 11 bytes into the string. The zero byte/space is not needed so memcpy is probably the better solution.
No, look again. The string terminator most certainly is needed the way you're using it. It happened to work out since you've initialized the temporary character array to all 0's, but there was no reason to even allocate 4k stack space to hold a copy of that string instead of using the pointing a 'const char *' directly to the literal.
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
How does this look? I've verified it on my Ubuntu system, but don't have a Fedora 28 system to verify the warning message has been resolved.
wdc-nvme.c +3 -3
@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type)static int wdc_crash_dump (int fd,char *file,int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char dump_type[12 ] = {0 };if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,11 ); +strcpy (dump_type,"_pfail_dump" );else -strncpy (dump_type,"_crash_dump" ,11 ); +strcpy (dump_type,"_crash_dump" );if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" );
Signature · 4 lines
1.8.3.1 Jeff Lien
-----Original Message-----
From: Chaitanya Kulkarni
Sent: Wednesday, October 24, 2018 5:20 PM
To: Keith Busch <keith.busch at intel.com>; Jeffrey Lien <Jeff.Lien at wdc.com>
Cc: Eyal BenDavid <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Keith Busch <keith.busch at linux.intel.com>
Subject: Re: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Hi Jeff,
-static int wdc_crash_dump(int fd, char *file) +static int wdc_crash_dump(int fd, char *file, int type)
{
char f[PATH_MAX] = {0};+ char dump_type[PATH_MAX] = {0};
Please follow the Keith's comment, "dump_type" should not use the PATH_MAX as this predefined macro is used to hold the pathname for the file, so we need to change the dump_type.
Either allocate a memory for dump_type or defined a macro as it is suffix we don't have to copy string literal.
?On 10/24/18, 2:54 PM, "Linux-nvme on behalf of Keith Busch" <[email protected] on behalf of [email protected]> wrote:
On Wed, Oct 24, 2018@09:42:28PM +0000, Jeffrey Lien wrote:
> Yea, the intent is to copy 11 bytes into the string. The zero byte/space is not needed so memcpy is probably the better solution.
No, look again. The string terminator most certainly is needed the way you're using it. It happened to work out since you've initialized the temporary character array to all 0's, but there was no reason to even allocate 4k stack space to hold a copy of that string instead of using the pointing a 'const char *' directly to the literal.
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Quoted 17 lines · Jeffrey Lien
How does this look? I've verified it on my Ubuntu system, but don't have a Fedora 28 system to verify the warning message has been resolved.
wdc-nvme.c +1
-1@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type)static int wdc_crash_dump (int fd,char *file,int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char dump_type[12 ] = {0 };if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }
wdc-nvme.c static int wdc_do_crash_dump(int fd, char *file, int type) · line 1156
Quoted 10 lines
wdc-nvme.c +2
-2if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,11 ); +strcpy (dump_type,"_pfail_dump" );else -strncpy (dump_type,"_crash_dump" ,11 ); +strcpy (dump_type,"_crash_dump" );if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" );
I just don't see the point in having multiple copies of the string. Why not just use it directly?
---
plugins/wdc/wdc-nvme.c +3 -3
@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type)static int wdc_crash_dump (int fd,char *file,int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char *dump_type;if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,PATH_MAX -1 ); + dump_type ="_pfail_dump" ;else -strncpy (dump_type,"_crash_dump" ,PATH_MAX -1 ); + dump_type ="_crash_dump" ;if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" );
--
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Keith,
I'm not sure how to get away from 2 string copys other than having 2 calls to the wdc_get_serial_name function. There are 2 potential dump types so I need to set dump_type to one or the other.
Or are you suggesting setting dump_type directly like this:
Review
Quoted 6 lines
(quoted diff) +2
-2if (type == WDC_NVME_PFAIL_DUMP_TYPE) - strncpy(dump_type, "_pfail_dump", 11); + dump_type = "_pfail_dump"; else - strncpy(dump_type, "_crash_dump", 11); + dump_type = "_crash_dump";
Versus using strncpy?
Jeff Lien
-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, October 25, 2018 11:00 AM
To: Jeffrey Lien <Jeff.Lien at wdc.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>; Eyal BenDavid <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Keith Busch <keith.busch at linux.intel.com>
Subject: Re: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Quoted 17 lines · Jeffrey Lien
How does this look? I've verified it on my Ubuntu system, but don't have a Fedora 28 system to verify the warning message has been resolved.
wdc-nvme.c +1
-1@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type) static int wdc_crash_dump(int fd, char *file, int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char dump_type[12 ] = {0 };if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }
wdc-nvme.c static int wdc_do_crash_dump(int fd, char
Quoted 10 lines
wdc-nvme.c +2
-3if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,11 ); +strcpy (dump_type,"_pfail_dump" );else -strncpy (dump_type,"_crash_dump" ,11 ); +strcpy (dump_type,"_crash_dump" );if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" ); --
I just don't see the point in having multiple copies of the string. Why not just use it directly?
---
plugins/wdc/wdc-nvme.c +3 -4
@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type) static int wdc_crash_dump(int fd, char *file, int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char *dump_type;if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,PATH_MAX -1 ); + dump_type ="_pfail_dump" ;else -strncpy (dump_type,"_crash_dump" ,PATH_MAX -1 ); + dump_type ="_crash_dump" ;if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" ); --
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
In reply to Keith BuschKeith,
Ignore that last note. I didn't scroll down far enough - my bad. I'll fix up the patch the way Keith suggested.
Jeff Lien
-----Original Message-----
From: Jeffrey Lien
Sent: Thursday, October 25, 2018 12:25 PM
To: 'Keith Busch' <keith.busch at intel.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>; Eyal BenDavid <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Keith Busch <keith.busch at linux.intel.com>
Subject: RE: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Keith,
I'm not sure how to get away from 2 string copys other than having 2 calls to the wdc_get_serial_name function. There are 2 potential dump types so I need to set dump_type to one or the other.
Or are you suggesting setting dump_type directly like this:
Review
Quoted 6 lines
(quoted diff) +2
-2if (type == WDC_NVME_PFAIL_DUMP_TYPE) - strncpy(dump_type, "_pfail_dump", 11); + dump_type = "_pfail_dump"; else - strncpy(dump_type, "_crash_dump", 11); + dump_type = "_crash_dump";
Versus using strncpy?
Jeff Lien
-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, October 25, 2018 11:00 AM
To: Jeffrey Lien <Jeff.Lien at wdc.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>; Eyal BenDavid <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Keith Busch <keith.busch at linux.intel.com>
Subject: Re: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Quoted 17 lines · Jeffrey Lien
How does this look? I've verified it on my Ubuntu system, but don't have a Fedora 28 system to verify the warning message has been resolved.
wdc-nvme.c +1
-1@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type) static int wdc_crash_dump(int fd, char *file, int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char dump_type[12 ] = {0 };if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }
wdc-nvme.c static int wdc_do_crash_dump(int fd, char
Quoted 10 lines
wdc-nvme.c +2
-3if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,11 ); +strcpy (dump_type,"_pfail_dump" );else -strncpy (dump_type,"_crash_dump" ,11 ); +strcpy (dump_type,"_crash_dump" );if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" ); --
I just don't see the point in having multiple copies of the string. Why not just use it directly?
---
plugins/wdc/wdc-nvme.c +3 -4
@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type) static int wdc_crash_dump(int fd, char *file, int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char *dump_type;if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,PATH_MAX -1 ); + dump_type ="_pfail_dump" ;else -strncpy (dump_type,"_crash_dump" ,PATH_MAX -1 ); + dump_type ="_crash_dump" ;if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" ); --
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
In reply to Keith BuschKeith,
Here's an updated patch, or just apply the version you had below. They are identical. Sorry for misunderstanding what you were trying to suggest.
Quoted 1 line
From
613eb0f5e235dc20fbeec1c944652ac889dc2988Mon Sep 17 00:00:00 2001
From: Jeff Lien <[email protected]>
Date: Thu, 25 Oct 2018 12:35:43 -0500
Subject: [PATCH] Fix compiler warning on string copy from Fedora 28
--- wdc-nvme.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
wdc-nvme.c +4 -4
@@ -1141,21 +1141,21 @@ static int wdc_do_crash_dump(int fd, char *file, int type)static int wdc_crash_dump (int fd,char *file,int type) {char f[PATH_MAX ] = {0 }; -char dump_type[12 ] = {0 }; +char *dump_type;if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strcpy (dump_type,"_pfail_dump" ); + dump_type ="_pfail_dump" ;else -strcpy (dump_type,"_crash_dump" ); + dump_type ="_crash_dump" ;if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" );return -1 ; - } + }return wdc_do_crash_dump (fd, f, type); }
Signature · 4 lines
1.8.3.1 Jeff Lien
-----Original Message-----
From: Jeffrey Lien
Sent: Thursday, October 25, 2018 12:28 PM
To: 'Keith Busch' <keith.busch at intel.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>; 'Eyal BenDavid' <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; 'linux-nvme' <linux-nvme at lists.infradead.org>; 'Keith Busch' <keith.busch at linux.intel.com>
Subject: RE: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Keith,
Ignore that last note. I didn't scroll down far enough - my bad. I'll fix up the patch the way Keith suggested.
Jeff Lien
-----Original Message-----
From: Jeffrey Lien
Sent: Thursday, October 25, 2018 12:25 PM
To: 'Keith Busch' <keith.busch at intel.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>; Eyal BenDavid <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Keith Busch <keith.busch at linux.intel.com>
Subject: RE: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Keith,
I'm not sure how to get away from 2 string copys other than having 2 calls to the wdc_get_serial_name function. There are 2 potential dump types so I need to set dump_type to one or the other.
Or are you suggesting setting dump_type directly like this:
Review
Quoted 6 lines
(quoted diff) +2
-2if (type == WDC_NVME_PFAIL_DUMP_TYPE) - strncpy(dump_type, "_pfail_dump", 11); + dump_type = "_pfail_dump"; else - strncpy(dump_type, "_crash_dump", 11); + dump_type = "_crash_dump";
Versus using strncpy?
Jeff Lien
-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, October 25, 2018 11:00 AM
To: Jeffrey Lien <Jeff.Lien at wdc.com>
Cc: Chaitanya Kulkarni <Chaitanya.Kulkarni at wdc.com>; Eyal BenDavid <bdeyal at gmail.com>; Dong Ho <Dong.Ho at wdc.com>; linux-nvme <linux-nvme at lists.infradead.org>; Keith Busch <keith.busch at linux.intel.com>
Subject: Re: [PATCH 1/2] nvme-cli wdc plugin: Add get pfail dump command.
Quoted 17 lines · Jeffrey Lien
How does this look? I've verified it on my Ubuntu system, but don't have a Fedora 28 system to verify the warning message has been resolved.
wdc-nvme.c +1
-1@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type) static int wdc_crash_dump(int fd, char *file, int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char dump_type[12 ] = {0 };if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }
wdc-nvme.c static int wdc_do_crash_dump(int fd, char
Quoted 10 lines
wdc-nvme.c +2
-3if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,11 ); +strcpy (dump_type,"_pfail_dump" );else -strncpy (dump_type,"_crash_dump" ,11 ); +strcpy (dump_type,"_crash_dump" );if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" ); --
I just don't see the point in having multiple copies of the string. Why not just use it directly?
---
plugins/wdc/wdc-nvme.c +3 -4
@@ -1141,16 +1141,16 @@ static int wdc_do_crash_dump(int fd, char *file, int type) static int wdc_crash_dump(int fd, char *file, int type) {char f[PATH_MAX ] = {0 }; -char dump_type[PATH_MAX ] = {0 }; +char *dump_type;if (file !=NULL ) {strncpy (f, file,PATH_MAX -1 ); }if (type ==WDC_NVME_PFAIL_DUMP_TYPE ) -strncpy (dump_type,"_pfail_dump" ,PATH_MAX -1 ); + dump_type ="_pfail_dump" ;else -strncpy (dump_type,"_crash_dump" ,PATH_MAX -1 ); + dump_type ="_crash_dump" ;if (wdc_get_serial_name (fd, f,PATH_MAX , dump_type) ==-1 ) {fprintf (stderr,"ERROR : WDC : failed to generate file name\n" ); --
PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.
Quoted 2 lines · Jeffrey Lien
Keith,
Here's an updated patch, or just apply the version you had below. They are identical. Sorry for misunderstanding what you were trying to suggest.
Okay, no problem. I'll go one step further and make this a 'const char*' and update wdc_get_serial_name to declare the suffix as 'const' since that is how it is used. My compiler didn't complain either way, but I suspect some setting might not like casting a string literal to a non-const pointer.