| Message ID | 20180807131756.23673-2-qi.fuli@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some fixups and improvements ndctl monitor | expand |
Hi Qi, On 08/07/2018 09:17 AM, QI Fuli wrote: > Currently the monitor can be started even with an invalid path of log file. > This patch adds a detection of invalid path of log file when starting monitor. > If the path of log file is invalid, the monitor will refuse to be started. > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > --- > ndctl/monitor.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > index f10384b..bf1f1d3 100644 > --- a/ndctl/monitor.c > +++ b/ndctl/monitor.c > @@ -603,6 +603,7 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > struct util_filter_ctx fctx = { 0 }; > struct monitor_filter_arg mfa = { 0 }; > int i, rc; > + FILE *f; > > argc = parse_options_prefix(argc, argv, prefix, options, u, 0); > for (i = 0; i < argc; i++) { > @@ -630,8 +631,16 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); > else if (strncmp(monitor.log, "./standard", 10) == 0) > ; /*default, already set */ > - else > + else { > + f = fopen(monitor.log, "a+"); > + if (!f) { > + error("open %s failed\n", monitor.log); > + rc = -errno; > + goto out; > + } > + fclose(f); > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file); > + } In log_file(), the log file does fallback to syslog if the fopen() fails. In my understanding here is that the fallback is needed to save in case of the monitor.log in trouble for example, the parent directory is removed. And, the new fopen() check, you have added by this patch, to inform the invalid log path for users. Is my understanding correct? If so, make sense to me. Please feel free to add: Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Thanks, Masa > } > > if (monitor.daemon) { >
> -----Original Message----- > From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com] > Sent: Wednesday, August 8, 2018 4:06 AM > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org > Subject: Re: [ndctl PATCH 1/4] ndctl, monitor: fix the lack of detection of invalid > path of log file > > Hi Qi, > > On 08/07/2018 09:17 AM, QI Fuli wrote: > > Currently the monitor can be started even with an invalid path of log file. > > This patch adds a detection of invalid path of log file when starting monitor. > > If the path of log file is invalid, the monitor will refuse to be started. > > > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > --- > > ndctl/monitor.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c index f10384b..bf1f1d3 > > 100644 > > --- a/ndctl/monitor.c > > +++ b/ndctl/monitor.c > > @@ -603,6 +603,7 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > > struct util_filter_ctx fctx = { 0 }; > > struct monitor_filter_arg mfa = { 0 }; > > int i, rc; > > + FILE *f; > > > > argc = parse_options_prefix(argc, argv, prefix, options, u, 0); > > for (i = 0; i < argc; i++) { > > @@ -630,8 +631,16 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); > > else if (strncmp(monitor.log, "./standard", 10) == 0) > > ; /*default, already set */ > > - else > > + else { > > + f = fopen(monitor.log, "a+"); > > + if (!f) { > > + error("open %s failed\n", monitor.log); > > + rc = -errno; > > + goto out; > > + } > > + fclose(f); > > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file); > > + } > Masa, Thanks for your comments. > In log_file(), the log file does fallback to syslog if the fopen() fails. > In my understanding here is that the fallback is needed to save in case of the > monitor.log in trouble for example, the parent directory is removed. > And, the new fopen() check, you have added by this patch, to inform the invalid log > path for users. > Yes, this is what I wanted to implement. > Is my understanding correct? If so, make sense to me. > Please feel free to add: > > Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > Thanks, QI > Thanks, > Masa > > > } > > > > if (monitor.daemon) { > > >
diff --git a/ndctl/monitor.c b/ndctl/monitor.c index f10384b..bf1f1d3 100644 --- a/ndctl/monitor.c +++ b/ndctl/monitor.c @@ -603,6 +603,7 @@ int cmd_monitor(int argc, const char **argv, void *ctx) struct util_filter_ctx fctx = { 0 }; struct monitor_filter_arg mfa = { 0 }; int i, rc; + FILE *f; argc = parse_options_prefix(argc, argv, prefix, options, u, 0); for (i = 0; i < argc; i++) { @@ -630,8 +631,16 @@ int cmd_monitor(int argc, const char **argv, void *ctx) ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); else if (strncmp(monitor.log, "./standard", 10) == 0) ; /*default, already set */ - else + else { + f = fopen(monitor.log, "a+"); + if (!f) { + error("open %s failed\n", monitor.log); + rc = -errno; + goto out; + } + fclose(f); ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file); + } } if (monitor.daemon) {
Currently the monitor can be started even with an invalid path of log file. This patch adds a detection of invalid path of log file when starting monitor. If the path of log file is invalid, the monitor will refuse to be started. Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> --- ndctl/monitor.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)