[ndctl PATCH v3 0/4] Add missing firmware_status checks

[ndctl PATCH v3 0/4] Add missing firmware_status checks

[Vishal Verma]

Changes in v3:
- Patch 2: Also fix open coded get_firmware_status (Dan)
- Patch 4: Change rc to an int as it is only used for the return status
  of cmd_submit.
- Patch 4: Fix another open coded get_firmware_status in
  test/ack-shutdown-count-set.c

Changes in v2:
- For the new helper, return original positive rc when we can (Dan)
- Convert previously missed locations to the new submission helper
  (patch 2)
- Add a new patch (4/4) to fix up all callers of ndctl_cmd_submit to
  the correct return convention.

There were several places in ndctl where we neglected to check the
firmware_status from a command submission.

Add an ndctl_dimm_op target to perform error translation for a DSM
family, and provide a new helper - ndctl_cmd_submit_xlat - that will
submit the command and call an error translation routine if one has been
registered. Switch the call sites in ndctl/inject-smart.c and
ndctl/monitor.c over to the new command submission helper.

Finally, clean up all remaining callers of ndctl_cmd_submit to use the
correct return convention, i.e. only treat negative returns as an error,
and accept positive return codes as OK.

Vishal Verma (4):
  libndctl, intel: Add infrastructure for firmware_status translation
  ndctl, inject-smart: switch to ndctl_cmd_submit_xlat
  ndctl, monitor: switch to ndctl_cmd_submit_xlat
  ndctl: clean up usage of ndctl_cmd_submit

 ndctl/inject-smart.c          | 16 ++++++------
 ndctl/lib/dimm.c              |  6 ++---
 ndctl/lib/inject.c            |  8 +++---
 ndctl/lib/intel.c             | 49 +++++++++++++++++++++++++++++++++++
 ndctl/lib/intel.h             |  1 +
 ndctl/lib/libndctl.c          | 28 ++++++++++++++++++++
 ndctl/lib/libndctl.sym        |  6 +++++
 ndctl/lib/nfit.c              |  2 +-
 ndctl/lib/private.h           |  1 +
 ndctl/libndctl.h              |  3 +++
 ndctl/monitor.c               |  6 ++---
 ndctl/util/json-firmware.c    |  2 +-
 ndctl/util/json-smart.c       |  8 +++---
 test/ack-shutdown-count-set.c |  8 ++----
 test/daxdev-errors.c          |  8 +++---
 test/libndctl.c               | 32 +++++++++++------------
 test/smart-notify.c           |  8 +++---
 17 files changed, 138 insertions(+), 54 deletions(-)

-- 
2.17.2

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v3 1/4] libndctl, intel: Add infrastructure for firmware_status translation

[Vishal Verma]

Add a new routine to ndctl_dimm_ops that allows a DSM family to provide
a translation routine that will translate the status codes of the result
of a DSM to generic errno style error codes. To use this routine
effectively, add a new wrapper around ndctl_cmd_submit (called
ndctl_cmd_submit_xlat) that submits the command, and also runs it
through the above translator dimm_op (if one is is defined).

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/intel.c      | 49 ++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/intel.h      |  1 +
 ndctl/lib/libndctl.c   | 28 ++++++++++++++++++++++++
 ndctl/lib/libndctl.sym |  6 ++++++
 ndctl/lib/private.h    |  1 +
 ndctl/libndctl.h       |  3 +++
 6 files changed, 88 insertions(+)

diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
index 744386f..d684bac 100644
--- a/ndctl/lib/intel.c
+++ b/ndctl/lib/intel.c
@@ -16,6 +16,54 @@
 #include <ndctl/libndctl.h>
 #include "private.h"
 
