diff mbox

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

Message ID CAPcyv4hc5OYNSJT2PpsCH-QcjtBFkMoc-D3TKvAL5UyB9ZZYHQ@mail.gmail.com
State New, archived
Headers show

Commit Message

Dan Williams April 17, 2018, 4:32 p.m. UTC
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:

 }

Comments

QI Fuli April 18, 2018, 10:09 a.m. UTC | #1
> >> 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.
Dan Williams April 18, 2018, 2:30 p.m. UTC | #2
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.
QI Fuli April 19, 2018, 2:06 a.m. UTC | #3
> > 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.
Dan Williams April 19, 2018, 2:09 a.m. UTC | #4
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.
QI Fuli April 19, 2018, 2:18 a.m. UTC | #5
> -----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.
diff mbox

Patch

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;