[ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log

[ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log

[QI Fuli]

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);
 		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)
-- 
2.18.0


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log

[Masayoshi Mizuma]

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)
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log

[Qi, Fuli]

> -----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)
> >
> 

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log

[Masayoshi Mizuma]

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
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndclt PATCH] ndctl, monitor: Fix duplicate prefix in monitor.log

[Verma, Vishal L]

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
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm