[PATCH v5 0/4] ndctl: convert actions to use util_filter_walk

[PATCH v5 0/4] ndctl: convert actions to use util_filter_walk

[Dave Jiang]

util_filter_walk() does the looping through bus/dimm/region/namespace
that a lot of the operations in ndctl uses. Converting them to common
code and reduce maintenance on individual versions of the same code.
In this series we are convering namespace, region, and dimm actions.

---

v5:
- fix behavior regression in filter_namespace (Dan)
- fix segfault caused by no namespace for create_namespace actions.

v4:
- change struct names to be less confusing. (Dan)

v3:
- fixed some corner cases in namespace patch.
- changed param renaming to reduce change for util_filter_params. (Dan)
- Adding conversion to region
- Adding conversion to dimm

v2:
- split out the conversion of util_filter_params to make things more
  readable (Dan).
- Not pass in mode as util_filter_params and put back the mode check in
  util_filter_walk() (Dan).

Dave Jiang (4):
      ndctl: convert namespace actions to use util_filter_params
      ndctl: convert namespace actions to use util_filter_walk()
      ndctl: convert region actions to use util_filter_walk()
      ndctl: convert dimm actions to use util_filter_walk()


 ndctl/dimm.c      |   83 ++++++++++++++---------
 ndctl/namespace.c |  194 +++++++++++++++++++++++++++++------------------------
 ndctl/region.c    |   59 ++++++++++------
 test/btt-check.sh |    2 -
 util/filter.c     |    5 +
 util/filter.h     |   23 ++++++
 6 files changed, 220 insertions(+), 146 deletions(-)

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

[PATCH v5 1/4] ndctl: convert namespace actions to use util_filter_params

[Dave Jiang]

In preparation of moving to using util_filter_walk, moving parts of
namespace params to util_filter_params.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/namespace.c |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index fe86d826..e61f500c 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -40,14 +40,11 @@ static struct parameters {
 	bool mode_default;
 	bool align_default;
 	bool autolabel;
-	const char *bus;
 	const char *map;
-	const char *type;
 	const char *uuid;
 	const char *name;
 	const char *size;
 	const char *mode;
-	const char *region;
 	const char *reconfig;
 	const char *sector_size;
 	const char *align;
@@ -55,6 +52,8 @@ static struct parameters {
 	.autolabel = true,
 };
 
+struct util_filter_params uf_param;
+
 void builtin_xaction_namespace_reset(void)
 {
 	/*
@@ -87,9 +86,9 @@ struct parsed_parameters {
 	}})
 
 #define BASE_OPTIONS() \
-OPT_STRING('b', "bus", &param.bus, "bus-id", \
+OPT_STRING('b', "bus", &uf_param.bus, "bus-id", \
 	"limit namespace to a bus with an id or provider of <bus-id>"), \
-OPT_STRING('r', "region", &param.region, "region-id", \
+OPT_STRING('r', "region", &uf_param.region, "region-id", \
 	"limit namespace to a region with an id or name of <region-id>"), \
 OPT_BOOLEAN('v', "verbose", &verbose, "emit extra debug messages to stderr")
 
@@ -108,7 +107,7 @@ OPT_STRING('M', "map", &param.map, "memmap-location", \
 	"specify 'mem' or 'dev' for the location of the memmap"), \
 OPT_STRING('l', "sector-size", &param.sector_size, "lba-size", \
 	"specify the logical sector size in bytes"), \
-OPT_STRING('t', "type", &param.type, "type", \
+OPT_STRING('t', "type", &uf_param.type, "type", \
 	"specify the type of namespace to create 'pmem' or 'blk'"), \
 OPT_STRING('a', "align", &param.align, "align", \
 	"specify the namespace alignment in bytes (default: 2M)"), \
@@ -148,18 +147,18 @@ static int set_defaults(enum device_action mode)
 {
 	int rc = 0;
 
-	if (param.type) {
-		if (strcmp(param.type, "pmem") == 0)
+	if (uf_param.type) {
+		if (strcmp(uf_param.type, "pmem") == 0)
 			/* pass */;
-		else if (strcmp(param.type, "blk") == 0)
+		else if (strcmp(uf_param.type, "blk") == 0)
 			/* pass */;
 		else {
 			error("invalid type '%s', must be either 'pmem' or 'blk'\n",
-				param.type);
+				uf_param.type);
 			rc = -EINVAL;
 		}
 	} else if (!param.reconfig && mode == ACTION_CREATE)
-		param.type = "pmem";
+		uf_param.type = "pmem";
 
 	if (param.mode) {
 		if (strcmp(param.mode, "safe") == 0)
@@ -180,8 +179,8 @@ static int set_defaults(enum device_action mode)
 			error("invalid mode '%s'\n", param.mode);
 			rc = -EINVAL;
 		}
