| Message ID | 20190107083833.25941-3-qi.fuli@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ndctl, monitor: refactor read_config_file | expand |
On Mon, 2019-01-07 at 17:38 +0900, QI Fuli wrote: > This patch is used for refactoring read_config_file by replacing the > open coded implementation with ccan/ciniparser library. > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > --- > ndctl/Makefile.am | 1 + > ndctl/monitor.c | 115 ++++++++++++--------------------------------- > ndctl/monitor.conf | 2 + > 3 files changed, 32 insertions(+), 86 deletions(-) > > diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am > index ff01e06..2f00b65 100644 > --- a/ndctl/Makefile.am > +++ b/ndctl/Makefile.am > @@ -28,6 +28,7 @@ ndctl_LDADD =\ > lib/libndctl.la \ > ../daxctl/lib/libdaxctl.la \ > ../libutil.a \ > + ../libccan.a \ > $(UUID_LIBS) \ > $(KMOD_LIBS) \ > $(JSON_LIBS) > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > index 233f2bb..8499fd4 100644 > --- a/ndctl/monitor.c > +++ b/ndctl/monitor.c [..] > + dictionary *dic; > + > + dic = ciniparser_load(config_file); > + if (!dic) > + return -errno; > + > + parse_config(¶m.bus, > + ciniparser_getstring(dic, "monitor:bus", NULL)); > + parse_config(¶m.dimm, > + ciniparser_getstring(dic, "monitor:dimm", NULL)); > + parse_config(¶m.region, > + ciniparser_getstring(dic, "monitor:region", NULL)); > + parse_config(¶m.namespace, > + ciniparser_getstring(dic, "monitor:namespace", NULL)); > + parse_config(&monitor.dimm_event, > + ciniparser_getstring(dic, "monitor:dimm-event", NULL)); Since the default return is always NULL in the above calls, we can use the more concise form, ciniparser_getstr() - #define ciniparser_getstr(d, k) ciniparser_getstring(d, k, NULL) > + if (monitor.log) > goto out; [..] > diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf > index 934e2c0..edcf8e2 100644 > --- a/ndctl/monitor.conf > +++ b/ndctl/monitor.conf > @@ -9,6 +9,8 @@ > # Multiple space-separated values are allowed, but except the following > # characters: : ? / \ % " ' $ & ! * { } [ ] ( ) = < > @ Is this character restriction still true? > > +[monitor] > + > # The objects to monitor are filtered via dimm's name by setting key "dimm". > # If this value is different from the value of [--dimm=<value>] option, > # both of the values will work. With Dan's recent reworks changing the build-time define to reflect an 'ndctl-wide' config, and with the introduction of ini style sections, I think this change merits a little more thought. The addition of the [monitor] section breaks any existing configs - which is fine, I don't expect we will break thousands, or even tens, of existing deployments out in the wild :) That being said, if we are breaking old config files, we can take this chance to rethink and restructure configuration a bit. Here is my proposal. - Rename monitor.conf to ndctl.conf, it will be a global ndctl-wide config file that all commands can refer to. - Define sections only for command-specific options, but have a [core] section for options that apply to several different commands. For example core:region and core:bus will specify the region and bus to always operate on by default. In contrast, monitor:dimm-event and monitor:log will be in the [monitor] section, and will always be monitor specific. - You can still have multiple monitor processes started each using a distinct config. The way this can work is that the 'global' ndctl config file will be read first, and will always apply (default /etc/ndctl/ndctl.conf, configurable at compile time). Any 'local' config supplied using the config-file option to monitor will override any field that it changes over the global config. This way the local configs for monitor don't need to duplicate any truly common core information (say bus, for example), but still have the option to override them if needed. This follows the user experience git provides with ~/.gitconfig vs. repo local config. - Existing commands can then be taught to respect the core config options, and even grow their own specific sections, but all that can be part of a longer term effort. Putting in the foundations for that now can allow us to convert individual commands as needed. For now monitor will be the first and only user of this override hierarchy. I suspect there will be some refactoring of option parsing in different commands, but we probably shouldn't attempt that until a second user of the config framework shows up, and then it is their responsibility :) With the 5.0-rc1 kernel out, we're overdue for an ndctl-64 release, I should be able to do that this week, but I'm thinking with the wider config reworks, this is really a ndctl-65 item. Thanks, -Vishal
> -----Original Message----- > From: Verma, Vishal L [mailto:vishal.l.verma@intel.com] > Sent: Tuesday, January 8, 2019 7:02 AM > To: linux-nvdimm@lists.01.org; Qi, Fuli/斉 福利 <qi.fuli@fujitsu.com> > Subject: Re: [ndctl PATCH v2 2/2] ndctl, monitor: refactor read_config_file > > > On Mon, 2019-01-07 at 17:38 +0900, QI Fuli wrote: > > This patch is used for refactoring read_config_file by replacing the > > open coded implementation with ccan/ciniparser library. > > > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > --- > > ndctl/Makefile.am | 1 + > > ndctl/monitor.c | 115 ++++++++++++--------------------------------- > > ndctl/monitor.conf | 2 + > > 3 files changed, 32 insertions(+), 86 deletions(-) > > > > diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am index > > ff01e06..2f00b65 100644 > > --- a/ndctl/Makefile.am > > +++ b/ndctl/Makefile.am > > @@ -28,6 +28,7 @@ ndctl_LDADD =\ > > lib/libndctl.la \ > > ../daxctl/lib/libdaxctl.la \ > > ../libutil.a \ > > + ../libccan.a \ > > $(UUID_LIBS) \ > > $(KMOD_LIBS) \ > > $(JSON_LIBS) > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c index 233f2bb..8499fd4 > > 100644 > > --- a/ndctl/monitor.c > > +++ b/ndctl/monitor.c > > [..] > > > + dictionary *dic; > > + > > + dic = ciniparser_load(config_file); > > + if (!dic) > > + return -errno; > > + > > + parse_config(¶m.bus, > > + ciniparser_getstring(dic, "monitor:bus", NULL)); > > + parse_config(¶m.dimm, > > + ciniparser_getstring(dic, "monitor:dimm", NULL)); > > + parse_config(¶m.region, > > + ciniparser_getstring(dic, "monitor:region", NULL)); > > + parse_config(¶m.namespace, > > + ciniparser_getstring(dic, "monitor:namespace", NULL)); > > + parse_config(&monitor.dimm_event, > > + ciniparser_getstring(dic, "monitor:dimm-event", NULL)); > > Since the default return is always NULL in the above calls, we can use the more concise > form, ciniparser_getstr() - > > #define ciniparser_getstr(d, k) ciniparser_getstring(d, k, NULL) > > > > + if (monitor.log) > > goto out; > > [..] > > > diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index > > 934e2c0..edcf8e2 100644 > > --- a/ndctl/monitor.conf > > +++ b/ndctl/monitor.conf > > @@ -9,6 +9,8 @@ > > # Multiple space-separated values are allowed, but except the > > following # characters: : ? / \ % " ' $ & ! * { } [ ] ( ) = < > @ > > Is this character restriction still true? > > > > > +[monitor] > > + > > # The objects to monitor are filtered via dimm's name by setting key "dimm". > > # If this value is different from the value of [--dimm=<value>] > > option, # both of the values will work. > > With Dan's recent reworks changing the build-time define to reflect an 'ndctl-wide' > config, and with the introduction of ini style sections, I think this change merits > a little more thought. > > The addition of the [monitor] section breaks any existing configs - which is fine, > I don't expect we will break thousands, or even tens, of existing deployments out > in the wild :) > > That being said, if we are breaking old config files, we can take this chance to > rethink and restructure configuration a bit. Here is my proposal. > > - Rename monitor.conf to ndctl.conf, it will be a global ndctl-wide config file that > all commands can refer to. > > - Define sections only for command-specific options, but have a [core] section for > options that apply to several different commands. For example core:region and > core:bus will specify the region and bus to always operate on by default. In contrast, > monitor:dimm-event and monitor:log will be in the [monitor] section, and will always > be monitor specific. > > - You can still have multiple monitor processes started each using a distinct config. > The way this can work is that the 'global' ndctl config file will be read first, > and will always apply (default /etc/ndctl/ndctl.conf, configurable at compile time). > Any 'local' > config supplied using the config-file option to monitor will override any field that > it changes over the global config. This way the local configs for monitor don't need > to duplicate any truly common core information (say bus, for example), but still > have the option to override them if needed. This follows the user experience git > provides with ~/.gitconfig vs. repo local config. > > - Existing commands can then be taught to respect the core config options, and even > grow their own specific sections, but all that can be part of a longer term effort. > Putting in the foundations for that now can allow us to convert individual commands > as needed. For now monitor will be the first and only user of this override hierarchy. > I suspect there will be some refactoring of option parsing in different commands, > but we probably shouldn't attempt that until a second user of the config framework > shows up, and then it is their responsibility :) > > With the 5.0-rc1 kernel out, we're overdue for an ndctl-64 release, I should be able > to do that this week, but I'm thinking with the wider config reworks, this is really > a ndctl-65 item. > > > Thanks, > -Vishal Hi Vishal, Thank you for your comments. I agree to rename monitor.conf to ndctl.conf and make it can be used by all commands. Also, thinking that we can add a parse_configs to util by referring to parse_options, therefore, every command can read the global configuration file by using it. Thanks, QI Fuli
On Thu, 2019-01-10 at 07:47 +0000, qi.fuli@fujitsu.com wrote: > > -----Original Message----- > > From: Verma, Vishal L [mailto:vishal.l.verma@intel.com] > > Sent: Tuesday, January 8, 2019 7:02 AM > > To: linux-nvdimm@lists.01.org; Qi, Fuli/斉 福利 <qi.fuli@fujitsu.com> > > Subject: Re: [ndctl PATCH v2 2/2] ndctl, monitor: refactor read_config_file > > > > > > On Mon, 2019-01-07 at 17:38 +0900, QI Fuli wrote: > > > This patch is used for refactoring read_config_file by replacing the > > > open coded implementation with ccan/ciniparser library. > > > > > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > > --- > > > ndctl/Makefile.am | 1 + > > > ndctl/monitor.c | 115 ++++++++++++--------------------------------- > > > ndctl/monitor.conf | 2 + > > > 3 files changed, 32 insertions(+), 86 deletions(-) > > > > > > diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am index > > > ff01e06..2f00b65 100644 > > > --- a/ndctl/Makefile.am > > > +++ b/ndctl/Makefile.am > > > @@ -28,6 +28,7 @@ ndctl_LDADD =\ > > > lib/libndctl.la \ > > > ../daxctl/lib/libdaxctl.la \ > > > ../libutil.a \ > > > + ../libccan.a \ > > > $(UUID_LIBS) \ > > > $(KMOD_LIBS) \ > > > $(JSON_LIBS) > > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c index 233f2bb..8499fd4 > > > 100644 > > > --- a/ndctl/monitor.c > > > +++ b/ndctl/monitor.c > > > > [..] > > > > > + dictionary *dic; > > > + > > > + dic = ciniparser_load(config_file); > > > + if (!dic) > > > + return -errno; > > > + > > > + parse_config(¶m.bus, > > > + ciniparser_getstring(dic, "monitor:bus", NULL)); > > > + parse_config(¶m.dimm, > > > + ciniparser_getstring(dic, "monitor:dimm", NULL)); > > > + parse_config(¶m.region, > > > + ciniparser_getstring(dic, "monitor:region", NULL)); > > > + parse_config(¶m.namespace, > > > + ciniparser_getstring(dic, "monitor:namespace", NULL)); > > > + parse_config(&monitor.dimm_event, > > > + ciniparser_getstring(dic, "monitor:dimm-event", NULL)); > > > > Since the default return is always NULL in the above calls, we can use the more concise > > form, ciniparser_getstr() - > > > > #define ciniparser_getstr(d, k) ciniparser_getstring(d, k, NULL) > > > > > > > + if (monitor.log) > > > goto out; > > > > [..] > > > > > diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index > > > 934e2c0..edcf8e2 100644 > > > --- a/ndctl/monitor.conf > > > +++ b/ndctl/monitor.conf > > > @@ -9,6 +9,8 @@ > > > # Multiple space-separated values are allowed, but except the > > > following # characters: : ? / \ % " ' $ & ! * { } [ ] ( ) = < > @ > > > > Is this character restriction still true? > > > > > > > > +[monitor] > > > + > > > # The objects to monitor are filtered via dimm's name by setting key "dimm". > > > # If this value is different from the value of [--dimm=<value>] > > > option, # both of the values will work. > > > > With Dan's recent reworks changing the build-time define to reflect an 'ndctl-wide' > > config, and with the introduction of ini style sections, I think this change merits > > a little more thought. > > > > The addition of the [monitor] section breaks any existing configs - which is fine, > > I don't expect we will break thousands, or even tens, of existing deployments out > > in the wild :) > > > > That being said, if we are breaking old config files, we can take this chance to > > rethink and restructure configuration a bit. Here is my proposal. > > > > - Rename monitor.conf to ndctl.conf, it will be a global ndctl-wide config file that > > all commands can refer to. > > > > - Define sections only for command-specific options, but have a [core] section for > > options that apply to several different commands. For example core:region and > > core:bus will specify the region and bus to always operate on by default. In contrast, > > monitor:dimm-event and monitor:log will be in the [monitor] section, and will always > > be monitor specific. > > > > - You can still have multiple monitor processes started each using a distinct config. > > The way this can work is that the 'global' ndctl config file will be read first, > > and will always apply (default /etc/ndctl/ndctl.conf, configurable at compile time). > > Any 'local' > > config supplied using the config-file option to monitor will override any field that > > it changes over the global config. This way the local configs for monitor don't need > > to duplicate any truly common core information (say bus, for example), but still > > have the option to override them if needed. This follows the user experience git > > provides with ~/.gitconfig vs. repo local config. > > > > - Existing commands can then be taught to respect the core config options, and even > > grow their own specific sections, but all that can be part of a longer term effort. > > Putting in the foundations for that now can allow us to convert individual commands > > as needed. For now monitor will be the first and only user of this override hierarchy. > > I suspect there will be some refactoring of option parsing in different commands, > > but we probably shouldn't attempt that until a second user of the config framework > > shows up, and then it is their responsibility :) > > > > With the 5.0-rc1 kernel out, we're overdue for an ndctl-64 release, I should be able > > to do that this week, but I'm thinking with the wider config reworks, this is really > > a ndctl-65 item. > > > > > > Thanks, > > -Vishal > > Hi Vishal, > > Thank you for your comments. > I agree to rename monitor.conf to ndctl.conf and make it can be used by all commands. > Also, thinking that we can add a parse_configs to util by referring to parse_options, > therefore, every command can read the global configuration file by using it. Yes, moving the config file parsing to util makes sense. Thank you for your continued efforts on this! -Vishal
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am index ff01e06..2f00b65 100644 --- a/ndctl/Makefile.am +++ b/ndctl/Makefile.am @@ -28,6 +28,7 @@ ndctl_LDADD =\ lib/libndctl.la \ ../daxctl/lib/libdaxctl.la \ ../libutil.a \ + ../libccan.a \ $(UUID_LIBS) \ $(KMOD_LIBS) \ $(JSON_LIBS) diff --git a/ndctl/monitor.c b/ndctl/monitor.c index 233f2bb..8499fd4 100644 --- a/ndctl/monitor.c +++ b/ndctl/monitor.c @@ -13,6 +13,7 @@ #include <ndctl/ndctl.h> #include <ndctl/libndctl.h> #include <sys/epoll.h> +#include <ccan/ciniparser/ciniparser.h> #define BUF_SIZE 2048 /* reuse the core log helpers for the monitor logger */ @@ -443,12 +444,12 @@ out: return rc; } -static void parse_config(const char **arg, char *key, char *val, char *ident) +static void parse_config(const char **arg, char *val) { struct strbuf value = STRBUF_INIT; size_t arg_len = *arg ? strlen(*arg) : 0; - if (!ident || !key || (strcmp(ident, key) != 0)) + if (!val) return; if (arg_len) { @@ -459,92 +460,31 @@ static void parse_config(const char **arg, char *key, char *val, char *ident) *arg = strbuf_detach(&value, NULL); } -static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor, - struct util_filter_params *_param) +static int read_config_file(const char *config_file) { - FILE *f; - size_t len = 0; - int line = 0, rc = 0; - char *buf = NULL, *seek, *value, *config_file; - - if (_monitor->config_file) - config_file = strdup(_monitor->config_file); - else - config_file = strdup(NDCTL_CONF_FILE); - if (!config_file) { - fail("strdup default config file failed\n"); - rc = -ENOMEM; - goto out; - } - - buf = malloc(BUF_SIZE); - if (!buf) { - fail("malloc read config-file buf error\n"); - rc = -ENOMEM; + dictionary *dic; + + dic = ciniparser_load(config_file); + if (!dic) + return -errno; + + parse_config(¶m.bus, + ciniparser_getstring(dic, "monitor:bus", NULL)); + parse_config(¶m.dimm, + ciniparser_getstring(dic, "monitor:dimm", NULL)); + parse_config(¶m.region, + ciniparser_getstring(dic, "monitor:region", NULL)); + parse_config(¶m.namespace, + ciniparser_getstring(dic, "monitor:namespace", NULL)); + parse_config(&monitor.dimm_event, + ciniparser_getstring(dic, "monitor:dimm-event", NULL)); + if (monitor.log) goto out; - } - seek = buf; - - f = fopen(config_file, "r"); - if (!f) { - err(&monitor, "config-file: %s cannot be opened\n", config_file); - rc = -errno; - goto out; - } - - while (fgets(seek, BUF_SIZE, f)) { - value = NULL; - line++; - - while (isspace(*seek)) - seek++; - - if (*seek == '#' || *seek == '\0') - continue; - - value = strchr(seek, '='); - if (!value) { - fail("config-file syntax error, skip line[%i]\n", line); - continue; - } - - value[0] = '\0'; - value++; - - while (isspace(value[0])) - value++; - - len = strlen(seek); - if (len == 0) - continue; - while (isspace(seek[len-1])) - len--; - seek[len] = '\0'; - - len = strlen(value); - if (len == 0) - continue; - while (isspace(value[len-1])) - len--; - value[len] = '\0'; - - if (len == 0) - continue; - - parse_config(&_param->bus, "bus", value, seek); - parse_config(&_param->dimm, "dimm", value, seek); - parse_config(&_param->region, "region", value, seek); - parse_config(&_param->namespace, "namespace", value, seek); - parse_config(&_monitor->dimm_event, "dimm-event", value, seek); - - if (!_monitor->log) - parse_config(&_monitor->log, "log", value, seek); - } - fclose(f); + parse_config(&monitor.log, + ciniparser_getstring(dic, "monitor:log", NULL)); out: - free(buf); - free(config_file); - return rc; + ciniparser_freedict(dic); + return 0; } int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx) @@ -596,7 +536,10 @@ int cmd_monitor(int argc, const char **argv, struct ndctl_ctx *ctx) else monitor.ctx.log_priority = LOG_INFO; - rc = read_config_file(ctx, &monitor, ¶m); + if (monitor.config_file) + rc = read_config_file(monitor.config_file); + else + rc = read_config_file(NDCTL_CONF_FILE); if (rc) goto out; diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index 934e2c0..edcf8e2 100644 --- a/ndctl/monitor.conf +++ b/ndctl/monitor.conf @@ -9,6 +9,8 @@ # Multiple space-separated values are allowed, but except the following # characters: : ? / \ % " ' $ & ! * { } [ ] ( ) = < > @ +[monitor] + # The objects to monitor are filtered via dimm's name by setting key "dimm". # If this value is different from the value of [--dimm=<value>] option, # both of the values will work.
This patch is used for refactoring read_config_file by replacing the open coded implementation with ccan/ciniparser library. Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> --- ndctl/Makefile.am | 1 + ndctl/monitor.c | 115 ++++++++++++--------------------------------- ndctl/monitor.conf | 2 + 3 files changed, 32 insertions(+), 86 deletions(-)