svn commit: r255521 - head/usr.sbin/daemon

[Mikolaj Golub]

On Fri, Sep 13, 2013 at 04:57:28PM +0000, John-Mark Gurney wrote:
> Author: jmg
> Date: Fri Sep 13 16:57:28 2013
> New Revision: 255521
> URL: http://svnweb.freebsd.org/changeset/base/255521
> 
> Log:
>   add support for writing the pid of the daemon program to a pid file so
>   that daemon can be used w/ rc.subr and ports can use the additional
>   functionality, such as keeping the ldap daemon up and running, and have
>   the proper program to signal to exit..
>   
>   PR:		bin/181341
>   Submitted by:	feld
>   Approved by:	re (glebius)

Thank you for doing this. I was aware about this issue after I had
added restart option but could not come with a solution that would
satisfy me.

Having 2 options for pid files might look a little confusing but I
think it is much better than we had before.

Still please see a couple of notes below.

> Modified: head/usr.sbin/daemon/daemon.c
> ==============================================================================
> --- head/usr.sbin/daemon/daemon.c	Fri Sep 13 14:15:52 2013	(r255520)
> +++ head/usr.sbin/daemon/daemon.c	Fri Sep 13 16:57:28 2013	(r255521)
> @@ -53,16 +53,17 @@ static void usage(void);
>  int
>  main(int argc, char *argv[])
>  {
> -	struct pidfh *pfh = NULL;
> +	struct pidfh  *ppfh, *pfh;
>  	sigset_t mask, oldmask;
>  	int ch, nochdir, noclose, restart;
> -	const char *pidfile, *user;
> +	const char *pidfile, *ppidfile,  *user;
>  	pid_t otherpid, pid;
>  
>  	nochdir = noclose = 1;
>  	restart = 0;
> -	pidfile = user = NULL;
> -	while ((ch = getopt(argc, argv, "cfp:ru:")) != -1) {
> +	ppfh = pfh = NULL;
> +	ppidfile = pidfile = user = NULL;
> +	while ((ch = getopt(argc, argv, "cfp:P:ru:")) != -1) {
>  		switch (ch) {
>  		case 'c':
>  			nochdir = 0;
> @@ -73,6 +74,9 @@ main(int argc, char *argv[])
>  		case 'p':
>  			pidfile = optarg;
>  			break;
> +		case 'P':
> +			ppidfile = optarg;
> +			break;
>  		case 'r':
>  			restart = 1;
>  			break;
> @@ -89,7 +93,7 @@ main(int argc, char *argv[])
>  	if (argc == 0)
>  		usage();
>  
> -	pfh = NULL;
> +	ppfh = pfh = NULL;
>  	/*
>  	 * Try to open the pidfile before calling daemon(3),
>  	 * to be able to report the error intelligently
> @@ -104,6 +108,18 @@ main(int argc, char *argv[])
>  			err(2, "pidfile ``%s''", pidfile);
>  		}
>  	}
> +	
> +	/* do same for actual daemon process */
> +	if (ppidfile != NULL) {
> +		ppfh = pidfile_open(ppidfile, 0600, &otherpid);
> +		if (ppfh == NULL) {
> +			if (errno == EEXIST) {
> +				errx(3, "process already running, pid: %d",
> +				     otherpid);
> +			}
> +			err(2, "ppidfile ``%s''", ppidfile);

You have to pidfile_remove(pfh) before exiting not to leave the child
pidfile on fs. Also pidfile_remove(ppfh) if fork() fails, and there
are several other places where pidfile_remove() is missed for the
child too before err exit (me should be blamed for most of these).

> +		}
> +	}
>  
>  	if (daemon(nochdir, noclose) == -1)
>  		err(1, NULL);
> @@ -176,12 +192,17 @@ restart:
>  		 */
>  		err(1, "%s", argv[0]);
>  	}
> +	/* write out parent pidfile if needed */
> +	if (ppidfile != NULL)
> +		pidfile_write(ppfh);
> +

No need to check for ppidfile != NULL. It is safe to call
pidfile_write() (and pidfile_remove()) with NULL argument.

Also, doing pidfile_write() here will cause needlessly rewriting the
file after every child restart. I think it is better to call
pidfile_write(ppfh) once, before the restart loop, just after
daemon().

>  	setproctitle("%s[%d]", argv[0], pid);
>  	if (wait_child(pid, &mask) == 0 && restart) {
>  		sleep(1);
>  		goto restart;
>  	}
>  	pidfile_remove(pfh);
> +	pidfile_remove(ppfh);
>  	exit(0); /* Exit status does not matter. */
>  }
>  
> @@ -240,7 +261,7 @@ static void
>  usage(void)
>  {
>  	(void)fprintf(stderr,
> -	    "usage: daemon [-cfr] [-p pidfile] [-u user] command "
> -		"arguments ...\n");
> +	    "usage: daemon [-cfr] [-p child_pidfile] [-P supervisor_pidfile] "
> +	    "[-u user]\n              command arguments ...\n");
>  	exit(1);
>  }

-- 
Mikolaj Golub