-	} else if (!param.reconfig && param.type) {
-		if (strcmp(param.type, "pmem") == 0)
+	} else if (!param.reconfig && uf_param.type) {
+		if (strcmp(uf_param.type, "pmem") == 0)
 			param.mode = "memory";
 		else
 			param.mode = "safe";
@@ -208,7 +207,7 @@ static int set_defaults(enum device_action mode)
 		param.map = "dev";
 
 	/* check for incompatible mode and type combinations */
-	if (param.type && param.mode && strcmp(param.type, "blk") == 0
+	if (uf_param.type && param.mode && strcmp(uf_param.type, "blk") == 0
 			&& (strcmp(param.mode, "memory") == 0
 				|| strcmp(param.mode, "dax") == 0)) {
 		error("only 'pmem' namespaces support dax operation\n");
@@ -244,7 +243,7 @@ static int set_defaults(enum device_action mode)
 			error("invalid sector size: %s\n", param.sector_size);
 			rc = -EINVAL;
 		}
-	} else if (((param.type && strcmp(param.type, "blk") == 0)
+	} else if (((uf_param.type && strcmp(uf_param.type, "blk") == 0)
 			|| (param.mode && strcmp(param.mode, "safe") == 0))) {
 		/* default sector size for blk-type or safe-mode */
 		param.sector_size = "4096";
@@ -1000,19 +999,19 @@ static int do_xaction_namespace(const char *namespace,
 		ndctl_set_log_priority(ctx, LOG_DEBUG);
 
         ndctl_bus_foreach(ctx, bus) {
-		if (!util_bus_filter(bus, param.bus))
+		if (!util_bus_filter(bus, uf_param.bus))
 			continue;
 
 		ndctl_region_foreach(bus, region) {
-			if (!util_region_filter(region, param.region))
+			if (!util_region_filter(region, uf_param.region))
 				continue;
 
-			if (param.type) {
-				if (strcmp(param.type, "pmem") == 0
+			if (uf_param.type) {
+				if (strcmp(uf_param.type, "pmem") == 0
 						&& ndctl_region_get_type(region)
 						== ND_DEVICE_REGION_PMEM)
 					/* pass */;
-				else if (strcmp(param.type, "blk") == 0
+				else if (strcmp(uf_param.type, "blk") == 0
 						&& ndctl_region_get_type(region)
 						== ND_DEVICE_REGION_BLK)
 					/* pass */;
@@ -1125,7 +1124,7 @@ int cmd_create_namespace(int argc, const char **argv, void *ctx)
 		 * fallback to blk before giving up.
 		 */
 		memset(&param, 0, sizeof(param));
-		param.type = "blk";
+		uf_param.type = "blk";
 		set_defaults(ACTION_CREATE);
 		created = do_xaction_namespace(NULL, ACTION_CREATE, ctx);
 	}

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

[PATCH v5 2/4] ndctl: convert namespace actions to use util_filter_walk()

[Dave Jiang]

util_filter_walk() does the looping through of busses and regions. Removing
duplicate code in namespace ops and provide filter functions so we can
utilize util_filter_walk() and share common code.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/namespace.c |  163 ++++++++++++++++++++++++++++++-----------------------
 test/btt-check.sh |    2 -
 util/filter.c     |    5 +-
 util/filter.h     |    8 +++
 4 files changed, 104 insertions(+), 74 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index e61f500c..b0505ce8 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -983,14 +983,91 @@ static int namespace_reconfig(struct ndctl_region *region,
 int namespace_check(struct ndctl_namespace *ndns, bool verbose, bool force,
 		bool repair, bool logfix);
 
+static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx)
+{
+	return true;
+}
+
+static bool filter_region(struct ndctl_region *region,
+		struct util_filter_ctx *ctx)
+{
+	struct nsaction_filter_arg *nfa = ctx->nsaction;
+	int rc = 0;
+	bool out = true;
+
+	if (nfa->action == ACTION_CREATE && !nfa->namespace) {
+		if (nfa->rc == 1)
+			return false;
+
+		rc = namespace_create(region);
+		if (rc != -EAGAIN) {
+			if (rc == 0)
+				rc = 1;
+			/* don't proceed in the filter loop */
+			out = false;
+		}
+		nfa->rc = rc;
+	}
+
+	return out;
+}
+
+static void filter_namespace(struct ndctl_namespace *ndns,
+		struct util_filter_ctx *ctx)
+{
+	struct ndctl_region *region = ndctl_namespace_get_region(ndns);
+	struct nsaction_filter_arg *nfa = ctx->nsaction;
+	const char *ndns_name;
+	int rc;
+
+	if (!nfa->namespace)
+		return;
+
+	ndns_name = ndctl_namespace_get_devname(ndns);
+	if (strcmp(nfa->namespace, "all") != 0
+			&& strcmp(nfa->namespace, ndns_name) != 0) {
+		return;
+	}
+
+	switch (nfa->action) {
+	case ACTION_DISABLE:
+		rc = ndctl_namespace_disable_safe(ndns);
+		break;
+	case ACTION_ENABLE:
+		rc = ndctl_namespace_enable(ndns);
+		break;
+	case ACTION_DESTROY:
+		rc = namespace_destroy(region, ndns);
+		break;
+	case ACTION_CHECK:
+		rc = namespace_check(ndns, verbose, force, repair, logfix);
+		if (rc < 0) {
+			nfa->rc = rc;
+			return;
+		}
+		break;
+	case ACTION_CREATE:
+		rc = namespace_reconfig(region, ndns);
+		if (rc < 0) {
+			nfa->rc = rc;
+			return;
+		}
+		break;
+	default:
+		rc = -EINVAL;
+		break;
+	}
+
+	if (rc >= 0)
+		nfa->rc++;
+}
+
 static int do_xaction_namespace(const char *namespace,
 		enum device_action action, struct ndctl_ctx *ctx)
 {
-	struct ndctl_namespace *ndns, *_n;
-	int rc = -ENXIO, success = 0;
-	struct ndctl_region *region;
-	const char *ndns_name;
-	struct ndctl_bus *bus;
+	int rc = -ENXIO;
+	struct util_filter_ctx fctx = { 0 };
+	struct nsaction_filter_arg nfa = { 0 };
 
 	if (!namespace && action != ACTION_CREATE)
 		return rc;
@@ -998,74 +1075,18 @@ static int do_xaction_namespace(const char *namespace,
 	if (verbose)
 		ndctl_set_log_priority(ctx, LOG_DEBUG);
 
-        ndctl_bus_foreach(ctx, bus) {
-		if (!util_bus_filter(bus, uf_param.bus))
-			continue;
-
-		ndctl_region_foreach(bus, region) {
-			if (!util_region_filter(region, uf_param.region))
-				continue;
-
-			if (uf_param.type) {
-				if (strcmp(uf_param.type, "pmem") == 0
-						&& ndctl_region_get_type(region)
-						== ND_DEVICE_REGION_PMEM)
-					/* pass */;
-				else if (strcmp(uf_param.type, "blk") == 0
-						&& ndctl_region_get_type(region)
-						== ND_DEVICE_REGION_BLK)
-					/* pass */;
-				else
-					continue;
-			}
+	fctx.filter_bus = filter_bus;
+	fctx.filter_region = filter_region;
+	fctx.filter_namespace = filter_namespace;
+	fctx.nsaction = &nfa;
+	fctx.nsaction->action = action;
+	fctx.nsaction->namespace = namespace;
 
-			if (action == ACTION_CREATE && !namespace) {
-				rc = namespace_create(region);
-				if (rc == -EAGAIN)
-					continue;
-				if (rc == 0)
-					rc = 1;
-				return rc;
-			}
-			ndctl_namespace_foreach_safe(region, ndns, _n) {
-				ndns_name = ndctl_namespace_get_devname(ndns);
-
-				if (strcmp(namespace, "all") != 0
-						&& strcmp(namespace, ndns_name) != 0)
-					continue;
-				switch (action) {
-				case ACTION_DISABLE:
-					rc = ndctl_namespace_disable_safe(ndns);
-					break;
-				case ACTION_ENABLE:
-					rc = ndctl_namespace_enable(ndns);
-					break;
-				case ACTION_DESTROY:
-					rc = namespace_destroy(region, ndns);
-					break;
-				case ACTION_CHECK:
-					rc = namespace_check(ndns, verbose,
-							force, repair, logfix);
-					if (rc < 0)
-						return rc;
-					break;
-				case ACTION_CREATE:
-					rc = namespace_reconfig(region, ndns);
-					if (rc < 0)
-						return rc;
-					return 1;
-				default:
-					rc = -EINVAL;
-					break;
-				}
-				if (rc >= 0)
-					success++;
-			}
-		}
-	}
+	rc = util_filter_walk(ctx, &fctx, &uf_param);
+	if (rc < 0)
+		return rc;
 
-	if (success)
-		return success;
+	rc = nfa.rc;
 	return rc;
 }
 
diff --git a/test/btt-check.sh b/test/btt-check.sh
index 353d4375..50c1b592 100755
--- a/test/btt-check.sh
+++ b/test/btt-check.sh
@@ -1,4 +1,4 @@
-#!/bin/bash -E
+#!/bin/bash -Ex
 
 # Copyright(c) 2015-2017 Intel Corporation. All rights reserved.
 # 
diff --git a/util/filter.c b/util/filter.c
index 0d3cc02f..b7a4fbd6 100644
--- a/util/filter.c
+++ b/util/filter.c
@@ -18,6 +18,7 @@
 #include <sys/stat.h>
 #include <util/util.h>
 #include <sys/types.h>
+#include <ndctl/action.h>
 #include <ndctl/ndctl.h>
 #include <util/filter.h>
 #include <ndctl/libndctl.h>
@@ -418,7 +419,7 @@ int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 		}
 
 		ndctl_region_foreach(bus, region) {
-			struct ndctl_namespace *ndns;
+			struct ndctl_namespace *ndns, *_n;
 
 			if (!util_region_filter(region, param->region)
 					|| !util_region_filter_by_dimm(region,
@@ -437,7 +438,7 @@ int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 			if (!fctx->filter_region(region, fctx))
 				continue;
 
-			ndctl_namespace_foreach(region, ndns) {
+			ndctl_namespace_foreach_safe(region, ndns, _n) {
 				enum ndctl_namespace_mode mode;
 
 				if (!fctx->filter_namespace)
diff --git a/util/filter.h b/util/filter.h
index effda24b..9f3786a9 100644
--- a/util/filter.h
+++ b/util/filter.h
@@ -13,6 +13,7 @@
 #ifndef _UTIL_FILTER_H_
 #define _UTIL_FILTER_H_
 #include <stdbool.h>
+#include <ndctl/action.h>
 
 struct ndctl_bus *util_bus_filter(struct ndctl_bus *bus, const char *ident);
 struct ndctl_region *util_region_filter(struct ndctl_region *region,
@@ -50,6 +51,12 @@ struct list_filter_arg {
 	unsigned long flags;
 };
 
+struct nsaction_filter_arg {
+	enum device_action action;
+	const char *namespace;
+	int rc;
+};
+
 /*
  * struct util_filter_ctx - control and callbacks for util_filter_walk()
  * ->filter_bus() and ->filter_region() return bool because the
@@ -67,6 +74,7 @@ struct util_filter_ctx {
 	union {
 		void *arg;
 		struct list_filter_arg *list;
+		struct nsaction_filter_arg *nsaction;
 	};
 };
 

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

[PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Dave Jiang]

util_filter_walk() does the looping through of busses and regions. Removing
duplicate code in region ops and provide filter functions so we can
utilize util_filter_walk() and share common code.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++----------------------
 util/filter.h  |    6 ++++++
 2 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/ndctl/region.c b/ndctl/region.c
index 9fc90808..9fd07af6 100644
--- a/ndctl/region.c
+++ b/ndctl/region.c
@@ -19,10 +19,7 @@
 #include <util/parse-options.h>
 #include <ndctl/libndctl.h>
 
-static struct {
-	const char *bus;
-	const char *type;
-} param;
+struct util_filter_params param;
 
 static const struct option region_options[] = {
 	OPT_STRING('b', "bus", &param.bus, "bus-id",
@@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
 	return 0;
 }
 
+static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx)
+{
+	return true;
+}
+
+static bool filter_region(struct ndctl_region *region,
+		struct util_filter_ctx *ctx)
+{
+	struct rgaction_filter_arg *rfa = ctx->rgaction;
+	int rc;
+
+	if (rfa->rc < 0)
+		return false;
+
+	rc = region_action(region, rfa->action);
+
+	if (rc == 0)
+		rfa->rc++;
+	else
+		rfa->rc = 0;
+
+	/* we don't need to fall through, can continue the loop */
+	return false;
+}
+
 static int do_xable_region(const char *region_arg, enum device_action mode,
 		struct ndctl_ctx *ctx)
 {
-	int rc = -ENXIO, success = 0;
-	struct ndctl_region *region;
-	struct ndctl_bus *bus;
+	int rc = -ENXIO;
+	struct util_filter_ctx fctx = { 0 };
+	struct rgaction_filter_arg rfa = { 0 };
 
 	if (!region_arg)
 		goto out;
 
-        ndctl_bus_foreach(ctx, bus) {
-		if (!util_bus_filter(bus, param.bus))
-			continue;
-
-		ndctl_region_foreach(bus, region) {
-			const char *type = ndctl_region_get_type_name(region);
-
-			if (param.type && strcmp(param.type, type) != 0)
-				continue;
-			if (!util_region_filter(region, region_arg))
-				continue;
-			if (region_action(region, mode) == 0)
-				success++;
-		}
-	}
+	fctx.filter_bus = filter_bus;
+	fctx.filter_region = filter_region;
+	fctx.rgaction = &rfa;
+	fctx.rgaction->action = mode;
+	rc = util_filter_walk(ctx, &fctx, &param);
+	if (rc == 0)
+		rc = fctx.rgaction->rc;
 
-	rc = success;
  out:
 	param.bus = NULL;
 	return rc;
diff --git a/util/filter.h b/util/filter.h
index 9f3786a9..0e0e3bec 100644
--- a/util/filter.h
+++ b/util/filter.h
@@ -57,6 +57,11 @@ struct nsaction_filter_arg {
 	int rc;
 };
 
+struct rgaction_filter_arg {
+	enum device_action action;
+	int rc;
+};
+
 /*
  * struct util_filter_ctx - control and callbacks for util_filter_walk()
  * ->filter_bus() and ->filter_region() return bool because the
@@ -75,6 +80,7 @@ struct util_filter_ctx {
 		void *arg;
 		struct list_filter_arg *list;
 		struct nsaction_filter_arg *nsaction;
+		struct rgaction_filter_arg *rgaction;
 	};
 };
 

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

[PATCH v5 4/4] ndctl: convert dimm actions to use util_filter_walk()

[Dave Jiang]

util_filter_walk() does the looping through of busses and dimms. Removing
duplicate code for dimm actions and provide filter functions so we can
utilize util_filter_walk() and share common code.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 ndctl/dimm.c  |   83 ++++++++++++++++++++++++++++++++++-----------------------
 util/filter.h |    9 ++++++
 2 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 1c168997..d652be69 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -822,7 +822,6 @@ static int action_update(struct ndctl_dimm *dimm, struct action_context *actx)
 }
 
 static struct parameters {
-	const char *bus;
 	const char *outfile;
 	const char *infile;
 	const char *labelversion;
@@ -833,6 +832,8 @@ static struct parameters {
 	.labelversion = "1.1",
 };
 
+struct util_filter_params uf_param;
+
 static int __action_init(struct ndctl_dimm *dimm,
 		enum ndctl_namespace_version version, int chk_only)
 {
@@ -899,7 +900,7 @@ static int action_check(struct ndctl_dimm *dimm, struct action_context *actx)
 
 
 #define BASE_OPTIONS() \
-OPT_STRING('b', "bus", &param.bus, "bus-id", \
+OPT_STRING('b', "bus", &uf_param.bus, "bus-id", \
 	"<nmem> must be on a bus with an id/provider of <bus-id>"), \
 OPT_BOOLEAN('v',"verbose", &param.verbose, "turn on debug")
 
@@ -951,18 +952,48 @@ static const struct option init_options[] = {
 	OPT_END(),
 };
 
+static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx)
+{
+	return true;
+}
+
+static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *ctx)
+{
+	struct dmaction_filter_arg *dfa = ctx->dmaction;
+	int rc;
+
+	if (dfa->action == action_write) {
+		dfa->dimm = dimm;
+		rc = 0;
+	} else
+		rc = dfa->action(dimm, dfa->actx);
+
+	if (rc == 0)
+		dfa->count++;
+	else if (rc && !dfa->rc)
+		dfa->rc = rc;
+}
+
+/* dummy filter function to skip the loop */
+static bool filter_region(struct ndctl_region *region,
+		struct util_filter_ctx *ctx)
+{
+	return false;
+}
+
 static int dimm_action(int argc, const char **argv, void *ctx,
 		int (*action)(struct ndctl_dimm *dimm, struct action_context *actx),
 		const struct option *options, const char *usage)
 {
 	struct action_context actx = { 0 };
-	int i, rc = 0, count = 0, err = 0;
-	struct ndctl_dimm *single = NULL;
+	int i, rc = 0, err = 0;
 	const char * const u[] = {
 		usage,
 		NULL
 	};
 	unsigned long id;
+	struct util_filter_ctx fctx = { 0 };
+	struct dmaction_filter_arg dfa = { 0 };
 
         argc = parse_options(argc, argv, options, u, 0);
 
@@ -1039,45 +1070,31 @@ static int dimm_action(int argc, const char **argv, void *ctx,
 		goto out;
 	}
 
+	fctx.filter_bus = filter_bus;
+	fctx.filter_dimm = filter_dimm;
+	fctx.filter_region = filter_region;
+	fctx.dmaction = &dfa;
+	dfa.action = action;
+	dfa.actx = &actx;
+
 	rc = 0;
-	err = 0;
-	count = 0;
 	for (i = 0; i < argc; i++) {
-		struct ndctl_dimm *dimm;
-		struct ndctl_bus *bus;
-
 		if (sscanf(argv[i], "nmem%lu", &id) != 1
 				&& strcmp(argv[i], "all") != 0)
 			continue;
 
-		ndctl_bus_foreach(ctx, bus) {
-			if (!util_bus_filter(bus, param.bus))
-				continue;
-			ndctl_dimm_foreach(bus, dimm) {
-				if (!util_dimm_filter(dimm, argv[i]))
-					continue;
-				if (action == action_write) {
-					single = dimm;
-					rc = 0;
-				} else
-					rc = action(dimm, &actx);
-
-				if (rc == 0)
-					count++;
-				else if (rc && !err)
-					err = rc;
-			}
-		}
+		uf_param.dimm = argv[i];
+		rc = util_filter_walk(ctx, &fctx, &uf_param);
 	}
-	rc = err;
+	rc = dfa.rc;
 
 	if (action == action_write) {
-		if (count > 1) {
+		if (dfa.count > 1) {
 			error("write-labels only supports writing a single dimm\n");
 			usage_with_options(u, options);
 			return -EINVAL;
-		} else if (single)
-			rc = action(single, &actx);
+		} else if (dfa.dimm)
+			rc = action(dfa.dimm, &actx);
 	}
 
 	if (actx.jdimms)
@@ -1095,8 +1112,8 @@ static int dimm_action(int argc, const char **argv, void *ctx,
 	 * count if some actions succeeded, 0 if none were attempted,
 	 * negative error code otherwise.
 	 */
-	if (count > 0)
-		return count;
+	if (dfa.count > 0)
+		return dfa.count;
 	return rc;
 }
 
diff --git a/util/filter.h b/util/filter.h
index 0e0e3bec..2bef9310 100644
--- a/util/filter.h
+++ b/util/filter.h
@@ -62,6 +62,14 @@ struct rgaction_filter_arg {
 	int rc;
 };
 
+struct dmaction_filter_arg {
+	struct action_context *actx;
+	struct ndctl_dimm *dimm;
+	int (*action)(struct ndctl_dimm *dimm, struct action_context *actx);
+	int count;
+	int rc;
+};
+
 /*
  * struct util_filter_ctx - control and callbacks for util_filter_walk()
  * ->filter_bus() and ->filter_region() return bool because the
@@ -81,6 +89,7 @@ struct util_filter_ctx {
 		struct list_filter_arg *list;
 		struct nsaction_filter_arg *nsaction;
 		struct rgaction_filter_arg *rgaction;
+		struct dmaction_filter_arg *dmaction;
 	};
 };
 

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

Re: [PATCH v5 2/4] ndctl: convert namespace actions to use util_filter_walk()

[Verma, Vishal L]

On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
> util_filter_walk() does the looping through of busses and regions.
> Removing
> duplicate code in namespace ops and provide filter functions so we can
> utilize util_filter_walk() and share common code.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/namespace.c |  163 ++++++++++++++++++++++++++++++-----------------
> ------
>  test/btt-check.sh |    2 -
>  util/filter.c     |    5 +-
>  util/filter.h     |    8 +++
>  4 files changed, 104 insertions(+), 74 deletions(-)
> 

[...]

>  
> diff --git a/test/btt-check.sh b/test/btt-check.sh
> index 353d4375..50c1b592 100755
> --- a/test/btt-check.sh
> +++ b/test/btt-check.sh
> @@ -1,4 +1,4 @@
> -#!/bin/bash -E
> +#!/bin/bash -Ex

Did you mean to add this hunk?

>  
>  # Copyright(c) 2015-2017 Intel Corporation. All rights reserved.
>  # 
> diff --git a/util/filter.c b/util/filter.c
> index 0d3cc02f..b7a4fbd6 100644
> --- a/util/filter.c
> +++ b/util/filter.c
> 
[...]
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 2/4] ndctl: convert namespace actions to use util_filter_walk()

[Dave Jiang]

On 05/09/2018 03:47 PM, Verma, Vishal L wrote:
> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
>> util_filter_walk() does the looping through of busses and regions.
>> Removing
>> duplicate code in namespace ops and provide filter functions so we can
>> utilize util_filter_walk() and share common code.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  ndctl/namespace.c |  163 ++++++++++++++++++++++++++++++-----------------
>> ------
>>  test/btt-check.sh |    2 -
>>  util/filter.c     |    5 +-
>>  util/filter.h     |    8 +++
>>  4 files changed, 104 insertions(+), 74 deletions(-)
>>
> 
> [...]
> 
>>  
>> diff --git a/test/btt-check.sh b/test/btt-check.sh
>> index 353d4375..50c1b592 100755
>> --- a/test/btt-check.sh
>> +++ b/test/btt-check.sh
>> @@ -1,4 +1,4 @@
>> -#!/bin/bash -E
>> +#!/bin/bash -Ex
> 
> Did you mean to add this hunk?

No, must be accident from debug.

> 
>>  
>>  # Copyright(c) 2015-2017 Intel Corporation. All rights reserved.
>>  # 
>> diff --git a/util/filter.c b/util/filter.c
>> index 0d3cc02f..b7a4fbd6 100644
>> --- a/util/filter.c
>> +++ b/util/filter.c
>>
> [...]
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Verma, Vishal L]

On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
> util_filter_walk() does the looping through of busses and regions.
> Removing
> duplicate code in region ops and provide filter functions so we can
> utilize util_filter_walk() and share common code.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++--------------
> --------
>  util/filter.h  |    6 ++++++
>  2 files changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/ndctl/region.c b/ndctl/region.c
> index 9fc90808..9fd07af6 100644
> --- a/ndctl/region.c
> +++ b/ndctl/region.c
> @@ -19,10 +19,7 @@
>  #include <util/parse-options.h>
>  #include <ndctl/libndctl.h>
>  
> -static struct {
> -	const char *bus;
> -	const char *type;
> -} param;
> +struct util_filter_params param;
>  
>  static const struct option region_options[] = {
>  	OPT_STRING('b', "bus", &param.bus, "bus-id",
> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region,
> enum device_action mode)
>  	return 0;
>  }
>  
> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
> *ctx)
> +{
> +	return true;
> +}
> +

Instead of creating these trivial functions everywhere (also applies to
namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix
util_filter_walk to check for fctx->ptr != NULL any time it calls one of
the functions..

> +static bool filter_region(struct ndctl_region *region,
> +		struct util_filter_ctx *ctx)
> +{
> +	struct rgaction_filter_arg *rfa = ctx->rgaction;
> +	int rc;
> +
> +	if (rfa->rc < 0)
> +		return false;
> +
> +	rc = region_action(region, rfa->action);
> +
> +	if (rc == 0)
> +		rfa->rc++;
> +	else
> +		rfa->rc = 0;
> +
> +	/* we don't need to fall through, can continue the loop */
> +	return false;
> +}
> +
>  static int do_xable_region(const char *region_arg, enum device_action
> mode,
>  		struct ndctl_ctx *ctx)
>  {
> -	int rc = -ENXIO, success = 0;
> -	struct ndctl_region *region;
> -	struct ndctl_bus *bus;
> +	int rc = -ENXIO;
> +	struct util_filter_ctx fctx = { 0 };
> +	struct rgaction_filter_arg rfa = { 0 };
>  
>  	if (!region_arg)
>  		goto out;
>  
> -        ndctl_bus_foreach(ctx, bus) {
> -		if (!util_bus_filter(bus, param.bus))
> -			continue;
> -
> -		ndctl_region_foreach(bus, region) {
> -			const char *type =
> ndctl_region_get_type_name(region);
> -
> -			if (param.type && strcmp(param.type, type) != 0)
> -				continue;
> -			if (!util_region_filter(region, region_arg))
> -				continue;
> -			if (region_action(region, mode) == 0)
> -				success++;
> -		}
> -	}
> +	fctx.filter_bus = filter_bus;
> +	fctx.filter_region = filter_region;
> +	fctx.rgaction = &rfa;
> +	fctx.rgaction->action = mode;
> +	rc = util_filter_walk(ctx, &fctx, &param);
> +	if (rc == 0)
> +		rc = fctx.rgaction->rc;
>  
> -	rc = success;
>   out:
>  	param.bus = NULL;
>  	return rc;
> diff --git a/util/filter.h b/util/filter.h
> index 9f3786a9..0e0e3bec 100644
> --- a/util/filter.h
> +++ b/util/filter.h
> @@ -57,6 +57,11 @@ struct nsaction_filter_arg {
>  	int rc;
>  };
>  
> +struct rgaction_filter_arg {
> +	enum device_action action;
> +	int rc;
> +};
> +
>  /*
>   * struct util_filter_ctx - control and callbacks for util_filter_walk()
>   * ->filter_bus() and ->filter_region() return bool because the
> @@ -75,6 +80,7 @@ struct util_filter_ctx {
>  		void *arg;
>  		struct list_filter_arg *list;
>  		struct nsaction_filter_arg *nsaction;
> +		struct rgaction_filter_arg *rgaction;
>  	};
>  };
>  
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Dan Williams]

On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
>> util_filter_walk() does the looping through of busses and regions.
>> Removing
>> duplicate code in region ops and provide filter functions so we can
>> utilize util_filter_walk() and share common code.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++--------------
>> --------
>>  util/filter.h  |    6 ++++++
>>  2 files changed, 42 insertions(+), 23 deletions(-)
>>
>> diff --git a/ndctl/region.c b/ndctl/region.c
>> index 9fc90808..9fd07af6 100644
>> --- a/ndctl/region.c
>> +++ b/ndctl/region.c
>> @@ -19,10 +19,7 @@
>>  #include <util/parse-options.h>
>>  #include <ndctl/libndctl.h>
>>
>> -static struct {
>> -     const char *bus;
>> -     const char *type;
>> -} param;
>> +struct util_filter_params param;
>>
>>  static const struct option region_options[] = {
>>       OPT_STRING('b', "bus", &param.bus, "bus-id",
>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region,
>> enum device_action mode)
>>       return 0;
>>  }
>>
>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
>> *ctx)
>> +{
>> +     return true;
>> +}
>> +
>
> Instead of creating these trivial functions everywhere (also applies to
> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix
> util_filter_walk to check for fctx->ptr != NULL any time it calls one of
> the functions..

I think I'd prefer a common nop routine. That way individual are
forced to decide to use the nop or implement something. Leaving it
NULL may just be a programming mistake. I.e. it's harder to get wrong
if it's required.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Dave Jiang]

On 05/09/2018 04:23 PM, Dan Williams wrote:
> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L
> <vishal.l.verma@intel.com> wrote:
>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
>>> util_filter_walk() does the looping through of busses and regions.
>>> Removing
>>> duplicate code in region ops and provide filter functions so we can
>>> utilize util_filter_walk() and share common code.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++--------------
>>> --------
>>>  util/filter.h  |    6 ++++++
>>>  2 files changed, 42 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/ndctl/region.c b/ndctl/region.c
>>> index 9fc90808..9fd07af6 100644
>>> --- a/ndctl/region.c
>>> +++ b/ndctl/region.c
>>> @@ -19,10 +19,7 @@
>>>  #include <util/parse-options.h>
>>>  #include <ndctl/libndctl.h>
>>>
>>> -static struct {
>>> -     const char *bus;
>>> -     const char *type;
>>> -} param;
>>> +struct util_filter_params param;
>>>
>>>  static const struct option region_options[] = {
>>>       OPT_STRING('b', "bus", &param.bus, "bus-id",
>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region,
>>> enum device_action mode)
>>>       return 0;
>>>  }
>>>
>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
>>> *ctx)
>>> +{
>>> +     return true;
>>> +}
>>> +
>>
>> Instead of creating these trivial functions everywhere (also applies to
>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix
>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of
>> the functions..
> 
> I think I'd prefer a common nop routine. That way individual are
> forced to decide to use the nop or implement something. Leaving it
> NULL may just be a programming mistake. I.e. it's harder to get wrong
> if it's required.
> 

I'll add a common nop routine.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Dan Williams]

On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 05/09/2018 04:23 PM, Dan Williams wrote:
>> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L
>> <vishal.l.verma@intel.com> wrote:
>>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
>>>> util_filter_walk() does the looping through of busses and regions.
>>>> Removing
>>>> duplicate code in region ops and provide filter functions so we can
>>>> utilize util_filter_walk() and share common code.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++--------------
>>>> --------
>>>>  util/filter.h  |    6 ++++++
>>>>  2 files changed, 42 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/ndctl/region.c b/ndctl/region.c
>>>> index 9fc90808..9fd07af6 100644
>>>> --- a/ndctl/region.c
>>>> +++ b/ndctl/region.c
>>>> @@ -19,10 +19,7 @@
>>>>  #include <util/parse-options.h>
>>>>  #include <ndctl/libndctl.h>
>>>>
>>>> -static struct {
>>>> -     const char *bus;
>>>> -     const char *type;
>>>> -} param;
>>>> +struct util_filter_params param;
>>>>
>>>>  static const struct option region_options[] = {
>>>>       OPT_STRING('b', "bus", &param.bus, "bus-id",
>>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region,
>>>> enum device_action mode)
>>>>       return 0;
>>>>  }
>>>>
>>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
>>>> *ctx)
>>>> +{
>>>> +     return true;
>>>> +}
>>>> +
>>>
>>> Instead of creating these trivial functions everywhere (also applies to
>>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix
>>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of
>>> the functions..
>>
>> I think I'd prefer a common nop routine. That way individual are
>> forced to decide to use the nop or implement something. Leaving it
>> NULL may just be a programming mistake. I.e. it's harder to get wrong
>> if it's required.
>>
>
> I'll add a common nop routine.

...but wait why would it be a nop in this case. Region action commands
take a --bus= option?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Verma, Vishal L]

On Wed, 2018-05-09 at 16:26 -0700, Dan Williams wrote:
> On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote:
> > 
> > 
> > On 05/09/2018 04:23 PM, Dan Williams wrote:
> > > On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L
> > > <vishal.l.verma@intel.com> wrote:
> > > > On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
> > > > > util_filter_walk() does the looping through of busses and
> > > > > regions.
> > > > > Removing
> > > > > duplicate code in region ops and provide filter functions so we
> > > > > can
> > > > > utilize util_filter_walk() and share common code.
> > > > > 
> > > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > > > ---
> > > > >  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++------
> > > > > --------
> > > > > --------
> > > > >  util/filter.h  |    6 ++++++
> > > > >  2 files changed, 42 insertions(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/ndctl/region.c b/ndctl/region.c
> > > > > index 9fc90808..9fd07af6 100644
> > > > > --- a/ndctl/region.c
> > > > > +++ b/ndctl/region.c
> > > > > @@ -19,10 +19,7 @@
> > > > >  #include <util/parse-options.h>
> > > > >  #include <ndctl/libndctl.h>
> > > > > 
> > > > > -static struct {
> > > > > -     const char *bus;
> > > > > -     const char *type;
> > > > > -} param;
> > > > > +struct util_filter_params param;
> > > > > 
> > > > >  static const struct option region_options[] = {
> > > > >       OPT_STRING('b', "bus", &param.bus, "bus-id",
> > > > > @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region
> > > > > *region,
> > > > > enum device_action mode)
> > > > >       return 0;
> > > > >  }
> > > > > 
> > > > > +static bool filter_bus(struct ndctl_bus *bus, struct
> > > > > util_filter_ctx
> > > > > *ctx)
> > > > > +{
> > > > > +     return true;
> > > > > +}
> > > > > +
> > > > 
> > > > Instead of creating these trivial functions everywhere (also
> > > > applies to
> > > > namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL.
> > > > And fix
> > > > util_filter_walk to check for fctx->ptr != NULL any time it calls
> > > > one of
> > > > the functions..
> > > 
> > > I think I'd prefer a common nop routine. That way individual are
> > > forced to decide to use the nop or implement something. Leaving it
> > > NULL may just be a programming mistake. I.e. it's harder to get wrong
> > > if it's required.
> > > 
> > 
> > I'll add a common nop routine.
> 
> ...but wait why would it be a nop in this case. Region action commands
> take a --bus= option?

Ah yeah, and so do namespace and dimm actions right?

Also, should we then error out in the util function in case a NULL is
encountered? Should be better than just dereferencing it..
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Dave Jiang]

On 05/09/2018 04:26 PM, Dan Williams wrote:
> On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>
>> On 05/09/2018 04:23 PM, Dan Williams wrote:
>>> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L
>>> <vishal.l.verma@intel.com> wrote:
>>>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
>>>>> util_filter_walk() does the looping through of busses and regions.
>>>>> Removing
>>>>> duplicate code in region ops and provide filter functions so we can
>>>>> utilize util_filter_walk() and share common code.
>>>>>
>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>> ---
>>>>>  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++--------------
>>>>> --------
>>>>>  util/filter.h  |    6 ++++++
>>>>>  2 files changed, 42 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/ndctl/region.c b/ndctl/region.c
>>>>> index 9fc90808..9fd07af6 100644
>>>>> --- a/ndctl/region.c
>>>>> +++ b/ndctl/region.c
>>>>> @@ -19,10 +19,7 @@
>>>>>  #include <util/parse-options.h>
>>>>>  #include <ndctl/libndctl.h>
>>>>>
>>>>> -static struct {
>>>>> -     const char *bus;
>>>>> -     const char *type;
>>>>> -} param;
>>>>> +struct util_filter_params param;
>>>>>
>>>>>  static const struct option region_options[] = {
>>>>>       OPT_STRING('b', "bus", &param.bus, "bus-id",
>>>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region,
>>>>> enum device_action mode)
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
>>>>> *ctx)
>>>>> +{
>>>>> +     return true;
>>>>> +}
>>>>> +
>>>>
>>>> Instead of creating these trivial functions everywhere (also applies to
>>>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix
>>>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of
>>>> the functions..
>>>
>>> I think I'd prefer a common nop routine. That way individual are
>>> forced to decide to use the nop or implement something. Leaving it
>>> NULL may just be a programming mistake. I.e. it's harder to get wrong
>>> if it's required.
>>>
>>
>> I'll add a common nop routine.
> 
> ...but wait why would it be a nop in this case. Region action commands
> take a --bus= option?
> 

It just passes through by return true because it doesn't do anything to
do the bus? The util_bus_filter() call before it filters appropriate bus
right?

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

Re: [PATCH v5 3/4] ndctl: convert region actions to use util_filter_walk()

[Dan Williams]

On Fri, May 11, 2018 at 12:52 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 05/09/2018 04:26 PM, Dan Williams wrote:
>> On Wed, May 9, 2018 at 4:24 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>>>
>>>
>>> On 05/09/2018 04:23 PM, Dan Williams wrote:
>>>> On Wed, May 9, 2018 at 4:17 PM, Verma, Vishal L
>>>> <vishal.l.verma@intel.com> wrote:
>>>>> On Fri, 2018-04-27 at 15:08 -0700, Dave Jiang wrote:
>>>>>> util_filter_walk() does the looping through of busses and regions.
>>>>>> Removing
>>>>>> duplicate code in region ops and provide filter functions so we can
>>>>>> utilize util_filter_walk() and share common code.
>>>>>>
>>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>>> ---
>>>>>>  ndctl/region.c |   59 ++++++++++++++++++++++++++++++++++--------------
>>>>>> --------
>>>>>>  util/filter.h  |    6 ++++++
>>>>>>  2 files changed, 42 insertions(+), 23 deletions(-)
>>>>>>
>>>>>> diff --git a/ndctl/region.c b/ndctl/region.c
>>>>>> index 9fc90808..9fd07af6 100644
>>>>>> --- a/ndctl/region.c
>>>>>> +++ b/ndctl/region.c
>>>>>> @@ -19,10 +19,7 @@
>>>>>>  #include <util/parse-options.h>
>>>>>>  #include <ndctl/libndctl.h>
>>>>>>
>>>>>> -static struct {
>>>>>> -     const char *bus;
>>>>>> -     const char *type;
>>>>>> -} param;
>>>>>> +struct util_filter_params param;
>>>>>>
>>>>>>  static const struct option region_options[] = {
>>>>>>       OPT_STRING('b', "bus", &param.bus, "bus-id",
>>>>>> @@ -92,33 +89,49 @@ static int region_action(struct ndctl_region *region,
>>>>>> enum device_action mode)
>>>>>>       return 0;
>>>>>>  }
>>>>>>
>>>>>> +static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx
>>>>>> *ctx)
>>>>>> +{
>>>>>> +     return true;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Instead of creating these trivial functions everywhere (also applies to
>>>>> namespaces.c, dimm.c), should we just leave fctx.bus_filter NULL. And fix
>>>>> util_filter_walk to check for fctx->ptr != NULL any time it calls one of
>>>>> the functions..
>>>>
>>>> I think I'd prefer a common nop routine. That way individual are
>>>> forced to decide to use the nop or implement something. Leaving it
>>>> NULL may just be a programming mistake. I.e. it's harder to get wrong
>>>> if it's required.
>>>>
>>>
>>> I'll add a common nop routine.
>>
>> ...but wait why would it be a nop in this case. Region action commands
>> take a --bus= option?
>>
>
> It just passes through by return true because it doesn't do anything to
> do the bus? The util_bus_filter() call before it filters appropriate bus
> right?

Ah true, sorry, I confused myself on when the filter should return false.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm