Fix net::FileStream to handle POSIX errors correctly.

net/base/file_stream_context.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 "net/base/file_stream_context.h"
 
 #include "base/location.h"
 #include "base/message_loop_proxy.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/worker_pool.h"
 #include "net/base/file_stream_net_log_parameters.h"
 #include "net/base/net_errors.h"
 
 namespace {
 
 void CallInt64ToInt(const net::CompletionCallback& callback, int64 result) {
   callback.Run(static_cast<int>(result));
 }
 
 }
 
 namespace net {
 
+FileStream::Context::IOResult::IOResult()
+    : result(0),

[mmenke, 2013/02/22 16:24:59]

Maybe result(OK)?

[Sergey Ulanov, 2013/02/22 21:04:06]

Done.

+      os_error(0) {
+}
+
+FileStream::Context::IOResult::IOResult(int64 result, int os_error)
+    : result(result),
+      os_error(os_error) {
+}
+
+// static
+FileStream::Context::IOResult FileStream::Context::IOResult::FromOSError(
+    int64 os_error) {
+  return IOResult(MapSystemError(os_error), os_error);
+}
+
+FileStream::Context::OpenResult::OpenResult(base::PlatformFile file,
+                                            IOResult error_code)
+    : file(file),
+      error_code(error_code) {
+}
+
+FileStream::Context::OpenResult::OpenResult()
+    : file(base::kInvalidPlatformFileValue) {
+}
+
 void FileStream::Context::Orphan() {
   DCHECK(!orphaned_);
 
   orphaned_ = true;
   if (file_ != base::kInvalidPlatformFileValue)
     bound_net_log_.EndEvent(NetLog::TYPE_FILE_STREAM_OPEN);
 
   if (!async_in_progress_) {
     CloseAndDelete();
   } else if (file_ != base::kInvalidPlatformFileValue) {
@@ skipping 20 matching lines @@
   async_in_progress_ = true;
 }
 
 int FileStream::Context::OpenSync(const base::FilePath& path, int open_flags) {
   DCHECK(!async_in_progress_);
 
   BeginOpenEvent(path);
   OpenResult result = OpenFileImpl(path, open_flags);
   file_ = result.file;
   if (file_ == base::kInvalidPlatformFileValue) {
-    result.error_code = ProcessOpenError(result.error_code);
+    ProcessOpenError(result.error_code);
   } else {
     // TODO(satorux): Remove this once all async clients are migrated to use
     // Open(). crbug.com/114783
     if (open_flags & base::PLATFORM_FILE_ASYNC)
       OnAsyncFileOpened();
   }
-  return result.error_code;
+  return result.error_code.result;
 }
 
 void FileStream::Context::CloseSync() {
   DCHECK(!async_in_progress_);
   if (file_ != base::kInvalidPlatformFileValue) {
     base::ClosePlatformFile(file_);
     file_ = base::kInvalidPlatformFileValue;
     bound_net_log_.EndEvent(NetLog::TYPE_FILE_STREAM_OPEN);
   }
 }
 
 void FileStream::Context::SeekAsync(Whence whence,
                                     int64 offset,
                                     const Int64CompletionCallback& callback) {
   DCHECK(!async_in_progress_);
 
   const bool posted = base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true /* task is slow */),
       FROM_HERE,
       base::Bind(&Context::SeekFileImpl,
                  base::Unretained(this), whence, offset),
       base::Bind(&Context::ProcessAsyncResult,
                  base::Unretained(this), callback, FILE_ERROR_SOURCE_SEEK));
   DCHECK(posted);
 
   async_in_progress_ = true;
 }
 
 int64 FileStream::Context::SeekSync(Whence whence, int64 offset) {
-  int64 result = SeekFileImpl(whence, offset);
-  CheckForIOError(&result, FILE_ERROR_SOURCE_SEEK);
-  return result;
+  IOResult result = SeekFileImpl(whence, offset);
+  RecordError(result, FILE_ERROR_SOURCE_SEEK);
+  return result.result;
 }
 
 void FileStream::Context::FlushAsync(const CompletionCallback& callback) {
   DCHECK(!async_in_progress_);
 
   const bool posted = base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true /* task is slow */),
       FROM_HERE,
       base::Bind(&Context::FlushFileImpl,
                  base::Unretained(this)),
       base::Bind(&Context::ProcessAsyncResult,
                  base::Unretained(this), IntToInt64(callback),
                  FILE_ERROR_SOURCE_FLUSH));
   DCHECK(posted);
 
   async_in_progress_ = true;
 }
 
 int FileStream::Context::FlushSync() {
-  int64 result = FlushFileImpl();
-  CheckForIOError(&result, FILE_ERROR_SOURCE_FLUSH);
-  return result;
+  IOResult result = FlushFileImpl();
+  RecordError(result, FILE_ERROR_SOURCE_FLUSH);
+  return result.result;
 }
 
-int FileStream::Context::RecordAndMapError(int error,
-                                           FileErrorSource source) const {
+void FileStream::Context::RecordError(const IOResult& result,
+                                      FileErrorSource source) const {
+  if (result.result >= 0) {
+    // |result| is not an error.
+    return;
+  }
+
   // The following check is against incorrect use or bug. File descriptor
   // shouldn't ever be closed outside of FileStream while it still tries to do
   // something with it.
-  DCHECK(error != ERROR_BAD_FILE);
-  Error net_error = MapSystemError(error);
+  DCHECK_NE(result.result, ERR_INVALID_HANDLE);
 
   if (!orphaned_) {
-    bound_net_log_.AddEvent(NetLog::TYPE_FILE_STREAM_ERROR,
-                            base::Bind(&NetLogFileStreamErrorCallback,
-                                       source, error, net_error));
+    bound_net_log_.AddEvent(
+        NetLog::TYPE_FILE_STREAM_ERROR,
+        base::Bind(&NetLogFileStreamErrorCallback,
+                   source, result.os_error,
+                   static_cast<net::Error>(result.result)));
   }
-  RecordFileError(error, source, record_uma_);
-  return net_error;
+
+  RecordFileError(result.os_error, source, record_uma_);
 }
 
 void FileStream::Context::BeginOpenEvent(const base::FilePath& path) {
   std::string file_name = path.AsUTF8Unsafe();
   bound_net_log_.BeginEvent(NetLog::TYPE_FILE_STREAM_OPEN,
                             NetLog::StringCallback("file_name", &file_name));
 }
 
 FileStream::Context::OpenResult FileStream::Context::OpenFileImpl(
     const base::FilePath& path, int open_flags) {
   // FileStream::Context actually closes the file asynchronously, independently
   // from FileStream's destructor. It can cause problems for users wanting to
   // delete the file right after FileStream deletion. Thus we are always
   // adding SHARE_DELETE flag to accommodate such use case.
   open_flags |= base::PLATFORM_FILE_SHARE_DELETE;
-  OpenResult result;
-  result.error_code = OK;
-  result.file = base::CreatePlatformFile(path, open_flags, NULL, NULL);
-  if (result.file == base::kInvalidPlatformFileValue)
-    result.error_code = GetLastErrno();
+  base::PlatformFile file =
+      base::CreatePlatformFile(path, open_flags, NULL, NULL);
+  if (file == base::kInvalidPlatformFileValue)
+    return OpenResult(file, IOResult::FromOSError(GetLastErrno()));
 
-  return result;
+  return OpenResult(file, IOResult(0, 0));

[mmenke, 2013/02/22 16:24:59]

Are the default constructors used anywhere?  If not, suggest getting rid of them
and replacing the first 0 with OK.  If so, suggest just using the default
constructor here.

[Sergey Ulanov, 2013/02/22 21:04:06]

They are used in base::PostTaskAndReplyWithResult() (see
base/task_runner_util.h), and so this code wouldn't compile without default
constructors :(

[mmenke, 2013/02/22 21:13:17]

Still suggest using IOResult(OK, 0), or even just IOResult().

[Sergey Ulanov, 2013/02/22 21:51:47]

Done.

 }
 
-int FileStream::Context::ProcessOpenError(int error_code) {
+void FileStream::Context::ProcessOpenError(const IOResult& error_code) {
   bound_net_log_.EndEvent(NetLog::TYPE_FILE_STREAM_OPEN);
-  return RecordAndMapError(error_code, FILE_ERROR_SOURCE_OPEN);
+  RecordError(error_code, FILE_ERROR_SOURCE_OPEN);
 }
 
 void FileStream::Context::OnOpenCompleted(const CompletionCallback& callback,
                                           OpenResult result) {
   file_ = result.file;
   if (file_ == base::kInvalidPlatformFileValue)
-    result.error_code = ProcessOpenError(result.error_code);
+    ProcessOpenError(result.error_code);
   else if (!orphaned_)
     OnAsyncFileOpened();
-  OnAsyncCompleted(IntToInt64(callback), result.error_code);
+  OnAsyncCompleted(IntToInt64(callback), result.error_code.result);

[mmenke, 2013/02/22 16:24:59]

Hmm...  This looks a little weird, though suppose it's better than
result.result.result.  No suggestions on better naming scheme.

[Sergey Ulanov, 2013/02/22 21:04:06]

Renamed |result| to |open_result| in this function - IMHO it's a bit more
readable this way.

 }
 
 void FileStream::Context::CloseAndDelete() {
   DCHECK(!async_in_progress_);
 
   if (file_ == base::kInvalidPlatformFileValue) {
     delete this;
   } else {
     const bool posted = base::WorkerPool::PostTaskAndReply(
         FROM_HERE,
         base::Bind(base::IgnoreResult(&base::ClosePlatformFile), file_),
         base::Bind(&Context::OnCloseCompleted, base::Unretained(this)),
         true /* task_is_slow */);
     DCHECK(posted);
     file_ = base::kInvalidPlatformFileValue;
   }
 }
 
 void FileStream::Context::OnCloseCompleted() {
   delete this;
 }
 
 Int64CompletionCallback FileStream::Context::IntToInt64(
     const CompletionCallback& callback) {
   return base::Bind(&CallInt64ToInt, callback);
 }
 
-void FileStream::Context::CheckForIOError(int64* result,
-                                          FileErrorSource source) {
-  if (*result < 0)
-    *result = RecordAndMapError(static_cast<int>(*result), source);
-}
-
 void FileStream::Context::ProcessAsyncResult(
     const Int64CompletionCallback& callback,
     FileErrorSource source,
-    int64 result) {
-  CheckForIOError(&result, source);
-  OnAsyncCompleted(callback, result);
+    const IOResult& result) {
+  RecordError(result, source);
+  OnAsyncCompleted(callback, result.result);
 }
 
 void FileStream::Context::OnAsyncCompleted(
     const Int64CompletionCallback& callback,
     int64 result) {
   // Reset this before Run() as Run() may issue a new async operation. Also it
   // should be reset before CloseAsync() because it shouldn't run if any async
   // operation is in progress.
   async_in_progress_ = false;
   if (orphaned_)
     CloseAndDelete();
   else
     callback.Run(result);
 }
 
 }  // namespace net