Remove extra lstat() calls from check_mount

The os.path.exists call performs an lstat, but os.path.ismount already
performs the same check. However, it performs a separate lstat() call
to check for a symlink, which we remove as well, cutting the number
performed in half.

Sample program to be straced for comparison:

from swift.common.constraints import check_mount
import os
os.write(1, "Starting\n")
if check_mount("/", "tmp"):
    os.write(1, "Mounted\n")

Here is the output of a check on a mounted file system (common case)
using the new method:

---- strace new ----
write(1, "Starting\n", 9) = 9
lstat("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=8460, ...}) = 0
lstat("/tmp/..", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
write(1, "Mounted\n", 8) = 8

---- strace old ----
write(1, "Starting\n", 9) = 9
stat("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=8460, ...}) = 0
lstat("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=8460, ...}) = 0
lstat("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=8460, ...}) = 0
lstat("/tmp/..", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
write(1, "Mounted\n", 8) = 8

Change-Id: I027c862a2b7d9ff99d7f61bd43ccc0825dba525c
Signed-off-by: Peter Portante <peter.portante@redhat.com>

swift/common/constraints.py

@@ -17,6 +17,7 @@ import os
 import urllib
 from ConfigParser import ConfigParser, NoSectionError, NoOptionError
 
+from swift.common.utils import ismount
 from swift.common.swob import HTTPBadRequest, HTTPLengthRequired, \
     HTTPRequestEntityTooLarge
 
@@ -169,7 +170,7 @@ def check_mount(root, drive):
     if not (urllib.quote_plus(drive) == drive):
         return False
     path = os.path.join(root, drive)
-    return os.path.exists(path) and os.path.ismount(path)
+    return ismount(path)
 
 
 def check_float(string):

swift/common/utils.py

@@ -45,6 +45,7 @@ import cPickle as pickle
 import glob
 from urlparse import urlparse as stdlib_urlparse, ParseResult
 import itertools
+import stat
 
 import eventlet
 import eventlet.semaphore
@@ -2174,3 +2175,38 @@ class ThreadPool(object):
             return self._run_in_eventlet_tpool(func, *args, **kwargs)
         else:
             return self.run_in_thread(func, *args, **kwargs)
+
+
+def ismount(path):
+    """
+    Test whether a path is a mount point.
+
+    This is code hijacked from C Python 2.6.8, adapted to remove the extra
+    lstat() system call.
+    """
+    try:
+        s1 = os.lstat(path)
+    except os.error as err:
+        if err.errno == errno.ENOENT:
+            # It doesn't exist -- so not a mount point :-)
+            return False
+        raise
+
+    if stat.S_ISLNK(s1.st_mode):
+        # A symlink can never be a mount point
+        return False
+
+    s2 = os.lstat(os.path.join(path, '..'))
+    dev1 = s1.st_dev
+    dev2 = s2.st_dev
+    if dev1 != dev2:
+        # path/.. on a different device as path
+        return True
+
+    ino1 = s1.st_ino
+    ino2 = s2.st_ino
+    if ino1 == ino2:
+        # path/.. is the same i-node as path
+        return True
+
+    return False

test/unit/common/middleware/test_recon.py

@@ -18,6 +18,7 @@ from unittest import TestCase
 from contextlib import contextmanager
 from posix import stat_result, statvfs_result
 import os
+import mock
 
 import swift.common.constraints
 from swift import __version__ as swiftver
@@ -30,9 +31,11 @@ class FakeApp(object):
     def __call__(self, env, start_response):
         return "FAKE APP"
 
+
 def start_response(*args):
     pass
 
+
 class FakeFromCache(object):
 
     def __init__(self, out=None):
@@ -43,6 +46,7 @@ class FakeFromCache(object):
         self.fakeout_calls.append((args, kwargs))
         return self.fakeout
 
+
 class OpenAndReadTester(object):
 
     def __init__(self, output_iter):
@@ -76,35 +80,29 @@ class OpenAndReadTester(object):
         self.open_calls.append((args, kwargs))
         yield self
 
+
 class MockOS(object):
 
-    def __init__(self, ls_out=None, pe_out=None, statvfs_out=None,
+    def __init__(self, ls_out=None, im_out=False, statvfs_out=None):
-                 lstat_out=(1, 1, 5, 4, 5, 5, 55, 55, 55, 55)):
         self.ls_output = ls_out
-        self.path_exists_output = pe_out
+        self.ismount_output = im_out
         self.statvfs_output = statvfs_out
-        self.lstat_output_tuple = lstat_out
         self.listdir_calls = []
         self.statvfs_calls = []
-        self.path_exists_calls = []
+        self.ismount_calls = []
-        self.lstat_calls = []
 
     def fake_listdir(self, *args, **kwargs):
         self.listdir_calls.append((args, kwargs))
         return self.ls_output
 
-    def fake_path_exists(self, *args, **kwargs):
+    def fake_ismount(self, *args, **kwargs):
-        self.path_exists_calls.append((args, kwargs))
+        self.ismount_calls.append((args, kwargs))
-        return self.path_exists_output
+        return self.ismount_output
 
     def fake_statvfs(self, *args, **kwargs):
         self.statvfs_calls.append((args, kwargs))
         return statvfs_result(self.statvfs_output)
 
-    def fake_lstat(self, *args, **kwargs):
-        self.lstat_calls.append((args, kwargs))
-        return stat_result(self.lstat_output_tuple)
-
 
 class FakeRecon(object):
 
@@ -175,6 +173,7 @@ class FakeRecon(object):
     def raise_Exception(self, *args, **kwargs):
         raise Exception
 
+
 class TestReconSuccess(TestCase):
 
     def setUp(self):
@@ -182,12 +181,10 @@ class TestReconSuccess(TestCase):
         self.mockos = MockOS()
         self.fakecache = FakeFromCache()
         self.real_listdir = os.listdir
-        self.real_path_exists = os.path.exists
+        self.real_ismount = swift.common.constraints.ismount
-        self.real_lstat = os.lstat
         self.real_statvfs = os.statvfs
         os.listdir = self.mockos.fake_listdir
-        os.path.exists = self.mockos.fake_path_exists
+        swift.common.constraints.ismount = self.mockos.fake_ismount
-        os.lstat = self.mockos.fake_lstat
         os.statvfs = self.mockos.fake_statvfs
         self.real_from_cache = self.app._from_recon_cache
         self.app._from_recon_cache = self.fakecache.fake_from_recon_cache
@@ -195,8 +192,7 @@ class TestReconSuccess(TestCase):
 
     def tearDown(self):
         os.listdir = self.real_listdir
-        os.path.exists = self.real_path_exists
+        swift.common.constraints.ismount = self.real_ismount
-        os.lstat = self.real_lstat
         os.statvfs = self.real_statvfs
         del self.mockos
         self.app._from_recon_cache = self.real_from_cache
@@ -389,7 +385,7 @@ class TestReconSuccess(TestCase):
                                     "no_change": 2, "remote_merge": 0,
                                     "remove": 0, "rsync": 0,
                                     "start": 1333044050.855202,
-                                    "success": 2, "ts_repl": 0 },
+                                    "success": 2, "ts_repl": 0},
                                "replication_time": 0.2615511417388916,
                                "replication_last": 1357969645.25}
         self.fakecache.fakeout = from_cache_response
@@ -405,7 +401,7 @@ class TestReconSuccess(TestCase):
                                     "no_change": 2, "remote_merge": 0,
                                     "remove": 0, "rsync": 0,
                                     "start": 1333044050.855202,
-                                    "success": 2, "ts_repl": 0 },
+                                    "success": 2, "ts_repl": 0},
                                 "replication_time": 0.2615511417388916,
                                 "replication_last": 1357969645.25})
 
@@ -516,14 +512,14 @@ class TestReconSuccess(TestCase):
                                     "completed": 115.4512460231781,
                                     "errors": 0,
                                     "files_processed": 2310,
-                                    "quarantined": 0 },
+                                    "quarantined": 0},
                                 "object_auditor_stats_ZBF": {
                                     "audit_time": 45.877294063568115,
                                     "bytes_processed": 0,
                                     "completed": 46.181446075439453,
                                     "errors": 0,
                                     "files_processed": 2310,
-                                    "quarantined": 0 }}
+                                    "quarantined": 0}}
         self.fakecache.fakeout_calls = []
         self.fakecache.fakeout = from_cache_response
         rv = self.app.get_auditor_info('object')
@@ -537,28 +533,22 @@ class TestReconSuccess(TestCase):
                                     "completed": 115.4512460231781,
                                     "errors": 0,
                                     "files_processed": 2310,
-                                    "quarantined": 0 },
+                                    "quarantined": 0},
                                 "object_auditor_stats_ZBF": {
                                     "audit_time": 45.877294063568115,
                                     "bytes_processed": 0,
                                     "completed": 46.181446075439453,
                                     "errors": 0,
                                     "files_processed": 2310,
-                                    "quarantined": 0 }})
+                                    "quarantined": 0}})
 
     def test_get_unmounted(self):
 
-        def fake_checkmount_true(*args):
-            return True
-
         unmounted_resp = [{'device': 'fakeone', 'mounted': False},
                           {'device': 'faketwo', 'mounted': False}]
-        self.mockos.ls_output=['fakeone', 'faketwo']
+        self.mockos.ls_output = ['fakeone', 'faketwo']
-        self.mockos.path_exists_output=False
+        self.mockos.ismount_output = False
-        real_checkmount = swift.common.constraints.check_mount
-        swift.common.constraints.check_mount = fake_checkmount_true
         rv = self.app.get_unmounted()
-        swift.common.constraints.check_mount = real_checkmount
         self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})])
         self.assertEquals(rv, unmounted_resp)
 
@@ -568,27 +558,25 @@ class TestReconSuccess(TestCase):
             return True
 
         unmounted_resp = []
-        self.mockos.ls_output=[]
+        self.mockos.ls_output = []
-        self.mockos.path_exists_output=False
+        self.mockos.ismount_output = False
-        real_checkmount = swift.common.constraints.check_mount
-        swift.common.constraints.check_mount = fake_checkmount_true
         rv = self.app.get_unmounted()
-        swift.common.constraints.check_mount = real_checkmount
         self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})])
         self.assertEquals(rv, unmounted_resp)
 
     def test_get_diskusage(self):
         #posix.statvfs_result(f_bsize=4096, f_frsize=4096, f_blocks=1963185,
-        #                     f_bfree=1113075, f_bavail=1013351, f_files=498736,
+        #                     f_bfree=1113075, f_bavail=1013351,
+        #                     f_files=498736,
         #                     f_ffree=397839, f_favail=397839, f_flag=0,
         #                     f_namemax=255)
-        statvfs_content=(4096, 4096, 1963185, 1113075, 1013351, 498736, 397839,
+        statvfs_content = (4096, 4096, 1963185, 1113075, 1013351, 498736,
-                         397839, 0, 255)
+                           397839, 397839, 0, 255)
         du_resp = [{'device': 'canhazdrive1', 'avail': 4150685696,
                     'mounted': True, 'used': 3890520064, 'size': 8041205760}]
-        self.mockos.ls_output=['canhazdrive1']
+        self.mockos.ls_output = ['canhazdrive1']
-        self.mockos.statvfs_output=statvfs_content
+        self.mockos.statvfs_output = statvfs_content
-        self.mockos.path_exists_output=True
+        self.mockos.ismount_output = True
         rv = self.app.get_diskusage()
         self.assertEquals(self.mockos.statvfs_calls,
                             [(('/srv/node/canhazdrive1',), {})])
@@ -597,22 +585,29 @@ class TestReconSuccess(TestCase):
     def test_get_diskusage_checkmount_fail(self):
         du_resp = [{'device': 'canhazdrive1', 'avail': '',
                     'mounted': False, 'used': '', 'size': ''}]
-        self.mockos.ls_output=['canhazdrive1']
+        self.mockos.ls_output = ['canhazdrive1']
-        self.mockos.path_exists_output=False
+        self.mockos.ismount_output = False
         rv = self.app.get_diskusage()
-        self.assertEquals(self.mockos.listdir_calls,[(('/srv/node/',), {})])
+        self.assertEquals(self.mockos.listdir_calls, [(('/srv/node/',), {})])
-        self.assertEquals(self.mockos.path_exists_calls,
+        self.assertEquals(self.mockos.ismount_calls,
                             [(('/srv/node/canhazdrive1',), {})])
         self.assertEquals(rv, du_resp)
 
     def test_get_quarantine_count(self):
+        self.mockos.ls_output = ['sda']
+        self.mockos.ismount_output = True
+
+        def fake_lstat(*args, **kwargs):
             #posix.lstat_result(st_mode=1, st_ino=2, st_dev=3, st_nlink=4,
             #                   st_uid=5, st_gid=6, st_size=7, st_atime=8,
             #                   st_mtime=9, st_ctime=10)
-        lstat_content = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)
+            return stat_result((1, 2, 3, 4, 5, 6, 7, 8, 9, 10))
-        self.mockos.ls_output=['sda']
+
-        self.mockos.path_exists_output=True
+        def fake_exists(*args, **kwargs):
-        self.mockos.lstat_output=lstat_content
+            return True
+
+        with mock.patch("os.lstat", fake_lstat):
+            with mock.patch("os.path.exists", fake_exists):
                 rv = self.app.get_quarantine_count()
         self.assertEquals(rv, {'objects': 2, 'accounts': 2, 'containers': 2})
 
@@ -627,6 +622,7 @@ class TestReconSuccess(TestCase):
         self.assertEquals(oart.open_calls, [(('/proc/net/sockstat', 'r'), {}),
                                             (('/proc/net/sockstat6', 'r'), {})])
 
+
 class TestReconMiddleware(unittest.TestCase):
 
     def setUp(self):

test/unit/common/test_constraints.py

@@ -14,6 +14,8 @@
 # limitations under the License.
 
 import unittest
+import mock
+
 from test.unit import MockTrue
 
 from swift.common.swob import HTTPBadRequest, Request
@@ -170,14 +172,13 @@ class TestConstraints(unittest.TestCase):
 
     def test_check_mount(self):
         self.assertFalse(constraints.check_mount('', ''))
-        constraints.os = MockTrue()  # mock os module
+        with mock.patch("swift.common.constraints.ismount", MockTrue()):
             self.assertTrue(constraints.check_mount('/srv', '1'))
             self.assertTrue(constraints.check_mount('/srv', 'foo-bar'))
             self.assertTrue(constraints.check_mount('/srv', '003ed03c-242a-4b2f-bee9-395f801d1699'))
             self.assertFalse(constraints.check_mount('/srv', 'foo bar'))
             self.assertFalse(constraints.check_mount('/srv', 'foo/bar'))
             self.assertFalse(constraints.check_mount('/srv', 'foo?bar'))
-        reload(constraints)  # put it back
 
     def test_check_float(self):
         self.assertFalse(constraints.check_float(''))
@@ -217,5 +218,6 @@ class TestConstraints(unittest.TestCase):
         self.assertTrue(c.MAX_HEADER_SIZE > c.MAX_META_NAME_LENGTH)
         self.assertTrue(c.MAX_HEADER_SIZE > c.MAX_META_VALUE_LENGTH)
 
+
 if __name__ == '__main__':
     unittest.main()

test/unit/common/test_db.py

@@ -149,10 +149,10 @@ class TestDatabaseBroker(unittest.TestCase):
             conn.execute('SELECT * FROM outgoing_sync')
             conn.execute('SELECT * FROM incoming_sync')
 
-        def my_exists(*a, **kw):
+        def my_ismount(*a, **kw):
             return True
 
