Fix error handling of CopyFileFromLocal and remove unnecessary PendingRequestDone.

chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h"
 
 #include <fcntl.h>
 #include <algorithm>
 #include <vector>
 
@@ skipping 269 matching lines @@
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   MTPDeviceTaskHelper* task_helper =
       GetDeviceTaskHelperForStorage(storage_name, read_only);
   if (!task_helper)
     return;
   task_helper->CloseStorage();
   MTPDeviceTaskHelperMapService::GetInstance()->DestroyDeviceTaskHelper(
       storage_name, read_only);
 }
 
-// Opens |file_path| with |flags|.
-int OpenFileDescriptor(const char* file_path, const int flags) {
+// Opens |file_path| with |flags|. Returns the result as a pair.
+// first is file descriptor.
+// second is errno if it failed.
+std::pair<int, int> OpenFileDescriptor(const char* file_path, const int flags) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
 
-  return open(file_path, flags);
+  int file_descriptor = open(file_path, flags);

[Lei Zhang, 2015/03/24 08:49:02]

Before you open the file, you also need to make sure it's not a directory.

It's missing from the CopyInForeignFile() comments in async_file_util.h, but
it's there for CopyFileLocal():

- File::FILE_ERROR_NOT_A_FILE if |src_url| exists but is not a file.

[yawano, 2015/03/25 05:01:32]

Done. I added check for ENOTDIR of open().

[Lei Zhang, 2015/03/27 00:08:19]

I don't think that does the same thing. Let's say |file_path| is
/path/to/file...

If /path/to is a file, then open() sets errno to ENOTDIR.
If /path/to/file is a directory, open() will return a valid file descriptor.

I think what you want here is:

if (base::DirectoryExists(base::FilePath(file_path)))
  return std::make_pair(-1, base::File::FILE_ERROR_NOT_A_FILE);
int fd = open(...);
if (fd >= 0)
  return std::make_pair(fd, base::file::FILE_ERROR_OK);
if (errno == ENOENT)
  return std::make_pair(fd, base::file::FILE_ERROR_NOT_FOUND);
return std::make_pair(fd, base::file::FILE_ERROR_FAIL);

[yawano, 2015/03/27 01:53:00]

Yes, ENOTDIR is not enough. Done. Thank you!

+  return std::make_pair(file_descriptor, file_descriptor < 0 ? errno : 0);
 }
 
 // Closes |file_descriptor| on file thread.
 void CloseFileDescriptor(const int file_descriptor) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
 
   IGNORE_EINTR(close(file_descriptor));
 }
 
 // Deletes a temporary file |file_path|.
@@ skipping 312 matching lines @@
 
 void MTPDeviceDelegateImplLinux::CopyFileFromLocal(
     const base::FilePath& source_file_path,
     const base::FilePath& device_file_path,
     const CopyFileFromLocalSuccessCallback& success_callback,
     const ErrorCallback& error_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(!source_file_path.empty());
   DCHECK(!device_file_path.empty());
 
-  content::BrowserThread::PostTaskAndReplyWithResult(
-      content::BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(&OpenFileDescriptor,
-                 source_file_path.value().c_str(),
-                 O_RDONLY),
-      base::Bind(&MTPDeviceDelegateImplLinux::CopyFileFromLocalInternal,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 device_file_path,
-                 success_callback,
-                 error_callback));
+  // Get file info of destination file path.
+  const GetFileInfoSuccessCallback success_callback_wrapper = base::Bind(
+      &MTPDeviceDelegateImplLinux::OnDidGetFileInfoToCopyFileFromLocal,
+      weak_ptr_factory_.GetWeakPtr(), error_callback);
+  const ErrorCallback error_callback_wrapper = base::Bind(
+      &MTPDeviceDelegateImplLinux::OnGetFileInfoErrorToCopyFileFromLocal,
+      weak_ptr_factory_.GetWeakPtr(), source_file_path, device_file_path,
+      success_callback, error_callback);
+  const base::Closure closure =
+      base::Bind(&MTPDeviceDelegateImplLinux::GetFileInfoInternal,
+                 weak_ptr_factory_.GetWeakPtr(), device_file_path,
+                 success_callback_wrapper, error_callback_wrapper);
+  EnsureInitAndRunTask(PendingTaskInfo(
+      device_file_path, content::BrowserThread::IO, FROM_HERE, closure));
 }
 
 void MTPDeviceDelegateImplLinux::DeleteFile(
     const base::FilePath& file_path,
     const DeleteFileSuccessCallback& success_callback,
     const ErrorCallback& error_callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(!file_path.empty());
 
   const GetFileInfoSuccessCallback& success_callback_wrapper =
@@ skipping 186 matching lines @@
     const base::FilePath& source_file_path,
     const base::FilePath& device_file_path,
     const CreateTemporaryFileCallback& create_temporary_file_callback,
     const MoveFileLocalSuccessCallback& success_callback,
     const ErrorCallback& error_callback,
     const base::File::Info& source_file_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (source_file_info.is_directory) {
     error_callback.Run(base::File::FILE_ERROR_NOT_A_FILE);
-    PendingRequestDone();
     return;
   }
 
   if (source_file_path.DirName() == device_file_path.DirName()) {
     // If a file is moved in a same directory, rename the file.
     uint32 file_id;
     if (CachedPathToId(source_file_path, &file_id)) {
       const MTPDeviceTaskHelper::RenameObjectSuccessCallback
           success_callback_wrapper = base::Bind(
               &MTPDeviceDelegateImplLinux::OnDidMoveFileLocalWithRename,
@@ skipping 15 matching lines @@
     // destination path, and remove source file.
     const CopyFileLocalSuccessCallback& success_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::DeleteFileInternal,
                    weak_ptr_factory_.GetWeakPtr(), source_file_path,
                    success_callback, error_callback, source_file_info);
     CopyFileLocal(source_file_path, device_file_path,
                   create_temporary_file_callback,
                   base::Bind(&FakeCopyFileProgressCallback),
                   success_callback_wrapper, error_callback);
   }
-
-  PendingRequestDone();
 }
 
-void MTPDeviceDelegateImplLinux::CopyFileFromLocalInternal(
+void MTPDeviceDelegateImplLinux::OnDidOpenFDToCopyFileFromLocal(
     const base::FilePath& device_file_path,
     const CopyFileFromLocalSuccessCallback& success_callback,
     const ErrorCallback& error_callback,
-    const int source_file_descriptor) {
+    const std::pair<int, int>& open_fd_result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
+  int source_file_descriptor = open_fd_result.first;
   if (source_file_descriptor < 0) {
-    error_callback.Run(base::File::FILE_ERROR_INVALID_OPERATION);
-    PendingRequestDone();
+    if (open_fd_result.second == EACCES)

[Lei Zhang, 2015/03/24 08:49:02]

I think you want ENOENT.

[yawano, 2015/03/25 05:01:32]

Done.

+      error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    else
+      error_callback.Run(base::File::FILE_ERROR_FAILED);
+
     return;
   }
 
   uint32 parent_id;
   if (CachedPathToId(device_file_path.DirName(), &parent_id)) {
     CopyFileFromLocalSuccessCallback success_callback_wrapper =
         base::Bind(&MTPDeviceDelegateImplLinux::OnDidCopyFileFromLocal,
                    weak_ptr_factory_.GetWeakPtr(), success_callback,
                    source_file_descriptor);
 
     ErrorCallback error_callback_wrapper = base::Bind(
         &MTPDeviceDelegateImplLinux::HandleCopyFileFromLocalError,
         weak_ptr_factory_.GetWeakPtr(), error_callback, source_file_descriptor);
 
     base::Closure closure = base::Bind(&CopyFileFromLocalOnUIThread,
                                        storage_name_,
                                        read_only_,
                                        source_file_descriptor,
                                        parent_id,
                                        device_file_path.BaseName().value(),
                                        success_callback_wrapper,
                                        error_callback_wrapper);
 
     EnsureInitAndRunTask(PendingTaskInfo(
         base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
   } else {
     HandleCopyFileFromLocalError(error_callback, source_file_descriptor,
-                                 base::File::FILE_ERROR_INVALID_OPERATION);
+                                 base::File::FILE_ERROR_NOT_FOUND);
   }
-
-  PendingRequestDone();
 }
 
 void MTPDeviceDelegateImplLinux::DeleteFileInternal(
     const base::FilePath& file_path,
     const DeleteFileSuccessCallback& success_callback,
     const ErrorCallback& error_callback,
     const base::File::Info& file_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (file_info.is_directory) {
     error_callback.Run(base::File::FILE_ERROR_NOT_A_FILE);
   } else {
     uint32 file_id;
     if (CachedPathToId(file_path, &file_id))
       RunDeleteObjectOnUIThread(file_id, success_callback, error_callback);
     else
       error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
   }
-
-  PendingRequestDone();
 }
 
 void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
     const base::FilePath& file_path,
     const DeleteDirectorySuccessCallback& success_callback,
     const ErrorCallback& error_callback,
     const base::File::Info& file_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (!file_info.is_directory) {
     error_callback.Run(base::File::FILE_ERROR_NOT_A_DIRECTORY);
-    PendingRequestDone();
     return;
   }
 
   uint32 directory_id;
   if (!CachedPathToId(file_path, &directory_id)) {
     error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
-    PendingRequestDone();
     return;
   }
 
   // Checks the cache first. If it has children in cache, the directory cannot
   // be empty.
   FileIdToMTPFileNodeMap::const_iterator it =
       file_id_to_node_map_.find(directory_id);
   if (it != file_id_to_node_map_.end() && it->second->HasChildren()) {
     error_callback.Run(base::File::FILE_ERROR_NOT_EMPTY);
-    PendingRequestDone();
     return;
   }
 
   // Since the directory can contain a file even if the cache returns it as
   // empty, read the directory and confirm the directory is actually empty.
   const MTPDeviceTaskHelper::ReadDirectorySuccessCallback
       success_callback_wrapper = base::Bind(
           &MTPDeviceDelegateImplLinux::OnDidReadDirectoryToDeleteDirectory,
           weak_ptr_factory_.GetWeakPtr(), directory_id, success_callback,
           error_callback);
   const MTPDeviceTaskHelper::ErrorCallback error_callback_wrapper =
       base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
                  weak_ptr_factory_.GetWeakPtr(), error_callback, directory_id);
   const base::Closure closure = base::Bind(
       &ReadDirectoryOnUIThread, storage_name_, read_only_, directory_id,
       1 /* max_size */, success_callback_wrapper, error_callback_wrapper);
   EnsureInitAndRunTask(PendingTaskInfo(
       base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
-  PendingRequestDone();
 }
 
 void MTPDeviceDelegateImplLinux::OnDidReadDirectoryToDeleteDirectory(
     const uint32 directory_id,
     const DeleteDirectorySuccessCallback& success_callback,
     const ErrorCallback& error_callback,
     const storage::AsyncFileUtil::EntryList& entries,
     const bool has_more) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(!has_more);
@@ skipping 183 matching lines @@
 
   current_snapshot_request_info_.reset(snapshot_request_info.release());
   if (file_info.size == 0) {
     // Empty snapshot file.
     return OnDidWriteDataIntoSnapshotFile(
         snapshot_file_info, current_snapshot_request_info_->snapshot_file_path);
   }
   WriteDataIntoSnapshotFile(snapshot_file_info);
 }
 
+void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToCopyFileFromLocal(

[Lei Zhang, 2015/03/24 08:49:02]

OnDidGetDestFileInfoToCopyFileFromLocal

[yawano, 2015/03/25 05:01:32]

Done.

+    const ErrorCallback& error_callback,
+    const base::File::Info& file_info) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (file_info.is_directory)
+    error_callback.Run(base::File::FILE_ERROR_INVALID_OPERATION);
+  else
+    error_callback.Run(base::File::FILE_ERROR_FAILED);

[Lei Zhang, 2015/03/24 08:49:02]

Are you sure this is an error case? If the destination exists, do we overwrite
it, or fail? NativeFileUtil::CopyOrMoveFile()'s treatment of its |dest_path|
proceeds with the copy/move if the file is found.

[yawano, 2015/03/25 05:01:32]

I also checked the implementation for Drive, it also overwrites a file when it
exists.

However, since whether an implementation should overwrite an existing file or
not is not defined in the interface of async_file_util.h, IMHO, it's okay to
return an error for overwriting an existing file.

+}
+
+void MTPDeviceDelegateImplLinux::OnGetFileInfoErrorToCopyFileFromLocal(
+    const base::FilePath& source_file_path,
+    const base::FilePath& device_file_path,
+    const CopyFileFromLocalSuccessCallback& success_callback,
+    const ErrorCallback& error_callback,
+    const base::File::Error error) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (error != base::File::FILE_ERROR_NOT_FOUND) {
+    error_callback.Run(error);

[Lei Zhang, 2015/03/24 08:49:02]

add a return and remove the else statement.

[yawano, 2015/03/25 05:01:32]

Done.

+  } else {
+    content::BrowserThread::PostTaskAndReplyWithResult(
+        content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&OpenFileDescriptor, source_file_path.value().c_str(),
+                   O_RDONLY),
+        base::Bind(&MTPDeviceDelegateImplLinux::OnDidOpenFDToCopyFileFromLocal,
+                   weak_ptr_factory_.GetWeakPtr(), device_file_path,
+                   success_callback, error_callback));
+  }
+}
+
 void MTPDeviceDelegateImplLinux::OnDidReadDirectory(
     uint32 dir_id,
     const ReadDirectorySuccessCallback& success_callback,
     const storage::AsyncFileUtil::EntryList& file_list,
     bool has_more) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   FileIdToMTPFileNodeMap::iterator it = file_id_to_node_map_.find(dir_id);
   DCHECK(it != file_id_to_node_map_.end());
   MTPFileNode* dir_node = it->second;
@@ skipping 311 matching lines @@
   }
 }
 
 void CreateMTPDeviceAsyncDelegate(
     const std::string& device_location,
     const bool read_only,
     const CreateMTPDeviceAsyncDelegateCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   callback.Run(new MTPDeviceDelegateImplLinux(device_location, read_only));
 }