Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 08 Feb 2012 10:32:41 +0200
From:      Mikolaj Golub <trociny@freebsd.org>
To:        Pawel Jakub Dawidek <pjd@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, Guy Helmer <ghelmer@palisadesystems.com>, svn-src-all@FreeBSD.org, Andrey Zonov <andrey@zonov.org>, src-committers@FreeBSD.org
Subject:   Re: svn commit: r230869 - head/usr.sbin/daemon
Message-ID:  <86liodu3me.fsf@in138.ua3>
In-Reply-To: <20120206221742.GA1336@garage.freebsd.pl> (Pawel Jakub Dawidek's message of "Mon, 6 Feb 2012 23:17:43 %2B0100")
References:  <20120204074201.GA1694@garage.freebsd.pl> <4F2CEB1D.10607@zonov.org> <27A0A960-F767-4D2C-BF3E-31F73FBF4E28@palisadesystems.com> <86zkcy5ur9.fsf@kopusha.home.net> <20120205093938.GC30033@garage.freebsd.pl> <86lioh7yz5.fsf@kopusha.home.net> <20120205214647.GI30033@garage.freebsd.pl> <86sjiov29o.fsf@in138.ua3> <20120206082706.GA1324@garage.freebsd.pl> <86wr7zmy8f.fsf@kopusha.home.net> <20120206221742.GA1336@garage.freebsd.pl>

next in thread | previous in thread | raw e-mail | index | archive | help
--=-=-=


On Mon, 6 Feb 2012 23:17:43 +0100 Pawel Jakub Dawidek wrote:

 PJD> On Mon, Feb 06, 2012 at 11:46:24PM +0200, Mikolaj Golub wrote:

 >> Thanks. The updated version is attached.

 PJD> Looks good to me.

Thanks. But I still think that adding some signal handling is a good idea. I
agree that there may be no sense in trying to handle many different signals,
but handling SIGTERM (software termination signal :-) nicely looks like a good
thing.

This would solve problems like stale pid files after shutdown or orphaned
programs (again with stale pid files and a possibility to start another
instance) due to a user mistakenly terminated the supervising daemons.

Also it is possible then to add "automatic restart" option, as Andrey has
proposed.

Here is the patch that does it. It also change proctitle to make identifying a
a supervisor with its charge.

On the other hand not being an active user of daemon(8) utility, I don't have
a strong opinion and would like to see what other people, especially those who
use it, think about this.

A technical question concerning the patch :-). Does sombody know if
sigwaitinfo() may be interrupted in my case and I should check for EINTR, as I
do in the patch?

-- 
Mikolaj Golub


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline; filename=daemon.c.3.patch

Index: usr.sbin/daemon/daemon.c
===================================================================
--- usr.sbin/daemon/daemon.c	(revision 231014)
+++ usr.sbin/daemon/daemon.c	(working copy)
@@ -32,30 +32,36 @@
 __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
-#include <pwd.h>
 #include <libutil.h>
 #include <login_cap.h>
+#include <pwd.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
 
+static void dummy_sighandler(int);
 static void restrict_process(const char *);
