[ndctl PATCH 0/4] Some fixups and improvements ndctl monitor

[ndctl PATCH 0/4] Some fixups and improvements ndctl monitor

[QI Fuli]

Fix a couple of issues about log collection, and add [Install] section
to systemd unit file of ndclt-monitor.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
QI Fuli (4):
  ndctl, monitor: fix the lack of detection of invalid path of log file
  ndctl, monitor: set default log destination to syslog if "--daemon" is specified
  ndctl, monitor: add timestamp and pid to log messages in log_file()
  ndctl, monitor: add [Install] Section to systemd unit file of ndctl-monitor

 Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
 ndctl/monitor.c                       | 27 ++++++++++++++++++++++++---
 ndctl/monitor.conf                    |  2 ++
 ndctl/ndctl-monitor.service           |  3 +++
 4 files changed, 44 insertions(+), 4 deletions(-)

-- 
2.18.0


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

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

[QI Fuli]

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);
+		}
 	}
 
 	if (monitor.daemon) {
-- 
2.18.0


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

[ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified

[QI Fuli]

When running monitor as a daemon, if the log destination is "standard" or
a relative path for log file, the messages will not be able to be logged.
Sometimes, users may not notice that the default log destination is "standard"
when they start monitor daemon by systemctl, so they will lose messages.
This patch is used to fix the unfriendly interface. When running monitor as a
daemon, the default log destination will be changed to syslog. Also, the messages
will be forwarded to syslog if the log destination is a relative path for log file.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
 ndctl/monitor.c                       |  5 ++++-
 ndctl/monitor.conf                    |  2 ++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/ndctl/ndctl-monitor.txt b/Documentation/ndctl/ndctl-monitor.txt
index 1cba9ea..9a8d76b 100644
--- a/Documentation/ndctl/ndctl-monitor.txt
+++ b/Documentation/ndctl/ndctl-monitor.txt
@@ -67,7 +67,21 @@ OPTIONS
 
 -l <file | syslog | standard>::
 --log=<file | syslog | standard>::
-	Output notifications to <file>, syslog or standard output.
+	Send log messages to the specified destination.
++
+--
+<file>::
+	Send log messages to specified <file>. When fopen() is not able
+	to open <file>, log messages will be forwarded to syslog.
+syslog::
+	Send messages to syslog.
+standard::
+	Send messages to standard output.
+--
++
+The default log destination is 'syslog' if "--daemon" is specified,
+otherwise 'standard'. Note that standard and relative path for <file>
+will not work if "--daemon" is specified.
 
 -c::
 --config-file=::
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index bf1f1d3..2f3d751 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -93,7 +93,8 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 	f = fopen(monitor.log, "a+");
 	if (!f) {
 		ndctl_set_log_fn(ctx, log_syslog);
-		err(ctx, "open logfile %s failed\n", monitor.log);
+		err(ctx, "open logfile %s failed, forward messages to syslog\n",
+				monitor.log);
 		did_fail = 1;
 		notice(ctx, "%s\n", buf);
 		goto end;
@@ -644,6 +645,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
 	}
 
 	if (monitor.daemon) {
+		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
+			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
 		if (daemon(0, 0) != 0) {
 			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
 			goto out;
diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf
index 857aadf..934e2c0 100644
--- a/ndctl/monitor.conf
+++ b/ndctl/monitor.conf
@@ -38,4 +38,6 @@
 # to standard output (log=standard) or to write into a special file (log=<file>)
 # by setting key "log". If this value is in conflict with the value of
 # [--log=<value>] option, this value will be ignored.
+# Note: Setting value to "standard" or relative path for <file> will not work
+# when running moniotr as a daemon.
 # log = /var/log/ndctl/monitor.log
-- 
2.18.0


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

[ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages in log_file()

[QI Fuli]

This patch is used to add timestamp and process id to log messages when logging
messages to a file.

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 2f3d751..d29e378 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -84,6 +84,8 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 {
 	FILE *f;
 	char *buf;
+	struct timespec ts;
+	char timestamp[32];
 
 	if (vasprintf(&buf, format, args) < 0) {
 		fail("vasprintf error\n");
@@ -99,7 +101,14 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
 		notice(ctx, "%s\n", buf);
 		goto end;
 	}
-	fprintf(f, "%s", buf);
+
+	if (priority != LOG_NOTICE) {
+		clock_gettime(CLOCK_REALTIME, &ts);
+		sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
+		fprintf(f, "[%s] [%d] %s", timestamp, getpid(), buf);
+	} else
+		fprintf(f, "%s", buf);
+
 	fflush(f);
 	fclose(f);
 end:
-- 
2.18.0


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

[ndctl PATCH 4/4] ndctl, monitor: add [Install] Section to systemd unit file of ndctl-monitor

[QI Fuli]

This patch is used to add "[Install]" section to systemd unit file of
ndctl-monitor. Therefore, users can set ndctl-monitor unit enable.

Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
---
 ndctl/ndctl-monitor.service | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ndctl/ndctl-monitor.service b/ndctl/ndctl-monitor.service
index 44f9326..342a1b1 100644
--- a/ndctl/ndctl-monitor.service
+++ b/ndctl/ndctl-monitor.service
@@ -5,3 +5,6 @@ Description=Ndctl Monitor Daemon
 Type=forking
 ExecStart=/usr/bin/ndctl monitor --daemon
 ExecStop=/bin/kill ${MAINPID}
+
+[Install]
+WantedBy=multi-user.target
-- 
2.18.0


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

Re: [ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages in log_file()

[Masayoshi Mizuma]

Hi Qi,

On 08/07/2018 09:17 AM, QI Fuli wrote:
> This patch is used to add timestamp and process id to log messages when logging
> messages to a file.
> 
> 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 2f3d751..d29e378 100644
> --- a/ndctl/monitor.c
> +++ b/ndctl/monitor.c
> @@ -84,6 +84,8 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
>  {
>  	FILE *f;
>  	char *buf;
> +	struct timespec ts;
> +	char timestamp[32];
>  
>  	if (vasprintf(&buf, format, args) < 0) {
>  		fail("vasprintf error\n");
> @@ -99,7 +101,14 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
>  		notice(ctx, "%s\n", buf);
>  		goto end;
>  	}
> -	fprintf(f, "%s", buf);
> +
> +	if (priority != LOG_NOTICE) {

Why is the timestamp not needed in case of LOG_NOTICE...?

> +		clock_gettime(CLOCK_REALTIME, &ts);
> +		sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
> +		fprintf(f, "[%s] [%d] %s", timestamp, getpid(), buf);

What is the pid for...?

Thanks,
Masa

> +	} else
> +		fprintf(f, "%s", buf);
> +>  	fflush(f);
>  	fclose(f);
>  end:
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 4/4] ndctl, monitor: add [Install] Section to systemd unit file of ndctl-monitor

[Masayoshi Mizuma]

Hi Qi,

Looks good to me. Please free to add:

    Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Thanks,
Masa

On 08/07/2018 09:17 AM, QI Fuli wrote:
> This patch is used to add "[Install]" section to systemd unit file of
> ndctl-monitor. Therefore, users can set ndctl-monitor unit enable.
> 
> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> ---
>  ndctl/ndctl-monitor.service | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ndctl/ndctl-monitor.service b/ndctl/ndctl-monitor.service
> index 44f9326..342a1b1 100644
> --- a/ndctl/ndctl-monitor.service
> +++ b/ndctl/ndctl-monitor.service
> @@ -5,3 +5,6 @@ Description=Ndctl Monitor Daemon
>  Type=forking
>  ExecStart=/usr/bin/ndctl monitor --daemon
>  ExecStop=/bin/kill ${MAINPID}
> +
> +[Install]
> +WantedBy=multi-user.target
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Masayoshi Mizuma]

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

Re: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified

[Masayoshi Mizuma]

Hi Qi,

On 08/07/2018 09:17 AM, QI Fuli wrote:
> When running monitor as a daemon, if the log destination is "standard" or
> a relative path for log file, the messages will not be able to be logged.
> Sometimes, users may not notice that the default log destination is "standard"
> when they start monitor daemon by systemctl, so they will lose messages.
> This patch is used to fix the unfriendly interface. When running monitor as a
> daemon, the default log destination will be changed to syslog. Also, the messages
> will be forwarded to syslog if the log destination is a relative path for log file.
> 
> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> ---
>  Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
>  ndctl/monitor.c                       |  5 ++++-
>  ndctl/monitor.conf                    |  2 ++
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ndctl/ndctl-monitor.txt b/Documentation/ndctl/ndctl-monitor.txt
> index 1cba9ea..9a8d76b 100644
> --- a/Documentation/ndctl/ndctl-monitor.txt
> +++ b/Documentation/ndctl/ndctl-monitor.txt
> @@ -67,7 +67,21 @@ OPTIONS
>  
>  -l <file | syslog | standard>::
>  --log=<file | syslog | standard>::
> -	Output notifications to <file>, syslog or standard output.
> +	Send log messages to the specified destination.
> ++
> +--
> +<file>::
> +	Send log messages to specified <file>. When fopen() is not able
> +	to open <file>, log messages will be forwarded to syslog.
> +syslog::
> +	Send messages to syslog.
> +standard::
> +	Send messages to standard output.
> +--
> ++
> +The default log destination is 'syslog' if "--daemon" is specified,
> +otherwise 'standard'. Note that standard and relative path for <file>
> +will not work if "--daemon" is specified.
>  
>  -c::
>  --config-file=::
> diff --git a/ndctl/monitor.c b/ndctl/monitor.c
> index bf1f1d3..2f3d751 100644
> --- a/ndctl/monitor.c
> +++ b/ndctl/monitor.c
> @@ -93,7 +93,8 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const char *file,
>  	f = fopen(monitor.log, "a+");
>  	if (!f) {
>  		ndctl_set_log_fn(ctx, log_syslog);
> -		err(ctx, "open logfile %s failed\n", monitor.log);
> +		err(ctx, "open logfile %s failed, forward messages to syslog\n",
> +				monitor.log);
>  		did_fail = 1;
>  		notice(ctx, "%s\n", buf);
>  		goto end;
> @@ -644,6 +645,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
>  	}
>  
>  	if (monitor.daemon) {

Why don't you add './standard' check? Like as:

                if (strncmp(monitor.log, "./standard", 10) == 0)
                        error("daemon doesn't work for 'standard' log option");
                        goto out;

Thanks,
Masa

> +		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
> +			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);

>  		if (daemon(0, 0) != 0) {
>  			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
>  			goto out;
> diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf
> index 857aadf..934e2c0 100644
> --- a/ndctl/monitor.conf
> +++ b/ndctl/monitor.conf
> @@ -38,4 +38,6 @@
>  # to standard output (log=standard) or to write into a special file (log=<file>)
>  # by setting key "log". If this value is in conflict with the value of
>  # [--log=<value>] option, this value will be ignored.
> +# Note: Setting value to "standard" or relative path for <file> will not work
> +# when running moniotr as a daemon.
>  # log = /var/log/ndctl/monitor.log
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages in log_file()

[Qi, Fuli]

> -----Original Message-----
> From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com]
> Sent: Wednesday, August 8, 2018 3:40 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org
> Subject: Re: [ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages
> in log_file()
> 
> Hi Qi,
> 
> On 08/07/2018 09:17 AM, QI Fuli wrote:
> > This patch is used to add timestamp and process id to log messages
> > when logging messages to a file.
> >
> > 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 2f3d751..d29e378
> > 100644
> > --- a/ndctl/monitor.c
> > +++ b/ndctl/monitor.c
> > @@ -84,6 +84,8 @@ static void log_file(struct ndctl_ctx *ctx, int
> > priority, const char *file,  {
> >  	FILE *f;
> >  	char *buf;
> > +	struct timespec ts;
> > +	char timestamp[32];
> >
> >  	if (vasprintf(&buf, format, args) < 0) {
> >  		fail("vasprintf error\n");
> > @@ -99,7 +101,14 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const
> char *file,
> >  		notice(ctx, "%s\n", buf);
> >  		goto end;
> >  	}
> > -	fprintf(f, "%s", buf);
> > +
> > +	if (priority != LOG_NOTICE) {
> 
Hi Masa,

Thanks for your comments.

> Why is the timestamp not needed in case of LOG_NOTICE...?

Because LOG_NOTICE level is only used for smart event notification in monitor.
Since the timestamp and pid are already added to json format messages by notify_dimm_event(),
so there is no need to add them again.

> 
> > +		clock_gettime(CLOCK_REALTIME, &ts);
> > +		sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
> > +		fprintf(f, "[%s] [%d] %s", timestamp, getpid(), buf);
> 
> What is the pid for...?
> 
Multiple monitor can be run at the same time and they may send messages to a common log file.
In this case, the pid could help users to know the monitor where each message came from.

Thanks,
QI

> Thanks,
> Masa
> 
> > +	} else
> > +		fprintf(f, "%s", buf);
> > +>  	fflush(f);
> >  	fclose(f);
> >  end:
> >
> 

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

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

[Qi, Fuli]

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

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

Re: [ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages in log_file()

[Masayoshi Mizuma]

On 08/07/2018 08:54 PM, Qi, Fuli wrote:
>> -----Original Message-----
>> From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com]
>> Sent: Wednesday, August 8, 2018 3:40 AM
>> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org
>> Subject: Re: [ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages
>> in log_file()
>>
>> Hi Qi,
>>
>> On 08/07/2018 09:17 AM, QI Fuli wrote:
>>> This patch is used to add timestamp and process id to log messages
>>> when logging messages to a file.
>>>
>>> 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 2f3d751..d29e378
>>> 100644
>>> --- a/ndctl/monitor.c
>>> +++ b/ndctl/monitor.c
>>> @@ -84,6 +84,8 @@ static void log_file(struct ndctl_ctx *ctx, int
>>> priority, const char *file,  {
>>>  	FILE *f;
>>>  	char *buf;
>>> +	struct timespec ts;
>>> +	char timestamp[32];
>>>
>>>  	if (vasprintf(&buf, format, args) < 0) {
>>>  		fail("vasprintf error\n");
>>> @@ -99,7 +101,14 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const
>> char *file,
>>>  		notice(ctx, "%s\n", buf);
>>>  		goto end;
>>>  	}
>>> -	fprintf(f, "%s", buf);
>>> +
>>> +	if (priority != LOG_NOTICE) {
>>
> Hi Masa,
> 
> Thanks for your comments.
> 
>> Why is the timestamp not needed in case of LOG_NOTICE...?
> 
> Because LOG_NOTICE level is only used for smart event notification in monitor.
> Since the timestamp and pid are already added to json format messages by notify_dimm_event(),
> so there is no need to add them again.

I see, thanks.

> 
>>
>>> +		clock_gettime(CLOCK_REALTIME, &ts);
>>> +		sprintf(timestamp, "%10ld.%09ld", ts.tv_sec, ts.tv_nsec);
>>> +		fprintf(f, "[%s] [%d] %s", timestamp, getpid(), buf);
>>
>> What is the pid for...?
>>
> Multiple monitor can be run at the same time and they may send messages to a common log file.
> In this case, the pid could help users to know the monitor where each message came from.

OK, got it.

Your patch is good to me. Please feel free to add:

     Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Thanks,
Masa

> 
> Thanks,
> QI
> 
>> Thanks,
>> Masa
>>
>>> +	} else
>>> +		fprintf(f, "%s", buf);
>>> +>  	fflush(f);
>>>  	fclose(f);
>>>  end:
>>>
>>
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified

[Qi, Fuli]

> -----Original Message-----
> From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com]
> Sent: Wednesday, August 8, 2018 4:30 AM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org
> Subject: Re: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog
> if "--daemon" is specified
> 
> Hi Qi,
> 
> On 08/07/2018 09:17 AM, QI Fuli wrote:
> > When running monitor as a daemon, if the log destination is "standard"
> > or a relative path for log file, the messages will not be able to be logged.
> > Sometimes, users may not notice that the default log destination is "standard"
> > when they start monitor daemon by systemctl, so they will lose messages.
> > This patch is used to fix the unfriendly interface. When running
> > monitor as a daemon, the default log destination will be changed to
> > syslog. Also, the messages will be forwarded to syslog if the log destination is
> a relative path for log file.
> >
> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> > ---
> >  Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
> >  ndctl/monitor.c                       |  5 ++++-
> >  ndctl/monitor.conf                    |  2 ++
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/ndctl/ndctl-monitor.txt
> > b/Documentation/ndctl/ndctl-monitor.txt
> > index 1cba9ea..9a8d76b 100644
> > --- a/Documentation/ndctl/ndctl-monitor.txt
> > +++ b/Documentation/ndctl/ndctl-monitor.txt
> > @@ -67,7 +67,21 @@ OPTIONS
> >
> >  -l <file | syslog | standard>::
> >  --log=<file | syslog | standard>::
> > -	Output notifications to <file>, syslog or standard output.
> > +	Send log messages to the specified destination.
> > ++
> > +--
> > +<file>::
> > +	Send log messages to specified <file>. When fopen() is not able
> > +	to open <file>, log messages will be forwarded to syslog.
> > +syslog::
> > +	Send messages to syslog.
> > +standard::
> > +	Send messages to standard output.
> > +--
> > ++
> > +The default log destination is 'syslog' if "--daemon" is specified,
> > +otherwise 'standard'. Note that standard and relative path for <file>
> > +will not work if "--daemon" is specified.
> >
> >  -c::
> >  --config-file=::
> > diff --git a/ndctl/monitor.c b/ndctl/monitor.c index bf1f1d3..2f3d751
> > 100644
> > --- a/ndctl/monitor.c
> > +++ b/ndctl/monitor.c
> > @@ -93,7 +93,8 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const
> char *file,
> >  	f = fopen(monitor.log, "a+");
> >  	if (!f) {
> >  		ndctl_set_log_fn(ctx, log_syslog);
> > -		err(ctx, "open logfile %s failed\n", monitor.log);
> > +		err(ctx, "open logfile %s failed, forward messages to syslog\n",
> > +				monitor.log);
> >  		did_fail = 1;
> >  		notice(ctx, "%s\n", buf);
> >  		goto end;
> > @@ -644,6 +645,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
> >  	}
> >
> >  	if (monitor.daemon) {
> 
> Why don't you add './standard' check? Like as:
> 
>                 if (strncmp(monitor.log, "./standard", 10) == 0)
>                         error("daemon doesn't work for 'standard' log option");
>                         goto out;
> 
Hi Masa,

Thank you for your comment.

When running monitor as a daemon, the messages will not be able to be logged in following cases.
a) Users set the log destination to standard by using [--log] option or setting value of "log" in config file.
b) The log destination is standard by default.
c) Users set the log destination to a relative path of log file by using [--log] option or setting value of "log" in config file.

The './standard' check will only works for case a).

Also, it would be more friendly to set a default log destination to monitor daemon.

Thanks,
QI

> Thanks,
> Masa
> 
> > +		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
> > +			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
> 
> >  		if (daemon(0, 0) != 0) {
> >  			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
> >  			goto out;
> > diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index
> > 857aadf..934e2c0 100644
> > --- a/ndctl/monitor.conf
> > +++ b/ndctl/monitor.conf
> > @@ -38,4 +38,6 @@
> >  # to standard output (log=standard) or to write into a special file
> > (log=<file>)  # by setting key "log". If this value is in conflict
> > with the value of  # [--log=<value>] option, this value will be ignored.
> > +# Note: Setting value to "standard" or relative path for <file> will
> > +not work # when running moniotr as a daemon.
> >  # log = /var/log/ndctl/monitor.log
> >
> 

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

Re: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified

[Masayoshi Mizuma]

HiQi,

On 08/07/2018 09:31 PM, Qi, Fuli wrote:
...
>> On 08/07/2018 09:17 AM, QI Fuli wrote:
>>> When running monitor as a daemon, if the log destination is "standard"
>>> or a relative path for log file, the messages will not be able to be logged.
>>> Sometimes, users may not notice that the default log destination is "standard"
>>> when they start monitor daemon by systemctl, so they will lose messages.
>>> This patch is used to fix the unfriendly interface. When running
>>> monitor as a daemon, the default log destination will be changed to
>>> syslog. Also, the messages will be forwarded to syslog if the log destination is
>> a relative path for log file.
>>>
>>> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
>>> ---
>>>  Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
>>>  ndctl/monitor.c                       |  5 ++++-
>>>  ndctl/monitor.conf                    |  2 ++
>>>  3 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/ndctl/ndctl-monitor.txt
>>> b/Documentation/ndctl/ndctl-monitor.txt
>>> index 1cba9ea..9a8d76b 100644
>>> --- a/Documentation/ndctl/ndctl-monitor.txt
>>> +++ b/Documentation/ndctl/ndctl-monitor.txt
>>> @@ -67,7 +67,21 @@ OPTIONS
>>>
>>>  -l <file | syslog | standard>::
>>>  --log=<file | syslog | standard>::
>>> -	Output notifications to <file>, syslog or standard output.
>>> +	Send log messages to the specified destination.
>>> ++
>>> +--
>>> +<file>::
>>> +	Send log messages to specified <file>. When fopen() is not able
>>> +	to open <file>, log messages will be forwarded to syslog.
>>> +syslog::
>>> +	Send messages to syslog.
>>> +standard::
>>> +	Send messages to standard output.
>>> +--
>>> ++
>>> +The default log destination is 'syslog' if "--daemon" is specified,
>>> +otherwise 'standard'. Note that standard and relative path for <file>
>>> +will not work if "--daemon" is specified.
>>>
>>>  -c::
>>>  --config-file=::
>>> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index bf1f1d3..2f3d751
>>> 100644
>>> --- a/ndctl/monitor.c
>>> +++ b/ndctl/monitor.c
>>> @@ -93,7 +93,8 @@ static void log_file(struct ndctl_ctx *ctx, int priority, const
>> char *file,
>>>  	f = fopen(monitor.log, "a+");
>>>  	if (!f) {
>>>  		ndctl_set_log_fn(ctx, log_syslog);
>>> -		err(ctx, "open logfile %s failed\n", monitor.log);
>>> +		err(ctx, "open logfile %s failed, forward messages to syslog\n",
>>> +				monitor.log);
>>>  		did_fail = 1;
>>>  		notice(ctx, "%s\n", buf);
>>>  		goto end;
>>> @@ -644,6 +645,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
>>>  	}
>>>
>>>  	if (monitor.daemon) {
>>
>> Why don't you add './standard' check? Like as:
>>
>>                 if (strncmp(monitor.log, "./standard", 10) == 0)
>>                         error("daemon doesn't work for 'standard' log option");
>>                         goto out;
>>
> Hi Masa,
> 
> Thank you for your comment.
> 
> When running monitor as a daemon, the messages will not be able to be logged in following cases.
> a) Users set the log destination to standard by using [--log] option or setting value of "log" in config file.

> b) The log destination is standard by default.

Ummm... is this right behavior...? If the monitor running as a daemon,
shouldn't the default log destination be standard...? 
How about the following change?

diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index f10384b..3778334 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -611,8 +611,11 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
        if (argc)
                usage_with_options(u, options);
 
-       /* default to log_standard */
-       ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
+       /* Set default log destination */
+       if (monitor.daemon)
+               ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
+       else
+               ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
 
        if (monitor.verbose)
                ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_DEBUG);

> c) Users set the log destination to a relative path of log file by using [--log] option or setting value of "log" in config file.
> 
> The './standard' check will only works for case a).

I'm sorry, but I'm not clear about c)... The './standard' check should not have
the effect in case of c), right?
We should check the following three cases for monitor daemon, right?

1. '--log standard' option, or
2. In config file 'log = standard'
3. Both 1. and 2. is not set (default behavior).

If I miss something, sorry about that...

Thanks,
Masa

> 
> Also, it would be more friendly to set a default log destination to monitor daemon.
> 
> Thanks,
> QI
> 
 >> Thanks,
>> Masa
>>
>>> +		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
>>> +			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
>>
>>>  		if (daemon(0, 0) != 0) {
>>>  			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
>>>  			goto out;
>>> diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index
>>> 857aadf..934e2c0 100644
>>> --- a/ndctl/monitor.conf
>>> +++ b/ndctl/monitor.conf
>>> @@ -38,4 +38,6 @@
>>>  # to standard output (log=standard) or to write into a special file
>>> (log=<file>)  # by setting key "log". If this value is in conflict
>>> with the value of  # [--log=<value>] option, this value will be ignored.
>>> +# Note: Setting value to "standard" or relative path for <file> will
>>> +not work # when running moniotr as a daemon.
>>>  # log = /var/log/ndctl/monitor.log
>>>
>>
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified

[Qi, Fuli]

> -----Original Message-----
> From: Masayoshi Mizuma [mailto:msys.mizuma@gmail.com]
> Sent: Wednesday, August 8, 2018 10:35 PM
> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com>; linux-nvdimm@lists.01.org
> Subject: Re: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if
> "--daemon" is specified
> 
> HiQi,
> 
> On 08/07/2018 09:31 PM, Qi, Fuli wrote:
> ...
> >> On 08/07/2018 09:17 AM, QI Fuli wrote:
> >>> When running monitor as a daemon, if the log destination is "standard"
> >>> or a relative path for log file, the messages will not be able to be logged.
> >>> Sometimes, users may not notice that the default log destination is "standard"
> >>> when they start monitor daemon by systemctl, so they will lose messages.
> >>> This patch is used to fix the unfriendly interface. When running
> >>> monitor as a daemon, the default log destination will be changed to
> >>> syslog. Also, the messages will be forwarded to syslog if the log
> >>> destination is
> >> a relative path for log file.
> >>>
> >>> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> >>> ---
> >>>  Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
> >>>  ndctl/monitor.c                       |  5 ++++-
> >>>  ndctl/monitor.conf                    |  2 ++
> >>>  3 files changed, 21 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/ndctl/ndctl-monitor.txt
> >>> b/Documentation/ndctl/ndctl-monitor.txt
> >>> index 1cba9ea..9a8d76b 100644
> >>> --- a/Documentation/ndctl/ndctl-monitor.txt
> >>> +++ b/Documentation/ndctl/ndctl-monitor.txt
> >>> @@ -67,7 +67,21 @@ OPTIONS
> >>>
> >>>  -l <file | syslog | standard>::
> >>>  --log=<file | syslog | standard>::
> >>> -	Output notifications to <file>, syslog or standard output.
> >>> +	Send log messages to the specified destination.
> >>> ++
> >>> +--
> >>> +<file>::
> >>> +	Send log messages to specified <file>. When fopen() is not able
> >>> +	to open <file>, log messages will be forwarded to syslog.
> >>> +syslog::
> >>> +	Send messages to syslog.
> >>> +standard::
> >>> +	Send messages to standard output.
> >>> +--
> >>> ++
> >>> +The default log destination is 'syslog' if "--daemon" is specified,
> >>> +otherwise 'standard'. Note that standard and relative path for
> >>> +<file> will not work if "--daemon" is specified.
> >>>
> >>>  -c::
> >>>  --config-file=::
> >>> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index
> >>> bf1f1d3..2f3d751
> >>> 100644
> >>> --- a/ndctl/monitor.c
> >>> +++ b/ndctl/monitor.c
> >>> @@ -93,7 +93,8 @@ static void log_file(struct ndctl_ctx *ctx, int
> >>> priority, const
> >> char *file,
> >>>  	f = fopen(monitor.log, "a+");
> >>>  	if (!f) {
> >>>  		ndctl_set_log_fn(ctx, log_syslog);
> >>> -		err(ctx, "open logfile %s failed\n", monitor.log);
> >>> +		err(ctx, "open logfile %s failed, forward messages to syslog\n",
> >>> +				monitor.log);
> >>>  		did_fail = 1;
> >>>  		notice(ctx, "%s\n", buf);
> >>>  		goto end;
> >>> @@ -644,6 +645,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
> >>>  	}
> >>>
> >>>  	if (monitor.daemon) {
> >>
> >> Why don't you add './standard' check? Like as:
> >>
> >>                 if (strncmp(monitor.log, "./standard", 10) == 0)
> >>                         error("daemon doesn't work for 'standard' log option");
> >>                         goto out;
> >>
> > Hi Masa,
> >
> > Thank you for your comment.
> >
> > When running monitor as a daemon, the messages will not be able to be logged in
> following cases.
> > a) Users set the log destination to standard by using [--log] option or setting value
> of "log" in config file.
> 
> > b) The log destination is standard by default.
> 
> Ummm... is this right behavior...? If the monitor running as a daemon, shouldn't the
> default log destination be standard...?

No, If the monitor runs as a daemon, the default log destination should not be standard.
The default log destination should be syslog, otherwise the messages will not be able to be logged like now.

> How about the following change?
> 
> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index f10384b..3778334 100644
> --- a/ndctl/monitor.c
> +++ b/ndctl/monitor.c
> @@ -611,8 +611,11 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
>         if (argc)
>                 usage_with_options(u, options);
> 
> -       /* default to log_standard */
> -       ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
> +       /* Set default log destination */
> +       if (monitor.daemon)
> +               ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
> +       else
> +               ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
> 
I think there is no need to additionally check the monitor.daemon, and we can just set the default log destination to standard here.
The log destination can be changed to syslog by the following code if monitor.log is NULL, which means case b).

	if (monitor.daemon) {
+		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
+			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);

>         if (monitor.verbose)
>                 ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_DEBUG);
> 
> > c) Users set the log destination to a relative path of log file by using [--log] option
> or setting value of "log" in config file.
> >
> > The './standard' check will only works for case a).
> 
> I'm sorry, but I'm not clear about c)... The './standard' check should not have the
> effect in case of c), right?

