[Pkg-owncloud-commits] [owncloud] 95/205: Fix locked paths in the moveMount case
David Prévot
taffit at moszumanska.debian.org
Thu Jul 2 17:37:00 UTC 2015
This is an automated email from the git hooks/post-receive script.
taffit pushed a commit to branch stable8
in repository owncloud.
commit 35047a23000fa4788a06332428aa0f844394816f
Author: Vincent Petry <pvince81 at owncloud.com>
Date: Mon Jun 22 12:38:48 2015 +0200
Fix locked paths in the moveMount case
When moving a mount point directly, the lock must be applied on the
local mount point path instead of the attached storage root.
Other operations will still lock the attached storage root.
---
lib/private/files/view.php | 80 ++++++++++++++++++++++++++++------------
tests/lib/files/view.php | 91 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 148 insertions(+), 23 deletions(-)
diff --git a/lib/private/files/view.php b/lib/private/files/view.php
index 73daf8a..61adc62 100644
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@@ -628,8 +628,8 @@ class View {
return false;
}
- $this->lockFile($path1, ILockingProvider::LOCK_SHARED);
- $this->lockFile($path2, ILockingProvider::LOCK_SHARED);
+ $this->lockFile($path1, ILockingProvider::LOCK_SHARED, true);
+ $this->lockFile($path2, ILockingProvider::LOCK_SHARED, true);
$run = true;
if ($this->shouldEmitHooks() && (Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2))) {
@@ -656,8 +656,8 @@ class View {
$internalPath1 = $mount1->getInternalPath($absolutePath1);
$internalPath2 = $mount2->getInternalPath($absolutePath2);
- $this->changeLock($path1, ILockingProvider::LOCK_EXCLUSIVE);
- $this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE);
+ $this->changeLock($path1, ILockingProvider::LOCK_EXCLUSIVE, true);
+ $this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE, true);
if ($internalPath1 === '' and $mount1 instanceof MoveableMount) {
if ($this->isTargetAllowed($absolutePath2)) {
@@ -670,12 +670,14 @@ class View {
} else {
$result = false;
}
+ // moving a file/folder within the same mount point
} elseif ($storage1 == $storage2) {
if ($storage1) {
$result = $storage1->rename($internalPath1, $internalPath2);
} else {
$result = false;
}
+ // moving a file/folder between storages (from $storage1 to $storage2)
} else {
$result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2);
}
@@ -693,13 +695,8 @@ class View {
}
}
- $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE);
- $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE);
-
- if ($internalPath1 === '' and $mount1 instanceof MoveableMount) {
- // since $path2 now points to a different storage we need to unlock the path on the old storage separately
- $storage2->releaseLock($internalPath2, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
- }
+ $this->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE, true);
+ $this->unlockFile($path2, ILockingProvider::LOCK_EXCLUSIVE, true);
if ((Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2)) && $result !== false) {
if ($this->shouldEmitHooks()) {
@@ -719,8 +716,8 @@ class View {
}
return $result;
} else {
- $this->unlockFile($path1, ILockingProvider::LOCK_SHARED);
- $this->unlockFile($path2, ILockingProvider::LOCK_SHARED);
+ $this->unlockFile($path1, ILockingProvider::LOCK_SHARED, true);
+ $this->unlockFile($path2, ILockingProvider::LOCK_SHARED, true);
return false;
}
} else {
@@ -1700,21 +1697,50 @@ class View {
}
/**
+ * Returns the mount point for which to lock
+ *
+ * @param string $absolutePath absolute path
+ * @param bool $useParentMount true to return parent mount instead of whatever
+ * is mounted directly on the given path, false otherwise
+ * @return \OC\Files\Mount\MountPoint mount point for which to apply locks
+ */
+ private function getMountForLock($absolutePath, $useParentMount = false) {
+ $results = [];
+ $mount = Filesystem::getMountManager()->find($absolutePath);
+ if (!$mount) {
+ return $results;
+ }
+
+ if ($useParentMount) {
+ // find out if something is mounted directly on the path
+ $internalPath = $mount->getInternalPath($absolutePath);
+ if ($internalPath === '') {
+ // resolve the parent mount instead
+ $mount = Filesystem::getMountManager()->find(dirname($absolutePath));
+ }
+ }
+
+ return $mount;
+ }
+
+ /**
* Lock the given path
*
* @param string $path the path of the file to lock, relative to the view
* @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+ * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage
+ *
* @return bool False if the path is excluded from locking, true otherwise
* @throws \OCP\Lock\LockedException if the path is already locked
*/
- private function lockPath($path, $type) {
+ private function lockPath($path, $type, $lockMountPoint = false) {
$absolutePath = $this->getAbsolutePath($path);
$absolutePath = Filesystem::normalizePath($absolutePath);
if (!$this->shouldLockFile($absolutePath)) {
return false;
}
- $mount = $this->getMount($path);
+ $mount = $this->getMountForLock($absolutePath, $lockMountPoint);
if ($mount) {
try {
$mount->getStorage()->acquireLock(
@@ -1739,10 +1765,12 @@ class View {
*
* @param string $path the path of the file to lock, relative to the view
* @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+ * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage
+ *
* @return bool False if the path is excluded from locking, true otherwise
* @throws \OCP\Lock\LockedException if the path is already locked
*/
- public function changeLock($path, $type) {
+ public function changeLock($path, $type, $lockMountPoint = false) {
$path = Filesystem::normalizePath($path);
$absolutePath = $this->getAbsolutePath($path);
$absolutePath = Filesystem::normalizePath($absolutePath);
@@ -1750,7 +1778,7 @@ class View {
return false;
}
- $mount = $this->getMount($path);
+ $mount = $this->getMountForLock($absolutePath, $lockMountPoint);
if ($mount) {
try {
$mount->getStorage()->changeLock(
@@ -1775,16 +1803,18 @@ class View {
*
* @param string $path the path of the file to unlock, relative to the view
* @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+ * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage
+ *
* @return bool False if the path is excluded from locking, true otherwise
*/
- private function unlockPath($path, $type) {
+ private function unlockPath($path, $type, $lockMountPoint = false) {
$absolutePath = $this->getAbsolutePath($path);
$absolutePath = Filesystem::normalizePath($absolutePath);
if (!$this->shouldLockFile($absolutePath)) {
return false;
}
- $mount = $this->getMount($path);
+ $mount = $this->getMountForLock($absolutePath, $lockMountPoint);
if ($mount) {
$mount->getStorage()->releaseLock(
$mount->getInternalPath($absolutePath),
@@ -1801,16 +1831,18 @@ class View {
*
* @param string $path the path of the file to lock relative to the view
* @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+ * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage
+ *
* @return bool False if the path is excluded from locking, true otherwise
*/
- public function lockFile($path, $type) {
+ public function lockFile($path, $type, $lockMountPoint = false) {
$absolutePath = $this->getAbsolutePath($path);
$absolutePath = Filesystem::normalizePath($absolutePath);
if (!$this->shouldLockFile($absolutePath)) {
return false;
}
- $this->lockPath($path, $type);
+ $this->lockPath($path, $type, $lockMountPoint);
$parents = $this->getParents($path);
foreach ($parents as $parent) {
@@ -1825,16 +1857,18 @@ class View {
*
* @param string $path the path of the file to lock relative to the view
* @param int $type \OCP\Lock\ILockingProvider::LOCK_SHARED or \OCP\Lock\ILockingProvider::LOCK_EXCLUSIVE
+ * @param bool $lockMountPoint true to lock the mount point, false to lock the attached mount/storage
+ *
* @return bool False if the path is excluded from locking, true otherwise
*/
- public function unlockFile($path, $type) {
+ public function unlockFile($path, $type, $lockMountPoint = false) {
$absolutePath = $this->getAbsolutePath($path);
$absolutePath = Filesystem::normalizePath($absolutePath);
if (!$this->shouldLockFile($absolutePath)) {
return false;
}
- $this->unlockPath($path, $type);
+ $this->unlockPath($path, $type, $lockMountPoint);
$parents = $this->getParents($path);
foreach ($parents as $parent) {
diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php
index 9862026..42768a0 100644
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@@ -1162,6 +1162,97 @@ class View extends \Test\TestCase {
$this->assertFalse($view->lockFile($pathPrefix . '/foo/bar', ILockingProvider::LOCK_EXCLUSIVE));
}
+ /**
+ * Test that locks are on mount point paths instead of mount root
+ */
+ public function testLockLocalMountPointPathInsteadOfStorageRoot() {
+ $lockingProvider = \OC::$server->getLockingProvider();
+ $view = new \OC\Files\View('/testuser/files/');
+ $storage = new Temporary([]);
+ \OC\Files\Filesystem::mount($storage, [], '/');
+ $mountedStorage = new Temporary([]);
+ \OC\Files\Filesystem::mount($mountedStorage, [], '/testuser/files/mountpoint');
+
+ $this->assertTrue(
+ $view->lockFile('/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, true),
+ 'Can lock mount point'
+ );
+
+ // no exception here because storage root was not locked
+ $mountedStorage->acquireLock('', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
+ $thrown = false;
+ try {
+ $storage->acquireLock('/testuser/files/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+ } catch (\OCP\Lock\LockedException $e) {
+ $thrown = true;
+ }
+ $this->assertTrue($thrown, 'Mount point path was locked on root storage');
+
+ $lockingProvider->releaseAll();
+ }
+
+ /**
+ * Test that locks are on mount point paths and also mount root when requested
+ */
+ public function testLockStorageRootButNotLocalMountPoint() {
+ $lockingProvider = \OC::$server->getLockingProvider();
+ $view = new \OC\Files\View('/testuser/files/');
+ $storage = new Temporary([]);
+ \OC\Files\Filesystem::mount($storage, [], '/');
+ $mountedStorage = new Temporary([]);
+ \OC\Files\Filesystem::mount($mountedStorage, [], '/testuser/files/mountpoint');
+
+ $this->assertTrue(
+ $view->lockFile('/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, false),
+ 'Can lock mount point'
+ );
+
+ $thrown = false;
+ try {
+ $mountedStorage->acquireLock('', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+ } catch (\OCP\Lock\LockedException $e) {
+ $thrown = true;
+ }
+ $this->assertTrue($thrown, 'Mount point storage root was locked on original storage');
+
+ // local mount point was not locked
+ $storage->acquireLock('/testuser/files/mountpoint', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
+ $lockingProvider->releaseAll();
+ }
+
+ /**
+ * Test that locks are on mount point paths and also mount root when requested
+ */
+ public function testLockMountPointPathFailReleasesBoth() {
+ $lockingProvider = \OC::$server->getLockingProvider();
+ $view = new \OC\Files\View('/testuser/files/');
+ $storage = new Temporary([]);
+ \OC\Files\Filesystem::mount($storage, [], '/');
+ $mountedStorage = new Temporary([]);
+ \OC\Files\Filesystem::mount($mountedStorage, [], '/testuser/files/mountpoint.txt');
+
+ // this would happen if someone is writing on the mount point
+ $mountedStorage->acquireLock('', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
+ $thrown = false;
+ try {
+ // this actually acquires two locks, one on the mount point and one no the storage root,
+ // but the one on the storage root will fail
+ $view->lockFile('/mountpoint.txt', ILockingProvider::LOCK_SHARED);
+ } catch (\OCP\Lock\LockedException $e) {
+ $thrown = true;
+ }
+ $this->assertTrue($thrown, 'Cannot acquire shared lock because storage root is already locked');
+
+ // from here we expect that the lock on the local mount point was released properly
+ // so acquiring an exclusive lock will succeed
+ $storage->acquireLock('/testuser/files/mountpoint.txt', ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
+ $lockingProvider->releaseAll();
+ }
+
public function dataLockPaths() {
return [
['/testuser/{folder}', ''],
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-owncloud/owncloud.git
More information about the Pkg-owncloud-commits
mailing list