From owner-svn-src-head@freebsd.org Sun Jan 1 14:56:13 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A1546C9AA27; Sun, 1 Jan 2017 14:56:13 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mailout.stack.nl (mailout05.stack.nl [IPv6:2001:610:1108:5010::202]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mailout.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 3184817F0; Sun, 1 Jan 2017 14:56:13 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mailout.stack.nl (Postfix) with ESMTP id C09FD40; Sun, 1 Jan 2017 15:56:00 +0100 (CET) Received: by snail.stack.nl (Postfix, from userid 1677) id AFCD428494; Sun, 1 Jan 2017 15:56:00 +0100 (CET) Date: Sun, 1 Jan 2017 15:56:00 +0100 From: Jilles Tjoelker To: Hiroki Sato 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> References: <201612311315.uBVDFqYl096848@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201612311315.uBVDFqYl096848@repo.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 01 Jan 2017 14:56:13 -0000 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