All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno
@ 2019-05-13 17:03 Minwoo Im
  2019-05-13 17:03 ` [PATCH V2 1/4] nvme.h: Fix typos in status code values Minwoo Im
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Minwoo Im @ 2019-05-13 17:03 UTC (permalink / raw)


Hi,

This patchset introduces nvme-status module to manage mapping
relationships between nvme error status and errno.  It cannot be
directly mapped in 1:1, but we can figure out what kind of errors
happended by the return value of nvme-cli.

NVMe status fields are 16bits to indicate, but UNIX return value from
main() will be parsed in 8bits so that we need to do something about
return value to indicate nvme error status.

Please review.
Thanks,

Changes to previous C1:
  - make switch-case inline in nvme-status (Chaitanya)

Minwoo Im (4):
  nvme.h: Fix typos in status code values
  nvme-status: Introduce nvme status module to map errno
  nvme: Return errno mapped for nvme error status
  fabrics: Return errno mapped for fabrics error status

 Makefile      |   3 +-
 fabrics.c     |  24 +++++--
 linux/nvme.h  |  10 ++-
 nvme-print.c  |   4 +-
 nvme-status.c | 147 +++++++++++++++++++++++++++++++++++++++++
 nvme-status.h |  14 ++++
 nvme.c        | 180 ++++++++++++++++++++++++++++++--------------------
 7 files changed, 303 insertions(+), 79 deletions(-)
 create mode 100644 nvme-status.c
 create mode 100644 nvme-status.h

-- 
2.21.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 1/4] nvme.h: Fix typos in status code values
  2019-05-13 17:03 [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
@ 2019-05-13 17:03 ` Minwoo Im
  2019-05-19 16:48   ` Chaitanya Kulkarni
  2019-05-13 17:03 ` [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno Minwoo Im
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Minwoo Im @ 2019-05-13 17:03 UTC (permalink / raw)


Fix typos in status code value.  linux/nvme.h in kernel project is going
to be fixed soon or later.

Cc: Keith Busch <keith.busch at intel.com>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 linux/nvme.h | 4 ++--
 nvme-print.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux/nvme.h b/linux/nvme.h
index 2c840b9..c99b438 100644
--- a/linux/nvme.h
+++ b/linux/nvme.h
@@ -1365,9 +1365,9 @@ enum {
 	NVME_SC_FW_NEEDS_SUBSYS_RESET	= 0x110,
 	NVME_SC_FW_NEEDS_RESET		= 0x111,
 	NVME_SC_FW_NEEDS_MAX_TIME	= 0x112,
-	NVME_SC_FW_ACIVATE_PROHIBITED	= 0x113,
+	NVME_SC_FW_ACTIVATE_PROHIBITED	= 0x113,
 	NVME_SC_OVERLAPPING_RANGE	= 0x114,
-	NVME_SC_NS_INSUFFICENT_CAP	= 0x115,
+	NVME_SC_NS_INSUFFICIENT_CAP	= 0x115,
 	NVME_SC_NS_ID_UNAVAILABLE	= 0x116,
 	NVME_SC_NS_ALREADY_ATTACHED	= 0x118,
 	NVME_SC_NS_IS_PRIVATE		= 0x119,
diff --git a/nvme-print.c b/nvme-print.c
index c038355..0ce88d4 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -1801,9 +1801,9 @@ const char *nvme_status_to_string(__u32 status)
 	case NVME_SC_FW_NEEDS_SUBSYS_RESET:	return "FW_NEEDS_SUBSYSTEM_RESET: The firmware commit was successful, however, activation of the firmware image requires an NVM Subsystem";
 	case NVME_SC_FW_NEEDS_RESET:		return "FW_NEEDS_RESET: The firmware commit was successful; however, the image specified does not support being activated without a reset";
 	case NVME_SC_FW_NEEDS_MAX_TIME:		return "FW_NEEDS_MAX_TIME_VIOLATION: The image specified if activated immediately would exceed the Maximum Time for Firmware Activation (MTFA) value reported in Identify Controller. To activate the firmware, the Firmware Commit command needs to be re-issued and the image activated using a reset";
-	case NVME_SC_FW_ACIVATE_PROHIBITED:	return "FW_ACTIVATION_PROHIBITED: The image specified is being prohibited from activation by the controller for vendor specific reasons";
+	case NVME_SC_FW_ACTIVATE_PROHIBITED:	return "FW_ACTIVATION_PROHIBITED: The image specified is being prohibited from activation by the controller for vendor specific reasons";
 	case NVME_SC_OVERLAPPING_RANGE:		return "OVERLAPPING_RANGE: This error is indicated if the firmware image has overlapping ranges";
-	case NVME_SC_NS_INSUFFICENT_CAP:	return "NS_INSUFFICIENT_CAPACITY: Creating the namespace requires more free space than is currently available. The Command Specific Information field of the Error Information Log specifies the total amount of NVM capacity required to create the namespace in bytes";
+	case NVME_SC_NS_INSUFFICIENT_CAP:	return "NS_INSUFFICIENT_CAPACITY: Creating the namespace requires more free space than is currently available. The Command Specific Information field of the Error Information Log specifies the total amount of NVM capacity required to create the namespace in bytes";
 	case NVME_SC_NS_ID_UNAVAILABLE:		return "NS_ID_UNAVAILABLE: The number of namespaces supported has been exceeded";
 	case NVME_SC_NS_ALREADY_ATTACHED:	return "NS_ALREADY_ATTACHED: The controller is already attached to the namespace specified";
 	case NVME_SC_NS_IS_PRIVATE:		return "NS_IS_PRIVATE: The namespace is private and is already attached to one controller";
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno
  2019-05-13 17:03 [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
  2019-05-13 17:03 ` [PATCH V2 1/4] nvme.h: Fix typos in status code values Minwoo Im
@ 2019-05-13 17:03 ` Minwoo Im
  2019-05-19 17:42   ` Chaitanya Kulkarni
  2019-05-13 17:03 ` [PATCH V2 3/4] nvme: Return errno mapped for nvme error status Minwoo Im
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Minwoo Im @ 2019-05-13 17:03 UTC (permalink / raw)


Background:
  It's not enough to return the nvme status value in main() because it's
allowed to be in 8bits, but nvme status is indicated in 16bits.  So we
has not been able to figure out what kind of nvme status has been
returned by return value.

  This patch introduces nvme-status module that manages mapping between
nvme status and errno.  It's not possible to make 1:1 mapping relations,
but we can map it as a groups.

All the internal errors which has been returned in a negative value will
be returned with ECOMM that indicates communication errors.  In this
case, we can see what happened via stderr.

Cc: Keith Busch <keith.busch at intel.com>
Cc: Chaitanya Kulkarni <chaitanya.Kulkarni at wdc.com>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 Makefile      |   3 +-
 linux/nvme.h  |   6 +++
 nvme-status.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++
 nvme-status.h |  14 +++++
 4 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100644 nvme-status.c
 create mode 100644 nvme-status.h

diff --git a/Makefile b/Makefile
index 4bfbebb..1e50a65 100644
--- a/Makefile
+++ b/Makefile
@@ -32,7 +32,8 @@ override CFLAGS += -DNVME_VERSION='"$(NVME_VERSION)"'
 NVME_DPKG_VERSION=1~`lsb_release -sc`
 
 OBJS := argconfig.o suffix.o parser.o nvme-print.o nvme-ioctl.o \
-	nvme-lightnvm.o fabrics.o json.o nvme-models.o plugin.o
+	nvme-lightnvm.o fabrics.o json.o nvme-models.o plugin.o \
+	nvme-status.o
 
 PLUGIN_OBJS :=					\
 	plugins/intel/intel-nvme.o		\
diff --git a/linux/nvme.h b/linux/nvme.h
index c99b438..5ca7442 100644
--- a/linux/nvme.h
+++ b/linux/nvme.h
@@ -1307,6 +1307,12 @@ static inline bool nvme_is_write(struct nvme_command *cmd)
 	return cmd->common.opcode & 1;
 }
 
+enum {
+	NVME_SCT_GENERIC		= 0x0,
+	NVME_SCT_CMD_SPECIFIC		= 0x1,
+	NVME_SCT_MEDIA			= 0x2,
+};
+
 enum {
 	/*
 	 * Generic Command Status:
diff --git a/nvme-status.c b/nvme-status.c
new file mode 100644
index 0000000..3f51ceb
--- /dev/null
+++ b/nvme-status.c
@@ -0,0 +1,147 @@
+#include <linux/types.h>
+#include <stdbool.h>
+#include <errno.h>
+
+#include "nvme.h"
+#include "nvme-status.h"
+
+static inline __u8 nvme_generic_status_to_errno(__u16 status)
+{
+	switch (status) {
+	case NVME_SC_INVALID_OPCODE:
+	case NVME_SC_INVALID_FIELD:
+	case NVME_SC_INVALID_NS:
+	case NVME_SC_SGL_INVALID_LAST:
+	case NVME_SC_SGL_INVALID_COUNT:
+	case NVME_SC_SGL_INVALID_DATA:
+	case NVME_SC_SGL_INVALID_METADATA:
+	case NVME_SC_SGL_INVALID_TYPE:
+	case NVME_SC_SGL_INVALID_OFFSET:
+	case NVME_SC_SGL_INVALID_SUBTYPE:
+		return EINVAL;
+	case NVME_SC_CMDID_CONFLICT:
+		return EADDRINUSE;
+	case NVME_SC_DATA_XFER_ERROR:
+	case NVME_SC_INTERNAL:
+	case NVME_SC_SANITIZE_FAILED:
+		return EIO;
+	case NVME_SC_POWER_LOSS:
+	case NVME_SC_ABORT_REQ:
+	case NVME_SC_ABORT_QUEUE:
+	case NVME_SC_FUSED_FAIL:
+	case NVME_SC_FUSED_MISSING:
+		return EWOULDBLOCK;
+	case NVME_SC_CMD_SEQ_ERROR:
+		return EILSEQ;
+	case NVME_SC_SANITIZE_IN_PROGRESS:
+		return EINPROGRESS;
+	case NVME_SC_NS_WRITE_PROTECTED:
+	case NVME_SC_NS_NOT_READY:
+	case NVME_SC_RESERVATION_CONFLICT:
+		return EACCES;
+	case NVME_SC_LBA_RANGE:
+		return EREMOTEIO;
+	case NVME_SC_CAP_EXCEEDED:
+		return ENOSPC;
+	}
+
+	return EIO;
+}
+
+static inline __u8 nvme_cmd_specific_status_to_errno(__u16 status)
+{
+	switch (status) {
+	case NVME_SC_CQ_INVALID:
+	case NVME_SC_QID_INVALID:
+	case NVME_SC_QUEUE_SIZE:
+	case NVME_SC_FIRMWARE_SLOT:
+	case NVME_SC_FIRMWARE_IMAGE:
+	case NVME_SC_INVALID_VECTOR:
+	case NVME_SC_INVALID_LOG_PAGE:
+	case NVME_SC_INVALID_FORMAT:
+	case NVME_SC_INVALID_QUEUE:
+	case NVME_SC_NS_INSUFFICIENT_CAP:
+	case NVME_SC_NS_ID_UNAVAILABLE:
+	case NVME_SC_CTRL_LIST_INVALID:
+	case NVME_SC_BAD_ATTRIBUTES:
+	case NVME_SC_INVALID_PI:
+		return EINVAL;
+	case NVME_SC_ABORT_LIMIT:
+	case NVME_SC_ASYNC_LIMIT:
+		return EDQUOT;
+	case NVME_SC_FW_NEEDS_CONV_RESET:
+	case NVME_SC_FW_NEEDS_SUBSYS_RESET:
+	case NVME_SC_FW_NEEDS_MAX_TIME:
+		return ERESTART;
+	case NVME_SC_FEATURE_NOT_SAVEABLE:
+	case NVME_SC_FEATURE_NOT_CHANGEABLE:
+	case NVME_SC_FEATURE_NOT_PER_NS:
+	case NVME_SC_FW_ACTIVATE_PROHIBITED:
+	case NVME_SC_NS_IS_PRIVATE:
+	case NVME_SC_BP_WRITE_PROHIBITED:
+	case NVME_SC_READ_ONLY:
+		return EPERM;
+	case NVME_SC_OVERLAPPING_RANGE:
+	case NVME_SC_NS_NOT_ATTACHED:
+		return ENOSPC;
+	case NVME_SC_NS_ALREADY_ATTACHED:
+		return EALREADY;
+	case NVME_SC_THIN_PROV_NOT_SUPP:
+	case NVME_SC_ONCS_NOT_SUPPORTED:
+		return EOPNOTSUPP;
+	}
+
+	return EIO;
+}
+
+static inline __u8 nvme_fabrics_status_to_errno(__u16 status)
+{
+	switch (status) {
+	case NVME_SC_CONNECT_FORMAT:
+	case NVME_SC_CONNECT_INVALID_PARAM:
+		return EINVAL;
+	case NVME_SC_CONNECT_CTRL_BUSY:
+		return EBUSY;
+	case NVME_SC_CONNECT_RESTART_DISC:
+		return ERESTART;
+	case NVME_SC_CONNECT_INVALID_HOST:
+		return ECONNREFUSED;
+	case NVME_SC_DISCOVERY_RESTART:
+		return EAGAIN;
+	case NVME_SC_AUTH_REQUIRED:
+		return EPERM;
+	}
+
+	return EIO;
+}
+
+/*
+ * nvme_status_to_errno - It converts given status to errno mapped
+ * @status: nvme status field in completion queue entry
+ * @fabrics: true if given status is for fabrics
+ *
+ * Notes: This function can handle generic command status only.
+ */
+__u8 nvme_status_to_errno(__u16 status, bool fabrics)
+{
+	__u8 sct = nvme_status_type(status);
+
+	/*
+	 * The actual status code is enough with masking 0xff, but we need to
+	 * cover status code type which is 3bits with 0x7ff.
+	 */
+	status &= 0x7ff;
+
+	if (sct == NVME_SCT_GENERIC)
+		return nvme_generic_status_to_errno(status);
+	else if (sct == NVME_SCT_CMD_SPECIFIC && !fabrics)
+		return nvme_cmd_specific_status_to_errno(status);
+	else if (sct == NVME_SCT_CMD_SPECIFIC && fabrics)
+		return nvme_fabrics_status_to_errno(status);
+
+	/*
+	 * Media, integrity related status, and the others will be mapped to
+	 * EIO.
+	 */
+	return EIO;
+}
diff --git a/nvme-status.h b/nvme-status.h
new file mode 100644
index 0000000..094132b
--- /dev/null
+++ b/nvme-status.h
@@ -0,0 +1,14 @@
+#include <linux/types.h>
+#include <stdbool.h>
+
+/*
+ * nvme_status_type - It gives SCT(Status Code Type) in status field in
+ *                    completion queue entry.
+ * @status: status field located at DW3 in completion queue entry
+ */
+static inline __u8 nvme_status_type(__u16 status)
+{
+	return (status & 0x700) >> 8;
+}
+
+__u8 nvme_status_to_errno(__u16 status, bool fabrics);
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V2 3/4] nvme: Return errno mapped for nvme error status
  2019-05-13 17:03 [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
  2019-05-13 17:03 ` [PATCH V2 1/4] nvme.h: Fix typos in status code values Minwoo Im
  2019-05-13 17:03 ` [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno Minwoo Im
@ 2019-05-13 17:03 ` Minwoo Im
  2019-05-19 17:41   ` Chaitanya Kulkarni
  2019-05-13 17:03 ` [PATCH V2 4/4] fabrics: Return errno mapped for fabrics " Minwoo Im
  2019-05-18  2:11 ` [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
  4 siblings, 1 reply; 13+ messages in thread
From: Minwoo Im @ 2019-05-13 17:03 UTC (permalink / raw)


If ioctl module has returned a nvme error status, it will be returned
by being converted to a mapped errno value which has been provided by
previous commits.

Cc: Keith Busch <keith.busch at intel.com>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 nvme.c | 180 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 110 insertions(+), 70 deletions(-)

diff --git a/nvme.c b/nvme.c
index 220fb18..2e1ce4f 100644
--- a/nvme.c
+++ b/nvme.c
@@ -48,6 +48,7 @@
 #include "common.h"
 #include "nvme-print.h"
 #include "nvme-ioctl.h"
+#include "nvme-status.h"
 #include "nvme-lightnvm.h"
 #include "plugin.h"
 
@@ -213,9 +214,10 @@ static int get_smart_log(int argc, char **argv, struct command *cmd, struct plug
 		else
 			show_smart_log(&smart_log, cfg.namespace_id, devicename);
 	}
-	else if (err > 0)
+	else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("smart log");
 
  close_fd:
@@ -286,9 +288,10 @@ static int get_ana_log(int argc, char **argv, struct command *cmd,
 			json_ana_log(ana_log, devicename);
 		else
 			show_ana_log(ana_log, devicename);
-	} else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("ana-log");
 	free(ana_log);
 close_fd:
@@ -364,6 +367,7 @@ static int get_telemetry_log(int argc, char **argv, struct command *cmd, struct
 		perror("get-telemetry-log");
 	else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 		fprintf(stderr, "Failed to acquire telemetry header %d!\n", err);
 		goto close_output;
 	}
@@ -402,6 +406,7 @@ static int get_telemetry_log(int argc, char **argv, struct command *cmd, struct
 		} else if (err > 0) {
 			fprintf(stderr, "Failed to acquire full telemetry log!\n");
 			show_nvme_status(err);
+			err = nvme_status_to_errno(err, false);
 			break;
 		}
 
@@ -467,9 +472,10 @@ static int get_endurance_log(int argc, char **argv, struct command *cmd, struct
 			json_endurance_log(&endurance_log, cfg.group_id, devicename);
 		else
 			show_endurance_log(&endurance_log, cfg.group_id, devicename);
-	} else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("endurance log");
 
  close_fd:
@@ -528,10 +534,10 @@ static int get_effects_log(int argc, char **argv, struct command *cmd, struct pl
 			json_effects_log(&effects, devicename);
 		else
 			show_effects_log(&effects, flags);
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("effects log page");
 
  close_fd:
@@ -610,10 +616,10 @@ static int get_error_log(int argc, char **argv, struct command *cmd, struct plug
 				json_error_log(err_log, cfg.log_entries, devicename);
 			else
 				show_error_log(err_log, cfg.log_entries, devicename);
-		}
-		else if (err > 0)
+		} else if (err > 0) {
 			show_nvme_status(err);
-		else
+			err = nvme_status_to_errno(err, false);
+		} else
 			perror("error log");
 		free(err_log);
 	}
@@ -666,10 +672,10 @@ static int get_fw_log(int argc, char **argv, struct command *cmd, struct plugin
 			json_fw_log(&fw_log, devicename);
 		else
 			show_fw_log(&fw_log, devicename);
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("fw log");
 
  close_fd:
@@ -722,9 +728,10 @@ static int get_changed_ns_list_log(int argc, char **argv, struct command *cmd, s
 			json_changed_ns_list_log(&changed_ns_list_log, devicename);
 		else
 			show_changed_ns_list_log(&changed_ns_list_log, devicename);
-	} else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("changed ns list log");
 
  close_fd:
@@ -820,9 +827,10 @@ static int get_log(int argc, char **argv, struct command *cmd, struct plugin *pl
 				d(log, cfg.log_len, 16, 1);
 			} else
 				d_raw((unsigned char *)log, cfg.log_len);
-		} else if (err > 0)
+		} else if (err > 0) {
 			show_nvme_status(err);
-		else
+			err = nvme_status_to_errno(err, false);
+		} else
 			perror("log page");
 		free(log);
 	}
@@ -883,10 +891,10 @@ static int sanitize_log(int argc, char **argv, struct command *command, struct p
 			json_sanitize_log(&sanitize_log, devicename);
 		else
 			show_sanitize_log(&sanitize_log, flags, devicename);
-	}
-	else if (ret > 0)
+	} else if (ret > 0) {
 		show_nvme_status(ret);
-	else
+		ret = nvme_status_to_errno(ret, false);
+	} else
 		perror("sanitize status log");
 
  close_fd:
@@ -934,10 +942,10 @@ static int list_ctrl(int argc, char **argv, struct command *cmd, struct plugin *
 
 		for (i = 0; i < (min(num, 2048)); i++)
 			printf("[%4u]:%#x\n", i, le16_to_cpu(cntlist->identifier[i]));
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("id controller list");
 
 	free(cntlist);
@@ -980,10 +988,10 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
 		for (i = 0; i < 1024; i++)
 			if (ns_list[i])
 				printf("[%4u]:%#x\n", i, le32_to_cpu(ns_list[i]));
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("id namespace list");
 
 	close(fd);
@@ -1052,9 +1060,10 @@ static int delete_ns(int argc, char **argv, struct command *cmd, struct plugin *
 	if (!err)
 		printf("%s: Success, deleted nsid:%d\n", cmd->name,
 								cfg.namespace_id);
-	else if (err > 0)
+	else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("delete namespace");
 
  close_fd:
@@ -1117,9 +1126,10 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
 
 	if (!err)
 		printf("%s: Success, nsid:%d\n", cmd->name, cfg.namespace_id);
-	else if (err > 0)
+	else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror(attach ? "attach namespace" : "detach namespace");
 
  close_fd:
@@ -1216,6 +1226,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
 			else {
 				fprintf(stderr, "identify failed\n");
 				show_nvme_status(err);
+				err = nvme_status_to_errno(err, false);
 			}
 			return err;
 		}
@@ -1241,9 +1252,10 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
 			     cfg.nmic, cfg.timeout, &nsid);
 	if (!err)
 		printf("%s: Success, created nsid:%d\n", cmd->name, nsid);
-	else if (err > 0)
+	else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("create namespace");
 
 	close(fd);
@@ -1840,6 +1852,7 @@ static int list(int argc, char **argv, struct command *cmd, struct plugin *plugi
 		else if (ret > 0) {
 			fprintf(stderr, "identify failed\n");
 			show_nvme_status(ret);
+			ret = nvme_status_to_errno(ret, false);
 		}
 		else {
 			fprintf(stderr, "%s: failed to obtain nvme info: %s\n",
@@ -1927,10 +1940,10 @@ int __id_ctrl(int argc, char **argv, struct command *cmd, struct plugin *plugin,
 			printf("NVME Identify Controller:\n");
 			__show_nvme_id_ctrl(&ctrl, flags, vs);
 		}
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("identify controller");
 
  close_fd:
@@ -2006,10 +2019,10 @@ static int ns_descs(int argc, char **argv, struct command *cmd, struct plugin *p
 			printf("NVME Namespace Identification Descriptors NS %d:\n", cfg.namespace_id);
 			show_nvme_id_ns_descs(nsdescs);
 		}
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("identify namespace");
 
 	free(nsdescs);
@@ -2095,10 +2108,10 @@ static int id_ns(int argc, char **argv, struct command *cmd, struct plugin *plug
 			printf("NVME Identify Namespace %d:\n", cfg.namespace_id);
 			show_nvme_id_ns(&ns, flags);
 		}
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("identify namespace");
 
  close_fd:
@@ -2153,10 +2166,10 @@ static int id_nvmset(int argc, char **argv, struct command *cmd, struct plugin *
 			printf("NVME Identify NVM Set List %d:\n", cfg.nvmset_id);
 			show_nvme_id_nvmset(&nvmset);
 		}
-	}
-	else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		perror("identify nvm set list");
 
  close_fd:
@@ -2247,6 +2260,7 @@ static int virtual_mgmt(int argc, char **argv, struct command *cmd, struct plugi
 	}
 	else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 	} else
 		perror("virt-mgmt");
 
@@ -2374,6 +2388,7 @@ static int device_self_test(int argc, char **argv, struct command *cmd, struct p
 			printf("Device self-test started\n");
 	} else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 	} else
 		perror("Device self-test");
 
@@ -2427,6 +2442,7 @@ static int self_test_log(int argc, char **argv, struct command *cmd, struct plug
 		}
 	} else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 	} else {
 		perror("self_test_log");
 	}
