| Message ID | CAPcyv4hc5OYNSJT2PpsCH-QcjtBFkMoc-D3TKvAL5UyB9ZZYHQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> >> 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.
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.
> > 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.
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.
> -----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 --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;