Date: Wed, 13 Mar 2013 22:27:50 +0100 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: Dirk Engling <erdgeist@erdgeist.org>, freebsd-current@freebsd.org Subject: Re: pidfile_open incorrectly returns EAGAIN when pidfile is locked Message-ID: <20130313212750.GC1372@garage.freebsd.pl> In-Reply-To: <201303131118.36811.jhb@freebsd.org> References: <513F8D20.2050707@erdgeist.org> <201303131118.36811.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--ncSAzJYg3Aa9+CRW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Mar 13, 2013 at 11:18:36AM -0400, John Baldwin wrote:
> On Tuesday, March 12, 2013 4:16:32 pm Dirk Engling wrote:
> > While debugging my own daemon I noticed that pidfile_open does not
> > perform the appropriate checks for a running daemon if the caller does
> > not provide a pidptr to pidfile_open
> >=20
> > fd =3D flopen(pfh->pf_path,
> > O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NONBLOCK, mode);
> >=20
> > fails when another daemon holds the lock and flopen sets errno to
> > EAGAIN, the check 4 lines below in
> >=20
> > if (errno =3D=3D EWOULDBLOCK && pidptr !=3D NULL) {
> >=20
> > means that the pidfile_read is never executed.
> >=20
> > This results in my second daemon receiving an EAGAIN which clearly was
> > meant to report a race condition between two daemons starting at the
> > same time and the first one not yet finishing pidfile_write.
> >=20
> > The expected behavior would be to set errno to EEXIST, even if no pidptr
> > was passed.
>=20
> Yes, I think it should actually perform the same logic even if pidptr is
> NULL of waiting for the other daemon to finish starting up. Something li=
ke
> this:
>=20
> Index: lib/libutil/pidfile.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
> --- pidfile.c (revision 248162)
> +++ pidfile.c (working copy)
> @@ -100,6 +100,7 @@ pidfile_open(const char *path, mode_t mode, pid_t
> struct stat sb;
> int error, fd, len, count;
> struct timespec rqtp;
> + pid_t dummy;
> =20
> pfh =3D malloc(sizeof(*pfh));
> if (pfh =3D=3D NULL)
> @@ -126,7 +127,9 @@ pidfile_open(const char *path, mode_t mode, pid_t
> fd =3D flopen(pfh->pf_path,
> O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NONBLOCK, mode);
> if (fd =3D=3D -1) {
> - if (errno =3D=3D EWOULDBLOCK && pidptr !=3D NULL) {
> + if (errno =3D=3D EWOULDBLOCK) {
> + if (pidptr =3D=3D NULL)
> + pidptr =3D &dummy;
> count =3D 20;
> rqtp.tv_sec =3D 0;
> rqtp.tv_nsec =3D 5000000;
I agree EEXIST should be returned, but I don't like reading existing
pidfile (including waiting for the other process to write its PID) just
to throw read PID away.
How about this patch?
http://people.freebsd.org/~pjd/patches/pidfile.c.patch
--=20
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
--ncSAzJYg3Aa9+CRW
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iEYEARECAAYFAlFA71YACgkQForvXbEpPzRVuACeNxSf6DagXiOt8Dv5/qaT4vbN
/RwAmwaTqTGScNqfgQIk7yOcPuJoNRsp
=OQHV
-----END PGP SIGNATURE-----
--ncSAzJYg3Aa9+CRW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130313212750.GC1372>