CA-323702: fcntl locks are per process, so need to reference count.

As fcntl locks are per-process it is not an error if a lock is acquired
that is already held, however the release is not reference counted by
the system. So, if a lock is taken by an API and subsequently another
method in the call chain takes the same lock it must not be released
until the API call completes.

To ensure this behaviour make Lock objects unique by name and namespace
so that subsequent calls obtain the same object reference. internally
to the object track how many calls to acquire and release have been made
and only release the underlying lock when all the acquires have had matching
release calls.

Signed-off-by: Mark Syms <mark.syms@citrix.com>

drivers/lock.py

@@ -25,56 +25,39 @@
 class LockException(util.SMException):
     pass
 
-class Lock:
+class Lock(object):
     """Simple file-based lock on a local FS. With shared reader/writer
     attributes."""
 
     BASE_DIR = "/var/lock/sm"
 
-    def _open(self):
-        """Create and open the lockable attribute base, if it doesn't exist.
-        (But don't lock it yet.)"""
-
-        # one directory per namespace
-        self.nspath = os.path.join(Lock.BASE_DIR, self.ns)
-
-        # the lockfile inside that namespace directory per namespace
-        self.lockpath = os.path.join(self.nspath, self.name)
+    INSTANCES = {}
+    BASE_INSTANCES = {}
 
-        number_of_enoent_retries = 10
+    def __new__(cls, name, ns=None, *args, **kwargs):
+        if ns:
+            if ns not in Lock.INSTANCES:
+                Lock.INSTANCES[ns] = {}
+            instances = Lock.INSTANCES[ns]
+        else:
+            instances= Lock.BASE_INSTANCES
 
-        while True:
-            self._mkdirs(self.nspath)
+        if name not in instances:
+            instances[name] = LockImplementation(name, ns)
+        return instances[name]
 
-            try:
-                self._open_lockfile()
-            except IOError, e:
-                # If another lock within the namespace has already
-                # cleaned up the namespace by removing the directory,
-                # _open_lockfile raises an ENOENT, in this case we retry.
-                if e.errno == errno.ENOENT:
-                    if number_of_enoent_retries > 0:
-                        number_of_enoent_retries -= 1
-                        continue
-                raise
-            break
+    # These are required to pacify pylint as it doesn't understand the __new__
+    def acquire(self):
+        raise NotImplementedError("Lock methods implemented in LockImplementation")
 
-        fd = self.lockfile.fileno()
-        self.lock = flock.WriteLock(fd)
+    def acquireNoblock(self):
+        raise NotImplementedError("Lock methods implemented in LockImplementation")
 
-    def _open_lockfile(self):
-        """Provide a seam, so extreme situations could be tested"""
-        util.SMlog("lock: opening lock file %s" % self.lockpath)
-        self.lockfile = file(self.lockpath, "w+")
+    def release(self):
+        raise NotImplementedError("Lock methods implemented in LockImplementation")
 
-    def _close(self):
-        """Close the lock, which implies releasing the lock."""
-        if self.lockfile is not None:
-            if self.held():
-                self.release()
-            self.lockfile.close()
-            util.SMlog("lock: closed %s" % self.lockpath)
-            self.lockfile = None
+    def held(self):
+        raise NotImplementedError("Lock methods implemented in LockImplementation")
 
     def _mknamespace(ns):
 
@@ -86,20 +69,16 @@ def _mknamespace(ns):
         return ns
     _mknamespace = staticmethod(_mknamespace)
 
-    def __init__(self, name, ns=None):
-        self.lockfile = None
-
-        self.ns = Lock._mknamespace(ns)
-
-        assert not name.startswith(".")
-        assert name.find(os.path.sep) < 0
-        self.name = name
-
-        self._open()
-
-    __del__ = _close
-
     def cleanup(name, ns = None):
+        if ns:
+            if ns in Lock.INSTANCES:
+                if name in Lock.INSTANCES[ns]:
+                    del Lock.INSTANCES[ns][name]
+                if len(Lock.INSTANCES[ns]) == 0:
+                    del Lock.INSTANCES[ns]
+        elif name in Lock.BASE_INSTANCES:
+            del Lock.BASE_INSTANCES[name]
+
         ns = Lock._mknamespace(ns)
         path = os.path.join(Lock.BASE_DIR, ns, name)
         if os.path.exists(path):
@@ -155,6 +134,77 @@ def _rmdir(path):
             util.SMlog("Failed to rmdir(%s): %s" % (path, e))
     _rmdir = staticmethod(_rmdir)
 
+
+class LockImplementation(object):
+
+    def __init__(self, name, ns=None):
+        self.lockfile = None
+
+        self.ns = Lock._mknamespace(ns)
+
+        assert not name.startswith(".")
+        assert name.find(os.path.sep) < 0
+        self.name = name
+
+        self.count = 0
+
+        self._open()
+
+    def _open(self):
+        """Create and open the lockable attribute base, if it doesn't exist.
+        (But don't lock it yet.)"""
+
+        # one directory per namespace
+        self.nspath = os.path.join(Lock.BASE_DIR, self.ns)
+
+        # the lockfile inside that namespace directory per namespace
+        self.lockpath = os.path.join(self.nspath, self.name)
+
+        number_of_enoent_retries = 10
+
+        while True:
+            Lock._mkdirs(self.nspath)
+
+            try:
+                self._open_lockfile()
+            except IOError, e:
+                # If another lock within the namespace has already
+                # cleaned up the namespace by removing the directory,
+                # _open_lockfile raises an ENOENT, in this case we retry.
+                if e.errno == errno.ENOENT:
+                    if number_of_enoent_retries > 0:
+                        number_of_enoent_retries -= 1
+                        continue
+                raise
+            break
+
+        fd = self.lockfile.fileno()
+        self.lock = flock.WriteLock(fd)
+
+    def _open_lockfile(self):
+        """Provide a seam, so extreme situations could be tested"""
+        util.SMlog("lock: opening lock file %s" % self.lockpath)
+        self.lockfile = file(self.lockpath, "w+")
+
+    def _close(self):
+        """Close the lock, which implies releasing the lock."""
+        if self.lockfile is not None:
+            if self.held():
+                # drop all reference counts
+                self.count = 0
+                self.release()
+            self.lockfile.close()
+            util.SMlog("lock: closed %s" % self.lockpath)
+            self.lockfile = None
+
+    __del__ = _close
+
+    def cleanup(self, name, ns = None):
+        Lock.cleanup(name, ns)
+
+    def cleanupAll(self, ns = None):
+        Lock.cleanupAll(ns)
+
     #
     # Actual Locking
     #
@@ -163,20 +213,29 @@ def acquire(self):
         """Blocking lock aquisition, with warnings. We don't expect to lock a
         lot. If so, not to collide. Coarse log statements should be ok
         and aid debugging."""
-        if not self.lock.trylock():
-            util.SMlog("Failed to lock %s on first attempt, " % self.lockpath
-                   + "blocked by PID %d" % self.lock.test())
-            self.lock.lock()
-        if VERBOSE:
-            util.SMlog("lock: acquired %s" % self.lockpath)
+        if not self.held():
+            if not self.lock.trylock():
+                util.SMlog("Failed to lock %s on first attempt, " % self.lockpath
+                       + "blocked by PID %d" % self.lock.test())
+                self.lock.lock()
+            if VERBOSE:
+                util.SMlog("lock: acquired %s" % self.lockpath)
+        self.count += 1
 
     def acquireNoblock(self):
         """Acquire lock if possible, or return false if lock already held"""
-        exists = os.path.exists(self.lockpath)
-        ret = self.lock.trylock()
-        if VERBOSE:
-            util.SMlog("lock: tried lock %s, acquired: %s (exists: %s)" % \
-                    (self.lockpath, ret, exists))
+        if not self.held():
+            exists = os.path.exists(self.lockpath)
+            ret = self.lock.trylock()
+            if VERBOSE:
+                util.SMlog("lock: tried lock %s, acquired: %s (exists: %s)" % \
+                        (self.lockpath, ret, exists))
+        else:
+            ret = True
+
+        if ret:
+            self.count += 1
+
         return ret
 
     def held(self):
@@ -185,6 +244,12 @@ def held(self):
 
     def release(self):
         """Release a previously acquired lock."""
+        if self.count >= 1:
+            self.count -= 1
+
+        if self.count > 0:
+            return
+
         self.lock.unlock()
         if VERBOSE:
             util.SMlog("lock: released %s" % self.lockpath)