From owner-svn-src-head@FreeBSD.ORG Mon Feb 6 21:57:34 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 E823B106564A; Mon, 6 Feb 2012 21:57:34 +0000 (UTC) (envelope-from ghelmer@palisadesystems.com) Received: from ps-1-a.compliancesafe.com (ps-1-a.compliancesafe.com [216.81.161.161]) by mx1.freebsd.org (Postfix) with ESMTP id 564D38FC0C; Mon, 6 Feb 2012 21:57:34 +0000 (UTC) Received: from mail.palisadesystems.com (localhost [127.0.0.1]) by ps-1-a.compliancesafe.com (8.14.4/8.14.3) with ESMTP id q16LvFZJ038565; Mon, 6 Feb 2012 15:57:16 -0600 (CST) (envelope-from ghelmer@palisadesystems.com) Received: from [192.168.221.90] ([192.168.221.90]) (authenticated bits=0) by mail.palisadesystems.com (8.14.3/8.14.3) with ESMTP id q16Lv0Uv087096 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Mon, 6 Feb 2012 15:57:03 -0600 (CST) (envelope-from ghelmer@palisadesystems.com) X-DKIM: Sendmail DKIM Filter v2.8.3 mail.palisadesystems.com q16Lv0Uv087096 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=palisadesystems.com; s=mail; t=1328565425; bh=2++lRsVqfKz9BBtMLY1lIDdQrLR7kn9EpbzGL/TpGDA=; l=128; h=Subject:Mime-Version:Content-Type:From:In-Reply-To:Date:Cc: Content-Transfer-Encoding:Message-Id:References:To; b=FrhSpdfxhsrBGMcoxfBi5Y0tj1R8kgfT9RKEIPVyMInXUHn8hnIDrpVDlWMKh9AvV eo8R2dZyOmkf05rXKbEf07WKlw9yDBsa9XYHZF60IreIzZsCZUqNRzlPbEB8eC7nje NE0p0sJijnpUkskyJu/oY+X0qYe3WgwgRezx3Dp8= Mime-Version: 1.0 (Apple Message framework v1257) Content-Type: text/plain; charset=us-ascii From: Guy Helmer In-Reply-To: <86wr7zmy8f.fsf@kopusha.home.net> Date: Mon, 6 Feb 2012 15:56:59 -0600 Content-Transfer-Encoding: quoted-printable Message-Id: 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> To: Mikolaj Golub X-Mailer: Apple Mail (2.1257) X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.5 (mail.palisadesystems.com [172.16.1.5]); Mon, 06 Feb 2012 15:57:05 -0600 (CST) X-Palisade-MailScanner-Information: Please contact the ISP for more information X-Palisade-MailScanner-ID: q16Lv0Uv087096 X-Palisade-MailScanner: Found to be clean X-Palisade-MailScanner-SpamCheck: not spam, SpamAssassin (score=-0.806, required 5, ALL_TRUSTED -1.00, BAYES_00 -1.90, J_CHICKENPOX_43 0.60, RP_8BIT 1.49) X-Palisade-MailScanner-From: ghelmer@palisadesystems.com X-Spam-Status: No X-PacketSure-Scanned: Yes Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Andrey Zonov , Pawel Jakub Dawidek , 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: Mon, 06 Feb 2012 21:57:35 -0000 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 > +#include >=20 > #include > #include > @@ -43,15 +44,16 @@ __FBSDID("$FreeBSD$"); > #include >=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.