| Message ID | 20180731051503.30719-1-qi.fuli@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndclt] ndctl, monitor: Fix duplicate prefix in monitor.log | expand |
Hi QI, On 07/31/2018 01:15 AM, QI Fuli wrote: > When a monitor runs with [--log] option, the prefix will be dually > added to monitor.log. Therefore, the monitor cannot log the smart > notification to syslog. This patch is used to prevent prefix from > being dually added to monitor.log. > > Fixes: fdf6b6844ccf ("ndctl, monitor: add a new command - monitor") > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > --- > ndctl/monitor.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c > index c6419ad..4e5daf5 100644 > --- a/ndctl/monitor.c > +++ b/ndctl/monitor.c > @@ -614,7 +614,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > goto out; > > if (monitor.log) { > - fix_filename(prefix, (const char **)&monitor.log); > + if (strncmp(monitor.log, "./", 2) != 0) > + fix_filename(prefix, (const char **)&monitor.log); prefix is not needed to 'syslog' and 'standard', so why don't you move the strncmp() before fix_filename(), like as: @@ -614,13 +619,14 @@ int cmd_monitor(int argc, const char **argv, void *ctx) goto out; if (monitor.log) { - fix_filename(prefix, (const char **)&monitor.log); if (strncmp(monitor.log, "./syslog", 8) == 0) ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); else if (strncmp(monitor.log, "./standard", 10) == 0) ; /*default, already set */ - else + else { + fix_filename(prefix, (const char **)&monitor.log); ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file); + } } if (monitor.daemon) { Thanks, Masa > if (strncmp(monitor.log, "./syslog", 8) == 0) > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); > else if (strncmp(monitor.log, "./standard", 10) == 0) >
> -----Original Message----- > From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com] > Sent: Thursday, August 2, 2018 3:15 AM > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org > Subject: Re: [ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log > > Hi QI, > > On 07/31/2018 01:15 AM, QI Fuli wrote: > > When a monitor runs with [--log] option, the prefix will be dually > > added to monitor.log. Therefore, the monitor cannot log the smart > > notification to syslog. This patch is used to prevent prefix from > > being dually added to monitor.log. > > > > Fixes: fdf6b6844ccf ("ndctl, monitor: add a new command - monitor") > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > --- > > ndctl/monitor.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c index c6419ad..4e5daf5 > > 100644 > > --- a/ndctl/monitor.c > > +++ b/ndctl/monitor.c > > @@ -614,7 +614,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > > goto out; > > > > if (monitor.log) { > > - fix_filename(prefix, (const char **)&monitor.log); > > + if (strncmp(monitor.log, "./", 2) != 0) > > + fix_filename(prefix, (const char **)&monitor.log); > > prefix is not needed to 'syslog' and 'standard', so why don't you move the strncmp() > before fix_filename(), like as: > > @@ -614,13 +619,14 @@ int cmd_monitor(int argc, const char **argv, void *ctx) > goto out; > > if (monitor.log) { > - fix_filename(prefix, (const char **)&monitor.log); > if (strncmp(monitor.log, "./syslog", 8) == 0) > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); > else if (strncmp(monitor.log, "./standard", 10) == 0) > ; /*default, already set */ > - else > + else { > + fix_filename(prefix, (const char > + **)&monitor.log); > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file); > + } > } > > if (monitor.daemon) { > > Thanks, > Masa Hi Masa, Thank you very much for your comments. There are two ways to set monitor.log. a) setting the argument of [--log] option b) setting the value of [log] key in configuration file When users set monitor.log by b, the prefix will not be added to monitor.log. Therefore, we should do fix_filename() before strncmp(). Thanks, QI > > > if (strncmp(monitor.log, "./syslog", 8) == 0) > > ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); > > else if (strncmp(monitor.log, "./standard", 10) == 0) > > >
Hi QI, On 08/02/2018 05:42 AM, Qi, Fuli wrote: ... >>> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index c6419ad..4e5daf5 >>> 100644 >>> --- a/ndctl/monitor.c >>> +++ b/ndctl/monitor.c >>> @@ -614,7 +614,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx) >>> goto out; >>> >>> if (monitor.log) { >>> - fix_filename(prefix, (const char **)&monitor.log); >>> + if (strncmp(monitor.log, "./", 2) != 0) >>> + fix_filename(prefix, (const char **)&monitor.log); >> >> prefix is not needed to 'syslog' and 'standard', so why don't you move the strncmp() >> before fix_filename(), like as: >> >> @@ -614,13 +619,14 @@ int cmd_monitor(int argc, const char **argv, void *ctx) >> goto out; >> >> if (monitor.log) { >> - fix_filename(prefix, (const char **)&monitor.log); >> if (strncmp(monitor.log, "./syslog", 8) == 0) >> ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); >> else if (strncmp(monitor.log, "./standard", 10) == 0) >> ; /*default, already set */ >> - else >> + else { >> + fix_filename(prefix, (const char >> + **)&monitor.log); >> ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file); >> + } >> } >> >> if (monitor.daemon) { >> >> Thanks, >> Masa > > Hi Masa, > > Thank you very much for your comments. > > There are two ways to set monitor.log. > a) setting the argument of [--log] option > b) setting the value of [log] key in configuration file > > When users set monitor.log by b, the prefix will not be added to monitor.log. > Therefore, we should do fix_filename() before strncmp(). Oh, my proposal patch does not cover in case of user set 'syslog' or 'standard'in config_file. I think your patch fixes the bug correctly, thanks. Please feel free to add: Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Thanks, Masa
On Thu, 2018-08-02 at 09:22 -0400, Masayoshi Mizuma wrote: > Hi QI, > > On 08/02/2018 05:42 AM, Qi, Fuli wrote: > ... > > > > diff --git a/ndctl/monitor.c b/ndctl/monitor.c index > > > > c6419ad..4e5daf5 > > > > 100644 > > > > --- a/ndctl/monitor.c > > > > +++ b/ndctl/monitor.c > > > > @@ -614,7 +614,8 @@ int cmd_monitor(int argc, const char > > > > **argv, void *ctx) > > > > goto out; > > > > > > > > if (monitor.log) { > > > > - fix_filename(prefix, (const char > > > > **)&monitor.log); > > > > + if (strncmp(monitor.log, "./", 2) != 0) > > > > + fix_filename(prefix, (const char > > > > **)&monitor.log); > > > > > > prefix is not needed to 'syslog' and 'standard', so why don't you > > > move the strncmp() > > > before fix_filename(), like as: > > > > > > @@ -614,13 +619,14 @@ int cmd_monitor(int argc, const char > > > **argv, void *ctx) > > > goto out; > > > > > > if (monitor.log) { > > > - fix_filename(prefix, (const char > > > **)&monitor.log); > > > if (strncmp(monitor.log, "./syslog", 8) == 0) > > > ndctl_set_log_fn((struct ndctl_ctx *)ctx, > > > log_syslog); > > > else if (strncmp(monitor.log, "./standard", 10) > > > == 0) > > > ; /*default, already set */ > > > - else > > > + else { > > > + fix_filename(prefix, (const char > > > + **)&monitor.log); > > > ndctl_set_log_fn((struct ndctl_ctx *)ctx, > > > log_file); > > > + } > > > } > > > > > > if (monitor.daemon) { > > > > > > Thanks, > > > Masa > > > > Hi Masa, > > > > Thank you very much for your comments. > > > > There are two ways to set monitor.log. > > a) setting the argument of [--log] option > > b) setting the value of [log] key in configuration file > > > > When users set monitor.log by b, the prefix will not be added to > > monitor.log. > > Therefore, we should do fix_filename() before strncmp(). > > Oh, my proposal patch does not cover in case of user set 'syslog' or > 'standard'in config_file. > I think your patch fixes the bug correctly, thanks. > > Please feel free to add: > > Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Thanks Qi, Masa - I've applied this. > > Thanks, > Masa > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
diff --git a/ndctl/monitor.c b/ndctl/monitor.c index c6419ad..4e5daf5 100644 --- a/ndctl/monitor.c +++ b/ndctl/monitor.c @@ -614,7 +614,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx) goto out; if (monitor.log) { - fix_filename(prefix, (const char **)&monitor.log); + if (strncmp(monitor.log, "./", 2) != 0) + fix_filename(prefix, (const char **)&monitor.log); if (strncmp(monitor.log, "./syslog", 8) == 0) ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog); else if (strncmp(monitor.log, "./standard", 10) == 0)
When a monitor runs with [--log] option, the prefix will be dually added to monitor.log. Therefore, the monitor cannot log the smart notification to syslog. This patch is used to prevent prefix from being dually added to monitor.log. Fixes: fdf6b6844ccf ("ndctl, monitor: add a new command - monitor") Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> --- ndctl/monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)