From 1076e1bd6751e4c3faf3b4fd53b68db8210ee8ef Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Fri, 23 Jun 2017 15:19:14 +0300 Subject: [PATCH] Windows: case insensitive path comparisons While Windows paths are case insensitive, some checks performed in the SMBFS driver as well as its parent class are not, breaking the driver's logic. We're most affected by the fact that some of the Windows API functions disregard path case sensitivity, always returning uppercase strings in some situations. This change adds a helper function that compares paths having the os type in mind and uses it throughout the remotefs and smbfs modules. Change-Id: Icfef1c8b9ae011d2b463aa5c1fb7f512388c8f88 Closes-Bug: #1694648 --- cinder/tests/unit/test_utils.py | 16 ++++++++++++++ .../unit/volume/drivers/test_remotefs.py | 2 +- cinder/utils.py | 4 ++++ cinder/volume/drivers/remotefs.py | 21 ++++++++++++------- cinder/volume/drivers/windows/smbfs.py | 7 ++++--- 5 files changed, 38 insertions(+), 12 deletions(-) diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py index 16446a4df7d..864605bf63a 100644 --- a/cinder/tests/unit/test_utils.py +++ b/cinder/tests/unit/test_utils.py @@ -65,6 +65,7 @@ class ExecuteTestCase(test.TestCase): root_helper=mock_helper) +@ddt.ddt class GenericUtilsTestCase(test.TestCase): def test_as_int(self): test_obj_int = '2' @@ -282,6 +283,21 @@ class GenericUtilsTestCase(test.TestCase): self.assertEqual('sudo cinder-rootwrap /path/to/conf', utils.get_root_helper()) + @ddt.data({'path_a': 'test', 'path_b': 'test', 'exp_eq': True}) + @ddt.data({'path_a': 'test', 'path_b': 'other', 'exp_eq': False}) + @ddt.unpack + @mock.patch('os.path.normcase') + def test_paths_normcase_equal(self, mock_normcase, path_a, + path_b, exp_eq): + # os.path.normcase will lower the path string on Windows + # while doing nothing on other platforms. + mock_normcase.side_effect = lambda x: x + + result = utils.paths_normcase_equal(path_a, path_b) + self.assertEqual(exp_eq, result) + + mock_normcase.assert_has_calls([mock.call(path_a), mock.call(path_b)]) + class TemporaryChownTestCase(test.TestCase): @mock.patch('os.stat') diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 24e7d436f90..3f276305a3d 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -435,7 +435,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): @ddt.data([None, '/fake_basedir'], ['/fake_basedir/cb2016/fake_vol_name', '/fake_basedir'], - ['/fake_basedir/cb2016/fake_vol_name.vhd', '/fake_basedir'], + ['/fake_basedir/cb2016/fake_vol_name.VHD', '/fake_basedir'], ['/fake_basedir/cb2016/fake_vol_name.404f-404', '/fake_basedir'], ['/fake_basedir/cb2016/fake_vol_name.tmp-snap-404f-404', diff --git a/cinder/utils.py b/cinder/utils.py index c260ec3c771..6f9a84c53de 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -1130,3 +1130,7 @@ def get_log_levels(prefix): return {k: logging.logging.getLevelName(v.logger.getEffectiveLevel()) for k, v in logging._loggers.items() if k and k.startswith(prefix)} + + +def paths_normcase_equal(path_a, path_b): + return os.path.normcase(path_a) == os.path.normcase(path_b) diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 4aa29398774..d771f50b464 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -747,7 +747,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): 'volname': volume_name, 'valid_ext': valid_ext, } - if not re.match(backing_file_template, info.backing_file): + if not re.match(backing_file_template, info.backing_file, + re.IGNORECASE): msg = _("File %(path)s has invalid backing file " "%(bfile)s, aborting.") % {'path': path, 'bfile': info.backing_file} @@ -803,8 +804,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): volume, active_file_path) higher_file = next((os.path.basename(f['filename']) for f in backing_chain - if f.get('backing-filename', '') == - snapshot_file), + if utils.paths_normcase_equal( + f.get('backing-filename', ''), + snapshot_file)), None) return higher_file @@ -1008,7 +1010,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): active_file = self.get_active_image_from_info(snapshot.volume) snapshot_path = os.path.join( self._local_volume_dir(snapshot.volume), snapshot_file) - if (snapshot_file == active_file): + if utils.paths_normcase_equal(snapshot_file, active_file): return LOG.info('Deleting stale snapshot: %s', snapshot.id) @@ -1094,7 +1096,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): base_id = None for key, value in snap_info.items(): - if value == base_file and key != 'active': + if utils.paths_normcase_equal(value, + base_file) and key != 'active': base_id = key break if base_id is None: @@ -1114,7 +1117,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): snapshot, online_delete_info) - if snapshot_file == active_file: + if utils.paths_normcase_equal(snapshot_file, active_file): # There is no top file # T0 | T1 | # base | snapshot_file | None @@ -1140,7 +1143,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): raise exception.RemoteFSException(msg) higher_id = next((i for i in snap_info - if snap_info[i] == higher_file + if utils.paths_normcase_equal(snap_info[i], + higher_file) and i != 'active'), None) if higher_id is None: @@ -1450,7 +1454,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): info_path = self._local_path_volume_info(snapshot.volume) snap_info = self._read_info_file(info_path) - if info['active_file'] == info['snapshot_file']: + if utils.paths_normcase_equal(info['active_file'], + info['snapshot_file']): # blockRebase/Pull base into active # info['base'] => snapshot_file diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index a0021539910..097dae5e2e0 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -29,6 +29,7 @@ from cinder import exception from cinder.i18n import _ from cinder.image import image_utils from cinder import interface +from cinder import utils from cinder.volume.drivers import remotefs as remotefs_drv VERSION = '1.1.0' @@ -439,9 +440,9 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, merged_img_path = os.path.join( self._local_volume_dir(snapshot.volume), file_to_merge) - if snap_info['active'] == file_to_merge: + if utils.paths_normcase_equal(snap_info['active'], file_to_merge): new_active_file_path = self._vhdutils.get_vhd_parent_path( - merged_img_path) + merged_img_path).lower() snap_info['active'] = os.path.basename(new_active_file_path) self._delete(merged_img_path) @@ -456,7 +457,7 @@ class WindowsSmbfsDriver(remotefs_drv.RemoteFSPoolMixin, active_file_path = os.path.join(self._local_volume_dir(volume), active_file) - if active_file_path != volume_path: + if not utils.paths_normcase_equal(active_file_path, volume_path): msg = _('Extend volume is only supported for this ' 'driver when no snapshots exist.') raise exception.InvalidVolume(msg)