openstack/cinder
Code Issues Proposed changes

Windows: case insensitive path comparisons

Browse Source 
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
This commit is contained in:
Lucian Petrut 2017-06-23 15:19:14 +03:00
parent 30b501f48d
commit 1076e1bd67
5 changed files with 38 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
cinder/tests/unit/test_utils.py
View File

@ -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')

2
cinder/tests/unit/volume/drivers/test_remotefs.py
View File

@ -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',

4
cinder/utils.py
View File

@ -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)

21
cinder/volume/drivers/remotefs.py
View File

@ -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

7
cinder/volume/drivers/windows/smbfs.py
View File

@ -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)