From owner-svn-src-head@FreeBSD.ORG Wed Feb 8 08:32:49 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3971A106566C; Wed, 8 Feb 2012 08:32:49 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-ey0-f182.google.com (mail-ey0-f182.google.com [209.85.215.182]) by mx1.freebsd.org (Postfix) with ESMTP id 082428FC08; Wed, 8 Feb 2012 08:32:47 +0000 (UTC) Received: by eaan10 with SMTP id n10so106281eaa.13 for ; Wed, 08 Feb 2012 00:32:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=from:to:cc:subject:organization:references:sender:date:in-reply-to :message-id:user-agent:mime-version:content-type; bh=DyZ86sHXu14jIrCZAqMYj7T4wZb/K/n/BKLvL1lbAV0=; b=Rv1CjnldyTzUj467EOb1FaV9LdlnOdOgyNFITQYd7Sl5N+tFdqnsmW3nuf/mgDSBI5 1sqpt1gKl/D242+NtdE5uuTXvsdKVZUC30UdwLzkLPNPL/RJX5BpDZiJkBqcq4fUnFde sxPlw6l9AYvxKgcRP4TOhN4gey6KJNIc7GchM= Received: by 10.14.23.201 with SMTP id v49mr5175027eev.92.1328689967148; Wed, 08 Feb 2012 00:32:47 -0800 (PST) Received: from localhost ([94.27.39.186]) by mx.google.com with ESMTPS id n56sm2734810eeh.6.2012.02.08.00.32.43 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 08 Feb 2012 00:32:45 -0800 (PST) From: Mikolaj Golub To: Pawel Jakub Dawidek Organization: TOA Ukraine 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> Sender: Mikolaj Golub Date: Wed, 08 Feb 2012 10:32:41 +0200 In-Reply-To: <20120206221742.GA1336@garage.freebsd.pl> (Pawel Jakub Dawidek's message of "Mon, 6 Feb 2012 23:17:43 +0100") Message-ID: <86liodu3me.fsf@in138.ua3> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (berkeley-unix) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Cc: svn-src-head@FreeBSD.org, Guy Helmer , svn-src-all@FreeBSD.org, Andrey Zonov , src-committers@FreeBSD.org Subject: Re: svn commit: r230869 - head/usr.sbin/daemon X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 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: Wed, 08 Feb 2012 08:32:49 -0000 --=-=-= 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 +#include #include #include -#include #include #include +#include +#include #include #include #include +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); } --=-=-=--