[PATCH 0/2] ndctl: change ndctl to support multiple arguments per option

[PATCH 0/2] ndctl: change ndctl to support multiple arguments per option

[QI Fuli]

This patch set is to support multiple arguments per option of ndctl.
Currently, ndctl supports only one argument per option, and only one dimm
can be filtered by dimm's name. As a result, when users want to moniotr
multiple dimms, they have to run multiple monitor processes. This feature
can let monitor support filter multiple dimms by dimms' name in one process.
Also applied in ndctl list:

 # ndclt list --dimm="nmem0 nmem1 nmem2"
  [
    {
      "dev":"nmem1",
      "id":"cdab-0a-07e0-feffffff",
      "handle":1,
      "phys_id":1
    },
    {
      "dev":"nmem0",
      "id":"cdab-0a-07e0-ffffffff",
      "handle":0,
      "phys_id":0
    },
    {
      "dev":"nmem2",
      "id":"cdab-0a-07e0-fffeffff",
      "handle":256,
      "phys_id":2
    }
  ]

QI Fuli (2):
  ndctl, util: add OPT_STRING_LIST to parse_option
  ndctl, list: change -b -d -r -n options to support multiple arguments

 ccan/list/list.h     |  6 ++++
 ndctl/list.c         | 35 ++++++++++++++-----
 util/filter.c        | 81 +++++++++++++++++++++++++++++++-------------
 util/filter.h        |  9 ++---
 util/parse-options.c | 25 ++++++++++++++
 util/parse-options.h |  3 ++
 6 files changed, 122 insertions(+), 37 deletions(-)

-- 
2.17.0.140.g0b0cc9f86


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

[PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[QI Fuli]

This patch adds OPT_STRING_LIST to parse_option in order to support
multiple space seperated string objects in one option.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 ccan/list/list.h     |  6 ++++++
 util/parse-options.c | 25 +++++++++++++++++++++++++
 util/parse-options.h |  3 +++
 3 files changed, 34 insertions(+)

diff --git a/ccan/list/list.h b/ccan/list/list.h
index 4d1d34e..f6c927f 100644
--- a/ccan/list/list.h
+++ b/ccan/list/list.h
@@ -26,6 +26,12 @@ struct list_node
 	struct list_node *next, *prev;
 };
 