+static int  wait_child(pid_t, sigset_t *);
 static void usage(void);
 
 int
 main(int argc, char *argv[])
 {
 	struct pidfh *pfh = NULL;
-	int ch, nochdir, noclose, errcode;
+	sigset_t mask, oldmask;
+	int ch, nochdir, noclose, restart;
 	const char *pidfile, *user;
-	pid_t otherpid;
+	pid_t otherpid, pid;
 
 	nochdir = noclose = 1;
+	restart = 0;
 	pidfile = user = NULL;
-	while ((ch = getopt(argc, argv, "-cfp:u:")) != -1) {
+	while ((ch = getopt(argc, argv, "-cfp:ru:")) != -1) {
 		switch (ch) {
 		case 'c':
 			nochdir = 0;
@@ -66,6 +72,9 @@ main(int argc, char *argv[])
 		case 'p':
 			pidfile = optarg;
 			break;
+		case 'r':
+			restart = 1;
+			break;
 		case 'u':
 			user = optarg;
 			break;
@@ -79,14 +88,12 @@ main(int argc, char *argv[])
 	if (argc == 0)
 		usage();
 
-	if (user != NULL)
-		restrict_process(user);
-
+	pfh = NULL;
 	/*
 	 * Try to open the pidfile before calling daemon(3),
 	 * to be able to report the error intelligently
 	 */
-	if (pidfile) {
+	if (pidfile != NULL) {
 		pfh = pidfile_open(pidfile, 0600, &otherpid);
 		if (pfh == NULL) {
 			if (errno == EEXIST) {
@@ -99,26 +106,83 @@ main(int argc, char *argv[])
 
 	if (daemon(nochdir, noclose) == -1)
 		err(1, NULL);
+	/*
+	 * If pidfile or restart option is specified the daemon
+	 * executes the command in a forked process and wait on child
+	 * exit to remove the pidfile or restart the command.
+	 * Normally we don't want the monitoring daemon to be
+	 * terminated leaving the running process and the stale
+	 * pidfile, so we catch SIGTERM and pass it to the children
+	 * expecting to get SIGCHLD eventually.
+	 */
+	pid = -1;
+	if (pidfile != NULL || restart) {
+		/*
+		 * Restore default action for SIGTERM in case the
+		 * parent process decided to ignore it.
+		 */
+		if (signal(SIGTERM, SIG_DFL) == SIG_ERR)
+			err(1, "signal");
+		/*
+		 * Because SIGCHLD is ignored by default, setup dummy handler
+		 * for it, so we can mask it.
+		 */
+		if (signal(SIGCHLD, dummy_sighandler) == SIG_ERR)
+			err(1, "signal");
+		/*
+		 * Block interesting signals.
+		 */
+		sigemptyset(&mask);
+		sigaddset(&mask, SIGTERM);
+		sigaddset(&mask, SIGCHLD);
+		if (sigprocmask(SIG_SETMASK, &mask, &oldmask) == -1)
+			err(1, "sigprocmask");
+restart:
+		/*
+		 * Spawn a child to exec the command, so in the parent
+		 * we could wait for it to exit and remove pidfile.
+		 */
+		pid = fork();
+		if (pid == -1) {
+			pidfile_remove(pfh);
+			err(1, "fork");
+		}
+	}
+	if (pid <= 0) {
+		if (pid == 0) {
+			/* Restore old sigmask in the child. */
+			if (sigprocmask(SIG_SETMASK, &oldmask, NULL) == -1)
+				err(1, "sigprocmask");
+		}
 
-	/* Now that we are the child, write out the pid */
-	if (pidfile)
+		/* Now that we are the child, write out the pid. */
 		pidfile_write(pfh);
 
-	execvp(argv[0], argv);
+		if (user != NULL)
+			restrict_process(user);
 
-	/*
-	 * execvp() failed -- unlink pidfile if any, and
-	 * report the error
-	 */
-	errcode = errno; /* Preserve errcode -- unlink may reset it */
-	if (pidfile)
-		pidfile_remove(pfh);
+		execvp(argv[0], argv);
 
-	/* The child is now running, so the exit status doesn't matter. */
-	errc(1, errcode, "%s", argv[0]);
+		/*
+		 * execvp() failed -- report the error. The child is
+		 * now running, so the exit status doesn't matter.
+		 */
+		err(1, "%s", argv[0]);
+	}
+	setproctitle("%s[%d]", argv[0], pid);
+	if (wait_child(pid, &mask) == 0 && restart)
+		goto restart;
+	pidfile_remove(pfh);
+	exit(0); /* Exit status does not matter. */
 }
 
 static void
+dummy_sighandler(int sig __unused)
+{
+	/* Nothing to do. */
+}
+
+static void
 restrict_process(const char *user)
 {
 	struct passwd *pw = NULL;
@@ -131,11 +195,42 @@ restrict_process(const char *user)
 		errx(1, "failed to set user environment");
 }
 
+static int
+wait_child(pid_t pid, sigset_t *mask)
+{
+	int terminate;
+	int signo;
+
+	terminate = 0;
+	for (;;) {
+		signo = sigwaitinfo(mask, NULL);
+		switch (signo) {
+		case SIGCHLD:
+			return terminate;
+		case SIGTERM:
+			terminate = 1;
+			if (kill(pid, signo) == -1) {
+				warn("kill");
+				return -1;
+			}
+			continue;
+		case -1:
+			if (errno == EINTR)
+				continue;
+			warn("sigwaitinfo");
+			return -1;
+		default:
+			warnx("sigwaitinfo: invalid signal: %d", signo);
+			return -1;
+		}
+	}
+}
+
 static void
 usage(void)
 {
 	(void)fprintf(stderr,
-	    "usage: daemon [-cf] [-p pidfile] [-u user] command "
+	    "usage: daemon [-cfr] [-p pidfile] [-u user] command "
 		"arguments ...\n");
 	exit(1);
 }

--=-=-=--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86liodu3me.fsf>