Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551913004:
arc: Implement ArcContentFileSystemFileStreamReader offset handling (Closed)
|
Created:
4 years ago by hashimoto Modified:
4 years ago Reviewers:
hidehiko CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tzik, hidehiko+watch_chromium.org, nhiroki, lhchavez+watch_chromium.org, oshima+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Implement ArcContentFileSystemFileStreamReader offset handling
Replace net::FileStream with base::File for finer errno handling.
BUG=671510
TEST=unit_tests
Committed: https://crrev.com/1110cd1a1affaaf2b956103ee7fd1b76fc926b11
Cr-Commit-Position: refs/heads/master@{#438473}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : std::unique_ptr<base::File> #
Total comments: 13
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : constexpr #
Messages
Total messages: 28 (16 generated)
Description was changed from ========== Stop using net::FileStream arc: Implement ArcContentFileSystemFileStreamReader offset handling BUG= TEST=unit_tests ========== to ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling BUG=671510 TEST=unit_tests ==========
Description was changed from ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling BUG=671510 TEST=unit_tests ========== to ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling Replace net::FileStream with base::File for finer errno handling. BUG=671510 TEST=unit_tests ==========
hashimoto@chromium.org changed reviewers: + hidehiko@chromium.org
The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for being late. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:18: void OnRead(const net::CompletionCallback& callback, int result) { Is it ok to call |callback| even after ACFSFileStreamReader is deleted? (I guess no). (Sorry, I overlooked in the last review for OnGetFileSize()). Making this a member of the reader class and using weak_ptr shold resolve it. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:29: int ReadFile(base::File* file, Looks potential crash? Isn't it allowed to delete the reader instance while there is inflight operation? If it is, |file| here can be a dangling pointer. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:48: auto* blocking_pool = content::BrowserThread::GetBlockingPool(); Optional: Could you make this a utility function so that task_runner_ can be initialized in initializer list at L47? https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:55: // Post an empty task to destroy |file_| on the blocking pool. Optional: if (file_.IsValid()) guard? https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:149: void ArcContentFileSystemFileStreamReader::ConsumeFileContents( Can this be merge into SeekFile()? My understanding is; - SeekFile() would fail if the fd is for pipe. - Then use read(2) repeatedly until the offset, and discards all the read data. - Both run on task_runner_. Then, something: int SeekFile(base::File* file, int offset) { int result = lseek(...); if (result != ESPIPE) return result; std::vector<char> buf(1024 * 1024); while (offset > 0) { ssize_t r = read(fd, &*buf.begin(), min(buf.size(), offset)); offset -= r; } return 0; } (Skipped any error handling in the example). looks simpler to me. WDYT? https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:158: TEST_F(ArcContentFileSystemFileStreamReaderTest, ReadRegularFile) { Nice test coverage!
PTAL (sorry, I forgot to reupload the same CL after rebase) https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:18: void OnRead(const net::CompletionCallback& callback, int result) { On 2016/12/08 15:36:27, hidehiko wrote: > Is it ok to call |callback| even after ACFSFileStreamReader is deleted? (I guess > no). > (Sorry, I overlooked in the last review for OnGetFileSize()). > > Making this a member of the reader class and using weak_ptr shold resolve it. Good point. Done. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:29: int ReadFile(base::File* file, On 2016/12/08 15:36:27, hidehiko wrote: > Looks potential crash? > Isn't it allowed to delete the reader instance while there is inflight > operation? If it is, |file| here can be a dangling pointer. ArcContentFileSystemFileStreamReader's dtor posts a task to destruct the File object on the task runner which is a SequencedTaskRunner so it is guaranteed that the File object is alive in ReadFile() and SeekFile(). https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:48: auto* blocking_pool = content::BrowserThread::GetBlockingPool(); On 2016/12/08 15:36:28, hidehiko wrote: > Optional: Could you make this a utility function so that task_runner_ can be > initialized in initializer list at L47? To make it easy to read, I slightly prefer having everything in this ctor, rather than putting the blocking pool related code in a separate function. It's still possible to put everything in the initializer list by using a lambda function, or repeating content::BrowserThread::GetBlockingPool() twice, but I find it too verbose. WDYT? https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:55: // Post an empty task to destroy |file_| on the blocking pool. On 2016/12/08 15:36:27, hidehiko wrote: > Optional: if (file_.IsValid()) guard? I see no harm in posting a task even when the file is not valid, so I'd like to avoid having an unnecessary check. That being said, if you spot any potentially serious problem, I'd like to add a check. WDYT? https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:149: void ArcContentFileSystemFileStreamReader::ConsumeFileContents( On 2016/12/08 15:36:27, hidehiko wrote: > Can this be merge into SeekFile()? > My understanding is; > - SeekFile() would fail if the fd is for pipe. > - Then use read(2) repeatedly until the offset, and discards all the read data. > - Both run on task_runner_. > > Then, something: > > int SeekFile(base::File* file, int offset) { > int result = lseek(...); > if (result != ESPIPE) > return result; > > std::vector<char> buf(1024 * 1024); > while (offset > 0) { > ssize_t r = read(fd, &*buf.begin(), min(buf.size(), offset)); > offset -= r; > } > return 0; > } > > (Skipped any error handling in the example). > looks simpler to me. WDYT? The pipe is provided by an Android app and it's not predictable how long it will take to read data. I thought it might be nice to split this into multiple tasks to avoid occupying the worker thread for very long time, but if you find the benefit not worth the complexity, I'll merge it into one function for the sake of simplicity. WDYT? https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:158: TEST_F(ArcContentFileSystemFileStreamReaderTest, ReadRegularFile) { On 2016/12/08 15:36:28, hidehiko wrote: > Nice test coverage! Acknowledged :)
https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:29: int ReadFile(base::File* file, On 2016/12/09 10:35:16, hashimoto wrote: > On 2016/12/08 15:36:27, hidehiko wrote: > > Looks potential crash? > > Isn't it allowed to delete the reader instance while there is inflight > > operation? If it is, |file| here can be a dangling pointer. > > ArcContentFileSystemFileStreamReader's dtor posts a task to destruct the File > object on the task runner which is a SequencedTaskRunner so it is guaranteed > that the File object is alive in ReadFile() and SeekFile(). IIUC, regardless of PostTask in dtor, |file_| member pointed by |file| will be destroyed immediately (even if the fd is still alive, which is passed to another base::File object in PostTask). So, if the reader is destroyed before file->ReadAtCurrentPosNoBestEffort (or lseek operation below), it touches invalid memory. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:48: auto* blocking_pool = content::BrowserThread::GetBlockingPool(); On 2016/12/09 10:35:16, hashimoto wrote: > On 2016/12/08 15:36:28, hidehiko wrote: > > Optional: Could you make this a utility function so that task_runner_ can be > > initialized in initializer list at L47? > > To make it easy to read, I slightly prefer having everything in this ctor, > rather than putting the blocking pool related code in a separate function. > It's still possible to put everything in the initializer list by using a lambda > function, or repeating content::BrowserThread::GetBlockingPool() twice, but I > find it too verbose. > WDYT? I'm ok with the current code. If we can initialize the variable in initializer list, an additional benefit would be that we can make the var "const", IIRC, which is preferred in */arc/* files. For inlining the initialization in ctor, AFAIK, lambda sounds a bit overkill to me, and it looks rare use cases in Chromium. Calling GetBlockingPool() twice sounds more Chromium code, but indeed it may be verbose... https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:55: // Post an empty task to destroy |file_| on the blocking pool. On 2016/12/09 10:35:16, hashimoto wrote: > On 2016/12/08 15:36:27, hidehiko wrote: > > Optional: if (file_.IsValid()) guard? > > I see no harm in posting a task even when the file is not valid, so I'd like to > avoid having an unnecessary check. > That being said, if you spot any potentially serious problem, I'd like to add a > check. > WDYT? SG. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:149: void ArcContentFileSystemFileStreamReader::ConsumeFileContents( On 2016/12/09 10:35:16, hashimoto wrote: > On 2016/12/08 15:36:27, hidehiko wrote: > > Can this be merge into SeekFile()? > > My understanding is; > > - SeekFile() would fail if the fd is for pipe. > > - Then use read(2) repeatedly until the offset, and discards all the read > data. > > - Both run on task_runner_. > > > > Then, something: > > > > int SeekFile(base::File* file, int offset) { > > int result = lseek(...); > > if (result != ESPIPE) > > return result; > > > > std::vector<char> buf(1024 * 1024); > > while (offset > 0) { > > ssize_t r = read(fd, &*buf.begin(), min(buf.size(), offset)); > > offset -= r; > > } > > return 0; > > } > > > > (Skipped any error handling in the example). > > looks simpler to me. WDYT? > > The pipe is provided by an Android app and it's not predictable how long it will > take to read data. > I thought it might be nice to split this into multiple tasks to avoid occupying > the worker thread for very long time, but if you find the benefit not worth the > complexity, I'll merge it into one function for the sake of simplicity. > WDYT? The given PIPE is blocking, right? If so, and if the data is not enough provided yet, anyway the Read*() would block at the point. If this is true, PostTask loop sounds less beneficial to me? Otherwise (if the pipe is non-blocking) EAGAIN needs to be handled. https://codereview.chromium.org/2551913004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:92: callback.Run(result < 0 ? net::ERR_FAILED : result); Optional: this is not a regression so I'm fine, but you may be interested in setting error code other than ERR_FAILED? If so, you may want to introduce a utility OSErrorToNetError, because it can be shared with L150.
PTAL https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:29: int ReadFile(base::File* file, On 2016/12/12 01:54:26, hidehiko wrote: > On 2016/12/09 10:35:16, hashimoto wrote: > > On 2016/12/08 15:36:27, hidehiko wrote: > > > Looks potential crash? > > > Isn't it allowed to delete the reader instance while there is inflight > > > operation? If it is, |file| here can be a dangling pointer. > > > > ArcContentFileSystemFileStreamReader's dtor posts a task to destruct the File > > object on the task runner which is a SequencedTaskRunner so it is guaranteed > > that the File object is alive in ReadFile() and SeekFile(). > > IIUC, regardless of PostTask in dtor, |file_| member pointed by |file| will be > destroyed immediately (even if the fd is still alive, which is passed to another > base::File object in PostTask). > So, if the reader is destroyed before file->ReadAtCurrentPosNoBestEffort (or > lseek operation below), it touches invalid memory. Ugh, that's right. Seems I'm not sufficiently familiar with std::move yet. Fixed. https://codereview.chromium.org/2551913004/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:149: void ArcContentFileSystemFileStreamReader::ConsumeFileContents( On 2016/12/12 01:54:26, hidehiko wrote: > On 2016/12/09 10:35:16, hashimoto wrote: > > On 2016/12/08 15:36:27, hidehiko wrote: > > > Can this be merge into SeekFile()? > > > My understanding is; > > > - SeekFile() would fail if the fd is for pipe. > > > - Then use read(2) repeatedly until the offset, and discards all the read > > data. > > > - Both run on task_runner_. > > > > > > Then, something: > > > > > > int SeekFile(base::File* file, int offset) { > > > int result = lseek(...); > > > if (result != ESPIPE) > > > return result; > > > > > > std::vector<char> buf(1024 * 1024); > > > while (offset > 0) { > > > ssize_t r = read(fd, &*buf.begin(), min(buf.size(), offset)); > > > offset -= r; > > > } > > > return 0; > > > } > > > > > > (Skipped any error handling in the example). > > > looks simpler to me. WDYT? > > > > The pipe is provided by an Android app and it's not predictable how long it > will > > take to read data. > > I thought it might be nice to split this into multiple tasks to avoid > occupying > > the worker thread for very long time, but if you find the benefit not worth > the > > complexity, I'll merge it into one function for the sake of simplicity. > > WDYT? > > The given PIPE is blocking, right? If so, and if the data is not enough provided > yet, anyway the Read*() would block at the point. If this is true, PostTask loop > sounds less beneficial to me? > Otherwise (if the pipe is non-blocking) EAGAIN needs to be handled. Badly(or maliciously)-written Android app can block the pipe forever, that's true (filed crbug.com/673222 for this). However, if the Android app is written reasonably, I think it's better to avoid occupying the worker thread for long. For example, when reading the last 100MB of a 1GB file, it can take a certain amount of time to download the first 900MB. IMHO, blocking the thread for the entire 900MB is totally different from allowing other tasks to run whenever each read() syscall returns. https://codereview.chromium.org/2551913004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:92: callback.Run(result < 0 ? net::ERR_FAILED : result); On 2016/12/12 01:54:26, hidehiko wrote: > Optional: this is not a regression so I'm fine, but you may be interested in > setting error code other than ERR_FAILED? If so, you may want to introduce a > utility OSErrorToNetError, because it can be shared with L150. base/files/file.h says base::File::Read() and its relatives don't return error code other than -1 when failing. Returning errno from ReadFile() might be useful, but not sure if it's worth doing (and it makes assumption about base::File::Read()'s implementation).
The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost looks good. My main comment is L119's. Others are minor. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:29: off_t result = lseek(file->GetPlatformFile(), offset, SEEK_SET); # Ugrrr... Sorry I overlooked in previous iterations. File::Seek() is what you need here? (To be consistent with above ReadAtCurrentPosNoBestEffort())? Or if this is for returning an errno, please comment so (E.g.; Use lseek instead of File::Seek here because ...) Also: #include <sys/types.h> #include <unistd.h> for lseek. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:46: task_runner_->PostTask( Could you revive the comment that this is to avoid race condition in case that there is inflight operation? https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:80: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Optional: How about adding: DCHECK(file_); DCHECK(file_->IsValid()); considering the following comment? https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:109: Optional: How about adding DCHECK(!file_); just in case Read() is (wrongly) called while there is inflight operation. Then, test may be able to capture it. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:119: LOG(ERROR) << "Invalid file."; file_.reset(); looks missing? With the last PS change, it looks assumed that whether file_ is evaluated to true and whether file_->IsValid() is evaluated to true are equivalent? Could you add DCHECK then? Or alternatively, you may want to fix L55 to look at file_->IsValid() condition. If file_->IsValid() is false, ReadInternal cannot be called because it (eventually) calls ReadAtCurrentPosNoBestEffort() which assumes file_->IsValid() is true. Note: for first approach, I assume that you'd like to retry to open the file, while for the second approach, I assume that you'd like to return an error once it fails to open the file. Probably it's better to document your policy. At the interface level, there seems no policy about calling Read() after another Read() returning an error. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:171: base::PostTaskAndReplyWithResult( Could you add TODO that this may block the blocking thread if data is not provided with linking to your filed bug?
https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:29: off_t result = lseek(file->GetPlatformFile(), offset, SEEK_SET); On 2016/12/12 14:53:01, hidehiko wrote: > # Ugrrr... Sorry I overlooked in previous iterations. > > File::Seek() is what you need here? (To be consistent with above > ReadAtCurrentPosNoBestEffort())? > > Or if this is for returning an errno, please comment so (E.g.; Use lseek instead > of File::Seek here because ...) > > Also: > #include <sys/types.h> > #include <unistd.h> > for lseek. File::Seek() makes no guarantee about the state of errno so it should be avoided. Added a comment. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:46: task_runner_->PostTask( On 2016/12/12 14:53:01, hidehiko wrote: > Could you revive the comment that this is to avoid race condition in case that > there is inflight operation? Done. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:80: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2016/12/12 14:53:01, hidehiko wrote: > Optional: How about adding: > > DCHECK(file_); > DCHECK(file_->IsValid()); > > considering the following comment? Done. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:109: On 2016/12/12 14:53:01, hidehiko wrote: > Optional: How about adding > > DCHECK(!file_); > > just in case Read() is (wrongly) called while there is inflight operation. Then, > test may be able to capture it. We can capture the said mistake only when the second Read() call was made before the fist Read() call reaches OnOpenFile(). That being said, having DCHECKs should be helpful for developers reading this code for the first time. Added DCHECKs about the state of |file_| to a number of places. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:119: LOG(ERROR) << "Invalid file."; On 2016/12/12 14:53:01, hidehiko wrote: > file_.reset(); looks missing? > > With the last PS change, it looks assumed that whether file_ is evaluated to > true and whether file_->IsValid() is evaluated to true are equivalent? Could you > add DCHECK then? > > Or alternatively, you may want to fix L55 to look at file_->IsValid() condition. > If file_->IsValid() is false, ReadInternal cannot be called because it > (eventually) calls ReadAtCurrentPosNoBestEffort() which assumes file_->IsValid() > is true. > > Note: for first approach, I assume that you'd like to retry to open the file, > while for the second approach, I assume that you'd like to return an error once > it fails to open the file. Probably it's better to document your policy. At the > interface level, there seems no policy about calling Read() after another Read() > returning an error. IIUC the result is undefined if any method of a FileStreamReader is called after an error is returned (i.e. dtor is the only method which is expected to work correctly in an error state). In other words, we don't have to clean up the half-constructed inner state of this object when returning an error. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:171: base::PostTaskAndReplyWithResult( On 2016/12/12 14:53:01, hidehiko wrote: > Could you add TODO that this may block the blocking thread if data is not > provided with linking to your filed bug? Done.
LGTM with minor comments. https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc (right): https://codereview.chromium.org/2551913004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:109: On 2016/12/14 08:41:29, hashimoto wrote: > On 2016/12/12 14:53:01, hidehiko wrote: > > Optional: How about adding > > > > DCHECK(!file_); > > > > just in case Read() is (wrongly) called while there is inflight operation. > Then, > > test may be able to capture it. > We can capture the said mistake only when the second Read() call was made before > the fist Read() call reaches OnOpenFile(). > That being said, having DCHECKs should be helpful for developers reading this > code for the first time. > Added DCHECKs about the state of |file_| to a number of places. Newly added DCHECKs makes the code more easier to read. Thank you for doing it. https://codereview.chromium.org/2551913004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2551913004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:172: const size_t kOffset = 10; constexpr? https://codereview.chromium.org/2551913004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:190: const size_t kOffset = 10; ditto.
https://codereview.chromium.org/2551913004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/2551913004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:172: const size_t kOffset = 10; On 2016/12/14 08:50:43, hidehiko wrote: > constexpr? Done. https://codereview.chromium.org/2551913004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:190: const size_t kOffset = 10; On 2016/12/14 08:50:43, hidehiko wrote: > ditto. Done.
The CQ bit was checked by hashimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2551913004/#ps80001 (title: "constexpr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481705856584420,
"parent_rev": "314df2ddec75e727246e123ccbdcc0131c9d082f", "commit_rev":
"6bb8dbe357649002198a29bf4c417899a38d526b"}
Message was sent while issue was closed.
Description was changed from ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling Replace net::FileStream with base::File for finer errno handling. BUG=671510 TEST=unit_tests ========== to ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling Replace net::FileStream with base::File for finer errno handling. BUG=671510 TEST=unit_tests Review-Url: https://codereview.chromium.org/2551913004 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling Replace net::FileStream with base::File for finer errno handling. BUG=671510 TEST=unit_tests Review-Url: https://codereview.chromium.org/2551913004 ========== to ========== arc: Implement ArcContentFileSystemFileStreamReader offset handling Replace net::FileStream with base::File for finer errno handling. BUG=671510 TEST=unit_tests Committed: https://crrev.com/1110cd1a1affaaf2b956103ee7fd1b76fc926b11 Cr-Commit-Position: refs/heads/master@{#438473} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1110cd1a1affaaf2b956103ee7fd1b76fc926b11 Cr-Commit-Position: refs/heads/master@{#438473} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