Yes, Let me use a sample to clarify the case c).
 # ndctl monitor --daemon --log ./monitor.log
In this case, the messages are not able to be logged.

> We should check the following three cases for monitor daemon, right?
> 
> 1. '--log standard' option, or
> 2. In config file 'log = standard'
> 3. Both 1. and 2. is not set (default behavior).
> 
Yes, and apart from the cases above, we should also check the case c). 

Thanks,
QI

> If I miss something, sorry about that...
> 
> Thanks,
> Masa
> 
> >
> > Also, it would be more friendly to set a default log destination to monitor daemon.
> >
> > Thanks,
> > QI
> >
>  >> Thanks,
> >> Masa
> >>
> >>> +		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
> >>> +			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
> >>
> >>>  		if (daemon(0, 0) != 0) {
> >>>  			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
> >>>  			goto out;
> >>> diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index
> >>> 857aadf..934e2c0 100644
> >>> --- a/ndctl/monitor.conf
> >>> +++ b/ndctl/monitor.conf
> >>> @@ -38,4 +38,6 @@
> >>>  # to standard output (log=standard) or to write into a special file
> >>> (log=<file>)  # by setting key "log". If this value is in conflict
> >>> with the value of  # [--log=<value>] option, this value will be ignored.
> >>> +# Note: Setting value to "standard" or relative path for <file>
> >>> +will not work # when running moniotr as a daemon.
> >>>  # log = /var/log/ndctl/monitor.log
> >>>
> >>
> >
> 

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

Re: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified

[Masayoshi Mizuma]

Hi Qi,

On 08/08/2018 11:59 AM, Qi, Fuli wrote:
...
>>>>> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index
>>>>> bf1f1d3..2f3d751
>>>>> 100644
>>>>> --- a/ndctl/monitor.c
>>>>> +++ b/ndctl/monitor.c
>>>>> @@ -93,7 +93,8 @@ static void log_file(struct ndctl_ctx *ctx, int
>>>>> priority, const
>>>> char *file,
>>>>>  	f = fopen(monitor.log, "a+");
>>>>>  	if (!f) {
>>>>>  		ndctl_set_log_fn(ctx, log_syslog);
>>>>> -		err(ctx, "open logfile %s failed\n", monitor.log);
>>>>> +		err(ctx, "open logfile %s failed, forward messages to syslog\n",
>>>>> +				monitor.log);
>>>>>  		did_fail = 1;
>>>>>  		notice(ctx, "%s\n", buf);
>>>>>  		goto end;
>>>>> @@ -644,6 +645,8 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
>>>>>  	}
>>>>>
>>>>>  	if (monitor.daemon) {
>>>>
>>>> Why don't you add './standard' check? Like as:
>>>>
>>>>                 if (strncmp(monitor.log, "./standard", 10) == 0)
>>>>                         error("daemon doesn't work for 'standard' log option");
>>>>                         goto out;
>>>>
>>> Hi Masa,
>>>
>>> Thank you for your comment.
>>>
>>> When running monitor as a daemon, the messages will not be able to be logged in
>> following cases.
>>> a) Users set the log destination to standard by using [--log] option or setting value
>> of "log" in config file.
>>
>>> b) The log destination is standard by default.
>>
>> Ummm... is this right behavior...? If the monitor running as a daemon, shouldn't the
>> default log destination be standard...?
> 
> No, If the monitor runs as a daemon, the default log destination should not be standard.
> The default log destination should be syslog, otherwise the messages will not be able to be logged like now.

I see. This is what I want to say. Sorry for my explanation is not enough. 

> 
>> How about the following change?
>>
>> diff --git a/ndctl/monitor.c b/ndctl/monitor.c index f10384b..3778334 100644
>> --- a/ndctl/monitor.c
>> +++ b/ndctl/monitor.c
>> @@ -611,8 +611,11 @@ int cmd_monitor(int argc, const char **argv, void *ctx)
>>         if (argc)
>>                 usage_with_options(u, options);
>>
>> -       /* default to log_standard */
>> -       ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
>> +       /* Set default log destination */
>> +       if (monitor.daemon)
>> +               ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
>> +       else
>> +               ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
>>
> I think there is no need to additionally check the monitor.daemon, and we can just set the default log destination to standard here.
> The log destination can be changed to syslog by the following code if monitor.log is NULL, which means case b).
> 
> 	if (monitor.daemon) {
> +		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
> +			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
> 

Ah, you are right, thanks.

>>         if (monitor.verbose)
>>                 ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_DEBUG);
>>
>>> c) Users set the log destination to a relative path of log file by using [--log] option
>> or setting value of "log" in config file.
>>>
>>> The './standard' check will only works for case a).
>>
>> I'm sorry, but I'm not clear about c)... The './standard' check should not have the
>> effect in case of c), right?
> 
> Yes, Let me use a sample to clarify the case c).
>  # ndctl monitor --daemon --log ./monitor.log
> In this case, the messages are not able to be logged.
> 
>> We should check the following three cases for monitor daemon, right?
>>
>> 1. '--log standard' option, or
>> 2. In config file 'log = standard'
>> 3. Both 1. and 2. is not set (default behavior).
>>
> Yes, and apart from the cases above, we should also check the case c). 

OK, thanks! I'm clear now, so please feel free to add:

    Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Thanks,
Masa

> 
> Thanks,
> QI
> 
>> If I miss something, sorry about that...
>>
>> Thanks,
>> Masa
>>
>>>
>>> Also, it would be more friendly to set a default log destination to monitor daemon.
>>>
>>> Thanks,
>>> QI
>>>
>>  >> Thanks,
>>>> Masa
>>>>
>>>>> +		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
>>>>> +			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
>>>>
>>>>>  		if (daemon(0, 0) != 0) {
>>>>>  			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
>>>>>  			goto out;
>>>>> diff --git a/ndctl/monitor.conf b/ndctl/monitor.conf index
>>>>> 857aadf..934e2c0 100644
>>>>> --- a/ndctl/monitor.conf
>>>>> +++ b/ndctl/monitor.conf
>>>>> @@ -38,4 +38,6 @@
>>>>>  # to standard output (log=standard) or to write into a special file
>>>>> (log=<file>)  # by setting key "log". If this value is in conflict
>>>>> with the value of  # [--log=<value>] option, this value will be ignored.
>>>>> +# Note: Setting value to "standard" or relative path for <file>
>>>>> +will not work # when running moniotr as a daemon.
>>>>>  # log = /var/log/ndctl/monitor.log
>>>>>
>>>>
>>>
>>
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [ndctl PATCH 0/4] Some fixups and improvements ndctl monitor

[Verma, Vishal L]

On Tue, 2018-08-07 at 22:17 +0900, QI Fuli wrote:
> Fix a couple of issues about log collection, and add [Install] section
> to systemd unit file of ndclt-monitor.
> 
> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com>
> ---
> QI Fuli (4):
>   ndctl, monitor: fix the lack of detection of invalid path of log file
>   ndctl, monitor: set default log destination to syslog if "--daemon" is specified
>   ndctl, monitor: add timestamp and pid to log messages in log_file()
>   ndctl, monitor: add [Install] Section to systemd unit file of ndctl-monitor
> 
>  Documentation/ndctl/ndctl-monitor.txt | 16 +++++++++++++++-
>  ndctl/monitor.c                       | 27 ++++++++++++++++++++++++---
>  ndctl/monitor.conf                    |  2 ++
>  ndctl/ndctl-monitor.service           |  3 +++
>  4 files changed, 44 insertions(+), 4 deletions(-)
> 
Hi Qi,

These look good, applied.

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