On Tue, Apr 17, 2018 at 8:29 AM, Qi, Fuli <[email protected]> wrote:
>
>> -----Original Message-----
>> From: Dan Williams [mailto:[email protected]]
>> Sent: Tuesday, April 17, 2018 11:57 PM
>> To: Qi, Fuli/斉 福利 <[email protected]>
>> Cc: linux-nvdimm <[email protected]>
>> 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 <[email protected]> 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 <[email protected]>
>> > ---
>> > 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
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
