diff mbox series

[ndctl,1/4] ndctl, monitor: fix the lack of detection of invalid path of log file

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

Commit Message

QI Fuli Aug. 7, 2018, 1:17 p.m. UTC
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(-)

Comments

Masayoshi Mizuma Aug. 7, 2018, 7:05 p.m. UTC | #1
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) {
>
QI Fuli Aug. 8, 2018, 1:01 a.m. UTC | #2
> -----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 mbox series

Patch

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