nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: "Qi, Fuli" <qi.fuli@jp.fujitsu.com>
To: 'Masayoshi Mizuma' <msys.mizuma@gmail.com>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: RE: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified
Date: Wed, 8 Aug 2018 15:59:55 +0000	[thread overview]
Message-ID: <0DEDF3B159719A448A49EF0E7B11E3223DA9F262@g01jpexmbkw24> (raw)
In-Reply-To: <01b090d0-64ee-8484-3183-313f1071421b@gmail.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

  reply	other threads:[~2018-08-08 16:00 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 13:17 [ndctl PATCH 0/4] Some fixups and improvements ndctl monitor QI Fuli
2018-08-07 13:17 ` [ndctl PATCH 1/4] ndctl, monitor: fix the lack of detection of invalid path of log file QI Fuli
2018-08-07 19:05   ` Masayoshi Mizuma
2018-08-08  1:01     ` Qi, Fuli
2018-08-07 13:17 ` [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified QI Fuli
2018-08-07 19:30   ` Masayoshi Mizuma
2018-08-08  1:31     ` Qi, Fuli
2018-08-08 13:35       ` Masayoshi Mizuma
2018-08-08 15:59         ` Qi, Fuli [this message]
2018-08-08 16:24           ` Masayoshi Mizuma
2018-08-07 13:17 ` [ndctl PATCH 3/4] ndctl, monitor: add timestamp and pid to log messages in log_file() QI Fuli
2018-08-07 18:39   ` Masayoshi Mizuma
2018-08-08  0:54     ` Qi, Fuli
2018-08-08  1:24       ` Masayoshi Mizuma
2018-08-07 13:17 ` [ndctl PATCH 4/4] ndctl, monitor: add [Install] Section to systemd unit file of ndctl-monitor QI Fuli
2018-08-07 18:41   ` Masayoshi Mizuma
2018-08-10 20:42 ` [ndctl PATCH 0/4] Some fixups and improvements ndctl monitor Verma, Vishal L

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0DEDF3B159719A448A49EF0E7B11E3223DA9F262@g01jpexmbkw24 \
    --to=qi.fuli@jp.fujitsu.com \
    --cc=linux-nvdimm@lists.01.org \
    --cc=msys.mizuma@gmail.com \
    --subject='RE: [ndctl PATCH 2/4] ndctl, monitor: set default log destination to syslog if "--daemon" is specified' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).