@@ -2556,6 +2572,7 @@ static int get_feature(int argc, char **argv, struct command *cmd, struct plugin
 			d_raw(buf, cfg.data_len);
 	} else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 	} else
 		perror("get-feature");
 
@@ -2654,6 +2671,7 @@ static int fw_download(int argc, char **argv, struct command *cmd, struct plugin
 			break;
 		} else if (err != 0) {
 			show_nvme_status(err);
+			err = nvme_status_to_errno(err, false);
 			break;
 		}
 		fw_buf     += cfg.xfer;
@@ -2750,6 +2768,7 @@ static int fw_commit(int argc, char **argv, struct command *cmd, struct plugin *
 			break;
 		default:
 			show_nvme_status(err);
+			err = nvme_status_to_errno(err, false);
 			break;
 		}
 	else {
@@ -2925,8 +2944,10 @@ static int sanitize(int argc, char **argv, struct command *cmd, struct plugin *p
 			    cfg.no_dealloc, cfg.ovrpat);
 	if (ret < 0)
 		perror("sanitize");
-	else if (ret > 0)
+	else if (ret > 0) {
 		show_nvme_status(ret);
+		ret = nvme_status_to_errno(ret, false);
+	}
 
  close_fd:
 	close(fd);
@@ -3051,6 +3072,7 @@ static int get_property(int argc, char **argv, struct command *cmd, struct plugi
 		show_single_property(cfg.offset, value, cfg.human_readable);
 	} else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 	}
 
  close_fd:
@@ -3106,6 +3128,7 @@ static int set_property(int argc, char **argv, struct command *cmd, struct plugi
 				nvme_register_to_string(cfg.offset), cfg.value);
 	} else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
 	}
 
  close_fd:
@@ -3201,6 +3224,7 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
 			else {
 				fprintf(stderr, "identify failed\n");
 				show_nvme_status(err);
+				err = nvme_status_to_errno(err, false);
 			}
 			return err;
 		}
@@ -3257,9 +3281,10 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
 				cfg.pil, cfg.ms, cfg.timeout);
 	if (err < 0)
 		perror("format");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else {
+		err = nvme_status_to_errno(err, false);
+	} else {
 		printf("Success formatting namespace:%x\n", cfg.namespace_id);
 		ioctl(fd, BLKRRPART);
 		if (cfg.reset && S_ISCHR(nvme_stat.st_mode))
@@ -3378,8 +3403,10 @@ static int set_feature(int argc, char **argv, struct command *cmd, struct plugin
 			else
 				d(buf, cfg.data_len, 16, 1);
 		}
-	} else if (err > 0)
+	} else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
+	}
 
  close_ffd:
 	close(ffd);
@@ -3625,8 +3652,10 @@ static int dir_send(int argc, char **argv, struct command *cmd, struct plugin *p
 				d_raw(buf, cfg.data_len);
 		}
 	}
-	else if (err > 0)
+	else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
+	}
 
 close_ffd:
 	close(ffd);
@@ -3682,9 +3711,10 @@ static int write_uncor(int argc, char **argv, struct command *cmd, struct plugin
 					cfg.block_count);
 	if (err < 0)
 		perror("write uncorrectable");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVME Write Uncorrectable Success\n");
 
 close_fd:
@@ -3774,9 +3804,10 @@ static int write_zeroes(int argc, char **argv, struct command *cmd, struct plugi
 			control, cfg.ref_tag, cfg.app_tag, cfg.app_tag_mask);
 	if (err < 0)
 		perror("write-zeroes");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVME Write Zeroes Success\n");
 
  close_fd:
@@ -3874,9 +3905,10 @@ static int dsm(int argc, char **argv, struct command *cmd, struct plugin *plugin
 	err = nvme_dsm(fd, cfg.namespace_id, cfg.cdw11, dsm, nr);
 	if (err < 0)
 		perror("data-set management");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVMe DSM: success\n");
 
  close_fd:
@@ -3922,9 +3954,10 @@ static int flush(int argc, char **argv, struct command *cmd, struct plugin *plug
 	err = nvme_flush(fd, cfg.namespace_id);
 	if (err < 0)
 		perror("flush");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVMe Flush: success\n");
 close_fd:
 	close(fd);
@@ -3995,9 +4028,10 @@ static int resv_acquire(int argc, char **argv, struct command *cmd, struct plugi
 				!!cfg.iekey, cfg.crkey, cfg.prkey);
 	if (err < 0)
 		perror("reservation acquire");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVME Reservation Acquire success\n");
 
  close_fd:
@@ -4072,9 +4106,10 @@ static int resv_register(int argc, char **argv, struct command *cmd, struct plug
 				!!cfg.iekey, cfg.crkey, cfg.nrkey);
 	if (err < 0)
 		perror("reservation register");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVME Reservation  success\n");
 
  close_fd:
@@ -4145,9 +4180,10 @@ static int resv_release(int argc, char **argv, struct command *cmd, struct plugi
 				!!cfg.iekey, cfg.crkey);
 	if (err < 0)
 		perror("reservation release");
-	else if (err != 0)
+	else if (err != 0) {
 		show_nvme_status(err);
-	else
+		err = nvme_status_to_errno(err, false);
+	} else
 		printf("NVME Reservation Release success\n");
 
  close_fd:
@@ -4463,9 +4499,10 @@ static int submit_io(int opcode, char *command, const char *desc,
 			command, elapsed_utime(start_time, end_time));
 	if (err < 0)
 		perror("submit-io");
-	else if (err)
+	else if (err) {
 		show_nvme_status(err);
-	else {
+		err = nvme_status_to_errno(err, false);
+	} else {
 		if (!(opcode & 1) && write(dfd, (void *)buffer, cfg.data_size) < 0) {
 			fprintf(stderr, "write: %s: failed to write buffer to output file\n",
 					strerror(errno));
@@ -4720,8 +4757,10 @@ static int dir_receive(int argc, char **argv, struct command *cmd, struct plugin
 			}
 		}
 	}
-	else if (err > 0)
+	else if (err > 0) {
 		show_nvme_status(err);
+		err = nvme_status_to_errno(err, false);
+	}
 free:
 	if (cfg.data_len)
 		free(buf);
@@ -4904,9 +4943,10 @@ static int passthru(int argc, char **argv, int ioctl_cmd, const char *desc, stru
 				cfg.timeout, &result);
 	if (err < 0)
 		perror("passthru");
-	else if (err)
+	else if (err) {
 		show_nvme_status(err);
-	else  {
+		err = nvme_status_to_errno(err, false);
+	} else  {
 		if (!cfg.raw_binary) {
 			fprintf(stderr, "NVMe command result:%08x\n", result);
 			if (data && cfg.read && !err)
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V2 4/4] fabrics: Return errno mapped for fabrics error status
  2019-05-13 17:03 [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
                   ` (2 preceding siblings ...)
  2019-05-13 17:03 ` [PATCH V2 3/4] nvme: Return errno mapped for nvme error status Minwoo Im
@ 2019-05-13 17:03 ` Minwoo Im
  2019-05-18  2:11 ` [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
  4 siblings, 0 replies; 13+ messages in thread
From: Minwoo Im @ 2019-05-13 17:03 UTC (permalink / raw)


If discover has been failed due to a nvme status, it will be returned to
main() with mapped value for fabrics get log page command.

Now connect command related status cannot be added in this patch because
kernel is not currently returning the nvme status, it's instead
returning -EIO if fails.  errno for connect command can be added once
kernel is ready to return the proper value for nvme status.

Cc: Keith Busch <keith.busch at intel.com>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 fabrics.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 511de06..ad61b07 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -35,6 +35,7 @@
 
 #include "parser.h"
 #include "nvme-ioctl.h"
+#include "nvme-status.h"
 #include "fabrics.h"
 
 #include "nvme.h"
@@ -295,7 +296,7 @@ enum {
 };
 
 static int nvmf_get_log_page_discovery(const char *dev_path,
-		struct nvmf_disc_rsp_page_hdr **logp, int *numrec)
+		struct nvmf_disc_rsp_page_hdr **logp, int *numrec, int *status)
 {
 	struct nvmf_disc_rsp_page_hdr *log;
 	unsigned int hdr_size;
@@ -328,6 +329,7 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 
 	error = nvme_discovery_log(fd, log, hdr_size);
 	if (error) {
+		*status = (error > 0) ? nvme_status_to_errno(error, true) : error;
 		error = DISC_GET_NUMRECS;
 		goto out_free_log;
 	}
@@ -366,6 +368,8 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 		 */
 		error = nvme_discovery_log(fd, log, log_size);
 		if (error) {
+			*status = (error > 0) ?
+				nvme_status_to_errno(error, true) : error;
 			error = DISC_GET_LOG;
 			goto out_free_log;
 		}
@@ -379,6 +383,8 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 		genctr = le64_to_cpu(log->genctr);
 		error = nvme_discovery_log(fd, log, hdr_size);
 		if (error) {
+			*status = (error > 0) ?
+				nvme_status_to_errno(error, true) : error;
 			error = DISC_GET_LOG;
 			goto out_free_log;
 		}
@@ -849,6 +855,7 @@ static int do_discover(char *argstr, bool connect)
 	struct nvmf_disc_rsp_page_hdr *log = NULL;
 	char *dev_name;
 	int instance, numrec = 0, ret, err;
+	int status = 0;
 
 	instance = add_ctrl(argstr);
 	if (instance < 0)
@@ -856,7 +863,7 @@ static int do_discover(char *argstr, bool connect)
 
 	if (asprintf(&dev_name, "/dev/nvme%d", instance) < 0)
 		return -errno;
-	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec);
+	ret = nvmf_get_log_page_discovery(dev_name, &log, &numrec, &status);
 	free(dev_name);
 	err = remove_ctrl(instance);
 	if (err)
@@ -874,9 +881,11 @@ static int do_discover(char *argstr, bool connect)
 	case DISC_GET_NUMRECS:
 		fprintf(stderr,
 			"Get number of discovery log entries failed.\n");
+		ret = status;
 		break;
 	case DISC_GET_LOG:
 		fprintf(stderr, "Get discovery log entries failed.\n");
+		ret = status;
 		break;
 	case DISC_NO_LOG:
 		fprintf(stdout, "No discovery log entries to fetch.\n");
@@ -885,6 +894,7 @@ static int do_discover(char *argstr, bool connect)
 	case DISC_NOT_EQUAL:
 		fprintf(stderr,
 		"Numrec values of last two get discovery log page not equal\n");
+		ret = DISC_OK;
 		break;
 	default:
 		fprintf(stderr, "Get discovery log page failed: %d\n", ret);
@@ -989,15 +999,21 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 	cfg.nqn = NVME_DISC_SUBSYS_NAME;
 
 	if (!cfg.transport && !cfg.traddr) {
-		return discover_from_conf_file(desc, argstr,
+		ret = discover_from_conf_file(desc, argstr,
 				command_line_options, connect);
+		if (ret > 0)
+			ret = nvme_status_to_errno(ret, true);
 	} else {
 		ret = build_options(argstr, BUF_SIZE);
 		if (ret)
 			return ret;
 
-		return do_discover(argstr, connect);
+		ret = do_discover(argstr, connect);
+		if (ret > 0)
+			ret = nvme_status_to_errno(ret, true);
 	}
+
+	return ret;
 }
 
 int connect(const char *desc, int argc, char **argv)
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno
  2019-05-13 17:03 [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
                   ` (3 preceding siblings ...)
  2019-05-13 17:03 ` [PATCH V2 4/4] fabrics: Return errno mapped for fabrics " Minwoo Im
@ 2019-05-18  2:11 ` Minwoo Im
  4 siblings, 0 replies; 13+ messages in thread
From: Minwoo Im @ 2019-05-18  2:11 UTC (permalink / raw)


On 19-05-14 02:03:38, Minwoo Im wrote:
> Hi,
> 
> This patchset introduces nvme-status module to manage mapping
> relationships between nvme error status and errno.  It cannot be
> directly mapped in 1:1, but we can figure out what kind of errors
> happended by the return value of nvme-cli.
> 
> NVMe status fields are 16bits to indicate, but UNIX return value from
> main() will be parsed in 8bits so that we need to do something about
> return value to indicate nvme error status.
> 
> Please review.
> Thanks,
> 
> Changes to previous C1:
>   - make switch-case inline in nvme-status (Chaitanya)
> 
> Minwoo Im (4):
>   nvme.h: Fix typos in status code values
>   nvme-status: Introduce nvme status module to map errno
>   nvme: Return errno mapped for nvme error status
>   fabrics: Return errno mapped for fabrics error status

Ping :)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 1/4] nvme.h: Fix typos in status code values
  2019-05-13 17:03 ` [PATCH V2 1/4] nvme.h: Fix typos in status code values Minwoo Im
@ 2019-05-19 16:48   ` Chaitanya Kulkarni
  0 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2019-05-19 16:48 UTC (permalink / raw)


Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
On 05/13/2019 10:04 AM, Minwoo Im wrote:
> Fix typos in status code value.  linux/nvme.h in kernel project is going
> to be fixed soon or later.
>
> Cc: Keith Busch <keith.busch at intel.com>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   linux/nvme.h | 4 ++--
>   nvme-print.c | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/linux/nvme.h b/linux/nvme.h
> index 2c840b9..c99b438 100644
> --- a/linux/nvme.h
> +++ b/linux/nvme.h
> @@ -1365,9 +1365,9 @@ enum {
>   	NVME_SC_FW_NEEDS_SUBSYS_RESET	= 0x110,
>   	NVME_SC_FW_NEEDS_RESET		= 0x111,
>   	NVME_SC_FW_NEEDS_MAX_TIME	= 0x112,
> -	NVME_SC_FW_ACIVATE_PROHIBITED	= 0x113,
> +	NVME_SC_FW_ACTIVATE_PROHIBITED	= 0x113,
>   	NVME_SC_OVERLAPPING_RANGE	= 0x114,
> -	NVME_SC_NS_INSUFFICENT_CAP	= 0x115,
> +	NVME_SC_NS_INSUFFICIENT_CAP	= 0x115,
>   	NVME_SC_NS_ID_UNAVAILABLE	= 0x116,
>   	NVME_SC_NS_ALREADY_ATTACHED	= 0x118,
>   	NVME_SC_NS_IS_PRIVATE		= 0x119,
> diff --git a/nvme-print.c b/nvme-print.c
> index c038355..0ce88d4 100644
> --- a/nvme-print.c
> +++ b/nvme-print.c
> @@ -1801,9 +1801,9 @@ const char *nvme_status_to_string(__u32 status)
>   	case NVME_SC_FW_NEEDS_SUBSYS_RESET:	return "FW_NEEDS_SUBSYSTEM_RESET: The firmware commit was successful, however, activation of the firmware image requires an NVM Subsystem";
>   	case NVME_SC_FW_NEEDS_RESET:		return "FW_NEEDS_RESET: The firmware commit was successful; however, the image specified does not support being activated without a reset";
>   	case NVME_SC_FW_NEEDS_MAX_TIME:		return "FW_NEEDS_MAX_TIME_VIOLATION: The image specified if activated immediately would exceed the Maximum Time for Firmware Activation (MTFA) value reported in Identify Controller. To activate the firmware, the Firmware Commit command needs to be re-issued and the image activated using a reset";
> -	case NVME_SC_FW_ACIVATE_PROHIBITED:	return "FW_ACTIVATION_PROHIBITED: The image specified is being prohibited from activation by the controller for vendor specific reasons";
> +	case NVME_SC_FW_ACTIVATE_PROHIBITED:	return "FW_ACTIVATION_PROHIBITED: The image specified is being prohibited from activation by the controller for vendor specific reasons";
>   	case NVME_SC_OVERLAPPING_RANGE:		return "OVERLAPPING_RANGE: This error is indicated if the firmware image has overlapping ranges";
> -	case NVME_SC_NS_INSUFFICENT_CAP:	return "NS_INSUFFICIENT_CAPACITY: Creating the namespace requires more free space than is currently available. The Command Specific Information field of the Error Information Log specifies the total amount of NVM capacity required to create the namespace in bytes";
> +	case NVME_SC_NS_INSUFFICIENT_CAP:	return "NS_INSUFFICIENT_CAPACITY: Creating the namespace requires more free space than is currently available. The Command Specific Information field of the Error Information Log specifies the total amount of NVM capacity required to create the namespace in bytes";
>   	case NVME_SC_NS_ID_UNAVAILABLE:		return "NS_ID_UNAVAILABLE: The number of namespaces supported has been exceeded";
>   	case NVME_SC_NS_ALREADY_ATTACHED:	return "NS_ALREADY_ATTACHED: The controller is already attached to the namespace specified";
>   	case NVME_SC_NS_IS_PRIVATE:		return "NS_IS_PRIVATE: The namespace is private and is already attached to one controller";
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 3/4] nvme: Return errno mapped for nvme error status
  2019-05-13 17:03 ` [PATCH V2 3/4] nvme: Return errno mapped for nvme error status Minwoo Im
@ 2019-05-19 17:41   ` Chaitanya Kulkarni
  2019-05-19 17:53     ` Minwoo Im
  0 siblings, 1 reply; 13+ messages in thread
From: Chaitanya Kulkarni @ 2019-05-19 17:41 UTC (permalink / raw)


On 05/13/2019 10:05 AM, Minwoo Im wrote:
> If ioctl module has returned a nvme error status, it will be returned
> by being converted to a mapped errno value which has been provided by
> previous commits.
>
> Cc: Keith Busch <keith.busch at intel.com>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   nvme.c | 180 +++++++++++++++++++++++++++++++++++----------------------
>   1 file changed, 110 insertions(+), 70 deletions(-)
>
> diff --git a/nvme.c b/nvme.c
> index 220fb18..2e1ce4f 100644
> --- a/nvme.c
> +++ b/nvme.c
> @@ -48,6 +48,7 @@
>   #include "common.h"
>   #include "nvme-print.h"
>   #include "nvme-ioctl.h"
> +#include "nvme-status.h"
>   #include "nvme-lightnvm.h"
>   #include "plugin.h"
>
> @@ -213,9 +214,10 @@ static int get_smart_log(int argc, char **argv, struct command *cmd, struct plug
>   		else
>   			show_smart_log(&smart_log, cfg.namespace_id, devicename);
>   	}
> -	else if (err > 0)
> +	else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("smart log");
>
>    close_fd:
> @@ -286,9 +288,10 @@ static int get_ana_log(int argc, char **argv, struct command *cmd,
>   			json_ana_log(ana_log, devicename);
>   		else
>   			show_ana_log(ana_log, devicename);
> -	} else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("ana-log");
>   	free(ana_log);
>   close_fd:
> @@ -364,6 +367,7 @@ static int get_telemetry_log(int argc, char **argv, struct command *cmd, struct
>   		perror("get-telemetry-log");
>   	else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   		fprintf(stderr, "Failed to acquire telemetry header %d!\n", err);

Following line to nvme_status_to_errno() call above assumes that err has 
a return value from nvme_get_telemetry_log() and we are overwriting it.

We need to avoid such scenarios going forward. Also, since each command 
assumes that err holds the NVMe status. Instead of having to call 
nvme_status_errno() along with nvme_show_status() we should call 
nvme_status_to_errno(err) at the end of the function in the return 
statement. This approach will not break the assumption that code is 
having now and less lines of code changes, obviously 
nvme_status_to_errno() will need some modifications but that
is fine, untested patch following  :-

diff --git a/nvme.c b/nvme.c
index e0f7552..0dd4f18 100644
--- a/nvme.c
+++ b/nvme.c
@@ -220,7 +220,7 @@ static int get_smart_log(int argc, char **argv, 
struct command *cmd, struct plug
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_ana_log(int argc, char **argv, struct command *cmd,
@@ -292,7 +292,7 @@ static int get_ana_log(int argc, char **argv, struct 
command *cmd,
         free(ana_log);
  close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_telemetry_log(int argc, char **argv, struct command 
*cmd, struct plugin *plugin)
@@ -419,7 +419,7 @@ static int get_telemetry_log(int argc, char **argv, 
struct command *cmd, struct
         free(page_log);
   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_endurance_log(int argc, char **argv, struct command 
*cmd, struct plugin *plugin)
@@ -473,7 +473,7 @@ static int get_endurance_log(int argc, char **argv, 
struct command *cmd, struct

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_effects_log(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -535,7 +535,7 @@ static int get_effects_log(int argc, char **argv, 
struct command *cmd, struct pl

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_error_log(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -619,7 +619,7 @@ static int get_error_log(int argc, char **argv, 
struct command *cmd, struct plug

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_fw_log(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -673,7 +673,7 @@ static int get_fw_log(int argc, char **argv, struct 
command *cmd, struct plugin

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_changed_ns_list_log(int argc, char **argv, struct 
command *cmd, struct plugin *plugin)
@@ -729,7 +729,7 @@ static int get_changed_ns_list_log(int argc, char 
**argv, struct command *cmd, s
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_log(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -828,7 +828,7 @@ static int get_log(int argc, char **argv, struct 
command *cmd, struct plugin *pl

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int sanitize_log(int argc, char **argv, struct command 
*command, struct plugin *plugin)
@@ -943,7 +943,7 @@ static int list_ctrl(int argc, char **argv, struct 
command *cmd, struct plugin *

         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int list_ns(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -987,7 +987,7 @@ static int list_ns(int argc, char **argv, struct 
command *cmd, struct plugin *pl

         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_nsid(int fd)
@@ -1059,7 +1059,7 @@ static int delete_ns(int argc, char **argv, struct 
command *cmd, struct plugin *
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int nvme_attach_ns(int argc, char **argv, int attach, const 
char *desc, struct command *cmd)
@@ -1124,7 +1124,7 @@ static int nvme_attach_ns(int argc, char **argv, 
int attach, const char *desc, s
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int attach_ns(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -1216,7 +1216,7 @@ static int create_ns(int argc, char **argv, struct 
command *cmd, struct plugin *
                                 fprintf(stderr, "identify failed\n");
                                 show_nvme_status(err);
                         }
-                       return err;
+                       return nvme_status_to_errno(err);
                 }
                 for (i = 0; i < 16; ++i) {
                         if ((1 << ns.lbaf[i].ds) == cfg.bs && 
ns.lbaf[i].ms == 0) {
@@ -1247,7 +1247,7 @@ static int create_ns(int argc, char **argv, struct 
command *cmd, struct plugin *

         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  char *nvme_char_from_block(char *block)
@@ -1935,7 +1935,7 @@ int __id_ctrl(int argc, char **argv, struct 
command *cmd, struct plugin *plugin,
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int id_ctrl(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -2016,7 +2016,7 @@ static int ns_descs(int argc, char **argv, struct 
command *cmd, struct plugin *p
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int id_ns(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -2103,7 +2103,7 @@ static int id_ns(int argc, char **argv, struct 
command *cmd, struct plugin *plug
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int id_nvmset(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2161,7 +2161,7 @@ static int id_nvmset(int argc, char **argv, struct 
command *cmd, struct plugin *
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_ns_id(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2249,7 +2249,7 @@ static int virtual_mgmt(int argc, char **argv, 
struct command *cmd, struct plugi
                 perror("virt-mgmt");

         close(fd);
-       return err;
+       return nvme_status_to_errno(err);

  }

@@ -2326,7 +2326,7 @@ static int list_secondary_ctrl(int argc, char 
**argv, struct command *cmd, struc
  close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int device_self_test(int argc, char **argv, struct command 
*cmd, struct plugin *plugin)
@@ -2374,7 +2374,7 @@ static int device_self_test(int argc, char **argv, 
struct command *cmd, struct p
                 perror("Device self-test");

         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int self_test_log(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2430,7 +2430,7 @@ static int self_test_log(int argc, char **argv, 
struct command *cmd, struct plug
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_feature(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2561,7 +2561,7 @@ static int get_feature(int argc, char **argv, 
struct command *cmd, struct plugin
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int fw_download(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2666,7 +2666,7 @@ static int fw_download(int argc, char **argv, 
struct command *cmd, struct plugin
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static char *nvme_fw_status_reset_type(__u32 status)
@@ -2759,7 +2759,7 @@ static int fw_commit(int argc, char **argv, struct 
command *cmd, struct plugin *
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int subsystem_reset(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2787,7 +2787,7 @@ static int subsystem_reset(int argc, char **argv, 
struct command *cmd, struct pl
         }

         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int reset(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -2811,7 +2811,7 @@ static int reset(int argc, char **argv, struct 
command *cmd, struct plugin *plug
         }

         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int ns_rescan(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -2835,7 +2835,7 @@ static int ns_rescan(int argc, char **argv, struct 
command *cmd, struct plugin *
         }

         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int sanitize(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -3000,7 +3000,7 @@ static int show_registers(int argc, char **argv, 
struct command *cmd, struct plu
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int get_property(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -3052,7 +3052,7 @@ static int get_property(int argc, char **argv, 
struct command *cmd, struct plugi
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int set_property(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -3107,7 +3107,7 @@ static int set_property(int argc, char **argv, 
struct command *cmd, struct plugi
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int format(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -3198,7 +3198,7 @@ static int format(int argc, char **argv, struct 
command *cmd, struct plugin *plu
                                 fprintf(stderr, "identify failed\n");
                                 show_nvme_status(err);
                         }
-                       return err;
+                       return nvme_status_to_errno(err);
                 }
                 prev_lbaf = ns.flbas & 0xf;

@@ -3265,7 +3265,7 @@ static int format(int argc, char **argv, struct 
command *cmd, struct plugin *plu
   close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int set_feature(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -3384,7 +3384,7 @@ static int set_feature(int argc, char **argv, 
struct command *cmd, struct plugin
                 free(buf);
   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int sec_send(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -3480,7 +3480,7 @@ static int sec_send(int argc, char **argv, struct 
command *cmd, struct plugin *p
         close(sec_fd);
   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int dir_send(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -3631,7 +3631,7 @@ free:
                 free(buf);
  close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int write_uncor(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -3686,7 +3686,7 @@ static int write_uncor(int argc, char **argv, 
struct command *cmd, struct plugin
  close_fd:
         close(fd);

-       return err;
+       return nvme_status_to_errno(err);
  }

  static int write_zeroes(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -3777,7 +3777,7 @@ static int write_zeroes(int argc, char **argv, 
struct command *cmd, struct plugi

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int dsm(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -3877,7 +3877,7 @@ static int dsm(int argc, char **argv, struct 
command *cmd, struct plugin *plugin

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int flush(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -3924,7 +3924,7 @@ static int flush(int argc, char **argv, struct 
command *cmd, struct plugin *plug
                 printf("NVMe Flush: success\n");
  close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int resv_acquire(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -3998,7 +3998,7 @@ static int resv_acquire(int argc, char **argv, 
struct command *cmd, struct plugi

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int resv_register(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -4075,7 +4075,7 @@ static int resv_register(int argc, char **argv, 
struct command *cmd, struct plug

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int resv_release(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -4148,7 +4148,7 @@ static int resv_release(int argc, char **argv, 
struct command *cmd, struct plugi

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int resv_report(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -4242,7 +4242,7 @@ static int resv_report(int argc, char **argv, 
struct command *cmd, struct plugin

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int submit_io(int opcode, char *command, const char *desc,
@@ -4487,7 +4487,7 @@ static int submit_io(int opcode, char *command, 
const char *desc,
         close(dfd);
   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int compare(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -4593,7 +4593,7 @@ static int sec_recv(int argc, char **argv, struct 
command *cmd, struct plugin *p

   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int dir_receive(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)
@@ -4723,7 +4723,7 @@ free:
                 free(buf);
  close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int passthru(int argc, char **argv, int ioctl_cmd, const char 
*desc, struct command *cmd)
@@ -4923,7 +4923,7 @@ static int passthru(int argc, char **argv, int 
ioctl_cmd, const char *desc, stru
                 close(wfd);
   close_fd:
         close(fd);
-       return err;
+       return nvme_status_to_errno(err);
  }

  static int io_passthru(int argc, char **argv, struct command *cmd, 
struct plugin *plugin)





>   		goto close_output;
>   	}
> @@ -402,6 +406,7 @@ static int get_telemetry_log(int argc, char **argv, struct command *cmd, struct
>   		} else if (err > 0) {
>   			fprintf(stderr, "Failed to acquire full telemetry log!\n");
>   			show_nvme_status(err);
> +			err = nvme_status_to_errno(err, false);
>   			break;
>   		}
>
> @@ -467,9 +472,10 @@ static int get_endurance_log(int argc, char **argv, struct command *cmd, struct
>   			json_endurance_log(&endurance_log, cfg.group_id, devicename);
>   		else
>   			show_endurance_log(&endurance_log, cfg.group_id, devicename);
> -	} else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("endurance log");
>
>    close_fd:
> @@ -528,10 +534,10 @@ static int get_effects_log(int argc, char **argv, struct command *cmd, struct pl
>   			json_effects_log(&effects, devicename);
>   		else
>   			show_effects_log(&effects, flags);
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("effects log page");
>
>    close_fd:
> @@ -610,10 +616,10 @@ static int get_error_log(int argc, char **argv, struct command *cmd, struct plug
>   				json_error_log(err_log, cfg.log_entries, devicename);
>   			else
>   				show_error_log(err_log, cfg.log_entries, devicename);
> -		}
> -		else if (err > 0)
> +		} else if (err > 0) {
>   			show_nvme_status(err);
> -		else
> +			err = nvme_status_to_errno(err, false);
> +		} else
>   			perror("error log");
>   		free(err_log);
>   	}
> @@ -666,10 +672,10 @@ static int get_fw_log(int argc, char **argv, struct command *cmd, struct plugin
>   			json_fw_log(&fw_log, devicename);
>   		else
>   			show_fw_log(&fw_log, devicename);
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("fw log");
>
>    close_fd:
> @@ -722,9 +728,10 @@ static int get_changed_ns_list_log(int argc, char **argv, struct command *cmd, s
>   			json_changed_ns_list_log(&changed_ns_list_log, devicename);
>   		else
>   			show_changed_ns_list_log(&changed_ns_list_log, devicename);
> -	} else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("changed ns list log");
>
>    close_fd:
> @@ -820,9 +827,10 @@ static int get_log(int argc, char **argv, struct command *cmd, struct plugin *pl
>   				d(log, cfg.log_len, 16, 1);
>   			} else
>   				d_raw((unsigned char *)log, cfg.log_len);
> -		} else if (err > 0)
> +		} else if (err > 0) {
>   			show_nvme_status(err);
> -		else
> +			err = nvme_status_to_errno(err, false);
> +		} else
>   			perror("log page");
>   		free(log);
>   	}
> @@ -883,10 +891,10 @@ static int sanitize_log(int argc, char **argv, struct command *command, struct p
>   			json_sanitize_log(&sanitize_log, devicename);
>   		else
>   			show_sanitize_log(&sanitize_log, flags, devicename);
> -	}
> -	else if (ret > 0)
> +	} else if (ret > 0) {
>   		show_nvme_status(ret);
> -	else
> +		ret = nvme_status_to_errno(ret, false);
> +	} else
>   		perror("sanitize status log");
>
>    close_fd:
> @@ -934,10 +942,10 @@ static int list_ctrl(int argc, char **argv, struct command *cmd, struct plugin *
>
>   		for (i = 0; i < (min(num, 2048)); i++)
>   			printf("[%4u]:%#x\n", i, le16_to_cpu(cntlist->identifier[i]));
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("id controller list");
>
>   	free(cntlist);
> @@ -980,10 +988,10 @@ static int list_ns(int argc, char **argv, struct command *cmd, struct plugin *pl
>   		for (i = 0; i < 1024; i++)
>   			if (ns_list[i])
>   				printf("[%4u]:%#x\n", i, le32_to_cpu(ns_list[i]));
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("id namespace list");
>
>   	close(fd);
> @@ -1052,9 +1060,10 @@ static int delete_ns(int argc, char **argv, struct command *cmd, struct plugin *
>   	if (!err)
>   		printf("%s: Success, deleted nsid:%d\n", cmd->name,
>   								cfg.namespace_id);
> -	else if (err > 0)
> +	else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("delete namespace");
>
>    close_fd:
> @@ -1117,9 +1126,10 @@ static int nvme_attach_ns(int argc, char **argv, int attach, const char *desc, s
>
>   	if (!err)
>   		printf("%s: Success, nsid:%d\n", cmd->name, cfg.namespace_id);
> -	else if (err > 0)
> +	else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror(attach ? "attach namespace" : "detach namespace");
>
>    close_fd:
> @@ -1216,6 +1226,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
>   			else {
>   				fprintf(stderr, "identify failed\n");
>   				show_nvme_status(err);
> +				err = nvme_status_to_errno(err, false);
>   			}
>   			return err;
>   		}
> @@ -1241,9 +1252,10 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
>   			     cfg.nmic, cfg.timeout, &nsid);
>   	if (!err)
>   		printf("%s: Success, created nsid:%d\n", cmd->name, nsid);
> -	else if (err > 0)
> +	else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("create namespace");
>
>   	close(fd);
> @@ -1840,6 +1852,7 @@ static int list(int argc, char **argv, struct command *cmd, struct plugin *plugi
>   		else if (ret > 0) {
>   			fprintf(stderr, "identify failed\n");
>   			show_nvme_status(ret);
> +			ret = nvme_status_to_errno(ret, false);
>   		}
>   		else {
>   			fprintf(stderr, "%s: failed to obtain nvme info: %s\n",
> @@ -1927,10 +1940,10 @@ int __id_ctrl(int argc, char **argv, struct command *cmd, struct plugin *plugin,
>   			printf("NVME Identify Controller:\n");
>   			__show_nvme_id_ctrl(&ctrl, flags, vs);
>   		}
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("identify controller");
>
>    close_fd:
> @@ -2006,10 +2019,10 @@ static int ns_descs(int argc, char **argv, struct command *cmd, struct plugin *p
>   			printf("NVME Namespace Identification Descriptors NS %d:\n", cfg.namespace_id);
>   			show_nvme_id_ns_descs(nsdescs);
>   		}
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("identify namespace");
>
>   	free(nsdescs);
> @@ -2095,10 +2108,10 @@ static int id_ns(int argc, char **argv, struct command *cmd, struct plugin *plug
>   			printf("NVME Identify Namespace %d:\n", cfg.namespace_id);
>   			show_nvme_id_ns(&ns, flags);
>   		}
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("identify namespace");
>
>    close_fd:
> @@ -2153,10 +2166,10 @@ static int id_nvmset(int argc, char **argv, struct command *cmd, struct plugin *
>   			printf("NVME Identify NVM Set List %d:\n", cfg.nvmset_id);
>   			show_nvme_id_nvmset(&nvmset);
>   		}
> -	}
> -	else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		perror("identify nvm set list");
>
>    close_fd:
> @@ -2247,6 +2260,7 @@ static int virtual_mgmt(int argc, char **argv, struct command *cmd, struct plugi
>   	}
>   	else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   	} else
>   		perror("virt-mgmt");
>
> @@ -2374,6 +2388,7 @@ static int device_self_test(int argc, char **argv, struct command *cmd, struct p
>   			printf("Device self-test started\n");
>   	} else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   	} else
>   		perror("Device self-test");
>
> @@ -2427,6 +2442,7 @@ static int self_test_log(int argc, char **argv, struct command *cmd, struct plug
>   		}
>   	} else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   	} else {
>   		perror("self_test_log");
>   	}
> @@ -2556,6 +2572,7 @@ static int get_feature(int argc, char **argv, struct command *cmd, struct plugin
>   			d_raw(buf, cfg.data_len);
>   	} else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   	} else
>   		perror("get-feature");
>
> @@ -2654,6 +2671,7 @@ static int fw_download(int argc, char **argv, struct command *cmd, struct plugin
>   			break;
>   		} else if (err != 0) {
>   			show_nvme_status(err);
> +			err = nvme_status_to_errno(err, false);
>   			break;
>   		}
>   		fw_buf     += cfg.xfer;
> @@ -2750,6 +2768,7 @@ static int fw_commit(int argc, char **argv, struct command *cmd, struct plugin *
>   			break;
>   		default:
>   			show_nvme_status(err);
> +			err = nvme_status_to_errno(err, false);
>   			break;
>   		}
>   	else {
> @@ -2925,8 +2944,10 @@ static int sanitize(int argc, char **argv, struct command *cmd, struct plugin *p
>   			    cfg.no_dealloc, cfg.ovrpat);
>   	if (ret < 0)
>   		perror("sanitize");
> -	else if (ret > 0)
> +	else if (ret > 0) {
>   		show_nvme_status(ret);
> +		ret = nvme_status_to_errno(ret, false);
> +	}
>
>    close_fd:
>   	close(fd);
> @@ -3051,6 +3072,7 @@ static int get_property(int argc, char **argv, struct command *cmd, struct plugi
>   		show_single_property(cfg.offset, value, cfg.human_readable);
>   	} else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   	}
>
>    close_fd:
> @@ -3106,6 +3128,7 @@ static int set_property(int argc, char **argv, struct command *cmd, struct plugi
>   				nvme_register_to_string(cfg.offset), cfg.value);
>   	} else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
>   	}
>
>    close_fd:
> @@ -3201,6 +3224,7 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
>   			else {
>   				fprintf(stderr, "identify failed\n");
>   				show_nvme_status(err);
> +				err = nvme_status_to_errno(err, false);
>   			}
>   			return err;
>   		}
> @@ -3257,9 +3281,10 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
>   				cfg.pil, cfg.ms, cfg.timeout);
>   	if (err < 0)
>   		perror("format");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else {
> +		err = nvme_status_to_errno(err, false);
> +	} else {
>   		printf("Success formatting namespace:%x\n", cfg.namespace_id);
>   		ioctl(fd, BLKRRPART);
>   		if (cfg.reset && S_ISCHR(nvme_stat.st_mode))
> @@ -3378,8 +3403,10 @@ static int set_feature(int argc, char **argv, struct command *cmd, struct plugin
>   			else
>   				d(buf, cfg.data_len, 16, 1);
>   		}
> -	} else if (err > 0)
> +	} else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
> +	}
>
>    close_ffd:
>   	close(ffd);
> @@ -3625,8 +3652,10 @@ static int dir_send(int argc, char **argv, struct command *cmd, struct plugin *p
>   				d_raw(buf, cfg.data_len);
>   		}
>   	}
> -	else if (err > 0)
> +	else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
> +	}
>
>   close_ffd:
>   	close(ffd);
> @@ -3682,9 +3711,10 @@ static int write_uncor(int argc, char **argv, struct command *cmd, struct plugin
>   					cfg.block_count);
>   	if (err < 0)
>   		perror("write uncorrectable");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVME Write Uncorrectable Success\n");
>
>   close_fd:
> @@ -3774,9 +3804,10 @@ static int write_zeroes(int argc, char **argv, struct command *cmd, struct plugi
>   			control, cfg.ref_tag, cfg.app_tag, cfg.app_tag_mask);
>   	if (err < 0)
>   		perror("write-zeroes");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVME Write Zeroes Success\n");
>
>    close_fd:
> @@ -3874,9 +3905,10 @@ static int dsm(int argc, char **argv, struct command *cmd, struct plugin *plugin
>   	err = nvme_dsm(fd, cfg.namespace_id, cfg.cdw11, dsm, nr);
>   	if (err < 0)
>   		perror("data-set management");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVMe DSM: success\n");
>
>    close_fd:
> @@ -3922,9 +3954,10 @@ static int flush(int argc, char **argv, struct command *cmd, struct plugin *plug
>   	err = nvme_flush(fd, cfg.namespace_id);
>   	if (err < 0)
>   		perror("flush");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVMe Flush: success\n");
>   close_fd:
>   	close(fd);
> @@ -3995,9 +4028,10 @@ static int resv_acquire(int argc, char **argv, struct command *cmd, struct plugi
>   				!!cfg.iekey, cfg.crkey, cfg.prkey);
>   	if (err < 0)
>   		perror("reservation acquire");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVME Reservation Acquire success\n");
>
>    close_fd:
> @@ -4072,9 +4106,10 @@ static int resv_register(int argc, char **argv, struct command *cmd, struct plug
>   				!!cfg.iekey, cfg.crkey, cfg.nrkey);
>   	if (err < 0)
>   		perror("reservation register");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVME Reservation  success\n");
>
>    close_fd:
> @@ -4145,9 +4180,10 @@ static int resv_release(int argc, char **argv, struct command *cmd, struct plugi
>   				!!cfg.iekey, cfg.crkey);
>   	if (err < 0)
>   		perror("reservation release");
> -	else if (err != 0)
> +	else if (err != 0) {
>   		show_nvme_status(err);
> -	else
> +		err = nvme_status_to_errno(err, false);
> +	} else
>   		printf("NVME Reservation Release success\n");
>
>    close_fd:
> @@ -4463,9 +4499,10 @@ static int submit_io(int opcode, char *command, const char *desc,
>   			command, elapsed_utime(start_time, end_time));
>   	if (err < 0)
>   		perror("submit-io");
> -	else if (err)
> +	else if (err) {
>   		show_nvme_status(err);
> -	else {
> +		err = nvme_status_to_errno(err, false);
> +	} else {
>   		if (!(opcode & 1) && write(dfd, (void *)buffer, cfg.data_size) < 0) {
>   			fprintf(stderr, "write: %s: failed to write buffer to output file\n",
>   					strerror(errno));
> @@ -4720,8 +4757,10 @@ static int dir_receive(int argc, char **argv, struct command *cmd, struct plugin
>   			}
>   		}
>   	}
> -	else if (err > 0)
> +	else if (err > 0) {
>   		show_nvme_status(err);
> +		err = nvme_status_to_errno(err, false);
> +	}
>   free:
>   	if (cfg.data_len)
>   		free(buf);
> @@ -4904,9 +4943,10 @@ static int passthru(int argc, char **argv, int ioctl_cmd, const char *desc, stru
>   				cfg.timeout, &result);
>   	if (err < 0)
>   		perror("passthru");
> -	else if (err)
> +	else if (err) {
>   		show_nvme_status(err);
> -	else  {
> +		err = nvme_status_to_errno(err, false);
> +	} else  {
>   		if (!cfg.raw_binary) {
>   			fprintf(stderr, "NVMe command result:%08x\n", result);
>   			if (data && cfg.read && !err)
>

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno
  2019-05-13 17:03 ` [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno Minwoo Im
@ 2019-05-19 17:42   ` Chaitanya Kulkarni
  2019-05-19 17:56     ` Minwoo Im
  0 siblings, 1 reply; 13+ messages in thread
From: Chaitanya Kulkarni @ 2019-05-19 17:42 UTC (permalink / raw)


On 05/13/2019 10:04 AM, Minwoo Im wrote:
> Background:
>    It's not enough to return the nvme status value in main() because it's
> allowed to be in 8bits, but nvme status is indicated in 16bits.  So we
> has not been able to figure out what kind of nvme status has been
> returned by return value.
>
>    This patch introduces nvme-status module that manages mapping between
> nvme status and errno.  It's not possible to make 1:1 mapping relations,
> but we can map it as a groups.
>
> All the internal errors which has been returned in a negative value will
> be returned with ECOMM that indicates communication errors.  In this
> case, we can see what happened via stderr.

I didn't understand this as I don't see ECOMM in this patch, also when 
you mentioned internal errors are you referring to the  NVME_SC_INTERNAL 
? in that case in this patch NVME_SC_INTERNAL is mapped to the EIO. Can 
you please explain ?

>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Chaitanya Kulkarni <chaitanya.Kulkarni at wdc.com>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   Makefile      |   3 +-
>   linux/nvme.h  |   6 +++
>   nvme-status.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   nvme-status.h |  14 +++++
>   4 files changed, 169 insertions(+), 1 deletion(-)
>   create mode 100644 nvme-status.c
>   create mode 100644 nvme-status.h
>
> diff --git a/Makefile b/Makefile
> index 4bfbebb..1e50a65 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -32,7 +32,8 @@ override CFLAGS += -DNVME_VERSION='"$(NVME_VERSION)"'
>   NVME_DPKG_VERSION=1~`lsb_release -sc`
>
>   OBJS := argconfig.o suffix.o parser.o nvme-print.o nvme-ioctl.o \
> -	nvme-lightnvm.o fabrics.o json.o nvme-models.o plugin.o
> +	nvme-lightnvm.o fabrics.o json.o nvme-models.o plugin.o \
> +	nvme-status.o
>
>   PLUGIN_OBJS :=					\
>   	plugins/intel/intel-nvme.o		\
> diff --git a/linux/nvme.h b/linux/nvme.h
> index c99b438..5ca7442 100644
> --- a/linux/nvme.h
> +++ b/linux/nvme.h
> @@ -1307,6 +1307,12 @@ static inline bool nvme_is_write(struct nvme_command *cmd)
>   	return cmd->common.opcode & 1;
>   }
>
> +enum {
> +	NVME_SCT_GENERIC		= 0x0,
> +	NVME_SCT_CMD_SPECIFIC		= 0x1,
> +	NVME_SCT_MEDIA			= 0x2,
> +};
> +
>   enum {
>   	/*
>   	 * Generic Command Status:
> diff --git a/nvme-status.c b/nvme-status.c
> new file mode 100644
> index 0000000..3f51ceb
> --- /dev/null
> +++ b/nvme-status.c
> @@ -0,0 +1,147 @@
> +#include <linux/types.h>
> +#include <stdbool.h>
> +#include <errno.h>
> +
> +#include "nvme.h"
> +#include "nvme-status.h"
> +
> +static inline __u8 nvme_generic_status_to_errno(__u16 status)
> +{
> +	switch (status) {
> +	case NVME_SC_INVALID_OPCODE:
> +	case NVME_SC_INVALID_FIELD:
> +	case NVME_SC_INVALID_NS:
> +	case NVME_SC_SGL_INVALID_LAST:
> +	case NVME_SC_SGL_INVALID_COUNT:
> +	case NVME_SC_SGL_INVALID_DATA:
> +	case NVME_SC_SGL_INVALID_METADATA:
> +	case NVME_SC_SGL_INVALID_TYPE:
> +	case NVME_SC_SGL_INVALID_OFFSET:
> +	case NVME_SC_SGL_INVALID_SUBTYPE:
> +		return EINVAL;
> +	case NVME_SC_CMDID_CONFLICT:
> +		return EADDRINUSE;
> +	case NVME_SC_DATA_XFER_ERROR:
> +	case NVME_SC_INTERNAL:
> +	case NVME_SC_SANITIZE_FAILED:
> +		return EIO;
> +	case NVME_SC_POWER_LOSS:
> +	case NVME_SC_ABORT_REQ:
> +	case NVME_SC_ABORT_QUEUE:
> +	case NVME_SC_FUSED_FAIL:
> +	case NVME_SC_FUSED_MISSING:
> +		return EWOULDBLOCK;
> +	case NVME_SC_CMD_SEQ_ERROR:
> +		return EILSEQ;
> +	case NVME_SC_SANITIZE_IN_PROGRESS:
> +		return EINPROGRESS;
> +	case NVME_SC_NS_WRITE_PROTECTED:
> +	case NVME_SC_NS_NOT_READY:
> +	case NVME_SC_RESERVATION_CONFLICT:
> +		return EACCES;
> +	case NVME_SC_LBA_RANGE:
> +		return EREMOTEIO;
> +	case NVME_SC_CAP_EXCEEDED:
> +		return ENOSPC;

nit:- Compiler/static code analysis tools might complain or generate a 
warning for not having default case for switch. Let's avoid that by 
declaring a variable ret and initializing it at each case and default 
(ret = EIO), then use only one return at the end of the function,
> +	}
> +
> +	return EIO;
> +}
> +
> +static inline __u8 nvme_cmd_specific_status_to_errno(__u16 status)
> +{
> +	switch (status) {
> +	case NVME_SC_CQ_INVALID:
> +	case NVME_SC_QID_INVALID:
> +	case NVME_SC_QUEUE_SIZE:
> +	case NVME_SC_FIRMWARE_SLOT:
> +	case NVME_SC_FIRMWARE_IMAGE:
> +	case NVME_SC_INVALID_VECTOR:
> +	case NVME_SC_INVALID_LOG_PAGE:
> +	case NVME_SC_INVALID_FORMAT:
> +	case NVME_SC_INVALID_QUEUE:
> +	case NVME_SC_NS_INSUFFICIENT_CAP:
> +	case NVME_SC_NS_ID_UNAVAILABLE:
> +	case NVME_SC_CTRL_LIST_INVALID:
> +	case NVME_SC_BAD_ATTRIBUTES:
> +	case NVME_SC_INVALID_PI:
> +		return EINVAL;
> +	case NVME_SC_ABORT_LIMIT:
> +	case NVME_SC_ASYNC_LIMIT:
> +		return EDQUOT;
> +	case NVME_SC_FW_NEEDS_CONV_RESET:
> +	case NVME_SC_FW_NEEDS_SUBSYS_RESET:
> +	case NVME_SC_FW_NEEDS_MAX_TIME:
> +		return ERESTART;
> +	case NVME_SC_FEATURE_NOT_SAVEABLE:
> +	case NVME_SC_FEATURE_NOT_CHANGEABLE:
> +	case NVME_SC_FEATURE_NOT_PER_NS:
> +	case NVME_SC_FW_ACTIVATE_PROHIBITED:
> +	case NVME_SC_NS_IS_PRIVATE:
> +	case NVME_SC_BP_WRITE_PROHIBITED:
> +	case NVME_SC_READ_ONLY:
> +		return EPERM;
> +	case NVME_SC_OVERLAPPING_RANGE:
> +	case NVME_SC_NS_NOT_ATTACHED:
> +		return ENOSPC;
> +	case NVME_SC_NS_ALREADY_ATTACHED:
> +		return EALREADY;
> +	case NVME_SC_THIN_PROV_NOT_SUPP:
> +	case NVME_SC_ONCS_NOT_SUPPORTED:
> +		return EOPNOTSUPP;
nit:- same here as mentioned.
> +	}
> +
> +	return EIO;
> +}
> +
> +static inline __u8 nvme_fabrics_status_to_errno(__u16 status)
> +{
> +	switch (status) {
> +	case NVME_SC_CONNECT_FORMAT:
> +	case NVME_SC_CONNECT_INVALID_PARAM:
> +		return EINVAL;
> +	case NVME_SC_CONNECT_CTRL_BUSY:
> +		return EBUSY;
> +	case NVME_SC_CONNECT_RESTART_DISC:
> +		return ERESTART;
> +	case NVME_SC_CONNECT_INVALID_HOST:
> +		return ECONNREFUSED;
> +	case NVME_SC_DISCOVERY_RESTART:
> +		return EAGAIN;
> +	case NVME_SC_AUTH_REQUIRED:
> +		return EPERM;
nit:- same here as mentioned.
> +	}
> +
> +	return EIO;
> +}
> +
> +/*
> + * nvme_status_to_errno - It converts given status to errno mapped
> + * @status: nvme status field in completion queue entry
> + * @fabrics: true if given status is for fabrics
> + *
> + * Notes: This function can handle generic command status only.
> + */
> +__u8 nvme_status_to_errno(__u16 status, bool fabrics)
> +{
> +	__u8 sct = nvme_status_type(status);
> +
> +	/*
> +	 * The actual status code is enough with masking 0xff, but we need to
> +	 * cover status code type which is 3bits with 0x7ff.
> +	 */
> +	status &= 0x7ff;
> +
> +	if (sct == NVME_SCT_GENERIC)
> +		return nvme_generic_status_to_errno(status);
> +	else if (sct == NVME_SCT_CMD_SPECIFIC && !fabrics)
> +		return nvme_cmd_specific_status_to_errno(status);
> +	else if (sct == NVME_SCT_CMD_SPECIFIC && fabrics)
> +		return nvme_fabrics_status_to_errno(status);
> +
> +	/*
> +	 * Media, integrity related status, and the others will be mapped to
> +	 * EIO.
> +	 */
> +	return EIO;
> +}
> diff --git a/nvme-status.h b/nvme-status.h
> new file mode 100644
> index 0000000..094132b
> --- /dev/null
> +++ b/nvme-status.h
> @@ -0,0 +1,14 @@
> +#include <linux/types.h>
> +#include <stdbool.h>
> +
> +/*
> + * nvme_status_type - It gives SCT(Status Code Type) in status field in
> + *                    completion queue entry.
> + * @status: status field located at DW3 in completion queue entry
> + */
> +static inline __u8 nvme_status_type(__u16 status)
> +{
> +	return (status & 0x700) >> 8;
> +}
> +
> +__u8 nvme_status_to_errno(__u16 status, bool fabrics);
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 3/4] nvme: Return errno mapped for nvme error status
  2019-05-19 17:41   ` Chaitanya Kulkarni
@ 2019-05-19 17:53     ` Minwoo Im
  2019-05-19 18:00       ` Chaitanya Kulkarni
  0 siblings, 1 reply; 13+ messages in thread
From: Minwoo Im @ 2019-05-19 17:53 UTC (permalink / raw)


> > @@ -364,6 +367,7 @@ static int get_telemetry_log(int argc, char **argv, struct command *cmd, struct
> >   		perror("get-telemetry-log");
> >   	else if (err > 0) {
> >   		show_nvme_status(err);
> > +		err = nvme_status_to_errno(err, false);
> >   		fprintf(stderr, "Failed to acquire telemetry header %d!\n", err);
> 
> Following line to nvme_status_to_errno() call above assumes that err has 
> a return value from nvme_get_telemetry_log() and we are overwriting it.
> 
> We need to avoid such scenarios going forward. Also, since each command 

Can you please explain why we need to avoid such overwriting scenario?

> assumes that err holds the NVMe status. Instead of having to call 
> nvme_status_errno() along with nvme_show_status() we should call 
> nvme_status_to_errno(err) at the end of the function in the return 
> statement. This approach will not break the assumption that code is 
> having now and less lines of code changes, obviously 
> nvme_status_to_errno() will need some modifications but that
> is fine, untested patch following  :-

Makes sense.  If we are going to convert the positive nvme status and
negative linux internal errno status to an errno inside of
nvme_st4atus_to_errno(), it will much more simpler for this patch.

Anyway, I will prepare V3 patch by updating the conversion at the end of
the each subcommandh handlers.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno
  2019-05-19 17:42   ` Chaitanya Kulkarni
@ 2019-05-19 17:56     ` Minwoo Im
  0 siblings, 0 replies; 13+ messages in thread
From: Minwoo Im @ 2019-05-19 17:56 UTC (permalink / raw)


On 19-05-19 17:42:19, Chaitanya Kulkarni wrote:
> On 05/13/2019 10:04 AM, Minwoo Im wrote:
> > Background:
> >    It's not enough to return the nvme status value in main() because it's
> > allowed to be in 8bits, but nvme status is indicated in 16bits.  So we
> > has not been able to figure out what kind of nvme status has been
> > returned by return value.
> >
> >    This patch introduces nvme-status module that manages mapping between
> > nvme status and errno.  It's not possible to make 1:1 mapping relations,
> > but we can map it as a groups.
> >
> > All the internal errors which has been returned in a negative value will
> > be returned with ECOMM that indicates communication errors.  In this
> > case, we can see what happened via stderr.
> 
> I didn't understand this as I don't see ECOMM in this patch, also when 
> you mentioned internal errors are you referring to the  NVME_SC_INTERNAL 
> ? in that case in this patch NVME_SC_INTERNAL is mapped to the EIO. Can 
> you please explain ?

Nice catch, Thanks, really.  I didn't make ECOMM errno included to this
patchset.  NVME_SC_INTERNAL will be mapped to EIO to make it
distinguished from ECOMM which indicates the internal linux errno.  All
the error status from nvme except cases defined in the following
switch-case will be mapped to EIO.

Will make this patch return ECOMM in case of internal linux error which
will be given in a negative value with the previos commit.

Thanks,

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 3/4] nvme: Return errno mapped for nvme error status
  2019-05-19 17:53     ` Minwoo Im
@ 2019-05-19 18:00       ` Chaitanya Kulkarni
  2019-05-19 19:03         ` Minwoo Im
  0 siblings, 1 reply; 13+ messages in thread
From: Chaitanya Kulkarni @ 2019-05-19 18:00 UTC (permalink / raw)


On 05/19/2019 10:53 AM, Minwoo Im wrote:
>>> @@ -364,6 +367,7 @@ static int get_telemetry_log(int argc, char **argv, struct command *cmd, struct
>>>    		perror("get-telemetry-log");
>>>    	else if (err > 0) {
>>>    		show_nvme_status(err);
>>> +		err = nvme_status_to_errno(err, false);
>>>    		fprintf(stderr, "Failed to acquire telemetry header %d!\n", err);
>>
>> Following line to nvme_status_to_errno() call above assumes that err has
>> a return value from nvme_get_telemetry_log() and we are overwriting it.
>>
>> We need to avoid such scenarios going forward. Also, since each command
>
> Can you please explain why we need to avoid such overwriting scenario?

Printing NVMe status with err by retaining existing behavior.

Since existing code might have assumption of err holding the nvme 
status. Also since we are changing the return code functionality so
it shouldn't affect the rest of the code pattern.

>
>> assumes that err holds the NVMe status. Instead of having to call
>> nvme_status_errno() along with nvme_show_status() we should call
>> nvme_status_to_errno(err) at the end of the function in the return
>> statement. This approach will not break the assumption that code is
>> having now and less lines of code changes, obviously
>> nvme_status_to_errno() will need some modifications but that
>> is fine, untested patch following  :-
>
> Makes sense.  If we are going to convert the positive nvme status and
> negative linux internal errno status to an errno inside of
> nvme_st4atus_to_errno(), it will much more simpler for this patch.
>
> Anyway, I will prepare V3 patch by updating the conversion at the end of
> the each subcommandh handlers.
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH V2 3/4] nvme: Return errno mapped for nvme error status
  2019-05-19 18:00       ` Chaitanya Kulkarni
@ 2019-05-19 19:03         ` Minwoo Im
  0 siblings, 0 replies; 13+ messages in thread
From: Minwoo Im @ 2019-05-19 19:03 UTC (permalink / raw)


> Printing NVMe status with err by retaining existing behavior.
> 
> Since existing code might have assumption of err holding the nvme 
> status. Also since we are changing the return code functionality so
> it shouldn't affect the rest of the code pattern.

Thanks for the review.  Please review V3 that has been updated.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-05-19 19:03 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 17:03 [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im
2019-05-13 17:03 ` [PATCH V2 1/4] nvme.h: Fix typos in status code values Minwoo Im
2019-05-19 16:48   ` Chaitanya Kulkarni
2019-05-13 17:03 ` [PATCH V2 2/4] nvme-status: Introduce nvme status module to map errno Minwoo Im
2019-05-19 17:42   ` Chaitanya Kulkarni
2019-05-19 17:56     ` Minwoo Im
2019-05-13 17:03 ` [PATCH V2 3/4] nvme: Return errno mapped for nvme error status Minwoo Im
2019-05-19 17:41   ` Chaitanya Kulkarni
2019-05-19 17:53     ` Minwoo Im
2019-05-19 18:00       ` Chaitanya Kulkarni
2019-05-19 19:03         ` Minwoo Im
2019-05-13 17:03 ` [PATCH V2 4/4] fabrics: Return errno mapped for fabrics " Minwoo Im
2019-05-18  2:11 ` [PATCH V2 0/4] nvme-cli: Introduce nvme-status mapping with errno Minwoo Im

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.