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

[PATCH v6 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.

---
v6:
- removed unintended changes (Vishal)
- added common function filter_bus_passhthrough() (Vishal, Dan)

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      |   78 +++++++++++++---------
 ndctl/namespace.c |  189 +++++++++++++++++++++++++++++------------------------
 ndctl/region.c    |   54 +++++++++------
 util/filter.c     |   11 +++
 util/filter.h     |   25 +++++++
 5 files changed, 212 insertions(+), 145 deletions(-)

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

[PATCH v6 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

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

[Ross Zwisler]

On Fri, May 11, 2018 at 01:24:41PM -0700, Dave Jiang wrote:
> 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;

Why not use the 'mode' in uf_param as well?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v6 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 |  158 +++++++++++++++++++++++++++++------------------------
 util/filter.c     |   11 +++-
 util/filter.h     |   10 +++
 3 files changed, 106 insertions(+), 73 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index e61f500c..1502a73a 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -983,14 +983,86 @@ 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_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 +1070,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_passthrough;
+	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/util/filter.c b/util/filter.c
index 1734bce5..c6ded8ab 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>
@@ -430,7 +431,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,
@@ -449,7 +450,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)
@@ -467,3 +468,9 @@ int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 	}
 	return 0;
 }
+
+bool filter_bus_passthrough(struct ndctl_bus *bus,
+		struct util_filter_ctx *ctx)
+{
+	return true;
+}
diff --git a/util/filter.h b/util/filter.h
index effda24b..e59bb8da 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;
 	};
 };
 
@@ -83,4 +91,6 @@ struct util_filter_params {
 struct ndctl_ctx;
 int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 		struct util_filter_params *param);
+bool filter_bus_passthrough(struct ndctl_bus *bus,
+		struct util_filter_ctx *ctx);
 #endif

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

[PATCH v6 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 |   54 +++++++++++++++++++++++++++++++-----------------------
 util/filter.h  |    6 ++++++
 2 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/ndctl/region.c b/ndctl/region.c
index 9fc90808..fa8f15ad 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,44 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
 	return 0;
 }
 
+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_passthrough;
+	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 e59bb8da..9aede013 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 v6 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  |   78 +++++++++++++++++++++++++++++++++------------------------
 util/filter.h |    9 +++++++
 2 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 1c168997..0b43dc1a 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,43 @@ static const struct option init_options[] = {
 	OPT_END(),
 };
 
+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 +1065,31 @@ static int dimm_action(int argc, const char **argv, void *ctx,
 		goto out;
 	}
 
+	fctx.filter_bus = filter_bus_passthrough;
+	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 +1107,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 9aede013..e0f83cbf 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 v6 0/4] ndctl: convert actions to use util_filter_walk

[Ross Zwisler]

On Fri, May 11, 2018 at 01:24:36PM -0700, Dave Jiang wrote:
> 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.

<>

>  ndctl/dimm.c      |   78 +++++++++++++---------
>  ndctl/namespace.c |  189 +++++++++++++++++++++++++++++------------------------
>  ndctl/region.c    |   54 +++++++++------
>  util/filter.c     |   11 +++
>  util/filter.h     |   25 +++++++
>  5 files changed, 212 insertions(+), 145 deletions(-)

Just from the cover letter I'm sort of confused - the paragraph above says we
are converging on common code and removing individual versions of the same
code...but the patch series grows this code by 70 lines?  I would have
expected this to be a net code reduction?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

On Fri, May 11, 2018 at 6:34 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Fri, May 11, 2018 at 01:24:36PM -0700, Dave Jiang wrote:
>> 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.
>
> <>
>
>>  ndctl/dimm.c      |   78 +++++++++++++---------
>>  ndctl/namespace.c |  189 +++++++++++++++++++++++++++++------------------------
>>  ndctl/region.c    |   54 +++++++++------
>>  util/filter.c     |   11 +++
>>  util/filter.h     |   25 +++++++
>>  5 files changed, 212 insertions(+), 145 deletions(-)
>
> Just from the cover letter I'm sort of confused - the paragraph above says we
> are converging on common code and removing individual versions of the same
> code...but the patch series grows this code by 70 lines?  I would have
> expected this to be a net code reduction?

It's a fair point about the changelog relative to the diffstat. I
assume we are also adding the common object looping pattern as part of
this conversion, so we're gaining functionality?

I.e. does:

    ndctl disable-region region0 region1

...work now? Previously it was only a singleton. I thought that was
the motivation?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm