[ovs-dev] [PATCH 3/7] daemon: Fix behavior of read_pidfile() for our own pidfile.

[Ben Pfaff]

On Wed, Sep 22, 2010 at 11:13:36PM -0700, Justin Pettit wrote:
> On Sep 22, 2010, at 4:45 PM, Ben Pfaff wrote:
> 
> > Opening a file descriptor and then closing it always discards any locks
> > held on the underlying file, even if the file is still open as another file
> > descriptor.  This meant that calling read_pidfile() on the process's own
> > pidfile would discard the lock and make other OVS processes think that the
> > process had died.  This commit fixes the problem.
> 
> Nice catch!

Thanks.  (I noticed this when ovs-vswitchd couldn't read its own
pidfile.)

> > +    if ((pidfile_ino || pidfile_dev)
> 
> I'm guessing it's pretty unlikely that both the inode and device are 0.  :-)

Yes, on Linux for example the dev will always be nonzero.

> Is this something we need to worry about in the equivalent Python functions?

The same problem can occur, even though we don't do anything to manifest
it there, so we might as fix it there too.  Here's the version with that
fix:

--8<--------------------------cut here-------------------------->8--

>From e1fa8f392e1094aa20f3ac93c4eaccb6fed47d13 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 23 Sep 2010 09:39:47 -0700
Subject: [PATCH] daemon: Fix behavior of read_pidfile() for our own pidfile.

Opening a file descriptor and then closing it always discards any locks
held on the underlying file, even if the file is still open as another file
descriptor.  This meant that calling read_pidfile() on the process's own
pidfile would discard the lock and make other OVS processes think that the
process had died.  This commit fixes the problem.
---
 lib/daemon.c         |   25 +++++++++++++++++++++++++
 python/ovs/daemon.py |   23 ++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/lib/daemon.c b/lib/daemon.c
index 6b61879..bbcfe6a 100644
--- a/lib/daemon.c
+++ b/lib/daemon.c
@@ -42,6 +42,10 @@ static bool detach;
 /* --pidfile: Name of pidfile (null if none). */
 static char *pidfile;
 
+/* Device and inode of pidfile, so we can avoid reopening it. */
+static dev_t pidfile_dev;
+static ino_t pidfile_ino;
+
 /* --overwrite-pidfile: Create pidfile even if one already exists and is
    locked? */
 static bool overwrite_pidfile;
@@ -208,6 +212,15 @@ make_pidfile(void)
                         close(fd);
                     } else {
                         /* Keep 'fd' open to retain the lock. */
+                        struct stat s;
+
+                        if (!fstat(fd, &s)) {
+                            pidfile_dev = s.st_dev;
+                            pidfile_ino = s.st_ino;
+                        } else {
+                            VLOG_ERR("%s: fstat failed: %s",
+                                     pidfile, strerror(errno));
+                        }
                     }
                     free(text);
                 } else {
@@ -494,9 +507,21 @@ read_pidfile(const char *pidfile)
 {
     char line[128];
     struct flock lck;
+    struct stat s;
     FILE *file;
     int error;
 
+    if ((pidfile_ino || pidfile_dev)
+        && !stat(pidfile, &s)
+        && s.st_ino == pidfile_ino && s.st_dev == pidfile_dev) {
+        /* It's our own pidfile.  We can't afford to open it, because closing
+         * *any* fd for a file that a process has locked also releases all the
+         * locks on that file.
+         *
+         * Fortunately, we know the associated pid anyhow: */
+        return getpid();
+    }
+
     file = fopen(pidfile, "r");
     if (!file) {
         error = errno;
diff --git a/python/ovs/daemon.py b/python/ovs/daemon.py
index a8373cf..eaaaa51 100644
--- a/python/ovs/daemon.py
+++ b/python/ovs/daemon.py
@@ -35,6 +35,10 @@ _detach = False
 # --pidfile: Name of pidfile (null if none).
 _pidfile = None
 
+# Our pidfile's inode and device, if we have created one.
+_pidfile_dev = None
+_pidfile_ino = None
+
 # --overwrite-pidfile: Create pidfile even if one already exists and is locked?
 _overwrite_pidfile = False
 
@@ -163,7 +167,7 @@ def _make_pidfile():
             logging.error("%s: create failed: %s"
                           % (tmpfile, os.strerror(e.errno)))
             return
-            
+
         try:
             fcntl.lockf(file, fcntl.LOCK_EX | fcntl.LOCK_NB)
         except IOError, e:
@@ -191,6 +195,10 @@ def _make_pidfile():
             file.close()
             return
 
+        s = os.fstat(file.fileno())
+        _pidfile_dev = s.st_dev
+        _pidfile_ino = s.st_ino
+
 def daemonize():
     """If configured with set_pidfile() or set_detach(), creates the pid file
     and detaches from the foreground session."""
@@ -368,6 +376,19 @@ Daemon options:
 def read_pidfile(pidfile):
     """Opens and reads a PID from 'pidfile'.  Returns the nonnegative PID if
     successful, otherwise a negative errno value."""
+    if _pidfile_dev is not None:
+        try:
+            s = os.stat(pidfile)
+            if s.st_ino == _pidfile_ino and s.st_dev == _pidfile_dev:
+                # It's our own pidfile.  We can't afford to open it,
+                # because closing *any* fd for a file that a process
+                # has locked also releases all the locks on that file.
+                #
+                # Fortunately, we know the associated pid anyhow.
+                return os.getpid()
+        except OSError:
+            pass
+
     try:
         file = open(pidfile, "r")
     except IOError, e:
-- 
1.7.1