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

nsoffer at redhat.com nsoffer at redhat.com
Thu Apr 28 01:30:33 UTC 2016


Nir Soffer has posted comments on this change.

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


Patch Set 10:

(6 comments)

https://gerrit.ovirt.org/#/c/55182/10/tests/fileUtilTests.py
File tests/fileUtilTests.py:

Line 279:             self.assertRaises(OSError, fileUtils.atomic_symlink, target, link)
Line 280: 
Line 281: 
Line 282: @expandPermutations
Line 283: class TestNormalizeRemotePath(TestCaseBase):
The function is called now normalize_path, so the test should be called TestNormalizePath.
Line 284: 
Line 285:     @permutations([
Line 286:         ("server:/path", "server:/path"),
Line 287:         ("server:/path/", "server:/path"),


Line 287:         ("server:/path/", "server:/path"),
Line 288:         ("server:/path//", "server:/path"),
Line 289:         ("server:/", "server:/"),
Line 290:         ("12.34.56.78:/", "12.34.56.78:/"),
Line 291:         ("[2001:db8::60fe:5bf:febc:912]:/", "[2001:db8::60fe:5bf:febc:912]:/"),
We need to add more permutations for normalizing local paths (these without ":").
Line 292:     ])
Line 293:     def test_normalize_path_equals(self, remote_path,
Line 294:                                    normalized_remote_path):
Line 295:         self.assertEquals(normalized_remote_path,


https://gerrit.ovirt.org/#/c/55182/10/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 2762:                     continue
Line 2763:                 # Filter domains according to 'remotePath'
Line 2764:                 if (remotePath and
Line 2765:                         fileUtils.transformPath(
Line 2766:                             fileUtils.normalize_path(remotePath)) !=
This code normalize removePath multiple times for each domain, instead of once.

Please do the normalization once at the start of the function - input validation must be done first.

    if remotePath:
        remotePath = fileUtils.transformPath(fileUtils.normalize_path(remotePath))

Then the check inside the loop becomes:

    if remotePath and remotePath != dom.getRemotePath():
        ...

More clear, safer (no way to use unnormalized remotePath), faster.
Line 2767:                         dom.getRemotePath()):
Line 2768:                     domains.remove(sdUUID)
Line 2769:                     continue
Line 2770:             except Exception:


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

Line 67
Line 68
Line 69
Line 70
Line 71
We should normalize the remotePath here - and use the same name so it is not possible to use unnormalized version in this function:

    remotePath = fileUtils.normalize_path(remotePath)

This is the only change needed in this function!


Line 76
Line 77
Line 78
Line 79
Line 80
We should pass normalized remote path.


Line 81
Line 82
Line 83
Line 84
Line 85
We should pass normalized remote path.


-- 
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: 10
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