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
[-- Attachment #1 --]
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
> >
> > fd = flopen(pfh->pf_path,
> > O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NONBLOCK, mode);
> >
> > fails when another daemon holds the lock and flopen sets errno to
> > EAGAIN, the check 4 lines below in
> >
> > if (errno == EWOULDBLOCK && pidptr != NULL) {
> >
> > means that the pidfile_read is never executed.
> >
> > 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.
> >
> > The expected behavior would be to set errno to EEXIST, even if no pidptr
> > was passed.
>
> 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 like
> this:
>
> Index: lib/libutil/pidfile.c
> ===================================================================
> --- 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;
>
> pfh = malloc(sizeof(*pfh));
> if (pfh == NULL)
> @@ -126,7 +127,9 @@ pidfile_open(const char *path, mode_t mode, pid_t
> fd = flopen(pfh->pf_path,
> O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NONBLOCK, mode);
> if (fd == -1) {
> - if (errno == EWOULDBLOCK && pidptr != NULL) {
> + if (errno == EWOULDBLOCK) {
> + if (pidptr == NULL)
> + pidptr = &dummy;
> count = 20;
> rqtp.tv_sec = 0;
> rqtp.tv_nsec = 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
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iEYEARECAAYFAlFA71YACgkQForvXbEpPzRVuACeNxSf6DagXiOt8Dv5/qaT4vbN
/RwAmwaTqTGScNqfgQIk7yOcPuJoNRsp
=OQHV
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130313212750.GC1372>