From 3eaacea25e09b806437b29d7203d28dfe7a7c0b5 Mon Sep 17 00:00:00 2001
From: Ben Nemec <bnemec@us.ibm.com>
Date: Tue, 9 Jul 2013 17:03:53 -0500
Subject: [PATCH] Fix locking bug

When lock_path is not set, lockutils creates a new temp dir for
each new call to synchronized.  This results in no actual lock
enforcement.  Require setting of lock_path by throwing an
exception if it is not found.

Fixes bug 1065531
Fixes bug 1162047

Change-Id: I178684a1d8649b5bcfcb768be0a68c8efa3f00e4
---
 openstack/common/lockutils.py | 21 ++++-----------------
 tests/unit/test_lockutils.py  | 10 ++++++++++
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/openstack/common/lockutils.py b/openstack/common/lockutils.py
index 01f6c94..e876be1 100644
--- a/openstack/common/lockutils.py
+++ b/openstack/common/lockutils.py
@@ -20,8 +20,6 @@ import contextlib
 import errno
 import functools
 import os
-import shutil
-import tempfile
 import time
 import weakref
 
@@ -41,8 +39,7 @@ util_opts = [
     cfg.BoolOpt('disable_process_locking', default=False,
                 help='Whether to disable inter-process locks'),
     cfg.StrOpt('lock_path',
-               help=('Directory to use for lock files. Default to a '
-                     'temp directory'))
+               help=('Directory to use for lock files.'))
 ]
 
 
@@ -177,19 +174,15 @@ def lock(name, lock_file_prefix=None, external=False, lock_path=None):
             if external and not CONF.disable_process_locking:
                 LOG.debug(_('Attempting to grab file lock "%(lock)s"'),
                           {'lock': name})
-                cleanup_dir = False
 
                 # We need a copy of lock_path because it is non-local
-                local_lock_path = lock_path
+                local_lock_path = lock_path or CONF.lock_path
                 if not local_lock_path:
-                    local_lock_path = CONF.lock_path
-
-                if not local_lock_path:
-                    cleanup_dir = True
-                    local_lock_path = tempfile.mkdtemp()
+                    raise cfg.RequiredOptError('lock_path')
 
                 if not os.path.exists(local_lock_path):
                     fileutils.ensure_tree(local_lock_path)
+                    LOG.info(_('Created lock path: %s'), local_lock_path)
 
                 def add_prefix(name, prefix):
                     if not prefix:
@@ -213,12 +206,6 @@ def lock(name, lock_file_prefix=None, external=False, lock_path=None):
                 finally:
                     LOG.debug(_('Released file lock "%(lock)s" at %(path)s'),
                               {'lock': name, 'path': lock_file_path})
-                    # NOTE(vish): This removes the tempdir if we needed
-                    #             to create one. This is used to
-                    #             cleanup the locks left behind by unit
-                    #             tests.
-                    if cleanup_dir:
-                        shutil.rmtree(local_lock_path)
             else:
                 yield sem
 
diff --git a/tests/unit/test_lockutils.py b/tests/unit/test_lockutils.py
index 7cf016c..b5783b8 100644
--- a/tests/unit/test_lockutils.py
+++ b/tests/unit/test_lockutils.py
@@ -23,6 +23,7 @@ import eventlet
 from eventlet import greenpool
 from eventlet import greenthread
 from eventlet import semaphore
+from oslo.config import cfg
 
 from openstack.common import lockutils
 from tests import utils
@@ -296,3 +297,12 @@ class LockTestCase(utils.BaseTestCase):
         finally:
             if os.path.exists(lock_dir):
                 shutil.rmtree(lock_dir, ignore_errors=True)
+
+    def test_synchronized_externally_without_lock_path(self):
+        self.config(lock_path=None)
+
+        @lockutils.synchronized('external', 'test-', external=True)
+        def foo():
+            pass
+
+        self.assertRaises(cfg.RequiredOptError, foo)