| Message ID | 1500926429-31822-17-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, 2017-07-24 at 14:00 -0600, Jason Gunthorpe wrote: > When running from systemd syslog will send messages to the log, but > so will stderr. Thus using LOG_PERROR doubles every log message. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > srp_daemon/srp_daemon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c > index c0e8d23d58faf5..f0dfe260945574 100644 > --- a/srp_daemon/srp_daemon.c > +++ b/srp_daemon/srp_daemon.c > @@ -2090,7 +2090,7 @@ int main(int argc, char *argv[]) > goto restore_sig; > } > > - openlog("srp_daemon", LOG_PID | LOG_PERROR, LOG_DAEMON); > + openlog("srp_daemon", LOG_PID, LOG_DAEMON); > > config = calloc(1, sizeof(*config)); > if (!config) { Hello Jason, LOG_PERROR is very convenient when debugging srp_daemon. How about using the result of isatty(STDERR_FILENO) to decide whether or not to enable LOG_PERROR? All the other srp_daemon-related patches in this series look fine to me. Thank you for having done this work! Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 25, 2017 at 04:47:59PM +0000, Bart Van Assche wrote: > LOG_PERROR is very convenient when debugging srp_daemon. How about using > the result of isatty(STDERR_FILENO) to decide whether or not to enable > LOG_PERROR? How about a --debug option that sets s_log_dest = log_to_stderr; ? > All the other srp_daemon-related patches in this series look fine to me. > Thank you for having done this work! Great, thanks, I'll add your Reviewed-by to SRP patches then? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-07-25 at 11:17 -0600, Jason Gunthorpe wrote: > On Tue, Jul 25, 2017 at 04:47:59PM +0000, Bart Van Assche wrote: > > LOG_PERROR is very convenient when debugging srp_daemon. How about using > > the result of isatty(STDERR_FILENO) to decide whether or not to enable > > LOG_PERROR? > > How about a --debug option that sets > > s_log_dest = log_to_stderr; Sorry but I prefer that LOG_PERROR is retained when srp_daemon is started in another way than by systemd. How about adding StandardError=null to the srp_daemon unit file? > > All the other srp_daemon-related patches in this series look fine to me. > > Thank you for having done this work! > > Great, thanks, I'll add your Reviewed-by to SRP patches then? Please go ahead. Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 25, 2017 at 05:28:10PM +0000, Bart Van Assche wrote: > On Tue, 2017-07-25 at 11:17 -0600, Jason Gunthorpe wrote: > > On Tue, Jul 25, 2017 at 04:47:59PM +0000, Bart Van Assche wrote: > > > LOG_PERROR is very convenient when debugging srp_daemon. How about using > > > the result of isatty(STDERR_FILENO) to decide whether or not to enable > > > LOG_PERROR? > > > > How about a --debug option that sets > > > > s_log_dest = log_to_stderr; > > Sorry but I prefer that LOG_PERROR is retained when srp_daemon is started in > another way than by systemd. How about adding StandardError=null to the > srp_daemon unit file? No, then emergency prints from libraries are lost forever eg glibc malloc corruption prints. How about a --systemd option? I think we are going to want that anyhow down the road. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-07-25 at 11:44 -0600, Jason Gunthorpe wrote: > On Tue, Jul 25, 2017 at 05:28:10PM +0000, Bart Van Assche wrote: > > On Tue, 2017-07-25 at 11:17 -0600, Jason Gunthorpe wrote: > > > On Tue, Jul 25, 2017 at 04:47:59PM +0000, Bart Van Assche wrote: > > > > LOG_PERROR is very convenient when debugging srp_daemon. How about using > > > > the result of isatty(STDERR_FILENO) to decide whether or not to enable > > > > LOG_PERROR? > > > > > > How about a --debug option that sets > > > > > > s_log_dest = log_to_stderr; > > > > Sorry but I prefer that LOG_PERROR is retained when srp_daemon is started in > > another way than by systemd. How about adding StandardError=null to the > > srp_daemon unit file? > > No, then emergency prints from libraries are lost forever eg glibc > malloc corruption prints. > > How about a --systemd option? I think we are going to want that anyhow > down the road. Hello Jason, That sounds fine to me. Thanks, Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 25, 2017 at 06:09:46PM +0000, Bart Van Assche wrote:
> That sounds fine to me.
Okay, I dropped the perror patch and replaced it with these two:
https://github.com/linux-rdma/rdma-core/pull/172/commits/f1f2e1b1bc3e90abdacdd06b3afe7ca4b107f9e3
https://github.com/linux-rdma/rdma-core/pull/172/commits/588c60a33ac5b846fee039a1cb8e622681fb980e
To implement a --systemd option to control the log behavior.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-07-25 at 17:08 -0600, Jason Gunthorpe wrote: > On Tue, Jul 25, 2017 at 06:09:46PM +0000, Bart Van Assche wrote: > > That sounds fine to me. > > Okay, I dropped the perror patch and replaced it with these two: > > https://github.com/linux-rdma/rdma-core/pull/172/commits/f1f2e1b1bc3e90abdacdd06b3afe7ca4b107f9e3 > https://github.com/linux-rdma/rdma-core/pull/172/commits/588c60a33ac5b846fee039a1cb8e622681fb980e > > To implement a --systemd option to control the log behavior. Hello Jason, Since both is_systemd() and get_config() ignore the local variable opt_idx, how about passing NULL as the fifth argument to getopt_long()? From the getopt_long() man page: "The variable optind is the index of the next element to be processed in argv. The system initializes this value to 1. The caller can reset it to 1 to restart scanning of the same argv, or when scanning a new argument vector." Since getopt_long() is called twice by srp_daemon, shouldn't get_config() reset the optind global variable before calling getopt_long()? Thanks, Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 25, 2017 at 11:50:51PM +0000, Bart Van Assche wrote: > On Tue, 2017-07-25 at 17:08 -0600, Jason Gunthorpe wrote: > > On Tue, Jul 25, 2017 at 06:09:46PM +0000, Bart Van Assche wrote: > > > That sounds fine to me. > > > > Okay, I dropped the perror patch and replaced it with these two: > > > > https://github.com/linux-rdma/rdma-core/pull/172/commits/f1f2e1b1bc3e90abdacdd06b3afe7ca4b107f9e3 > > https://github.com/linux-rdma/rdma-core/pull/172/commits/588c60a33ac5b846fee039a1cb8e622681fb980e > > > > To implement a --systemd option to control the log behavior. > > Hello Jason, > > Since both is_systemd() and get_config() ignore the local variable opt_idx, > how about passing NULL as the fifth argument to getopt_long()? sure, done > Since getopt_long() is called twice by srp_daemon, shouldn't get_config() > reset the optind global variable before calling getopt_long()? Oops! Certainly! done Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c index c0e8d23d58faf5..f0dfe260945574 100644 --- a/srp_daemon/srp_daemon.c +++ b/srp_daemon/srp_daemon.c @@ -2090,7 +2090,7 @@ int main(int argc, char *argv[]) goto restore_sig; } - openlog("srp_daemon", LOG_PID | LOG_PERROR, LOG_DAEMON); + openlog("srp_daemon", LOG_PID, LOG_DAEMON); config = calloc(1, sizeof(*config)); if (!config) {
When running from systemd syslog will send messages to the log, but so will stderr. Thus using LOG_PERROR doubles every log message. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- srp_daemon/srp_daemon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)