+static int intel_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
+{
+	struct nd_pkg_intel *pkg = cmd->intel;
+	unsigned int status, ext_status;
+
+	status = (*cmd->firmware_status) & ND_INTEL_STATUS_MASK;
+	ext_status = (*cmd->firmware_status) & ND_INTEL_STATUS_EXTEND_MASK;
+
+	/* Common statuses */
+	switch (status) {
+	case ND_INTEL_STATUS_SUCCESS:
+		return 0;
+	case ND_INTEL_STATUS_NOTSUPP:
+		return -EOPNOTSUPP;
+	case ND_INTEL_STATUS_NOTEXIST:
+		return -ENXIO;
+	case ND_INTEL_STATUS_INVALPARM:
+		return -EINVAL;
+	case ND_INTEL_STATUS_HWERR:
+		return -EIO;
+	case ND_INTEL_STATUS_RETRY:
+		return -EAGAIN;
+	case ND_INTEL_STATUS_EXTEND:
+		/* refer to extended status, break out of this */
+		break;
+	case ND_INTEL_STATUS_NORES:
+		return -EAGAIN;
+	case ND_INTEL_STATUS_NOTREADY:
+		return -EBUSY;
+	}
+
+	/* Extended status is command specific */
+	switch (pkg->gen.nd_command) {
+	case ND_INTEL_SMART:
+	case ND_INTEL_SMART_THRESHOLD:
+	case ND_INTEL_SMART_SET_THRESHOLD:
+		/* ext status not specified */
+		break;
+	case ND_INTEL_SMART_INJECT:
+		/* smart injection not enabled */
+		if (ext_status == ND_INTEL_STATUS_INJ_DISABLED)
+			return -ENXIO;
+		break;
+	}
+
+	return -ENOMSG;
+}
+
 static struct ndctl_cmd *alloc_intel_cmd(struct ndctl_dimm *dimm,
 		unsigned func, size_t in_size, size_t out_size)
 {
@@ -780,4 +828,5 @@ struct ndctl_dimm_ops * const intel_dimm_ops = &(struct ndctl_dimm_ops) {
 	.fw_xlat_firmware_status = intel_cmd_fw_xlat_firmware_status,
 	.new_ack_shutdown_count = intel_dimm_cmd_new_lss,
 	.fw_update_supported = intel_dimm_fw_update_supported,
+	.xlat_firmware_status = intel_cmd_xlat_firmware_status,
 };
diff --git a/ndctl/lib/intel.h b/ndctl/lib/intel.h
index 3b01bba..530c996 100644
--- a/ndctl/lib/intel.h
+++ b/ndctl/lib/intel.h
@@ -179,5 +179,6 @@ struct nd_pkg_intel {
 #define ND_INTEL_STATUS_FQ_BUSY		0x20000
 #define ND_INTEL_STATUS_FQ_BAD		0x30000
 #define ND_INTEL_STATUS_FQ_ORDER	0x40000
+#define ND_INTEL_STATUS_INJ_DISABLED	0x10000
 
 #endif /* __INTEL_H__ */
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index e82a08d..830b791 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2704,6 +2704,16 @@ static const char *ndctl_dimm_get_cmd_subname(struct ndctl_cmd *cmd)
 	return ops->cmd_desc(cmd->pkg->nd_command);
 }
 
+NDCTL_EXPORT int ndctl_cmd_xlat_firmware_status(struct ndctl_cmd *cmd)
+{
+	struct ndctl_dimm *dimm = cmd->dimm;
+	struct ndctl_dimm_ops *ops = dimm ? dimm->ops : NULL;
+
+	if (!dimm || !ops || !ops->xlat_firmware_status)
+		return -ENOMSG;
+	return ops->xlat_firmware_status(cmd);
+}
+
 static int do_cmd(int fd, int ioctl_cmd, struct ndctl_cmd *cmd)
 {
 	int rc;
@@ -2819,6 +2829,24 @@ NDCTL_EXPORT int ndctl_cmd_submit(struct ndctl_cmd *cmd)
 	return rc;
 }
 
+NDCTL_EXPORT int ndctl_cmd_submit_xlat(struct ndctl_cmd *cmd)
+{
+	int rc, xlat_rc;
+
+	rc = ndctl_cmd_submit(cmd);
+	if (rc < 0)
+		return rc;
+
+	/*
+	 * NOTE: This can lose a positive rc when xlat_rc is non-zero. The
+	 * positive rc indicates a buffer underrun from the original command
+	 * submission. If the caller cares about that (generally not very
+	 * useful), then the xlat function is available separately as well.
+	 */
+	xlat_rc = ndctl_cmd_xlat_firmware_status(cmd);
+	return (xlat_rc == 0) ? rc : xlat_rc;
+}
+
 NDCTL_EXPORT int ndctl_cmd_get_status(struct ndctl_cmd *cmd)
 {
 	return cmd->status;
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 6c4c8b4..275db92 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -385,3 +385,9 @@ global:
 	ndctl_namespace_get_next_badblock;
 	ndctl_dimm_get_dirty_shutdown;
 } LIBNDCTL_17;
+
+LIBNDCTL_19 {
+global:
+	ndctl_cmd_xlat_firmware_status;
+	ndctl_cmd_submit_xlat;
+} LIBNDCTL_18;
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index 4fbea56..a387b0b 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -338,6 +338,7 @@ struct ndctl_dimm_ops {
 	enum ND_FW_STATUS (*fw_xlat_firmware_status)(struct ndctl_cmd *);
 	struct ndctl_cmd *(*new_ack_shutdown_count)(struct ndctl_dimm *);
 	int (*fw_update_supported)(struct ndctl_dimm *);
+	int (*xlat_firmware_status)(struct ndctl_cmd *);
 };
 
 struct ndctl_dimm_ops * const intel_dimm_ops;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index c81cc03..e55a593 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -681,6 +681,9 @@ enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd);
 struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm);
 int ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
 
+int ndctl_cmd_xlat_firmware_status(struct ndctl_cmd *cmd);
+int ndctl_cmd_submit_xlat(struct ndctl_cmd *cmd);
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
-- 
2.17.2

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v3 2/4] ndctl, inject-smart: switch to ndctl_cmd_submit_xlat

[Vishal Verma]

The ndctl inject-smart command was neglecting to check the
'firmware_status' field that is set by the platform firmware to indicate
failure. Use the new ndctl_cmd_submit_xlat facility to include the
firmware_status check as part of the command submission.

Reported-by: Ami Pathak <ami.g.pathak@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/inject-smart.c    | 16 ++++++++--------
 ndctl/util/json-smart.c |  8 ++++----
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/ndctl/inject-smart.c b/ndctl/inject-smart.c
index eaa137a..00c81b8 100644
--- a/ndctl/inject-smart.c
+++ b/ndctl/inject-smart.c
@@ -280,8 +280,8 @@ static int smart_set_thresh(struct ndctl_dimm *dimm)
 		goto out;
 	}
 
