* Follow-up r109009: Check that paths are usable in FileOp::doPrecheck(). Also lock...

* Fixed bogus $params var in logException() call in SwiftFileBackend.
* Added 'latest' param to FileBackendMultliWrite::consistencyCheck().
* Dummy-proof FileBackend::getFileStat() w.r.t the 'latest' param and removed related FileOp::allowStaleReads() comment.
* Tweaked backend-fail-batchsize message from r109469.

diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php
index dc81c48..8aaba70 100644 (file)
--- a/includes/filerepo/backend/FSFileBackend.php
+++ b/includes/filerepo/backend/FSFileBackend.php
@@ -101,6 +101,27 @@ class FSFileBackend extends FileBackend {
                return $fsPath;
        }
 
+       /**
+        * @see FileBackend::isPathUsableInternal()
+        */
+       public function isPathUsableInternal( $storagePath ) {
+               $fsPath = $this->resolveToFSPath( $storagePath );
+               if ( $fsPath === null ) {
+                       return false; // invalid
+               }
+               $parentDir = dirname( $fsPath );
+
+               wfSuppressWarnings();
+               if ( file_exists( $fsPath ) ) {
+                       $ok = is_file( $fsPath ) && is_writable( $fsPath );
+               } else {
+                       $ok = is_dir( $parentDir ) && is_writable( $parentDir );
+               }
+               wfRestoreWarnings();
+
+               return $ok;
+       }
+
        /**
         * @see FileBackend::doStoreInternal()
         */

diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php
index 636b054..baa18c1 100644 (file)
--- a/includes/filerepo/backend/FileBackend.php
+++ b/includes/filerepo/backend/FileBackend.php
@@ -419,6 +419,7 @@ abstract class FileBackendBase {
         * Otherwise, the result is an associative array that includes:
         *     mtime  : the last-modified timestamp (TS_MW)
         *     size   : the file size (bytes)
+        * Additional values may be included for internal use only.
         * 
         * $params include:
         *     src    : source storage path
@@ -577,11 +578,11 @@ abstract class FileBackendBase {
  * This class defines the methods as abstract that subclasses must implement.
  * Callers outside of FileBackend and its helper classes, such as FileOp,
  * should only call functions that are present in FileBackendBase.
- *
+ * 
  * The FileBackendBase operations are implemented using primitive functions
  * such as storeInternal(), copyInternal(), deleteInternal() and the like.
  * This class is also responsible for path resolution and sanitization.
- *
+ * 
  * @ingroup FileBackend
  * @since 1.19
  */
@@ -605,6 +606,16 @@ abstract class FileBackend extends FileBackendBase {
                return $this->maxFileSize;
        }
 
+       /**
+        * Check if a file can be created at a given storage path.
+        * FS backends should check if the parent directory exists and the file is writable.
+        * Backends using key/value stores should check if the container exists.
+        *
+        * @param $storagePath string
+        * @return bool
+        */
+       abstract public function isPathUsableInternal( $storagePath );
+
        /**
         * Create a file in the backend with the given contents.
         * Do not call this function from places outside FileBackend and FileOp.
@@ -878,6 +889,12 @@ abstract class FileBackend extends FileBackendBase {
                        $status->fatal( 'backend-fail-invalidpath', $params['dir'] );
                        return $status; // invalid storage path
                }
+               // Attempt to lock this directory...
+               $filesLockEx = array( $params['dir'] );
+               $scopedLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
+               if ( !$status->isOK() ) {
+                       return $status; // abort
+               }
                if ( $shard !== null ) { // confined to a single container/shard
                        $status->merge( $this->doCleanInternal( $fullCont, $dir, $params ) );
                } else { // directory is on several shards
@@ -937,13 +954,19 @@ abstract class FileBackend extends FileBackendBase {
         */
        final public function getFileStat( array $params ) {
                $path = $params['src'];
+               $latest = !empty( $params['latest'] );
                if ( isset( $this->cache[$path]['stat'] ) ) {
-                       return $this->cache[$path]['stat'];
+                       // If we want the latest data, check that this cached
+                       // value was in fact fetched with the latest available data.
+                       if ( !$latest || $this->cache[$path]['stat']['latest'] ) {
+                               return $this->cache[$path]['stat'];
+                       }
                }
                $stat = $this->doGetFileStat( $params );
                if ( is_array( $stat ) ) { // don't cache negatives
                        $this->trimCache(); // limit memory
                        $this->cache[$path]['stat'] = $stat;
+                       $this->cache[$path]['stat']['latest'] = $latest;
                }
                return $stat;
        }
@@ -1161,6 +1184,8 @@ abstract class FileBackend extends FileBackendBase {
                        }
                        // Optimization: if doing an EX lock anyway, don't also set an SH one
                        $filesLockSh = array_diff( $filesLockSh, $filesLockEx );
+                       // Get a shared lock on the parent directory of each path changed
+                       $filesLockSh = array_merge( $filesLockSh, array_map( 'dirname', $filesLockEx ) );
                        // Try to lock those files for the scope of this function...
                        $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
                        $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );

diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php
index 8685774..44a07be 100644 (file)
--- a/includes/filerepo/backend/FileBackendMultiWrite.php
+++ b/includes/filerepo/backend/FileBackendMultiWrite.php
@@ -96,6 +96,9 @@ class FileBackendMultiWrite extends FileBackendBase {
                if ( empty( $opts['nonLocking'] ) ) {
                        $filesLockSh = array_diff( $filesRead, $filesChanged ); // optimization
                        $filesLockEx = $filesChanged;
+                       // Get a shared lock on the parent directory of each path changed
+                       $filesLockSh = array_merge( $filesLockSh, array_map( 'dirname', $filesLockEx ) );
+                       // Try to lock those files for the scope of this function...
                        $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
                        $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
                        if ( !$status->isOK() ) {
@@ -156,7 +159,7 @@ class FileBackendMultiWrite extends FileBackendBase {
 
                $mBackend = $this->backends[$this->masterIndex];
                foreach ( array_unique( $paths ) as $path ) {
-                       $params = array( 'src' => $path );
+                       $params = array( 'src' => $path, 'latest' => true );
                        // Stat the file on the 'master' backend
                        $mStat = $mBackend->getFileStat( $this->substOpPaths( $params, $mBackend ) );
                        // Check of all clone backends agree with the master...

diff --git a/includes/filerepo/backend/FileOp.php b/includes/filerepo/backend/FileOp.php
index 86f2a94..c325190 100644 (file)
--- a/includes/filerepo/backend/FileOp.php
+++ b/includes/filerepo/backend/FileOp.php
@@ -55,11 +55,7 @@ abstract class FileOp {
        }
 
        /**
-        * Allow stale data for file reads and existence checks.
-        * 
-        * Note that we don't want to mix stale and non-stale reads
-        * because stat calls are cached: if we read X without 'latest'
-        * and then read it with 'latest', the data may still be stale.
+        * Allow stale data for file reads and existence checks
         *
         * @return void
         */
@@ -72,11 +68,11 @@ abstract class FileOp {
         * Callers are responsible for handling file locking.
         * 
         * $opts is an array of options, including:
-        * 'force'               : Errors that would normally cause a rollback do not.
-        *                         The remaining operations are still attempted if any fail.
-        * 'allowStale'          : Don't require the latest available data.
-        *                         This can increase performance for non-critical writes.
-        *                         This has no effect unless the 'force' flag is set.
+        * 'force'      : Errors that would normally cause a rollback do not.
+        *                The remaining operations are still attempted if any fail.
+        * 'allowStale' : Don't require the latest available data.
+        *                This can increase performance for non-critical writes.
+        *                This has no effect unless the 'force' flag is set.
         * 
         * @param $performOps Array List of FileOp operations
         * @param $opts Array Batch operation options
@@ -155,7 +151,7 @@ abstract class FileOp {
        /**
         * Check if this operation failed precheck() or attempt()
         * 
-        * @return type 
+        * @return bool 
         */
        final public function failed() {
                return $this->failed;
@@ -396,20 +392,22 @@ class StoreFileOp extends FileOp {
                if ( !is_file( $this->params['src'] ) ) {
                        $status->fatal( 'backend-fail-notexists', $this->params['src'] );
                        return $status;
-               }
                // Check if the source file is too big
-               if ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) {
-                       $status->fatal( 'backend-fail-store', $this->params['dst'] );
+               } elseif ( filesize( $this->params['src'] ) > $this->backend->maxFileSizeInternal() ) {
+                       $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] );
+                       return $status;
+               // Check if a file can be placed at the destination
+               } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
+                       $status->fatal( 'backend-fail-store', $this->params['src'], $this->params['dst'] );
                        return $status;
                }
                // Check if destination file exists
                $status->merge( $this->precheckDestExistence( $predicates ) );
-               if ( !$status->isOK() ) {
-                       return $status;
+               if ( $status->isOK() ) {
+                       // Update file existence predicates
+                       $predicates['exists'][$this->params['dst']] = true;
+                       $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                }
-               // Update file existence predicates
-               $predicates['exists'][$this->params['dst']] = true;
-               $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                return $status; // safe to call attempt()
        }
 
@@ -456,15 +454,18 @@ class CreateFileOp extends FileOp {
                if ( strlen( $this->params['content'] ) > $this->backend->maxFileSizeInternal() ) {
                        $status->fatal( 'backend-fail-create', $this->params['dst'] );
                        return $status;
+               // Check if a file can be placed at the destination
+               } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
+                       $status->fatal( 'backend-fail-create', $this->params['dst'] );
+                       return $status;
                }
                // Check if destination file exists
                $status->merge( $this->precheckDestExistence( $predicates ) );
-               if ( !$status->isOK() ) {
-                       return $status;
+               if ( $status->isOK() ) {
+                       // Update file existence predicates
+                       $predicates['exists'][$this->params['dst']] = true;
+                       $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                }
-               // Update file existence predicates
-               $predicates['exists'][$this->params['dst']] = true;
-               $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                return $status; // safe to call attempt()
        }
 
@@ -505,15 +506,18 @@ class CopyFileOp extends FileOp {
                if ( !$this->fileExists( $this->params['src'], $predicates ) ) {
                        $status->fatal( 'backend-fail-notexists', $this->params['src'] );
                        return $status;
+               // Check if a file can be placed at the destination
+               } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
+                       $status->fatal( 'backend-fail-copy', $this->params['src'], $this->params['dst'] );
+                       return $status;
                }
                // Check if destination file exists
                $status->merge( $this->precheckDestExistence( $predicates ) );
-               if ( !$status->isOK() ) {
-                       return $status;
+               if ( $status->isOK() ) {
+                       // Update file existence predicates
+                       $predicates['exists'][$this->params['dst']] = true;
+                       $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                }
-               // Update file existence predicates
-               $predicates['exists'][$this->params['dst']] = true;
-               $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                return $status; // safe to call attempt()
        }
 
@@ -557,17 +561,20 @@ class MoveFileOp extends FileOp {
                if ( !$this->fileExists( $this->params['src'], $predicates ) ) {
                        $status->fatal( 'backend-fail-notexists', $this->params['src'] );
                        return $status;
+               // Check if a file can be placed at the destination
+               } elseif ( !$this->backend->isPathUsableInternal( $this->params['dst'] ) ) {
+                       $status->fatal( 'backend-fail-move', $this->params['src'], $this->params['dst'] );
+                       return $status;
                }
                // Check if destination file exists
                $status->merge( $this->precheckDestExistence( $predicates ) );
-               if ( !$status->isOK() ) {
-                       return $status;
+               if ( $status->isOK() ) {
+                       // Update file existence predicates
+                       $predicates['exists'][$this->params['src']] = false;
+                       $predicates['sha1'][$this->params['src']] = false;
+                       $predicates['exists'][$this->params['dst']] = true;
+                       $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                }
-               // Update file existence predicates
-               $predicates['exists'][$this->params['src']] = false;
-               $predicates['sha1'][$this->params['src']] = false;
-               $predicates['exists'][$this->params['dst']] = true;
-               $predicates['sha1'][$this->params['dst']] = $this->sourceSha1;
                return $status; // safe to call attempt()
        }
 
@@ -582,9 +589,6 @@ class MoveFileOp extends FileOp {
                                // Just delete source as the destination needs no changes
                                $params = array( 'src' => $this->params['src'] );
                                $status->merge( $this->backend->deleteInternal( $params ) );
-                               if ( !$status->isOK() ) {
-                                       return $status;
-                               }
                        }
                }
                return $status;
@@ -633,9 +637,6 @@ class DeleteFileOp extends FileOp {
                if ( $this->needsDelete ) {
                        // Delete the source file
                        $status->merge( $this->backend->deleteInternal( $this->params ) );
-                       if ( !$status->isOK() ) {
-                               return $status;
-                       }
                }
                return $status;
        }

diff --git a/includes/filerepo/backend/SwiftFileBackend.php b/includes/filerepo/backend/SwiftFileBackend.php
index a2d9a38..29c0fa8 100644 (file)
--- a/includes/filerepo/backend/SwiftFileBackend.php
+++ b/includes/filerepo/backend/SwiftFileBackend.php
@@ -72,6 +72,27 @@ class SwiftFileBackend extends FileBackend {
                return $relStoragePath;
        }
 
+       /**
+        * @see FileBackend::isPathUsableInternal()
+        */
+       public function isPathUsableInternal( $storagePath ) {
+               list( $container, $rel ) = $this->resolveStoragePathReal( $storagePath );
+               if ( $rel === null ) {
+                       return false; // invalid
+               }
+
+               try {
+                       $this->getContainer( $container );
+                       return true; // container exists
+               } catch ( NoSuchContainerException $e ) {
+               } catch ( InvalidResponseException $e ) {
+               } catch ( Exception $e ) { // some other exception?
+                       $this->logException( $e, __METHOD__, array( 'path' => $storagePath ) );
+               }
+
+               return false;
+       }
+
        /**
         * @see FileBackend::doCopyInternal()
         */
@@ -492,7 +513,7 @@ class SwiftFileBackend extends FileBackend {
                } catch ( NoSuchObjectException $e ) {
                } catch ( InvalidResponseException $e ) {
                } catch ( Exception $e ) { // some other exception?
-                       $this->logException( $e, __METHOD__, $params );
+                       $this->logException( $e, __METHOD__, array( 'cont' => $fullCont, 'dir' => $dir ) );
                }
 
                return $files;