Skip to contentSkip to conversation

nvme-cli wdc plugin: Add get pfail dump command.

PATCH New Dong Ho 6 participants 15 messages Oct 18, 2018 – Oct 25, 2018

Subsystems (2)
lore
Share & tools
lore

Dong Ho

author
#0 lore raw

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
index 330150a..55b2889 100644
@@ -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
index f5531a2..3732005 100644
@@ -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
author
#1 lore raw

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
new file mode 100644
index 0000000..63688c0
@@ -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
#2 lore raw

PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.

In reply to Dong Ho
Quoted 2 lines · Dong Ho

---
This patch implements the function to retreive a WDC device's pfail crash dump.

Thanks, both applied.

#3 lore raw

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

#4 lore raw

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.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);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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.

#5 lore raw

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.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);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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.

#6 lore raw

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.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);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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.

#7 lore raw

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.

#8 lore raw

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

#9 lore raw

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
index 55b2889..db68d58 100644
@@ -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

#10 lore raw

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
index 55b2889..db68d58 100644
@@ -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 -2
 
 	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");

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
index 8d61fa7..cd12edb 100644
@@ -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");

--

#11 lore raw

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 -2
 	if (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
index 55b2889..db68d58 100644
@@ -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 -3
 	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");
--

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");
--
#12 lore raw

PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.

In reply to Keith Busch

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 -2
 	if (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
index 55b2889..db68d58 100644
@@ -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 -3
 	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");
--

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");
--
#13 lore raw

PATCH 1/2 nvme-cli wdc plugin: Add get pfail dump command.

In reply to Keith Busch

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.

Quoted 1 line

From 613eb0f5e235dc20fbeec1c944652ac889dc2988 Mon 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

Signed-off-by Jeff Lien <jeff.lien at wdc.com>
---
 wdc-nvme.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
wdc-nvme.c +4 -4
index db68d58..0449c68 100644
@@ -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 -2
 	if (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
index 55b2889..db68d58 100644
@@ -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 -3
 	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");
--

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");
--
#14 lore raw

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.