+struct string_list_node
+{
+	char *str;
+	struct list_node list;
+};
+
 /**
  * struct list_head - the head of a doubly-linked list
  * @h: the list_head (containing next and prev pointers)
diff --git a/util/parse-options.c b/util/parse-options.c
index 751c091..cac18f0 100644
--- a/util/parse-options.c
+++ b/util/parse-options.c
@@ -20,6 +20,7 @@
 #include <util/util.h>
 #include <util/strbuf.h>
 #include <util/parse-options.h>
+#include <ccan/list/list.h>
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -695,3 +696,27 @@ int parse_opt_verbosity_cb(const struct option *opt,
 	}
 	return 0;
 }
+
+int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		return 0;
+
+	if (!arg)
+		return -1;
+
+	struct list_head *v = opt->value;
+	char *temp = strdup(arg);
+	const char *deli = " ";
+
+	temp = strtok(temp, deli);
+	while (temp != NULL) {
+		struct string_list_node *sln = malloc(sizeof(*sln));
+		sln->str = temp;
+		list_add_tail(v, &sln->list);
+		temp = strtok(NULL, deli);
+	}
+
+	free(temp);
+	return 0;
+}
diff --git a/util/parse-options.h b/util/parse-options.h
index 6fd6b24..10b79cb 100644
--- a/util/parse-options.h
+++ b/util/parse-options.h
@@ -135,6 +135,8 @@ struct option {
 #define OPT_LONG(s, l, v, h)        { .type = OPTION_LONG, .short_name = (s), .long_name = (l), .value = check_vtype(v, long *), .help = (h) }
 #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
 #define OPT_STRING(s, l, v, a, h)   { .type = OPTION_STRING,  .short_name = (s), .long_name = (l), .value = check_vtype(v, const char **), (a), .help = (h) }
+#define OPT_STRING_LIST(s, l, v, a, h) \
+	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = check_vtype(v, struct list_head *), (a), .help = (h) , 0, &parse_opt_string_list }
 #define OPT_DATE(s, l, v, h) \
 	{ .type = OPTION_CALLBACK, .short_name = (s), .long_name = (l), .value = (v), .argh = "time", .help = (h), .callback = parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
@@ -206,6 +208,7 @@ extern int parse_options_end(struct parse_opt_ctx_t *ctx);
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
+extern int parse_opt_string_list(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var)  OPT_BOOLEAN('v', "verbose", (var), "be verbose")
 #define OPT__QUIET(var)    OPT_BOOLEAN('q', "quiet",   (var), "be quiet")
-- 
2.17.0.140.g0b0cc9f86


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

[PATCH 2/2] ndctl, list: change -b -d -r -n options to support multiple arguments

[QI Fuli]

This patch changes --bus, --dimm, --region --namespace options to
OPT_STRING_LIST in order to support multiple arguments. 
Users can specify multiple dimm like following:

	ndctl list --dimm="nmem1 nmem2" --bus="ndbus0 ndbus1"
	ndctl list -d "nmem1 nmem2" -b "ndbus0 ndbus1"

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 ndctl/list.c  | 35 ++++++++++++++++------
 util/filter.c | 81 ++++++++++++++++++++++++++++++++++++---------------
 util/filter.h |  9 +++---
 3 files changed, 88 insertions(+), 37 deletions(-)

diff --git a/ndctl/list.c b/ndctl/list.c
index 6cf7c39..14da736 100644
--- a/ndctl/list.c
+++ b/ndctl/list.c
@@ -129,11 +129,23 @@ static struct json_object *region_to_json(struct ndctl_region *region,
 	ndctl_mapping_foreach(region, mapping) {
 		struct ndctl_dimm *dimm = ndctl_mapping_get_dimm(mapping);
 		struct json_object *jmapping;
+		struct string_list_node *sln;
+		bool flag = false;
 
 		if (!list.dimms)
 			break;
 
-		if (!util_dimm_filter(dimm, param.dimm))
+		if (!list_empty(&param.dimm)) {
+			list_for_each(&param.dimm, sln, list) {
+				if (util_dimm_filter(dimm, sln->str)) {
+					flag = true;
+					break;
+				}
+			}
+		} else
+			flag = true;
+
+		if (!flag)
 			continue;
 
 		if (!list.idle && !ndctl_dimm_is_enabled(dimm))
@@ -415,14 +427,19 @@ static int num_list_flags(void)
 
 int cmd_list(int argc, const char **argv, void *ctx)
 {
+	list_head_init(&param.bus);
+	list_head_init(&param.dimm);
+	list_head_init(&param.region);
+	list_head_init(&param.namespace);
 	const struct option options[] = {
-		OPT_STRING('b', "bus", &param.bus, "bus-id", "filter by bus"),
-		OPT_STRING('r', "region", &param.region, "region-id",
+		OPT_STRING_LIST('b', "bus", &param.bus, "bus-id",
+				"filter by bus"),
+		OPT_STRING_LIST('r', "region", &param.region, "region-id",
 				"filter by region"),
-		OPT_STRING('d', "dimm", &param.dimm, "dimm-id",
+		OPT_STRING_LIST('d', "dimm", &param.dimm, "dimm-id",
 				"filter by dimm"),
-		OPT_STRING('n', "namespace", &param.namespace, "namespace-id",
-				"filter by namespace id"),
+		OPT_STRING_LIST('n', "namespace", &param.namespace,
+				"namespace-id", "filter by namespace id"),
 		OPT_STRING('m', "mode", &param.mode, "namespace-mode",
 				"filter by namespace mode"),
 		OPT_STRING('t', "type", &param.type, "region-type",
@@ -461,9 +478,9 @@ int cmd_list(int argc, const char **argv, void *ctx)
 		usage_with_options(u, options);
 
 	if (num_list_flags() == 0) {
-		list.buses = !!param.bus;
-		list.regions = !!param.region;
-		list.dimms = !!param.dimm;
+		list.buses = !list_empty(&param.bus);
+		list.regions = !list_empty(&param.region);
+		list.dimms = !list_empty(&param.dimm);
 		if (list.dax && !param.mode)
 			param.mode = "dax";
 	}
diff --git a/util/filter.c b/util/filter.c
index 6ab391a..3313f53 100644
--- a/util/filter.c
+++ b/util/filter.c
@@ -303,13 +303,47 @@ static enum ndctl_namespace_mode mode_to_type(const char *mode)
 	return NDCTL_NS_MODE_UNKNOWN;
 }
 
+#define util_field_filter_by_list(field, list_head) \
+if (!list_empty(list_head)) { \
+	list_for_each(list_head, sln, list) { \
+		if (util_##field##_filter(field, sln->str)) { \
+			flag = true; \
+			break; \
+		} \
+	} \
+} else \
+	flag = true; \
+\
+if (flag) \
+	flag = false; \
+else \
+	continue;
+
+#define util_field_filter_by_list_item(field, item, list_head) \
+if (!list_empty(list_head)) { \
+	list_for_each(list_head, sln, list) { \
+		if (util_##field##_filter_by_##item(field, sln->str)) { \
+			flag = true; \
+			break; \
+		} \
+	} \
+} else \
+	flag = true; \
+\
+if (flag) \
+	flag = false; \
+else \
+	continue;
+
 int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 		struct util_filter_params *param)
 {
 	struct ndctl_bus *bus;
+	struct string_list_node *sln;
 	unsigned int type = 0;
 	int numa_node = NUMA_NO_NODE;
 	char *end = NULL;
+	bool flag = false;
 
 	if (param->type && (strcmp(param->type, "pmem") != 0
 				&& strcmp(param->type, "blk") != 0)) {
@@ -349,11 +383,11 @@ int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 		struct ndctl_region *region;
 		struct ndctl_dimm *dimm;
 
-		if (!util_bus_filter(bus, param->bus)
-				|| !util_bus_filter_by_dimm(bus, param->dimm)
-				|| !util_bus_filter_by_region(bus, param->region)
-				|| !util_bus_filter_by_namespace(bus, param->namespace))
-			continue;
+		util_field_filter_by_list(bus, &param->bus);
+		util_field_filter_by_list_item(bus, dimm, &param->dimm);
+		util_field_filter_by_list_item(bus, region, &param->region);
+		util_field_filter_by_list_item(bus, namespace,
+					&param->namespace);
 
 		if (!fctx->filter_bus(bus, fctx))
 			continue;
@@ -362,27 +396,25 @@ int util_filter_walk(struct ndctl_ctx *ctx, struct util_filter_ctx *fctx,
 			if (!fctx->filter_dimm)
 				break;
 
-			if (!util_dimm_filter(dimm, param->dimm)
-					|| !util_dimm_filter_by_region(dimm,
-						param->region)
-					|| !util_dimm_filter_by_namespace(dimm,
-						param->namespace)
-					|| !util_dimm_filter_by_numa_node(dimm,
-						numa_node))
+			util_field_filter_by_list(dimm, &param->dimm);
+			util_field_filter_by_list_item(dimm, region,
+					&param->region);
+			util_field_filter_by_list_item(dimm, namespace,
+					&param->namespace);
+			if (!util_dimm_filter_by_numa_node(dimm, numa_node))
 				continue;
 
 			fctx->filter_dimm(dimm, fctx);
 		}
 
 		ndctl_region_foreach(bus, region) {
-			struct ndctl_namespace *ndns;
+			struct ndctl_namespace *namespace;
 
-			if (!util_region_filter(region, param->region)
-					|| !util_region_filter_by_dimm(region,
-						param->dimm)
-					|| !util_region_filter_by_namespace(region,
-						param->namespace))
-				continue;
+			util_field_filter_by_list(region, &param->region);
+			util_field_filter_by_list_item(region, dimm,
+					&param->dimm);
+			util_field_filter_by_list_item(region, namespace,
+					&param->namespace);
 
 			if (numa_node != NUMA_NO_NODE &&
 			    ndctl_region_get_numa_node(region) != numa_node)
@@ -394,19 +426,20 @@ 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(region, namespace) {
 				enum ndctl_namespace_mode mode;
 
 				if (!fctx->filter_namespace)
 					break;
-				if (!util_namespace_filter(ndns, param->namespace))
-					continue;
 
-				mode = ndctl_namespace_get_mode(ndns);
+				util_field_filter_by_list(namespace,
+						&param->namespace);
+
+				mode = ndctl_namespace_get_mode(namespace);
 				if (param->mode && mode_to_type(param->mode) != mode)
 					continue;
 
-				fctx->filter_namespace(ndns, fctx);
+				fctx->filter_namespace(namespace, fctx);
 			}
 		}
 	}
diff --git a/util/filter.h b/util/filter.h
index effda24..4b21cb3 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 <ccan/list/list.h>
 
 struct ndctl_bus *util_bus_filter(struct ndctl_bus *bus, const char *ident);
 struct ndctl_region *util_region_filter(struct ndctl_region *region,
@@ -71,12 +72,12 @@ struct util_filter_ctx {
 };
 
 struct util_filter_params {
-	const char *bus;
-	const char *region;
+	struct list_head bus;
+	struct list_head dimm;
+	struct list_head region;
+	struct list_head namespace;
 	const char *type;
-	const char *dimm;
 	const char *mode;
-	const char *namespace;
 	const char *numa_node;
 };
 
-- 
2.17.0.140.g0b0cc9f86


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

Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Dan Williams]

On Mon, Apr 16, 2018 at 11:38 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> This patch adds OPT_STRING_LIST to parse_option in order to support
> multiple space seperated string objects in one option.
>
> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> ---
>  ccan/list/list.h     |  6 ++++++
>  util/parse-options.c | 25 +++++++++++++++++++++++++
>  util/parse-options.h |  3 +++
>  3 files changed, 34 insertions(+)
>
> diff --git a/ccan/list/list.h b/ccan/list/list.h
> index 4d1d34e..f6c927f 100644
> --- a/ccan/list/list.h
> +++ b/ccan/list/list.h
> @@ -26,6 +26,12 @@ struct list_node
>         struct list_node *next, *prev;
>  };
>
> +struct string_list_node
> +{
> +       char *str;
> +       struct list_node list;
> +};
> +
>  /**
>   * struct list_head - the head of a doubly-linked list
>   * @h: the list_head (containing next and prev pointers)
> diff --git a/util/parse-options.c b/util/parse-options.c
> index 751c091..cac18f0 100644
> --- a/util/parse-options.c
> +++ b/util/parse-options.c
> @@ -20,6 +20,7 @@
>  #include <util/util.h>
>  #include <util/strbuf.h>
>  #include <util/parse-options.h>
> +#include <ccan/list/list.h>
>
>  #define OPT_SHORT 1
>  #define OPT_UNSET 2
> @@ -695,3 +696,27 @@ int parse_opt_verbosity_cb(const struct option *opt,
>         }
>         return 0;
>  }
> +
> +int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
> +{
> +       if (unset)
> +               return 0;
> +
> +       if (!arg)
> +               return -1;
> +
> +       struct list_head *v = opt->value;
> +       char *temp = strdup(arg);
> +       const char *deli = " ";
> +
> +       temp = strtok(temp, deli);
> +       while (temp != NULL) {
> +               struct string_list_node *sln = malloc(sizeof(*sln));
> +               sln->str = temp;
> +               list_add_tail(v, &sln->list);
> +               temp = strtok(NULL, deli);
> +       }
> +
> +       free(temp);
> +       return 0;
> +}

As far as I can see we do not need to allocate a list or add this new
OPT_STRING_LIST argument type. Just teach the util_<object>_filter()
routines that the 'ident' argument may be a space delimited list.  See
the attached patch:
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Qi, Fuli]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Tuesday, April 17, 2018 11:57 PM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option
> 
> On Mon, Apr 16, 2018 at 11:38 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
> > This patch adds OPT_STRING_LIST to parse_option in order to support
> > multiple space seperated string objects in one option.
> >
> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> > ---
> >  ccan/list/list.h     |  6 ++++++
> >  util/parse-options.c | 25 +++++++++++++++++++++++++
> > util/parse-options.h |  3 +++
> >  3 files changed, 34 insertions(+)
> >
> > diff --git a/ccan/list/list.h b/ccan/list/list.h index
> > 4d1d34e..f6c927f 100644
> > --- a/ccan/list/list.h
> > +++ b/ccan/list/list.h
> > @@ -26,6 +26,12 @@ struct list_node
> >         struct list_node *next, *prev;  };
> >
> > +struct string_list_node
> > +{
> > +       char *str;
> > +       struct list_node list;
> > +};
> > +
> >  /**
> >   * struct list_head - the head of a doubly-linked list
> >   * @h: the list_head (containing next and prev pointers) diff --git
> > a/util/parse-options.c b/util/parse-options.c index 751c091..cac18f0
> > 100644
> > --- a/util/parse-options.c
> > +++ b/util/parse-options.c
> > @@ -20,6 +20,7 @@
> >  #include <util/util.h>
> >  #include <util/strbuf.h>
> >  #include <util/parse-options.h>
> > +#include <ccan/list/list.h>
> >
> >  #define OPT_SHORT 1
> >  #define OPT_UNSET 2
> > @@ -695,3 +696,27 @@ int parse_opt_verbosity_cb(const struct option *opt,
> >         }
> >         return 0;
> >  }
> > +
> > +int parse_opt_string_list(const struct option *opt, const char *arg,
> > +int unset) {
> > +       if (unset)
> > +               return 0;
> > +
> > +       if (!arg)
> > +               return -1;
> > +
> > +       struct list_head *v = opt->value;
> > +       char *temp = strdup(arg);
> > +       const char *deli = " ";
> > +
> > +       temp = strtok(temp, deli);
> > +       while (temp != NULL) {
> > +               struct string_list_node *sln = malloc(sizeof(*sln));
> > +               sln->str = temp;
> > +               list_add_tail(v, &sln->list);
> > +               temp = strtok(NULL, deli);
> > +       }
> > +
> > +       free(temp);
> > +       return 0;
> > +}
> 
> As far as I can see we do not need to allocate a list or add this new
> OPT_STRING_LIST argument type. Just teach the util_<object>_filter() routines
> that the 'ident' argument may be a space delimited list.  See the attached patch:

Thank you for your comment.

The OPT_STRING_LIST is copied from git.
Consider multiple arguments per option should be supported not only in
monitor and list but also in other commands, as Vishal mentioned:
  "ndctl disable-namespace namespace1.0 namespace2.0 ..."

If you think this feature is not needed in other commands, I will delete
OPT_STRING_LIST and make a v2 patch.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Dan Williams]

On Tue, Apr 17, 2018 at 8:29 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
>
>> -----Original Message-----
>> From: Dan Williams [mailto:dan.j.williams@intel.com]
>> Sent: Tuesday, April 17, 2018 11:57 PM
>> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
>> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
>> Subject: Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option
>>
>> On Mon, Apr 16, 2018 at 11:38 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote:
>> > This patch adds OPT_STRING_LIST to parse_option in order to support
>> > multiple space seperated string objects in one option.
>> >
>> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
>> > ---
>> >  ccan/list/list.h     |  6 ++++++
>> >  util/parse-options.c | 25 +++++++++++++++++++++++++
>> > util/parse-options.h |  3 +++
>> >  3 files changed, 34 insertions(+)
>> >
>> > diff --git a/ccan/list/list.h b/ccan/list/list.h index
>> > 4d1d34e..f6c927f 100644
>> > --- a/ccan/list/list.h
>> > +++ b/ccan/list/list.h
>> > @@ -26,6 +26,12 @@ struct list_node
>> >         struct list_node *next, *prev;  };
>> >
>> > +struct string_list_node
>> > +{
>> > +       char *str;
>> > +       struct list_node list;
>> > +};
>> > +
>> >  /**
>> >   * struct list_head - the head of a doubly-linked list
>> >   * @h: the list_head (containing next and prev pointers) diff --git
>> > a/util/parse-options.c b/util/parse-options.c index 751c091..cac18f0
>> > 100644
>> > --- a/util/parse-options.c
>> > +++ b/util/parse-options.c
>> > @@ -20,6 +20,7 @@
>> >  #include <util/util.h>
>> >  #include <util/strbuf.h>
>> >  #include <util/parse-options.h>
>> > +#include <ccan/list/list.h>
>> >
>> >  #define OPT_SHORT 1
>> >  #define OPT_UNSET 2
>> > @@ -695,3 +696,27 @@ int parse_opt_verbosity_cb(const struct option *opt,
>> >         }
>> >         return 0;
>> >  }
>> > +
>> > +int parse_opt_string_list(const struct option *opt, const char *arg,
>> > +int unset) {
>> > +       if (unset)
>> > +               return 0;
>> > +
>> > +       if (!arg)
>> > +               return -1;
>> > +
>> > +       struct list_head *v = opt->value;
>> > +       char *temp = strdup(arg);
>> > +       const char *deli = " ";
>> > +
>> > +       temp = strtok(temp, deli);
>> > +       while (temp != NULL) {
>> > +               struct string_list_node *sln = malloc(sizeof(*sln));
>> > +               sln->str = temp;
>> > +               list_add_tail(v, &sln->list);
>> > +               temp = strtok(NULL, deli);
>> > +       }
>> > +
>> > +       free(temp);
>> > +       return 0;
>> > +}
>>
>> As far as I can see we do not need to allocate a list or add this new
>> OPT_STRING_LIST argument type. Just teach the util_<object>_filter() routines
>> that the 'ident' argument may be a space delimited list.  See the attached patch:
>
> Thank you for your comment.
>
> The OPT_STRING_LIST is copied from git.
> Consider multiple arguments per option should be supported not only in
> monitor and list but also in other commands, as Vishal mentioned:
>   "ndctl disable-namespace namespace1.0 namespace2.0 ..."

In this case there's no "-n" option so the list parsing is already
handled by standard shell argument parsing, i.e. the argv[] array
already has the list split.

> If you think this feature is not needed in other commands, I will delete
> OPT_STRING_LIST and make a v2 patch.

I can see OPT_STRING_LIST potentially being useful in other scenarios,
but for the util_<object>_filter functions it seems an awkward fit to
me. We end up doing the parsing twice. Once to parse the space
separated list and again to parse each entry against the object to be
filtered. In this case I think it is cleaner to handle it internally
to the utility functions.  It also makes the util functions more
generically useful as I can now use them in scenarios where the string
list is not coming from the command line.

Ross noted that the attachment got swallowed by the list, so here it
is pasted, please forgive any whitespace damage:

diff --git a/util/filter.c b/util/filter.c
index 6ab391a81d94..c37b62c7e858 100644
--- a/util/filter.c
+++ b/util/filter.c
@@ -98,28 +98,38 @@ struct ndctl_namespace
*util_namespace_filter(struct ndctl_namespace *ndns
        return NULL;
 }

-struct ndctl_dimm *util_dimm_filter(struct ndctl_dimm *dimm, const char *ident)
+struct ndctl_dimm *util_dimm_filter(struct ndctl_dimm *dimm, const
char *__ident)
 {
-       char *end = NULL;
-       const char *name;
+       char *end = NULL, *ident, *save;
+       const char *name, *dimm_name;
        unsigned long dimm_id, id;

-       if (!ident || strcmp(ident, "all") == 0)
+       if (!__ident || strcmp(__ident, "all") == 0)
                return dimm;

-       dimm_id = strtoul(ident, &end, 0);
-       if (end == ident || end[0])
-               dimm_id = ULONG_MAX;
+       ident = strdup(__ident);
+       if (!ident)
+               return NULL;

-       name = ndctl_dimm_get_devname(dimm);
-       id = ndctl_dimm_get_id(dimm);
+       for (name = strtok_r(ident, " ", &save); name;
+                       name = strtok_r(NULL, " ", &save)) {
+               dimm_id = strtoul(name, &end, 0);
+               if (end == ident || end[0])
+                       dimm_id = ULONG_MAX;

-       if (dimm_id < ULONG_MAX && dimm_id == id)
-               return dimm;
+               dimm_name = ndctl_dimm_get_devname(dimm);
+               id = ndctl_dimm_get_id(dimm);

-       if (dimm_id == ULONG_MAX && strcmp(ident, name) == 0)
-               return dimm;
+               if (dimm_id < ULONG_MAX && dimm_id == id)
+                       break;
+
+               if (dimm_id == ULONG_MAX && strcmp(dimm_name, name) == 0)
+                       break;
+       }
+       free(ident);

+       if (name)
+               return dimm;
        return NULL;
 }
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Qi, Fuli]

> >> As far as I can see we do not need to allocate a list or add this new
> >> OPT_STRING_LIST argument type. Just teach the util_<object>_filter()
> routines
> >> that the 'ident' argument may be a space delimited list.  See the attached
> patch:
> >
> > Thank you for your comment.
> >
> > The OPT_STRING_LIST is copied from git.
> > Consider multiple arguments per option should be supported not only in
> > monitor and list but also in other commands, as Vishal mentioned:
> >   "ndctl disable-namespace namespace1.0 namespace2.0 ..."
> 
> In this case there's no "-n" option so the list parsing is already
> handled by standard shell argument parsing, i.e. the argv[] array
> already has the list split.
> 
> > If you think this feature is not needed in other commands, I will delete
> > OPT_STRING_LIST and make a v2 patch.
> 
> I can see OPT_STRING_LIST potentially being useful in other scenarios,
> but for the util_<object>_filter functions it seems an awkward fit to
> me. We end up doing the parsing twice. Once to parse the space
> separated list and again to parse each entry against the object to be
> filtered. In this case I think it is cleaner to handle it internally
> to the utility functions.  It also makes the util functions more
> generically useful as I can now use them in scenarios where the string
> list is not coming from the command line.
> 
> Ross noted that the attachment got swallowed by the list, so here it
> is pasted, please forgive any whitespace damage:

Thank you very much, I made a v2 patch by referring to your sample patch.

Going back to the OPT_STRING_LIST, I think it is necessary for ndctl.
Because the other options in monitor like --dimm-event, --bus-event 
also need to support multiple space-separated arguments, these options 
should be made as a string_list.

One more question is that do you think it is necessary for ndctl to copy 
OPT_FILENAME in parse_option from git? The difference between OPT_FILENAME 
and OPT_STRING is that there is an additional fix_filename() in OPTION_FILENAME 
to process file's path. Considering that ndctl monitor also has file options like 
--logfile, --conf-file, I think we'd better to copy it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Dan Williams]

On Wed, Apr 18, 2018 at 3:09 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
>> >> As far as I can see we do not need to allocate a list or add this new
>> >> OPT_STRING_LIST argument type. Just teach the util_<object>_filter()
>> routines
>> >> that the 'ident' argument may be a space delimited list.  See the attached
>> patch:
>> >
>> > Thank you for your comment.
>> >
>> > The OPT_STRING_LIST is copied from git.
>> > Consider multiple arguments per option should be supported not only in
>> > monitor and list but also in other commands, as Vishal mentioned:
>> >   "ndctl disable-namespace namespace1.0 namespace2.0 ..."
>>
>> In this case there's no "-n" option so the list parsing is already
>> handled by standard shell argument parsing, i.e. the argv[] array
>> already has the list split.
>>
>> > If you think this feature is not needed in other commands, I will delete
>> > OPT_STRING_LIST and make a v2 patch.
>>
>> I can see OPT_STRING_LIST potentially being useful in other scenarios,
>> but for the util_<object>_filter functions it seems an awkward fit to
>> me. We end up doing the parsing twice. Once to parse the space
>> separated list and again to parse each entry against the object to be
>> filtered. In this case I think it is cleaner to handle it internally
>> to the utility functions.  It also makes the util functions more
>> generically useful as I can now use them in scenarios where the string
>> list is not coming from the command line.
>>
>> Ross noted that the attachment got swallowed by the list, so here it
>> is pasted, please forgive any whitespace damage:
>
> Thank you very much, I made a v2 patch by referring to your sample patch.
>
> Going back to the OPT_STRING_LIST, I think it is necessary for ndctl.
> Because the other options in monitor like --dimm-event, --bus-event
> also need to support multiple space-separated arguments, these options
> should be made as a string_list.

Ok, yes, lets bring in string_list for that case.

> One more question is that do you think it is necessary for ndctl to copy
> OPT_FILENAME in parse_option from git? The difference between OPT_FILENAME
> and OPT_STRING is that there is an additional fix_filename() in OPTION_FILENAME
> to process file's path. Considering that ndctl monitor also has file options like
> --logfile, --conf-file, I think we'd better to copy it.

Yes, let's copy git's lead for filename fixups.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Qi, Fuli]

> > Thank you very much, I made a v2 patch by referring to your sample patch.
> >
> > Going back to the OPT_STRING_LIST, I think it is necessary for ndctl.
> > Because the other options in monitor like --dimm-event, --bus-event
> > also need to support multiple space-separated arguments, these options
> > should be made as a string_list.
> 
> Ok, yes, lets bring in string_list for that case.
> 

Could you merge the OPT_STRING_LIST first?

> > One more question is that do you think it is necessary for ndctl to
> > copy OPT_FILENAME in parse_option from git? The difference between
> > OPT_FILENAME and OPT_STRING is that there is an additional
> > fix_filename() in OPTION_FILENAME to process file's path. Considering
> > that ndctl monitor also has file options like --logfile, --conf-file, I think we'd better
> to copy it.
> 
> Yes, let's copy git's lead for filename fixups.
> 
OK, I will make a patch for adding the OPTION_FILENAME to parse_option.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Dan Williams]

On Wed, Apr 18, 2018 at 7:06 PM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
>> > Thank you very much, I made a v2 patch by referring to your sample patch.
>> >
>> > Going back to the OPT_STRING_LIST, I think it is necessary for ndctl.
>> > Because the other options in monitor like --dimm-event, --bus-event
>> > also need to support multiple space-separated arguments, these options
>> > should be made as a string_list.
>>
>> Ok, yes, lets bring in string_list for that case.
>>
>
> Could you merge the OPT_STRING_LIST first?

Oh, I was assuming when you said string_list that you would redo the
patch to bring in the struct string_list implementation from git, and
not use the strtok() approach. I'd like to not diverge from that
implementation.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option

[Qi, Fuli]

> -----Original Message-----
> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Thursday, April 19, 2018 11:10 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>
> Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
> Subject: Re: [PATCH 1/2] ndctl, util: add OPT_STRING_LIST to parse_option
> 
> On Wed, Apr 18, 2018 at 7:06 PM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote:
> >> > Thank you very much, I made a v2 patch by referring to your sample patch.
> >> >
> >> > Going back to the OPT_STRING_LIST, I think it is necessary for ndctl.
> >> > Because the other options in monitor like --dimm-event, --bus-event
> >> > also need to support multiple space-separated arguments, these options
> >> > should be made as a string_list.
> >>
> >> Ok, yes, lets bring in string_list for that case.
> >>
> >
> > Could you merge the OPT_STRING_LIST first?
> 
> Oh, I was assuming when you said string_list that you would redo the
> patch to bring in the struct string_list implementation from git, and
> not use the strtok() approach. I'd like to not diverge from that
> implementation.
> 
Ok, I see. I will redo the patch to bring in the struct string_list from git.
Thank you.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm