Fix error handling of CopyFileFromLocal and remove unnecessary PendingRequestDone.

chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc

Index: chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
diff --git a/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc b/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
index b14461db62a7e6d33a6b4acaa36a58e7b5616b1b..9f0032119f2c4142a32628c3bc2fd9da00a80e14 100644
--- a/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
+++ b/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
@@ -287,11 +287,14 @@ void CloseStorageAndDestroyTaskHelperOnUIThread(
       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.
@@ -624,17 +627,20 @@ void MTPDeviceDelegateImplLinux::CopyFileFromLocal(
   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(
@@ -841,7 +847,6 @@ void MTPDeviceDelegateImplLinux::MoveFileLocalInternal(
 
   if (source_file_info.is_directory) {
     error_callback.Run(base::File::FILE_ERROR_NOT_A_FILE);
-    PendingRequestDone();
     return;
   }
 
@@ -877,20 +882,22 @@ void MTPDeviceDelegateImplLinux::MoveFileLocalInternal(
                   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;
   }
 
@@ -918,10 +925,8 @@ void MTPDeviceDelegateImplLinux::CopyFileFromLocalInternal(
         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(
@@ -940,8 +945,6 @@ void MTPDeviceDelegateImplLinux::DeleteFileInternal(
     else
       error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
   }
-
-  PendingRequestDone();
 }
 
 void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
@@ -953,14 +956,12 @@ void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
 
   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;
   }
 
@@ -970,7 +971,6 @@ void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
       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;
   }
 
@@ -989,7 +989,6 @@ void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
       1 /* max_size */, success_callback_wrapper, error_callback_wrapper);
   EnsureInitAndRunTask(PendingTaskInfo(
       base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
-  PendingRequestDone();
 }
 
 void MTPDeviceDelegateImplLinux::OnDidReadDirectoryToDeleteDirectory(
@@ -1193,6 +1192,38 @@ void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToCreateSnapshotFile(
   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,