Change in vdsm[master]: storage: fix spec normalization when mounting

nsoffer at redhat.com nsoffer at redhat.com
Sun Mar 27 12:20:39 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: storage: fix spec normalization when mounting
......................................................................


Patch Set 1:

(6 comments)

https://gerrit.ovirt.org/#/c/55182/1/tests/mountTests.py
File tests/mountTests.py:

Line 315
Line 316
Line 317
Line 318
Line 319
All these tests are incorrect, as the input to the test is local file, not a spec, see my comment in the previous patch.


Line 322:             self.assertTrue(mount.isMounted(self.generate_fs_file(fs_spec)))
Line 323: 
Line 324:     def generate_fs_file(self, fs_spec):
Line 325:         normalized_spec = mount.normalize_spec(fs_spec)
Line 326:         mnt_path = normalized_spec.replace('_', '__').replace('/', '_')
You are not testing mount.isMounted here.


https://gerrit.ovirt.org/#/c/55182/1/vdsm/storage/mount.py
File vdsm/storage/mount.py:

Line 52
Line 53
Line 54
Line 55
Line 56
This may need to use the new normalization, adding more test cases to isMounted tests will prove that.


Line 50:      fs_freq, fs_passno) = line.split()[:6]
Line 51:     fs_mntops = fs_mntops.split(",")
Line 52:     fs_freq = int(fs_freq)
Line 53:     fs_passno = int(fs_passno)
Line 54:     fs_spec = normalize_spec(_parseFstabPath(fs_spec))
I think that this change does not fix any failing test. Please write a test that fails without this change first. Most likely, this will be Mount.isMounted() test.
Line 55: 
Line 56:     fs_file = normpath(_parseFstabPath(fs_file))
Line 57:     for suffix in (" (deleted)", ):
Line 58:         if not fs_file.endswith(suffix):


https://gerrit.ovirt.org/#/c/55182/1/vdsm/storage/nfsSD.py
File vdsm/storage/nfsSD.py:

Line 72:         if not misc.isAscii(domainName) and not sd.supportsUnicode(version):
Line 73:             raise se.UnicodeArgumentException()
Line 74: 
Line 75:         # Create local path
Line 76:         mntPath = fileUtils.transformPath(mount.normalize_spec(remotePath))
This should not be here - we have duplicated code converting spec to local file.  We should first (in another patch), remove the duplication, doing all conversions in one place (fileUtils.transformPath looks like the correct place), and in this function use the correct normalization. 

Then this code should not be changed. This code has no tests and is known to break every time we fix it in unexpected ways, so we should not touch it unless we have a very good reason.
Line 77: 
Line 78:         mntPoint = cls.getMountPoint(mntPath)
Line 79: 
Line 80:         cls._preCreateValidation(sdUUID, mntPoint, remotePath, storageType,


https://gerrit.ovirt.org/#/c/55182/1/vdsm/storage/storageServer.py
File vdsm/storage/storageServer.py:

Line 209:     def __init__(self, spec, vfsType=None, options="", mountClass=mount.Mount):
Line 210:         self._vfsType = vfsType
Line 211:         # Note: must be normalized before we escape "/" in _getLocalPath.
Line 212:         # See https://bugzilla.redhat.com/1300749
Line 213:         self._remotePath = mount.normalize_spec(spec)
Looks good, will fix the storageServer failing test.
Line 214:         self._options = options
Line 215:         self._mount = mountClass(spec, self._getLocalPath())
Line 216: 
Line 217:     def _getLocalPath(self):


-- 
To view, visit https://gerrit.ovirt.org/55182
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f244eb903fc049c726efba69f37b3b5fb01b561
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list