Date: Sun, 1 Jan 2017 15:56:00 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Hiroki Sato <hrs@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r310974 - head/usr.sbin/syslogd Message-ID: <20170101145600.GA80448@stack.nl> In-Reply-To: <201612311315.uBVDFqYl096848@repo.freebsd.org> References: <201612311315.uBVDFqYl096848@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Dec 31, 2016 at 01:15:52PM +0000, Hiroki Sato wrote: > Author: hrs > Date: Sat Dec 31 13:15:52 2016 > New Revision: 310974 > URL: https://svnweb.freebsd.org/changeset/base/310974 > Log: > - Use more descriptive names for variables. > - Set O_CLOEXEC to the signal pipe and /dev/klog. > - Use a single signal handler to catch both SIGHUP and SIGCHLD. > - Fix a bug which did FD_SET() the writer-end of the pipe. > Modified: > head/usr.sbin/syslogd/syslogd.c > Modified: head/usr.sbin/syslogd/syslogd.c > ============================================================================== > --- head/usr.sbin/syslogd/syslogd.c Sat Dec 31 13:10:08 2016 (r310973) > +++ head/usr.sbin/syslogd/syslogd.c Sat Dec 31 13:15:52 2016 (r310974) > [snip] > @@ -582,18 +581,18 @@ main(int argc, char *argv[]) > usage(); > > /* Pipe to catch a signal during select(). */ > - s = pipe2(sigp, O_NONBLOCK); > + s = pipe2(sigpipe, O_CLOEXEC); A blocking pipe will cause a deadlock if too many signals arrive before the main loop reads them out. > if (s < 0) { > err(1, "cannot open a pipe for signals"); > } else { > addsock(NULL, 0, &(struct socklist){ > - .sl_socket = sigp[1], > + .sl_socket = sigpipe[0], > .sl_recv = socklist_recv_signal > }); > } > > /* Listen by default: /dev/klog. */ > - s = open(_PATH_KLOG, O_RDONLY|O_NONBLOCK, 0); > + s = open(_PATH_KLOG, O_RDONLY | O_NONBLOCK | O_CLOEXEC, 0); > if (s < 0) { > dprintf("can't open %s (%d)\n", _PATH_KLOG, errno); > } else { > @@ -646,8 +645,8 @@ main(int argc, char *argv[]) > (void)signal(SIGTERM, dodie); > (void)signal(SIGINT, Debug ? dodie : SIG_IGN); > (void)signal(SIGQUIT, Debug ? dodie : SIG_IGN); > - (void)signal(SIGHUP, init_sh); > - (void)signal(SIGCHLD, reapchild_sh); > + (void)signal(SIGHUP, sighandler); > + (void)signal(SIGCHLD, sighandler); > (void)signal(SIGALRM, domark); > (void)signal(SIGPIPE, SIG_IGN); /* We'll catch EPIPE instead. */ > (void)alarm(TIMERINTVL); Although it is not broken to use signal() on FreeBSD, it is more portable and possibly easier to understand the exact semantics if sigaction() is used instead of signal() to set handlers. Setting SIG_DFL or SIG_IGN with signal() is acceptable. Given that you're changing things here anyway, it may be a good idea to start using sigaction() here. > @@ -717,12 +716,31 @@ static int > socklist_recv_signal(struct socklist *sl __unused) > { > ssize_t len; > - static char buf[BUFSIZ]; > - > - /* Clear an wake-up signal by reading dummy data. */ > - while ((len = read(sigp[0], buf, sizeof(buf))) > 0) > - ; > + int i, nsig, signo; > > + if (ioctl(sigpipe[0], FIONREAD, &i) != 0) { > + logerror("ioctl(FIONREAD)"); > + err(1, "signal pipe read failed"); > + } With a non-blocking pipe, this can go away and you can just read until an [EAGAIN] error. > [snip] > @@ -1512,17 +1530,6 @@ ttymsg_check(struct iovec *iov, int iovc > } > > static void > -reapchild_sh(int signo) > -{ > - static char buf[BUFSIZ]; > - > - WantReapchild = signo; I think setting a volatile sig_atomic_t variable in a signal handler is fine. This would avoid dropping different signal numbers if many signals arrive before the main loop gets around to process them. > - /* Send an wake-up signal to the select() loop. */ > - read(sigp[0], buf, sizeof(buf)); > - write(sigp[1], &signo, sizeof(signo)); > -} > - > -static void > reapchild(int signo __unused) > { > int status; The parameter can go away now. > [snip] -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170101145600.GA80448>