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

[Guy Helmer]

On Feb 6, 2012, at 3:46 PM, Mikolaj Golub wrote:

> 
> On Mon, 6 Feb 2012 09:27:06 +0100 Pawel Jakub Dawidek wrote:
> 
> PJD> For the patch itself.
> 
> PJD> You don't have to have two separate cases depending on request for
> PJD> pidfile. You can specify NULL pfh to the pidfile functions.
> PJD> Even in example from the manual page when pfh is NULL there is a case
> PJD> where we warn, but continue execution and call pidfile functions.
> PJD> This should simplify the code.
> 
> PJD> If you do that (actually even if you don't), remember to either use
> PJD> warn(3) before pidfile_remove(3) and exit(3) after or preserve errno
> PJD> before calling pidfile_remove(3), as pidfile_remove(3) can modify it if
> PJD> unlink(2) is unsuccessful or pfh is NULL.
> 
> Thanks. The updated version is attached.
> 
> -- 
> Mikolaj Golub
> 
> Index: usr.sbin/daemon/daemon.c
> ===================================================================
> --- usr.sbin/daemon/daemon.c	(revision 231014)
> +++ usr.sbin/daemon/daemon.c	(working copy)
> @@ -32,6 +32,7 @@
> __FBSDID("$FreeBSD$");
> 
> #include <sys/param.h>
> +#include <sys/wait.h>
> 
> #include <err.h>
> #include <errno.h>
> @@ -43,15 +44,16 @@ __FBSDID("$FreeBSD$");
> #include <unistd.h>
> 
> static void restrict_process(const char *);
> +static void wait_child(pid_t pid);
> static void usage(void);
> 
> int
> main(int argc, char *argv[])
> {
> 	struct pidfh *pfh = NULL;
> -	int ch, nochdir, noclose, errcode;
> +	int ch, nochdir, noclose;
> 	const char *pidfile, *user;
> -	pid_t otherpid;
> +	pid_t otherpid, pid;
> 
> 	nochdir = noclose = 1;
> 	pidfile = user = NULL;
> @@ -79,9 +81,7 @@ 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
> @@ -100,22 +100,36 @@ main(int argc, char *argv[])
> 	if (daemon(nochdir, noclose) == -1)
> 		err(1, NULL);
> 
> -	/* Now that we are the child, write out the pid */
> -	if (pidfile)
> +	pid = 0;
> +	if (pidfile) {
> +		/*
> +		 * 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) {
> +		/* 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]);
> +	}
> +	wait_child(pid);
> +	pidfile_remove(pfh);
> +	exit(0); /* Exit status does not metter. */
> }
> 
> static void
> @@ -132,6 +146,19 @@ restrict_process(const char *user)
> }
> 
> static void
> +wait_child(pid_t pid)
> +{
> +	int status;
> +
> +	while (waitpid(pid, &status, 0) == -1) {
> +		if (errno != EINTR) {
> +			warn("waitpid");
> +			break;
> +		}
> +	}
> +}
> +
> +static void
> usage(void)
> {
> 	(void)fprintf(stderr,

Generally looks good to me -- I had patches to do a similar change but yours looks better.  When I get a chance, I will test your change with the O_CLOEXEC flag added back to the open() call in pidfile_open() -- I'm not sure how soon I will be able to do that, though.

Guy--------
This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure.