| Message ID | 20190329110150.3725-1-qi.fuli@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 401fa3cbc724ba87ccc4f828b52ba3d687c14170 |
| Headers | show |
| Series | [ndctl] monitor: removing the requirement of default config | expand |
On Fri, 2019-03-29 at 20:01 +0900, QI Fuli wrote: > Removing the requirement of default configuration file. > If "-c, --config-file" option is not specified, default configuration > file should be dispensable. > > Link: https://github.com/pmem/ndctl/issues/83 > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > --- > ndctl/monitor.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > index 346a6df..6829a6b 100644 > --- a/ndctl/monitor.c > +++ b/ndctl/monitor.c > @@ -484,8 +484,11 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor, > > f = fopen(config_file, "r"); > if (!f) { > - err(&monitor, "config-file: %s cannot be opened\n", config_file); > - rc = -errno; > + if (_monitor->config_file) { > + err(&monitor, "config-file: %s cannot be opened\n", > + config_file); > + rc = -errno; > + } > goto out; > } > Hi Qi, Thanks for the quick patch. However, this makes it so that the only way in which a config file gets used is by explicitly specifying it with a -c option, and that kind of makes a 'default' config file pointless.. The ideal scenario would be: 1. Check if default config exists a. if it does, use options from there b. if any cli options provided, use those to override the ones in config file (if any) 2. If default config file is missing, only use cli options 3. If -c <file> provided, use that, but perform the same treatment as 1b above, i.e. any cli options override anything in the config file from -c as well.
> -----Original Message----- > From: Verma, Vishal L [mailto:vishal.l.verma@intel.com] > Sent: Saturday, March 30, 2019 2:37 AM > To: linux-nvdimm@lists.01.org; Qi, Fuli/斉 福利 <qi.fuli@fujitsu.com> > Subject: Re: [ndctl PATCH] monitor: removing the requirement of default > config > > > On Fri, 2019-03-29 at 20:01 +0900, QI Fuli wrote: > > Removing the requirement of default configuration file. > > If "-c, --config-file" option is not specified, default configuration > > file should be dispensable. > > > > Link: https://github.com/pmem/ndctl/issues/83 > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > > > --- > > ndctl/monitor.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > > index 346a6df..6829a6b 100644 > > --- a/ndctl/monitor.c > > +++ b/ndctl/monitor.c > > @@ -484,8 +484,11 @@ static int read_config_file(struct ndctl_ctx *ctx, > struct monitor *_monitor, > > > > f = fopen(config_file, "r"); > > if (!f) { > > - err(&monitor, "config-file: %s cannot be opened\n", > config_file); > > - rc = -errno; > > + if (_monitor->config_file) { > > + err(&monitor, "config-file: %s cannot be > opened\n", > > + config_file); > > + rc = -errno; > > + } > > goto out; > > } > > > Hi Qi, > > Thanks for the quick patch. However, this makes it so that the only way > in which a config file gets used is by explicitly specifying it with a > -c option, and that kind of makes a 'default' config file pointless.. > > The ideal scenario would be: > 1. Check if default config exists > a. if it does, use options from there > b. if any cli options provided, use those to override the ones in > config file (if any) > > 2. If default config file is missing, only use cli options > > 3. If -c <file> provided, use that, but perform the same treatment as > 1b > above, i.e. any cli options override anything in the config file from > -c > as well. Hi Vishal, Thanks for your comment. I think it might be misunderstood. Current scenario is: 1. Check if "-c <file>" option is provided a. if it is, make the <file> as config_file b. if it is not, make default configuration file(/etc/ndctl/monitor.conf) as config_file 2. Try to read options from config_file a. if config_file cannot be opened, then check if "-c" option is provided (1) if it is, stop starting monitor (user may make a wrong <file> in "-c" option) (2) if it is not, finish read_config_file (default configuration file(/etc/ndctl/monitor.conf) is missing) b. if config_file can be opened successfully, merge the options from config_file with cli options It is consistent with the ideal scenario as you mentioned. Thanks, QI Fuli
On Mon, 2019-04-01 at 06:20 +0000, qi.fuli@fujitsu.com wrote: > > Hi Vishal, > > Thanks for your comment. > I think it might be misunderstood. > > Current scenario is: > 1. Check if "-c <file>" option is provided > a. if it is, make the <file> as config_file > b. if it is not, make default configuration file(/etc/ndctl/monitor.conf) as config_file > 2. Try to read options from config_file > a. if config_file cannot be opened, then check if "-c" option is provided > (1) if it is, stop starting monitor (user may make a wrong <file> in "-c" option) > (2) if it is not, finish read_config_file (default configuration file(/etc/ndctl/monitor.conf) is missing) > b. if config_file can be opened successfully, merge the options from config_file with cli options > > It is consistent with the ideal scenario as you mentioned. Hi Qi, Looking at it in more detail, I see you're right. I've applied the patch - thanks for the explanation. > > Thanks, > QI Fuli
diff --git a/ndctl/monitor.c b/ndctl/monitor.c index 346a6df..6829a6b 100644 --- a/ndctl/monitor.c +++ b/ndctl/monitor.c @@ -484,8 +484,11 @@ static int read_config_file(struct ndctl_ctx *ctx, struct monitor *_monitor, f = fopen(config_file, "r"); if (!f) { - err(&monitor, "config-file: %s cannot be opened\n", config_file); - rc = -errno; + if (_monitor->config_file) { + err(&monitor, "config-file: %s cannot be opened\n", + config_file); + rc = -errno; + } goto out; }
Removing the requirement of default configuration file. If "-c, --config-file" option is not specified, default configuration file should be dispensable. Link: https://github.com/pmem/ndctl/issues/83 Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> --- ndctl/monitor.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)