Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1025553006:
Fix error handling of CopyFileFromLocal and remove unnecessary PendingRequestDone. (Closed)
|
Created:
5 years, 9 months ago by yawano Modified:
5 years, 9 months ago Reviewers:
Lei Zhang CC:
chromium-reviews, Lei Zhang, tommycli, Greg Billock, vandebo (ex-Chrome) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix error handling of CopyFileFromLocal and remove unnecessary PendingRequestDone.
Error codes which CopyInForeignFile(CopyFileFromLocal) returns are defined in the definition of async_file_util.h.
BUG=413541
TEST=manually tested.
Committed: https://crrev.com/bc1b5cbc3ad7704bbac6c93105a803706250d826
Cr-Commit-Position: refs/heads/master@{#322522}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fix #
Total comments: 2
Patch Set 3 : Fix error handling for src_path is directory case. #
Total comments: 2
Patch Set 4 : Update comment. #
Messages
Total messages: 13 (3 generated)
yawano@chromium.org changed reviewers: + thestig@chromium.org
Extracted the other fixes from crrev.com/1030523002. PTAL. Thank you!
https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:296: int file_descriptor = open(file_path, flags); 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. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:896: if (open_fd_result.second == EACCES) I think you want ENOENT. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1195: void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToCopyFileFromLocal( OnDidGetDestFileInfoToCopyFileFromLocal https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1203: error_callback.Run(base::File::FILE_ERROR_FAILED); 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. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1215: error_callback.Run(error); add a return and remove the else statement.
Thank you for the review! PTAL. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:296: int file_descriptor = open(file_path, flags); On 2015/03/24 08:49:02, Lei Zhang wrote: > 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. Done. I added check for ENOTDIR of open(). https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:896: if (open_fd_result.second == EACCES) On 2015/03/24 08:49:02, Lei Zhang wrote: > I think you want ENOENT. Done. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1195: void MTPDeviceDelegateImplLinux::OnDidGetFileInfoToCopyFileFromLocal( On 2015/03/24 08:49:02, Lei Zhang wrote: > OnDidGetDestFileInfoToCopyFileFromLocal Done. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1203: error_callback.Run(base::File::FILE_ERROR_FAILED); On 2015/03/24 08:49:02, Lei Zhang wrote: > 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. 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. https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:1215: error_callback.Run(error); On 2015/03/24 08:49:02, Lei Zhang wrote: > add a return and remove the else statement. Done.
https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:296: int file_descriptor = open(file_path, flags); On 2015/03/25 05:01:32, yawano wrote: > On 2015/03/24 08:49:02, Lei Zhang wrote: > > 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. > > Done. I added check for ENOTDIR of open(). 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); https://codereview.chromium.org/1025553006/diff/20001/chrome/browser/media_ga... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h (right): https://codereview.chromium.org/1025553006/diff/20001/chrome/browser/media_ga... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:245: // Called when GetFileInfo() for destination path succeeded to copy file from ... succeeded for a CopyFileFromLocal operation.
PTAL. Thank you! https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/1025553006/diff/1/chrome/browser/media_galler... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:296: int file_descriptor = open(file_path, flags); On 2015/03/27 00:08:19, Lei Zhang wrote: > On 2015/03/25 05:01:32, yawano wrote: > > On 2015/03/24 08:49:02, Lei Zhang wrote: > > > 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. > > > > Done. I added check for ENOTDIR of open(). > > 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); Yes, ENOTDIR is not enough. Done. Thank you! https://codereview.chromium.org/1025553006/diff/20001/chrome/browser/media_ga... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h (right): https://codereview.chromium.org/1025553006/diff/20001/chrome/browser/media_ga... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:245: // Called when GetFileInfo() for destination path succeeded to copy file from On 2015/03/27 00:08:19, Lei Zhang wrote: > ... succeeded for a CopyFileFromLocal operation. Done.
lgtm https://codereview.chromium.org/1025553006/diff/40001/chrome/browser/media_ga... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/1025553006/diff/40001/chrome/browser/media_ga... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:292: // second is errno if it failed. nit: update comment.
Thank you! https://codereview.chromium.org/1025553006/diff/40001/chrome/browser/media_ga... File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc (right): https://codereview.chromium.org/1025553006/diff/40001/chrome/browser/media_ga... chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:292: // second is errno if it failed. On 2015/03/27 01:55:32, Lei Zhang wrote: > nit: update comment. Done.
The CQ bit was checked by yawano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1025553006/#ps60001 (title: "Update comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1025553006/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bc1b5cbc3ad7704bbac6c93105a803706250d826 Cr-Commit-Position: refs/heads/master@{#322522} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
