Re: pidfile_lock(3)

[Roy Marples <roy%marples.name@localhost>]

Updated patch with man page changes, including an example of it's use.
Added pidfile_remove() which fixes the existing BUGS section.
Added pidfile_close() so a forked process can close it safely.
Made pidfile_read() visible so it's easy to obtain the PID to send a signal to.

Rpy

Index: include/util.h
===================================================================
RCS file: /cvsroot/src/include/util.h,v
retrieving revision 1.68
diff -u -p -r1.68 util.h
--- include/util.h	24 Sep 2015 14:39:37 -0000	1.68
+++ include/util.h	20 Mar 2016 13:59:33 -0000
@@ -103,6 +103,10 @@ time_t		parsedate(const char *, const ti
     __RENAME(__parsedate50);
 #endif
 int		pidfile(const char *);
+int		pidfile_close(void);
+pid_t		pidfile_lock(const char *);
+pid_t		pidfile_read(const char *);
+int		pidfile_remove(void);
 int		pidlock(const char *, int, pid_t *, const char *);
 int		pw_abort(void);
 #ifndef __LIBC12_SOURCE__
Index: lib/libutil/pidfile.3
===================================================================
RCS file: /cvsroot/src/lib/libutil/pidfile.3,v
retrieving revision 1.13
diff -u -p -r1.13 pidfile.3
--- lib/libutil/pidfile.3	29 Mar 2011 13:55:37 -0000	1.13
+++ lib/libutil/pidfile.3	20 Mar 2016 13:59:33 -0000
@@ -27,7 +27,7 @@
 .\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 .\" POSSIBILITY OF SUCH DAMAGE.
 .\"
-.Dd March 23, 2011
+.Dd March 20, 2016
 .Dt PIDFILE 3
 .Os
 .Sh NAME
@@ -39,9 +39,20 @@
 .In util.h
 .Ft int
 .Fn pidfile "const char *path"
+.Ft pid_t
+.Fn pidfile_lock "const char *path"
+.Ft pid_t
+.Fn pidfile_read "const char *path"
+.Ft int
+.Fn pidfile_close "void"
+.Ft int
+.Fn pidfile_remove "void"
 .Sh DESCRIPTION
 .Fn pidfile
-creates a file containing the process ID of the caller program.
+and
+.Fn pidfile_lock
+create a file containing the process ID of the caller program.
+The pid file is opened with O_EXLOCK to ensure the process owns it.
 The pid file can be used as a quick reference if
 the process needs to be sent a signal.
 When the program exits, the pid file is removed automatically, unless
@@ -72,21 +83,158 @@ is an absolute or relative path (i.e. it
 character),
 the pid file is created in the provided location.
 .Pp
-Note that only the first invocation of
-.Fn pidfile
-causes a pid file to be written; subsequent invocations have no effect
-unless a new
-.Ar path
-is supplied.
 If called with a new
 .Ar path ,
 .Fn pidfile
 will remove the old pid file and write the new one.
+.Pp
+.Fn pidfile_read
+will read the last pid file created, or specified by
+.Ar path ,
+and return the process ID it contains.
+.Pp
+.Fn pidfile_remove
+will close and remove the current pid file.
+.Pp
+.Fn pidfile_close
+will close the current pid file.
+It should be called after the process
+.Fn fork Ns s
+to start a child process which does not
+.Fn exec
+or try to take ownership of the pid file itself.
 .Sh RETURN VALUES
+.Fn pidfile ,
+.Fn pidfile_close
+and
+.Fn pidfile_remove
+return 0 on success and -1 on failure.
+.Pp
+.Fn pidfile_lock
+returns 0 on success.
+Otherwise, the process ID who owns the lock is returned and if that
+cannot be derived then -1 is returned.
+.Pp
+.Fn pidfile_read
+returns the process ID if known, otherwise -1.
+.Sh EXAMPLES
+The following example shows in which order these functions should be used.
+.Bd -literal
+pid_t otherpid, childpid;
+
+if ((pid = pidfile_lock(NULL)) != 0) {
+	if (pid != -1)
+		errx(EXIT_FAILURE, "already running on pid %jd", (intmax_t)pid);
+	else
+		warn("pidfile_lock");
+}
+
+if (daemon(0, 0) == -1)
+	err("daemon");
+
+if (pid != -1 && (pid = pidfile_lock(NULL)) != 0)
+	syslog(LOG_ERR, "pidfile_lock %jd:%m", (intmax_t)pid);
+
+for (;;) {
+	/* Do work. */
+	childpid = fork();
+	switch (childpid) {
+	case -1:
+		syslog(LOG_ERR, "fork: %m");
+		break;
+	case 0:
+		pidfile_close(NULL);
+		/* Do child work. */
+		break;
+	default:
+		syslog(LOG_INFO, "Child %jd started.", (intmax_t)childpid);
+		break;
+	}
+}
+
+exit(EXIT_SUCCESS);
+.Ed
+.Sh ERRORS
+The
+.Fn pidfile
+and
+.Fn pidfile_lock
+functions will fail if:
+.Bl -tag -width Er
+.It Bq Er EEXIST
+Some process already holds the lock on the given pid file, meaning that a
+daemon is already running.
+.It Bq Er ENAMETOOLONG
+Specified pidfile's name is too long.
+.El
+.Pp
+The
 .Fn pidfile
-returns 0 on success and -1 on failure.
+and
+.Fn pidfile_lock
+functions may also fail and set
+.Va errno
+for any errors specified for the
+.Xr open 2 ,
+.Xr dprintf 3 ,
+.Xr lseek 2 ,
+.Xr ftruncate 2 ,
+.Xr pidfile_read 3
+and
+.Xr strdup 3
+calls.
+.Pp
+The
+.Fn pidfile_read
+function may also fail if:
+.Bl -tag -width Er
+.It Bq Er EINVAL
+Some process already holds the lock on the given pid file, but the process ID
+read from there is invalid.
+.El
+.Pp
+The
+.Fn pidfile_close
+function may also fail if:
+.Bl -tag -width Er
+.It Bq Er EINVAL
+A sucessful prior call to
+.Fn pidfile
+or
+.Fn pidfile_lock
+has not been made.
+.El
+.Pp
+The
+.Fn pidfile_close
+function may also fail and set
+.Va errno
+for any errors specified for the
+.Xr close 2
+call.
+.Pp
+The
+.Fn pidfile_remove
+function may also fail if:
+.Bl -tag -width Er
+.It Bq Er ESRCH
+The current process ID does not match the process ID last stored.
+.El
+.Pp
+The
+.Fn pidfile_remove
+function may also fail and set
+.Va errno
+for any errors specified for the
+.Xr pidfile_close 3
+and
+.Xr unlink 2
+calls.
 .Sh SEE ALSO
-.Xr atexit 3
+.Xr atexit 3 ,
+.Xr exec 3 ,
+.Xr flock 2 ,
+.Xr fork 2
 .Sh HISTORY
 The
 .Fn pidfile
@@ -94,12 +242,24 @@ function call appeared in
 .Nx 1.5 .
 Support for creating pid files in any arbitrary path was added in
 .Nx 6.0 .
-.Sh BUGS
-.Fn pidfile
-uses
+.Pp
+The
+.Fn pidfile_lock ,
+.Fn pidfile_read
+and
+.Fn pidfile_close
+function calls appeared in
+.Nx 8.0 .
+.Sh CAVEATS
+.Fn pidfile
+and
+.Fn pidfile_lock
+use
 .Xr atexit 3
 to ensure the pid file is unlinked at program exit.
 However, programs that use the
 .Xr _exit 2
 function (for example, in signal handlers)
-will not trigger this behaviour.
+will not trigger this behaviour and will need to call
+.Fn pidfile_remove
+to ensure the pid file is removed.
Index: lib/libutil/pidfile.c
===================================================================
RCS file: /cvsroot/src/lib/libutil/pidfile.c,v
retrieving revision 1.11
diff -u -p -r1.11 pidfile.c
--- lib/libutil/pidfile.c	22 Jan 2015 19:04:28 -0000	1.11
+++ lib/libutil/pidfile.c	20 Mar 2016 13:59:33 -0000
@@ -35,7 +35,11 @@ __RCSID("$NetBSD: pidfile.c,v 1.11 2015/
 #endif
 
 #include <sys/param.h>
+#include <sys/proc.h>
 
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
 #include <paths.h>
 #include <stdbool.h>
 #include <stdlib.h>
@@ -46,14 +50,62 @@ __RCSID("$NetBSD: pidfile.c,v 1.11 2015/
 
 static pid_t pidfile_pid;
 static char *pidfile_path;
+static int   pidfile_fd = -1;
+
+/* Close and optionally remove an existent pidfile,
+ * if it was created by this process.
+ *
+ * Returns 0 on success, otherwise -1. */
+static int
+pidfile_free(bool pidfile_unlink)
+{
+	int r;
+
+	if (pidfile_path == NULL) {
+		r = -1;
+		errno = EINVAL;
+	} else if (pidfile_unlink && pidfile_pid != getpid()) {
+		r = -1;
+		errno = ESRCH;
+	} else {
+		r = 0;
+		if (close(pidfile_fd) == -1)
+			r = -1;
+		if (pidfile_unlink && unlink(pidfile_path) == -1)
+			r = -1;
+	}
+
+	pidfile_pid = 0;
+	free(pidfile_path);
+	pidfile_path = NULL;
+	pidfile_fd = -1;
+	return r;
+}
+
+/* Closes the previously opened pidfile.
+ *
+ * Returns 0 on success, otherwise -1. */
+int
+pidfile_close(void)
+{
+
+	return pidfile_free(false);
+}
+
+/* Closes and removes the previously opened pidfile.
+ *
+ * Returns 0 on success, otherwise -1. */
+int pidfile_remove(void)
+{
+
+	return pidfile_free(true);
+}
 
-/* Deletes an existent pidfile iff it was created by this process. */
 static void
 pidfile_cleanup(void)
 {
 
-	if ((pidfile_path != NULL) && (pidfile_pid == getpid()))
-		(void) unlink(pidfile_path);
+	(void) pidfile_remove();
 }
 
 /* Registers an atexit(3) handler to delete the pidfile we have generated.
@@ -62,12 +114,12 @@ pidfile_cleanup(void)
  *
  * Returns 0 on success or -1 if the handler could not be registered. */
 static int
-register_atexit_handler(void)
+pidfile_atexit(void)
 {
 	static bool done = false;
 
 	if (!done) {
-		if (atexit(pidfile_cleanup) < 0)
+		if (atexit(pidfile_cleanup) == -1)
 			return -1;
 		done = true;
 	}
@@ -75,29 +127,6 @@ register_atexit_handler(void)
 	return 0;
 }
 
-/* Given a new pidfile name in 'path', deletes any previously-created pidfile
- * if the previous file differs to the new one.
- *
- * If a previous file is deleted, returns 1, which means that a new pidfile
- * must be created.  Otherwise, this returns 0, which means that the existing
- * file does not need to be touched. */
-static int
-cleanup_old_pidfile(const char* path)
-{
-	if (pidfile_path != NULL) {
-		if (strcmp(pidfile_path, path) != 0) {
-			pidfile_cleanup();
-
-			free(pidfile_path);
-			pidfile_path = NULL;
-
-			return 1;
-		} else
-			return 0;
-	} else
-		return 1;
-}
-
 /* Constructs a name for a pidfile in the default location (/var/run).  If
  * 'bname' is NULL, uses the name of the current program for the name of
  * the pidfile.
@@ -105,7 +134,7 @@ cleanup_old_pidfile(const char* path)
  * Returns a pointer to a dynamically-allocatd string containing the absolute
  * path to the pidfile; NULL on failure. */
 static char *
-generate_varrun_path(const char *bname)
+pidfile_varrun_path(const char *bname)
 {
 	char *path;
 
@@ -118,58 +147,127 @@ generate_varrun_path(const char *bname)
 	return path;
 }
 
-/* Creates a pidfile with the provided name.  The new pidfile is "registered"
- * in the global variables pidfile_path and pidfile_pid so that any further
- * call to pidfile(3) can check if we are recreating the same file or a new
- * one.
- *
- * Returns 0 on success or -1 if there is any error. */
-static int
-create_pidfile(const char* path)
-{
-	FILE *f;
+/* Returns the pid inside path on success, otherwise -1.
+ * If no path is give, use the last pidfile path, othewise the default one. */
+pid_t
+pidfile_read(const char *path)
+{
+	char *default_path, buf[16], *eptr;
+	int fd, error, n, e;
+	pid_t pid;
+
+	default_path = NULL;
+	if (path == NULL) {
+		if (pidfile_path != NULL)
+			path = pidfile_path;
+		else {
+			if ((default_path = pidfile_varrun_path(NULL)) == NULL)
+				return -1;
+			path = default_path;
+		}
+	}
 
-	if (register_atexit_handler() == -1)
+	fd = open(path, O_RDONLY);
+	free(default_path);
+	if (fd == -1)
+		return  -1;
+
+	n = read(fd, buf, sizeof(buf) - 1);
+	error = errno;
+	(void) close(fd);
+	if (n == -1) {
+		errno = error;
+		return -1;
+	}
+	buf[n] = '\0';
+	pid = (pid_t)strtoi(buf, &eptr, 10, 1, INT_MAX, &e);
+	if (e && !(e == ENOTSUP && *eptr == '\n')) {
+		errno = e;
 		return -1;
+	}
+	return pid;
+}
 
-	if (cleanup_old_pidfile(path) == 0)
-		return 0;
+/* Locks the pidfile specified by path and writes the process pid to it.
+ *
+ * Returns 0 on success, otherwise the pid of the process who owns the
+ * lock if it can be read, otherwise -1. */
+pid_t
+pidfile_lock(const char *path)
+{
+	char *default_path;
 
-	pidfile_path = strdup(path);
-	if (pidfile_path == NULL)
+	if (pidfile_atexit() == -1)
 		return -1;
 
-	if ((f = fopen(path, "w")) == NULL) {
-		free(pidfile_path);
-		pidfile_path = NULL;
-		return -1;
+	if (path == NULL || strchr(path, '/') == NULL) {
+		if ((default_path = pidfile_varrun_path(path)) == NULL)
+			return -1;
+		path = default_path;
+	} else
+		default_path = NULL;
+
+	/* If path has changed (no good reason), clean up the old pidfile. */
+	if (pidfile_path != NULL && strcmp(pidfile_path, path) != 0)
+		(void) pidfile_remove();
+
+	if (pidfile_fd == -1) {
+		pid_t pid = -1;
+
+		pidfile_fd = open(path,
+		    O_WRONLY | O_CREAT | O_CLOEXEC | O_NONBLOCK | O_EXLOCK,
+		    0666);
+		if (pidfile_fd == -1) {
+			if (errno == EAGAIN) {
+				pid = pidfile_read(path);
+				if (errno == EAGAIN)
+					errno = EEXIST;
+			}
+		} else	if ((pidfile_path = strdup(path)) == NULL) {
+			int error = errno;
+
+			(void) close(pidfile_fd);
+			(void) unlink(path);
+			pidfile_fd = -1;
+			errno = error;
+		}
+		if (pidfile_fd == -1) {
+			free(default_path);
+			return pid;
+		}
 	}
 
 	pidfile_pid = getpid();
+	free(default_path);
+
+	/* Truncate the file, as we could be re-writing it after forking.
+	 * Then write the pidfile. */
+	if (ftruncate(pidfile_fd, 0) == -1 ||
+	    lseek(pidfile_fd, 0, SEEK_SET) == -1 ||
+	    dprintf(pidfile_fd, "%jd\n", (intmax_t)pidfile_pid) == -1)
+	{
+		int error = errno;
 
-	(void) fprintf(f, "%d\n", pidfile_pid);
-	(void) fclose(f);
+		(void) pidfile_remove();
+		errno = error;
+		return -1;
+	}
 
+	/* Hold the fd open to persist the lock. */
 	return 0;
 }
 
+/* The old function.
+ * Historical behaviour is that pidfile is not re-written
+ * if path has not changed.
+ *
+ * Returns 0 on success, otherwise -1.
+ * As such we have no way of knowing the pid who owns the lock. */
 int
 pidfile(const char *path)
 {
+	pid_t pid;
 
-	if (path == NULL || strchr(path, '/') == NULL) {
-		char *default_path;
-
-		if ((default_path = generate_varrun_path(path)) == NULL)
-			return -1;
-
-		if (create_pidfile(default_path) == -1) {
-			free(default_path);
-			return -1;
-		}
-
-		free(default_path);
-		return 0;
-	} else
-		return create_pidfile(path);
+	pid = pidfile_lock(path);
+	return pid == 0 ? 0 : -1;
 }