-        with patch('os.path.exists', my_exists):
+        with patch('os.path.ismount', my_ismount):
             broker = DatabaseBroker(os.path.join(self.testdir, '1.db'))
             broker._initialize = stub
             self.assertRaises(swift.common.db.DatabaseAlreadyExists,

test/unit/common/test_utils.py

@@ -17,6 +17,7 @@
 
 from __future__ import with_statement
 from test.unit import temptree
+
 import ctypes
 import errno
 import eventlet
@@ -26,17 +27,21 @@ import random
 import re
 import socket
 import sys
+
 from textwrap import dedent
+
 import threading
 import time
 import unittest
 import fcntl
+import shutil
+
 from Queue import Queue, Empty
 from getpass import getuser
 from shutil import rmtree
 from StringIO import StringIO
 from functools import partial
-from tempfile import TemporaryFile, NamedTemporaryFile
+from tempfile import TemporaryFile, NamedTemporaryFile, mkdtemp
 
 from mock import MagicMock, patch
 
@@ -335,7 +340,7 @@ class TestUtils(unittest.TestCase):
         lfo.tell()
 
     def test_parse_options(self):
-        # use mkstemp to get a file that is definitely on disk
+        # Get a file that is definitely on disk
         with NamedTemporaryFile() as f:
             conf_file = f.name
             conf, options = utils.parse_options(test_args=[conf_file])
@@ -414,9 +419,11 @@ class TestUtils(unittest.TestCase):
     def test_get_logger_sysloghandler_plumbing(self):
         orig_sysloghandler = utils.SysLogHandler
         syslog_handler_args = []
+
         def syslog_handler_catcher(*args, **kwargs):
             syslog_handler_args.append((args, kwargs))
             return orig_sysloghandler(*args, **kwargs)
+
         syslog_handler_catcher.LOG_LOCAL0 = orig_sysloghandler.LOG_LOCAL0
         syslog_handler_catcher.LOG_LOCAL3 = orig_sysloghandler.LOG_LOCAL3
 
@@ -1375,6 +1382,133 @@ log_name = %(yarr)s'''
 
             self.assertRaises(OSError, os.remove, nt.name)
 
+    def test_ismount_path_does_not_exist(self):
+        tmpdir = mkdtemp()
+        try:
+            assert utils.ismount(os.path.join(tmpdir, 'bar')) is False
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def test_ismount_path_not_mount(self):
+        tmpdir = mkdtemp()
+        try:
+            assert utils.ismount(tmpdir) is False
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def test_ismount_path_error(self):
+
+        def _mock_os_lstat(path):
+            raise OSError(13, "foo")
+
+        tmpdir = mkdtemp()
+        try:
+            with patch("os.lstat", _mock_os_lstat):
+                try:
+                    utils.ismount(tmpdir)
+                except OSError:
+                    pass
+                else:
+                    self.fail("Expected OSError")
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def test_ismount_path_is_symlink(self):
+        tmpdir = mkdtemp()
+        try:
+            link = os.path.join(tmpdir, "tmp")
+            os.symlink("/tmp", link)
+            assert utils.ismount(link) is False
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def test_ismount_path_is_root(self):
+        assert utils.ismount('/') is True
+
+    def test_ismount_parent_path_error(self):
+
+        _os_lstat = os.lstat
+
+        def _mock_os_lstat(path):
+            if path.endswith(".."):
+                raise OSError(13, "foo")
+            else:
+                return _os_lstat(path)
+
+        tmpdir = mkdtemp()
+        try:
+            with patch("os.lstat", _mock_os_lstat):
+                try:
+                    utils.ismount(tmpdir)
+                except OSError:
+                    pass
+                else:
+                    self.fail("Expected OSError")
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def test_ismount_successes_dev(self):
+
+        _os_lstat = os.lstat
+
+        class MockStat(object):
+            def __init__(self, mode, dev, ino):
+                self.st_mode = mode
+                self.st_dev = dev
+                self.st_ino = ino
+
+        def _mock_os_lstat(path):
+            if path.endswith(".."):
+                parent = _os_lstat(path)
+                return MockStat(parent.st_mode, parent.st_dev + 1,
+                                parent.st_ino)
+            else:
+                return _os_lstat(path)
+
+        tmpdir = mkdtemp()
+        try:
+            with patch("os.lstat", _mock_os_lstat):
+                try:
+                    utils.ismount(tmpdir)
+                except OSError:
+                    self.fail("Unexpected exception")
+                else:
+                    pass
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def test_ismount_successes_ino(self):
+
+        _os_lstat = os.lstat
+
+        class MockStat(object):
+            def __init__(self, mode, dev, ino):
+                self.st_mode = mode
+                self.st_dev = dev
+                self.st_ino = ino
+
+        def _mock_os_lstat(path):
+            if path.endswith(".."):
+                return _os_lstat(path)
+            else:
+                parent_path = os.path.join(path, "..")
+                child = _os_lstat(path)
+                parent = _os_lstat(parent_path)
+                return MockStat(child.st_mode, parent.st_ino,
+                                child.st_dev)
+
+        tmpdir = mkdtemp()
+        try:
+            with patch("os.lstat", _mock_os_lstat):
+                try:
+                    utils.ismount(tmpdir)
+                except OSError:
+                    self.fail("Unexpected exception")
+                else:
+                    pass
+        finally:
+            shutil.rmtree(tmpdir)
+
 
 class TestFileLikeIter(unittest.TestCase):
 
@@ -1399,7 +1533,6 @@ class TestFileLikeIter(unittest.TestCase):
 
     def test_read(self):
         in_iter = ['abc', 'de', 'fghijk', 'l']
-        chunks = []
         iter_file = utils.FileLikeIter(in_iter)
         self.assertEquals(iter_file.read(), ''.join(in_iter))
 
@@ -1758,7 +1891,9 @@ class TestAffinityLocalityPredicate(unittest.TestCase):
         self.assertRaises(ValueError,
                           utils.affinity_locality_predicate, 'r1z1=1')
 
+
 class TestGreenthreadSafeIterator(unittest.TestCase):
+
     def increment(self, iterable):
         plus_ones = []
         for n in iterable:
@@ -1790,6 +1925,7 @@ class TestGreenthreadSafeIterator(unittest.TestCase):
 
 
 class TestStatsdLoggingDelegation(unittest.TestCase):
+
     def setUp(self):
         self.sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
         self.sock.bind(('localhost', 0))
@@ -2030,10 +2166,13 @@ class TestStatsdLoggingDelegation(unittest.TestCase):
 
     def test_no_fdatasync(self):
         called = []
+
         class NoFdatasync:
             pass
+
         def fsync(fd):
             called.append(fd)
+
         with patch('swift.common.utils.os', NoFdatasync()):
             with patch('swift.common.utils.fsync', fsync):
                 utils.fdatasync(12345)
@@ -2041,38 +2180,52 @@ class TestStatsdLoggingDelegation(unittest.TestCase):
 
     def test_yes_fdatasync(self):
         called = []
+
         class YesFdatasync:
+
             def fdatasync(self, fd):
                 called.append(fd)
+
         with patch('swift.common.utils.os', YesFdatasync()):
             utils.fdatasync(12345)
             self.assertEquals(called, [12345])
 
     def test_fsync_bad_fullsync(self):
+
         class FCNTL:
+
             F_FULLSYNC = 123
+
             def fcntl(self, fd, op):
                 raise IOError(18)
+
         with patch('swift.common.utils.fcntl', FCNTL()):
             self.assertRaises(OSError, lambda: utils.fsync(12345))
 
     def test_fsync_f_fullsync(self):
         called = []
+
         class FCNTL:
+
             F_FULLSYNC = 123
+
             def fcntl(self, fd, op):
                 called[:] = [fd, op]
                 return 0
+
         with patch('swift.common.utils.fcntl', FCNTL()):
             utils.fsync(12345)
             self.assertEquals(called, [12345, 123])
 
     def test_fsync_no_fullsync(self):
         called = []
+
         class FCNTL:
             pass
+
         def fsync(fd):
             called.append(fd)
+
         with patch('swift.common.utils.fcntl', FCNTL()):
             with patch('os.fsync', fsync):
                 utils.fsync(12345)