-	rc = ndctl_cmd_submit(st_cmd);
-	if (rc) {
+	rc = ndctl_cmd_submit_xlat(st_cmd);
+	if (rc < 0) {
 		error("%s: smart threshold command failed: %s (%d)\n",
 			name, strerror(abs(rc)), rc);
 		goto out;
@@ -320,8 +320,8 @@ static int smart_set_thresh(struct ndctl_dimm *dimm)
 		ndctl_cmd_smart_threshold_set_alarm_control(sst_cmd, alarm);
 	}
 
-	rc = ndctl_cmd_submit(sst_cmd);
-	if (rc)
+	rc = ndctl_cmd_submit_xlat(sst_cmd);
+	if (rc < 0)
 		error("%s: smart set threshold command failed: %s (%d)\n",
 			name, strerror(abs(rc)), rc);
 
@@ -351,8 +351,8 @@ out:
 			if (sctx.err_continue == false) \
 				goto out; \
 		} \
-		rc = ndctl_cmd_submit(si_cmd); \
-		if (rc) { \
+		rc = ndctl_cmd_submit_xlat(si_cmd); \
+		if (rc < 0) { \
 			error("%s: smart inject %s command failed: %s (%d)\n", \
 				name, #arg, strerror(abs(rc)), rc); \
 			if (sctx.err_continue == false) \
@@ -382,8 +382,8 @@ out:
 			if (sctx.err_continue == false) \
 				goto out; \
 		} \
-		rc = ndctl_cmd_submit(si_cmd); \
-		if (rc) { \
+		rc = ndctl_cmd_submit_xlat(si_cmd); \
+		if (rc < 0) { \
 			error("%s: smart inject %s command failed: %s (%d)\n", \
 				name, #arg, strerror(abs(rc)), rc); \
 			if (sctx.err_continue == false) \
diff --git a/ndctl/util/json-smart.c b/ndctl/util/json-smart.c
index 3c1b917..a9bd17b 100644
--- a/ndctl/util/json-smart.c
+++ b/ndctl/util/json-smart.c
@@ -30,8 +30,8 @@ static void smart_threshold_to_json(struct ndctl_dimm *dimm,
 	if (!cmd)
 		return;
 
-	rc = ndctl_cmd_submit(cmd);
-	if (rc || ndctl_cmd_get_firmware_status(cmd))
+	rc = ndctl_cmd_submit_xlat(cmd);
+	if (rc < 0)
 		goto out;
 
 	alarm_control = ndctl_cmd_smart_threshold_get_alarm_control(cmd);
@@ -115,8 +115,8 @@ struct json_object *util_dimm_health_to_json(struct ndctl_dimm *dimm)
 	if (!cmd)
 		goto err;
 
-	rc = ndctl_cmd_submit(cmd);
-	if (rc || ndctl_cmd_get_firmware_status(cmd)) {
+	rc = ndctl_cmd_submit_xlat(cmd);
+	if (rc < 0) {
 		jobj = json_object_new_string("unknown");
 		if (jobj)
 			json_object_object_add(jhealth, "health_state", jobj);
-- 
2.17.2

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v3 3/4] ndctl, monitor: switch to ndctl_cmd_submit_xlat

[Vishal Verma]

The ndctl monitor command was neglecting to check the 'firmware_status'
field that is set by the platform firmware to indicate failure. Use
the new ndctl_cmd_submit_xlat facility to include the firmware_status
check as part of the command submission.

Cc: QI Fuli <qi.fuli@jp.fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/monitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index 233f2bb..4ad1429 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -226,7 +226,7 @@ static int enable_dimm_supported_threshold_alarms(struct ndctl_dimm *dimm)
 		err(&monitor, "%s: no smart threshold command support\n", name);
 		goto out;
 	}
-	if (ndctl_cmd_submit(st_cmd)) {
+	if (ndctl_cmd_submit_xlat(st_cmd) < 0) {
 		err(&monitor, "%s: smart threshold command failed\n", name);
 		goto out;
 	}
@@ -246,8 +246,8 @@ static int enable_dimm_supported_threshold_alarms(struct ndctl_dimm *dimm)
 		alarm |= ND_SMART_CTEMP_TRIP;
 	ndctl_cmd_smart_threshold_set_alarm_control(sst_cmd, alarm);
 
-	rc = ndctl_cmd_submit(sst_cmd);
-	if (rc) {
+	rc = ndctl_cmd_submit_xlat(sst_cmd);
+	if (rc < 0) {
 		err(&monitor, "%s: smart set threshold command failed\n", name);
 		goto out;
 	}
-- 
2.17.2

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit

[Vishal Verma]

It is possible for ndctl_cmd_submit to return a positive number,
indicating a buffer underrun. It is only truly an error if it returns a
negative number. Several places in the library, the ndctl utility, and
in test/ were simply checking for an error with "if (rc)". Fix these to
only error out for negative returns.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/dimm.c              |  6 +++---
 ndctl/lib/inject.c            |  8 ++++----
 ndctl/lib/nfit.c              |  2 +-
 ndctl/util/json-firmware.c    |  2 +-
 test/ack-shutdown-count-set.c |  8 ++------
 test/daxdev-errors.c          |  8 ++++----
 test/libndctl.c               | 32 ++++++++++++++++----------------
 test/smart-notify.c           |  8 ++++----
 8 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 79e2ca0..12dc74a 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct nvdimm_data *ndd, size_t offset,
 		goto out;
 
 	rc = ndctl_cmd_submit(cmd_write);
-	if (rc || ndctl_cmd_get_firmware_status(cmd_write))
+	if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write))
 		rc = -ENXIO;
  out:
 	ndctl_cmd_unref(cmd_write);
@@ -488,14 +488,14 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
         if (!cmd_size)
                 return NULL;
         rc = ndctl_cmd_submit(cmd_size);
-        if (rc || ndctl_cmd_get_firmware_status(cmd_size))
+        if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_size))
                 goto out_size;
 
         cmd_read = ndctl_dimm_cmd_new_cfg_read(cmd_size);
         if (!cmd_read)
                 goto out_size;
         rc = ndctl_cmd_submit(cmd_read);
-        if (rc || ndctl_cmd_get_firmware_status(cmd_read))
+        if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_read))
                 goto out_read;
 	ndctl_cmd_unref(cmd_size);
 
diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
index 2b0702e..c35d0f3 100644
--- a/ndctl/lib/inject.c
+++ b/ndctl/lib/inject.c
@@ -156,7 +156,7 @@ static int ndctl_namespace_inject_one_error(struct ndctl_namespace *ndns,
 			(1 << ND_ARS_ERR_INJ_OPT_NOTIFY);
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		dbg(ctx, "Error submitting command: %d\n", rc);
 		goto out;
 	}
@@ -234,7 +234,7 @@ static int ndctl_namespace_uninject_one_error(struct ndctl_namespace *ndns,
 	err_inj_clr->err_inj_clr_spa_range_length = length;
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		dbg(ctx, "Error submitting command: %d\n", rc);
 		goto out;
 	}
@@ -443,7 +443,7 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
 
 		cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
 		rc = ndctl_cmd_submit(cmd);
-		if (rc) {
+		if (rc < 0) {
 			dbg(ctx, "Error submitting ars_cap: %d\n", rc);
 			goto out;
 		}
@@ -464,7 +464,7 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
 			(struct nd_cmd_ars_err_inj_stat *)&pkg->nd_payload[0];
 
 		rc = ndctl_cmd_submit(cmd);
-		if (rc) {
+		if (rc < 0) {
 			dbg(ctx, "Error submitting command: %d\n", rc);
 			goto out;
 		}
diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
index 2ae3f07..b10edb1 100644
--- a/ndctl/lib/nfit.c
+++ b/ndctl/lib/nfit.c
@@ -133,7 +133,7 @@ int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
 	translate_spa->spa = address;
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
diff --git a/ndctl/util/json-firmware.c b/ndctl/util/json-firmware.c
index 118424f..f7150d8 100644
--- a/ndctl/util/json-firmware.c
+++ b/ndctl/util/json-firmware.c
@@ -25,7 +25,7 @@ struct json_object *util_dimm_firmware_to_json(struct ndctl_dimm *dimm,
 		goto err;
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
+	if ((rc < 0) || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
 		jobj = util_json_object_hex(-1, flags);
 		if (jobj)
 			json_object_object_add(jfirmware, "current_version",
diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
index 9baa2f1..742e976 100644
--- a/test/ack-shutdown-count-set.c
+++ b/test/ack-shutdown-count-set.c
@@ -27,12 +27,8 @@ static int test_dimm(struct ndctl_dimm *dimm)
 	if (!cmd)
 		return -ENOMEM;
 
-	rc = ndctl_cmd_submit(cmd);
-	if (rc < 0)
-		goto out;
-
-	rc = ndctl_cmd_get_firmware_status(cmd);
-	if (rc != 0) {
+	rc = ndctl_cmd_submit_xlat(cmd);
+	if (rc < 0) {
 		fprintf(stderr, "dimm %s LSS enable set failed\n",
 				ndctl_dimm_get_devname(dimm));
 		goto out;
diff --git a/test/daxdev-errors.c b/test/daxdev-errors.c
index 94fbebe..29de16b 100644
--- a/test/daxdev-errors.c
+++ b/test/daxdev-errors.c
@@ -75,7 +75,7 @@ static int check_ars_cap(struct ndctl_bus *bus, uint64_t start,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(cmd);
@@ -115,7 +115,7 @@ static int check_ars_start(struct ndctl_bus *bus, struct check_cmd *check)
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(cmd);
@@ -149,7 +149,7 @@ static int check_ars_status(struct ndctl_bus *bus, struct check_cmd *check)
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(cmd);
@@ -210,7 +210,7 @@ static int check_clear_error(struct ndctl_bus *bus, struct check_cmd *check)
 	}
 
 	rc = ndctl_cmd_submit(clear_err);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(clear_err);
diff --git a/test/libndctl.c b/test/libndctl.c
index 50365f0..02bb9cc 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -2057,7 +2057,7 @@ static int check_get_config_size(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2091,7 +2091,7 @@ static int check_get_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %zd\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2116,7 +2116,7 @@ static int check_set_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	struct ndctl_cmd *cmd_read = check_cmds[ND_CMD_GET_CONFIG_DATA].cmd;
 	struct ndctl_cmd *cmd = ndctl_dimm_cmd_new_cfg_write(cmd_read);
 	char buf[20], result[sizeof(buf)];
-	size_t rc;
+	int rc;
 
 	if (!cmd) {
 		fprintf(stderr, "%s: dimm: %#x failed to create cmd\n",
@@ -2127,23 +2127,23 @@ static int check_set_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	memset(buf, 0, sizeof(buf));
 	ndctl_cmd_cfg_write_set_data(cmd, buf, sizeof(buf), 0);
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 
 	rc = ndctl_cmd_submit(cmd_read);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit read1: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit read1: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 	ndctl_cmd_cfg_read_get_data(cmd_read, result, sizeof(result), 0);
 	if (memcmp(result, buf, sizeof(result)) != 0) {
-		fprintf(stderr, "%s: dimm: %#x read1 data miscompare: %zd\n",
+		fprintf(stderr, "%s: dimm: %#x read1 data miscompare: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return -ENXIO;
@@ -2152,23 +2152,23 @@ static int check_set_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	sprintf(buf, "dimm-%#x", ndctl_dimm_get_handle(dimm));
 	ndctl_cmd_cfg_write_set_data(cmd, buf, sizeof(buf), 0);
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 
 	rc = ndctl_cmd_submit(cmd_read);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit read2: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit read2: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 	ndctl_cmd_cfg_read_get_data(cmd_read, result, sizeof(result), 0);
 	if (memcmp(result, buf, sizeof(result)) != 0) {
-		fprintf(stderr, "%s: dimm: %#x read2 data miscompare: %zd\n",
+		fprintf(stderr, "%s: dimm: %#x read2 data miscompare: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
@@ -2225,7 +2225,7 @@ static int check_smart(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2326,7 +2326,7 @@ static int check_smart_threshold(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2375,7 +2375,7 @@ static int check_smart_threshold(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 		}
 
 		rc = ndctl_cmd_submit(cmd_set);
-		if (rc) {
+		if (rc < 0) {
 			fprintf(stderr, "%s: dimm: %#x failed to submit cmd_set: %d\n",
 					__func__, ndctl_dimm_get_handle(dimm), rc);
 			ndctl_cmd_unref(cmd_set);
diff --git a/test/smart-notify.c b/test/smart-notify.c
index 24e9a21..e28e0f4 100644
--- a/test/smart-notify.c
+++ b/test/smart-notify.c
@@ -22,7 +22,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 	}
 
 	rc = ndctl_cmd_submit(s_cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: smart command failed: %d %s\n", name,
 				rc, strerror(errno));
 		goto out;
@@ -49,7 +49,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 	}
 
 	rc = ndctl_cmd_submit(st_cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: smart threshold command failed: %d %s\n",
 				name, rc, strerror(errno));
 		goto out;
@@ -148,7 +148,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 
 		ndctl_cmd_smart_threshold_set_alarm_control(sst_cmd, set_alarm);
 		rc = ndctl_cmd_submit(sst_cmd);
-		if (rc) {
+		if (rc < 0) {
 			fprintf(stderr, "%s: smart set threshold command failed: %d %s\n",
 					name, rc, strerror(errno));
 			goto out;
@@ -166,7 +166,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 	}
 
 	rc = ndctl_cmd_submit(sst_cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: smart set threshold defaults failed: %d %s\n",
 				name, rc, strerror(errno));
 		goto out;
-- 
2.17.2

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit

[Dan Williams]

On Mon, Jan 14, 2019 at 10:11 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> It is possible for ndctl_cmd_submit to return a positive number,
> indicating a buffer underrun. It is only truly an error if it returns a
> negative number. Several places in the library, the ndctl utility, and
> in test/ were simply checking for an error with "if (rc)". Fix these to
> only error out for negative returns.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/lib/dimm.c              |  6 +++---
>  ndctl/lib/inject.c            |  8 ++++----
>  ndctl/lib/nfit.c              |  2 +-
>  ndctl/util/json-firmware.c    |  2 +-
>  test/ack-shutdown-count-set.c |  8 ++------
>  test/daxdev-errors.c          |  8 ++++----
>  test/libndctl.c               | 32 ++++++++++++++++----------------
>  test/smart-notify.c           |  8 ++++----
>  8 files changed, 35 insertions(+), 39 deletions(-)
>
> diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
> index 79e2ca0..12dc74a 100644
> --- a/ndctl/lib/dimm.c
> +++ b/ndctl/lib/dimm.c
> @@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct nvdimm_data *ndd, size_t offset,
>                 goto out;
>
>         rc = ndctl_cmd_submit(cmd_write);
> -       if (rc || ndctl_cmd_get_firmware_status(cmd_write))
> +       if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write))

With ndctl_cmd_submit_xlat() the ndctl_cmd_get_firmware_status() can
be dropped, right?

    rc = ndctl_cmd_submit_xlat(cmd_write);
    if (rc < 0)
        rc = -ENXIO;


...or are you saving that conversion for a follow on patch?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit

[Verma, Vishal L]

On Mon, 2019-01-14 at 10:17 -0800, Dan Williams wrote:
> On Mon, Jan 14, 2019 at 10:11 AM Vishal Verma <
> vishal.l.verma@intel.com> wrote:
> > 
> > It is possible for ndctl_cmd_submit to return a positive number,
> > indicating a buffer underrun. It is only truly an error if it
> > returns a
> > negative number. Several places in the library, the ndctl utility,
> > and
> > in test/ were simply checking for an error with "if (rc)". Fix
> > these to
> > only error out for negative returns.
> > 
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  ndctl/lib/dimm.c              |  6 +++---
> >  ndctl/lib/inject.c            |  8 ++++----
> >  ndctl/lib/nfit.c              |  2 +-
> >  ndctl/util/json-firmware.c    |  2 +-
> >  test/ack-shutdown-count-set.c |  8 ++------
> >  test/daxdev-errors.c          |  8 ++++----
> >  test/libndctl.c               | 32 ++++++++++++++++---------------
> > -
> >  test/smart-notify.c           |  8 ++++----
> >  8 files changed, 35 insertions(+), 39 deletions(-)
> > 
> > diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
> > index 79e2ca0..12dc74a 100644
> > --- a/ndctl/lib/dimm.c
> > +++ b/ndctl/lib/dimm.c
> > @@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct
> > nvdimm_data *ndd, size_t offset,
> >                 goto out;
> > 
> >         rc = ndctl_cmd_submit(cmd_write);
> > -       if (rc || ndctl_cmd_get_firmware_status(cmd_write))
> > +       if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write))
> 
> With ndctl_cmd_submit_xlat() the ndctl_cmd_get_firmware_status() can
> be dropped, right?
> 
>     rc = ndctl_cmd_submit_xlat(cmd_write);
>     if (rc < 0)
>         rc = -ENXIO;
> 
> 
> ...or are you saving that conversion for a follow on patch?

The config get/write commands are not dimm_ops right? I thought we
could only use _xlat for dimm_ops?


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit

[Vishal Verma]

On 01/14, Verma, Vishal L wrote:
> 
> On Mon, 2019-01-14 at 10:17 -0800, Dan Williams wrote:
> > On Mon, Jan 14, 2019 at 10:11 AM Vishal Verma <
> > vishal.l.verma@intel.com> wrote:
> > > 
> > > It is possible for ndctl_cmd_submit to return a positive number,
> > > indicating a buffer underrun. It is only truly an error if it
> > > returns a
> > > negative number. Several places in the library, the ndctl utility,
> > > and
> > > in test/ were simply checking for an error with "if (rc)". Fix
> > > these to
> > > only error out for negative returns.
> > > 
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > >  ndctl/lib/dimm.c              |  6 +++---
> > >  ndctl/lib/inject.c            |  8 ++++----
> > >  ndctl/lib/nfit.c              |  2 +-
> > >  ndctl/util/json-firmware.c    |  2 +-
> > >  test/ack-shutdown-count-set.c |  8 ++------
> > >  test/daxdev-errors.c          |  8 ++++----
> > >  test/libndctl.c               | 32 ++++++++++++++++---------------
> > > -
> > >  test/smart-notify.c           |  8 ++++----
> > >  8 files changed, 35 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
> > > index 79e2ca0..12dc74a 100644
> > > --- a/ndctl/lib/dimm.c
> > > +++ b/ndctl/lib/dimm.c
> > > @@ -332,7 +332,7 @@ static int nvdimm_set_config_data(struct
> > > nvdimm_data *ndd, size_t offset,
> > >                 goto out;
> > > 
> > >         rc = ndctl_cmd_submit(cmd_write);
> > > -       if (rc || ndctl_cmd_get_firmware_status(cmd_write))
> > > +       if ((rc < 0) || ndctl_cmd_get_firmware_status(cmd_write))
> > 
> > With ndctl_cmd_submit_xlat() the ndctl_cmd_get_firmware_status() can
> > be dropped, right?
> > 
> >     rc = ndctl_cmd_submit_xlat(cmd_write);
> >     if (rc < 0)
> >         rc = -ENXIO;
> > 
> > 
> > ...or are you saving that conversion for a follow on patch?
> 
> The config get/write commands are not dimm_ops right? I thought we
> could only use _xlat for dimm_ops?

I see how it can be replaced now. Here is a revised patch 4 that
includes thses conversions:

8<----


>From 5c19c269dd0037c3f68725e0f721784056172433 Mon Sep 17 00:00:00 2001
From: Vishal Verma <vishal.l.verma@intel.com>
Date: Fri, 11 Jan 2019 18:20:31 -0700
Subject: [ndctl PATCH] ndctl: clean up usage of ndctl_cmd_submit

It is possible for ndctl_cmd_submit to return a positive number,
indicating a buffer underrun. It is only truly an error if it returns a
negative number. Several places in the library, the ndctl utility, and
in test/ were simply checking for an error with "if (rc)". Fix these to
only error out for negative returns.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/dimm.c              | 18 +++++++-----------
 ndctl/lib/inject.c            |  8 ++++----
 ndctl/lib/nfit.c              |  2 +-
 ndctl/util/json-firmware.c    |  2 +-
 test/ack-shutdown-count-set.c |  8 ++------
 test/daxdev-errors.c          |  8 ++++----
 test/libndctl.c               | 32 ++++++++++++++++----------------
 test/smart-notify.c           |  8 ++++----
 8 files changed, 39 insertions(+), 47 deletions(-)

diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 79e2ca0..f5a955e 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -331,8 +331,8 @@ static int nvdimm_set_config_data(struct nvdimm_data *ndd, size_t offset,
 	if (rc < 0)
 		goto out;
 
-	rc = ndctl_cmd_submit(cmd_write);
-	if (rc || ndctl_cmd_get_firmware_status(cmd_write))
+	rc = ndctl_cmd_submit_xlat(cmd_write);
+	if (rc < 0)
 		rc = -ENXIO;
  out:
 	ndctl_cmd_unref(cmd_write);
@@ -487,15 +487,15 @@ NDCTL_EXPORT struct ndctl_cmd *ndctl_dimm_read_labels(struct ndctl_dimm *dimm)
         cmd_size = ndctl_dimm_cmd_new_cfg_size(dimm);
         if (!cmd_size)
                 return NULL;
-        rc = ndctl_cmd_submit(cmd_size);
-        if (rc || ndctl_cmd_get_firmware_status(cmd_size))
+        rc = ndctl_cmd_submit_xlat(cmd_size);
+        if (rc < 0)
                 goto out_size;
 
         cmd_read = ndctl_dimm_cmd_new_cfg_read(cmd_size);
         if (!cmd_read)
                 goto out_size;
-        rc = ndctl_cmd_submit(cmd_read);
-        if (rc || ndctl_cmd_get_firmware_status(cmd_read))
+        rc = ndctl_cmd_submit_xlat(cmd_read);
+        if (rc < 0)
                 goto out_read;
 	ndctl_cmd_unref(cmd_size);
 
@@ -536,13 +536,9 @@ NDCTL_EXPORT int ndctl_dimm_zero_labels(struct ndctl_dimm *dimm)
 		rc = -ENXIO;
 		goto out_write;
 	}
-	rc = ndctl_cmd_submit(cmd_write);
+	rc = ndctl_cmd_submit_xlat(cmd_write);
 	if (rc < 0)
 		goto out_write;
-	if (ndctl_cmd_get_firmware_status(cmd_write)) {
-		rc = -ENXIO;
-		goto out_write;
-	}
 
 	/*
 	 * If the dimm is already disabled the kernel is not holding a cached
diff --git a/ndctl/lib/inject.c b/ndctl/lib/inject.c
index 2b0702e..c35d0f3 100644
--- a/ndctl/lib/inject.c
+++ b/ndctl/lib/inject.c
@@ -156,7 +156,7 @@ static int ndctl_namespace_inject_one_error(struct ndctl_namespace *ndns,
 			(1 << ND_ARS_ERR_INJ_OPT_NOTIFY);
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		dbg(ctx, "Error submitting command: %d\n", rc);
 		goto out;
 	}
@@ -234,7 +234,7 @@ static int ndctl_namespace_uninject_one_error(struct ndctl_namespace *ndns,
 	err_inj_clr->err_inj_clr_spa_range_length = length;
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		dbg(ctx, "Error submitting command: %d\n", rc);
 		goto out;
 	}
@@ -443,7 +443,7 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
 
 		cmd = ndctl_bus_cmd_new_ars_cap(bus, ns_offset, ns_size);
 		rc = ndctl_cmd_submit(cmd);
-		if (rc) {
+		if (rc < 0) {
 			dbg(ctx, "Error submitting ars_cap: %d\n", rc);
 			goto out;
 		}
@@ -464,7 +464,7 @@ NDCTL_EXPORT int ndctl_namespace_injection_status(struct ndctl_namespace *ndns)
 			(struct nd_cmd_ars_err_inj_stat *)&pkg->nd_payload[0];
 
 		rc = ndctl_cmd_submit(cmd);
-		if (rc) {
+		if (rc < 0) {
 			dbg(ctx, "Error submitting command: %d\n", rc);
 			goto out;
 		}
diff --git a/ndctl/lib/nfit.c b/ndctl/lib/nfit.c
index 2ae3f07..b10edb1 100644
--- a/ndctl/lib/nfit.c
+++ b/ndctl/lib/nfit.c
@@ -133,7 +133,7 @@ int ndctl_bus_nfit_translate_spa(struct ndctl_bus *bus,
 	translate_spa->spa = address;
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
diff --git a/ndctl/util/json-firmware.c b/ndctl/util/json-firmware.c
index 118424f..f7150d8 100644
--- a/ndctl/util/json-firmware.c
+++ b/ndctl/util/json-firmware.c
@@ -25,7 +25,7 @@ struct json_object *util_dimm_firmware_to_json(struct ndctl_dimm *dimm,
 		goto err;
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
+	if ((rc < 0) || ndctl_cmd_fw_xlat_firmware_status(cmd) != FW_SUCCESS) {
 		jobj = util_json_object_hex(-1, flags);
 		if (jobj)
 			json_object_object_add(jfirmware, "current_version",
diff --git a/test/ack-shutdown-count-set.c b/test/ack-shutdown-count-set.c
index 9baa2f1..742e976 100644
--- a/test/ack-shutdown-count-set.c
+++ b/test/ack-shutdown-count-set.c
@@ -27,12 +27,8 @@ static int test_dimm(struct ndctl_dimm *dimm)
 	if (!cmd)
 		return -ENOMEM;
 
-	rc = ndctl_cmd_submit(cmd);
-	if (rc < 0)
-		goto out;
-
-	rc = ndctl_cmd_get_firmware_status(cmd);
-	if (rc != 0) {
+	rc = ndctl_cmd_submit_xlat(cmd);
+	if (rc < 0) {
 		fprintf(stderr, "dimm %s LSS enable set failed\n",
 				ndctl_dimm_get_devname(dimm));
 		goto out;
diff --git a/test/daxdev-errors.c b/test/daxdev-errors.c
index 94fbebe..29de16b 100644
--- a/test/daxdev-errors.c
+++ b/test/daxdev-errors.c
@@ -75,7 +75,7 @@ static int check_ars_cap(struct ndctl_bus *bus, uint64_t start,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(cmd);
@@ -115,7 +115,7 @@ static int check_ars_start(struct ndctl_bus *bus, struct check_cmd *check)
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(cmd);
@@ -149,7 +149,7 @@ static int check_ars_status(struct ndctl_bus *bus, struct check_cmd *check)
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(cmd);
@@ -210,7 +210,7 @@ static int check_clear_error(struct ndctl_bus *bus, struct check_cmd *check)
 	}
 
 	rc = ndctl_cmd_submit(clear_err);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: bus: %s failed to submit cmd: %d\n",
 				__func__, ndctl_bus_get_provider(bus), rc);
 		ndctl_cmd_unref(clear_err);
diff --git a/test/libndctl.c b/test/libndctl.c
index 50365f0..02bb9cc 100644
--- a/test/libndctl.c
+++ b/test/libndctl.c
@@ -2057,7 +2057,7 @@ static int check_get_config_size(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2091,7 +2091,7 @@ static int check_get_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %zd\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2116,7 +2116,7 @@ static int check_set_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	struct ndctl_cmd *cmd_read = check_cmds[ND_CMD_GET_CONFIG_DATA].cmd;
 	struct ndctl_cmd *cmd = ndctl_dimm_cmd_new_cfg_write(cmd_read);
 	char buf[20], result[sizeof(buf)];
-	size_t rc;
+	int rc;
 
 	if (!cmd) {
 		fprintf(stderr, "%s: dimm: %#x failed to create cmd\n",
@@ -2127,23 +2127,23 @@ static int check_set_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	memset(buf, 0, sizeof(buf));
 	ndctl_cmd_cfg_write_set_data(cmd, buf, sizeof(buf), 0);
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 
 	rc = ndctl_cmd_submit(cmd_read);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit read1: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit read1: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 	ndctl_cmd_cfg_read_get_data(cmd_read, result, sizeof(result), 0);
 	if (memcmp(result, buf, sizeof(result)) != 0) {
-		fprintf(stderr, "%s: dimm: %#x read1 data miscompare: %zd\n",
+		fprintf(stderr, "%s: dimm: %#x read1 data miscompare: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return -ENXIO;
@@ -2152,23 +2152,23 @@ static int check_set_config_data(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	sprintf(buf, "dimm-%#x", ndctl_dimm_get_handle(dimm));
 	ndctl_cmd_cfg_write_set_data(cmd, buf, sizeof(buf), 0);
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 
 	rc = ndctl_cmd_submit(cmd_read);
-	if (rc) {
-		fprintf(stderr, "%s: dimm: %#x failed to submit read2: %zd\n",
+	if (rc < 0) {
+		fprintf(stderr, "%s: dimm: %#x failed to submit read2: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
 	}
 	ndctl_cmd_cfg_read_get_data(cmd_read, result, sizeof(result), 0);
 	if (memcmp(result, buf, sizeof(result)) != 0) {
-		fprintf(stderr, "%s: dimm: %#x read2 data miscompare: %zd\n",
+		fprintf(stderr, "%s: dimm: %#x read2 data miscompare: %d\n",
 				__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
 		return rc;
@@ -2225,7 +2225,7 @@ static int check_smart(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2326,7 +2326,7 @@ static int check_smart_threshold(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 	}
 
 	rc = ndctl_cmd_submit(cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: dimm: %#x failed to submit cmd: %d\n",
 			__func__, ndctl_dimm_get_handle(dimm), rc);
 		ndctl_cmd_unref(cmd);
@@ -2375,7 +2375,7 @@ static int check_smart_threshold(struct ndctl_bus *bus, struct ndctl_dimm *dimm,
 		}
 
 		rc = ndctl_cmd_submit(cmd_set);
-		if (rc) {
+		if (rc < 0) {
 			fprintf(stderr, "%s: dimm: %#x failed to submit cmd_set: %d\n",
 					__func__, ndctl_dimm_get_handle(dimm), rc);
 			ndctl_cmd_unref(cmd_set);
diff --git a/test/smart-notify.c b/test/smart-notify.c
index 24e9a21..e28e0f4 100644
--- a/test/smart-notify.c
+++ b/test/smart-notify.c
@@ -22,7 +22,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 	}
 
 	rc = ndctl_cmd_submit(s_cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: smart command failed: %d %s\n", name,
 				rc, strerror(errno));
 		goto out;
@@ -49,7 +49,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 	}
 
 	rc = ndctl_cmd_submit(st_cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: smart threshold command failed: %d %s\n",
 				name, rc, strerror(errno));
 		goto out;
@@ -148,7 +148,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 
 		ndctl_cmd_smart_threshold_set_alarm_control(sst_cmd, set_alarm);
 		rc = ndctl_cmd_submit(sst_cmd);
-		if (rc) {
+		if (rc < 0) {
 			fprintf(stderr, "%s: smart set threshold command failed: %d %s\n",
 					name, rc, strerror(errno));
 			goto out;
@@ -166,7 +166,7 @@ static void do_notify(struct ndctl_dimm *dimm)
 	}
 
 	rc = ndctl_cmd_submit(sst_cmd);
-	if (rc) {
+	if (rc < 0) {
 		fprintf(stderr, "%s: smart set threshold defaults failed: %d %s\n",
 				name, rc, strerror(errno));
 		goto out;
-- 
2.17.2
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH v3 4/4] ndctl: clean up usage of ndctl_cmd_submit

[Dan Williams]

On Mon, Jan 14, 2019 at 10:49 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
[..]
> I see how it can be replaced now. Here is a revised patch 4 that
> includes thses conversions:
>
> 8<----
>
>
> From 5c19c269dd0037c3f68725e0f721784056172433 Mon Sep 17 00:00:00 2001
> From: Vishal Verma <vishal.l.verma@intel.com>
> Date: Fri, 11 Jan 2019 18:20:31 -0700
> Subject: [ndctl PATCH] ndctl: clean up usage of ndctl_cmd_submit
>
> It is possible for ndctl_cmd_submit to return a positive number,
> indicating a buffer underrun. It is only truly an error if it returns a
> negative number. Several places in the library, the ndctl utility, and
> in test/ were simply checking for an error with "if (rc)". Fix these to
> only error out for negative returns.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Looks good to me. For the series:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm