Date: Mon, 6 Feb 2012 15:56:59 -0600 From: Guy Helmer <ghelmer@palisadesystems.com> To: Mikolaj Golub <trociny@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Andrey Zonov <andrey@zonov.org>, Pawel Jakub Dawidek <pjd@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r230869 - head/usr.sbin/daemon Message-ID: <BA38E5D5-858F-4F4E-8F2E-6E61DC866260@palisadesystems.com> In-Reply-To: <86wr7zmy8f.fsf@kopusha.home.net> References: <201202011641.q11Gf0j6095461@svn.freebsd.org> <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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Feb 6, 2012, at 3:46 PM, Mikolaj Golub wrote:
>=20
> On Mon, 6 Feb 2012 09:27:06 +0100 Pawel Jakub Dawidek wrote:
>=20
> PJD> For the patch itself.
>=20
> 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.
>=20
> 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.
>=20
> Thanks. The updated version is attached.
>=20
> --=20
> Mikolaj Golub
>=20
> Index: usr.sbin/daemon/daemon.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- usr.sbin/daemon/daemon.c (revision 231014)
> +++ usr.sbin/daemon/daemon.c (working copy)
> @@ -32,6 +32,7 @@
> __FBSDID("$FreeBSD$");
>=20
> #include <sys/param.h>
> +#include <sys/wait.h>
>=20
> #include <err.h>
> #include <errno.h>
> @@ -43,15 +44,16 @@ __FBSDID("$FreeBSD$");
> #include <unistd.h>
>=20
> static void restrict_process(const char *);
> +static void wait_child(pid_t pid);
> static void usage(void);
>=20
> int
> main(int argc, char *argv[])
> {
> struct pidfh *pfh =3D NULL;
> - int ch, nochdir, noclose, errcode;
> + int ch, nochdir, noclose;
> const char *pidfile, *user;
> - pid_t otherpid;
> + pid_t otherpid, pid;
>=20
> nochdir =3D noclose =3D 1;
> pidfile =3D user =3D NULL;
> @@ -79,9 +81,7 @@ main(int argc, char *argv[])
> if (argc =3D=3D 0)
> usage();
>=20
> - if (user !=3D NULL)
> - restrict_process(user);
> -
> + pfh =3D 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) =3D=3D -1)
> err(1, NULL);
>=20
> - /* Now that we are the child, write out the pid */
> - if (pidfile)
> + pid =3D 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 =3D fork();
> + if (pid =3D=3D -1) {
> + pidfile_remove(pfh);
> + err(1, "fork");
> + }
> + }
> + if (pid =3D=3D 0) {
> + /* Now that we are the child, write out the pid. */
> pidfile_write(pfh);
>=20
> - execvp(argv[0], argv);
> + if (user !=3D NULL)
> + restrict_process(user);
>=20
> - /*
> - * execvp() failed -- unlink pidfile if any, and
> - * report the error
> - */
> - errcode =3D errno; /* Preserve errcode -- unlink may reset it */
> - if (pidfile)
> - pidfile_remove(pfh);
> + execvp(argv[0], argv);
>=20
> - /* 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. */
> }
>=20
> static void
> @@ -132,6 +146,19 @@ restrict_process(const char *user)
> }
>=20
> static void
> +wait_child(pid_t pid)
> +{
> + int status;
> +
> + while (waitpid(pid, &status, 0) =3D=3D -1) {
> + if (errno !=3D 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BA38E5D5-858F-4F4E-8F2E-6E61DC866260>