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

["Qi, Fuli" <qi.fuli@jp.fujitsu.com>]

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