Fix net::FileStream to handle POSIX errors correctly.

net/base/file_stream_context_win.cc

Index: net/base/file_stream_context_win.cc
diff --git a/net/base/file_stream_context_win.cc b/net/base/file_stream_context_win.cc
index a514ab0391682a4cdd140edfe31d7e48d63caf7d..b4bf34b70df2a0193b3b20da03e44cb412e0d4cd 100644
--- a/net/base/file_stream_context_win.cc
+++ b/net/base/file_stream_context_win.cc
@@ -72,9 +72,10 @@ FileStream::Context::~Context() {
 int64 FileStream::Context::GetFileSize() const {
   LARGE_INTEGER file_size;
   if (!GetFileSizeEx(file_, &file_size)) {
-    DWORD error = GetLastError();
-    LOG(WARNING) << "GetFileSizeEx failed: " << error;
-    return RecordAndMapError(error, FILE_ERROR_SOURCE_GET_SIZE);
+    IOResult error = IOResult::FromOSError(GetLastError());
+    LOG(WARNING) << "GetFileSizeEx failed: " << error.os_error;
+    RecordError(error, FILE_ERROR_SOURCE_GET_SIZE);
+    return error.result;
   }
 
   return file_size.QuadPart;
@@ -86,44 +87,38 @@ int FileStream::Context::ReadAsync(IOBuffer* buf,
   DCHECK(!async_in_progress_);
   error_source_ = FILE_ERROR_SOURCE_READ;
 
-  int rv = 0;
-
   DWORD bytes_read;
   if (!ReadFile(file_, buf->data(), buf_len,
                 &bytes_read, &io_context_.overlapped)) {
-    DWORD error = GetLastError();
-    if (error == ERROR_IO_PENDING) {
+    IOResult error = IOResult::FromOSError(GetLastError());
+    if (error.os_error == ERROR_IO_PENDING) {
       IOCompletionIsPending(callback, buf);
-      rv = ERR_IO_PENDING;
-    } else if (error == ERROR_HANDLE_EOF) {
-      rv = 0;  // Report EOF by returning 0 bytes read.
+    } else if (error.os_error == ERROR_HANDLE_EOF) {
+      return 0;  // Report EOF by returning 0 bytes read.
     } else {
-      LOG(WARNING) << "ReadFile failed: " << error;
-      rv = RecordAndMapError(error, FILE_ERROR_SOURCE_READ);
+      LOG(WARNING) << "ReadFile failed: " << error.os_error;
+      RecordError(error, FILE_ERROR_SOURCE_READ);
     }
-  } else {
-    IOCompletionIsPending(callback, buf);
-    rv = ERR_IO_PENDING;
+    return error.result;
   }
-  return rv;
+
+  return bytes_read;

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

Skimming over the MSDN docs, the old behavior looks wrong, but could we do this
in a followup CL?  Think this change is a bit too far removed from the purpose
of this CL.

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

This method would have to be updated either way (for RecordError()), so not
fixing this bug in this CL would not make this CL smaller. I updated CL
description to list all bugs fixed by this CL.

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

It's not about the size, it's about the unrelated change in behavior.  If the
change unexpectedly causes a breakage, then the preferred behavior would be to
revert the entire CL.

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

Yeah, I understand and agree that in general it would be better to have a
separate CL for this. It's just this looked like an obvious bug and it's not
independent of my other changes in this CL.

 }
 
 int FileStream::Context::ReadSync(char* buf, int buf_len) {
-  int rv = 0;
-
   DWORD bytes_read;
   if (!ReadFile(file_, buf, buf_len, &bytes_read, NULL)) {
-    DWORD error = GetLastError();
-    if (error == ERROR_HANDLE_EOF) {
-      rv = 0;  // Report EOF by returning 0 bytes read.
+    IOResult error = IOResult::FromOSError(GetLastError());
+    if (error.os_error == ERROR_HANDLE_EOF) {
+      return 0;  // Report EOF by returning 0 bytes read.
     } else {
-      LOG(WARNING) << "ReadFile failed: " << error;
-      rv = RecordAndMapError(error, FILE_ERROR_SOURCE_READ);
+      LOG(WARNING) << "ReadFile failed: " << error.os_error;
+      RecordError(error, FILE_ERROR_SOURCE_READ);
     }
-  } else {
-    rv = static_cast<int>(bytes_read);
+    return error.result;
   }
-  return rv;
+
+  return bytes_read;
 }
 
 int FileStream::Context::WriteAsync(IOBuffer* buf,
@@ -131,69 +126,67 @@ int FileStream::Context::WriteAsync(IOBuffer* buf,
                                     const CompletionCallback& callback) {
   error_source_ = FILE_ERROR_SOURCE_WRITE;
 
-  int rv = 0;
   DWORD bytes_written = 0;
   if (!WriteFile(file_, buf->data(), buf_len,
                  &bytes_written, &io_context_.overlapped)) {
-    DWORD error = GetLastError();
-    if (error == ERROR_IO_PENDING) {
+    IOResult error = IOResult::FromOSError(GetLastError());
+    if (error.os_error == ERROR_IO_PENDING) {
       IOCompletionIsPending(callback, buf);
-      rv = ERR_IO_PENDING;
     } else {
-      LOG(WARNING) << "WriteFile failed: " << error;
-      rv = RecordAndMapError(error, FILE_ERROR_SOURCE_WRITE);
+      LOG(WARNING) << "WriteFile failed: " << error.os_error;
+      RecordError(error, FILE_ERROR_SOURCE_WRITE);
     }
-  } else {
-    IOCompletionIsPending(callback, buf);
-    rv = ERR_IO_PENDING;
+    return error.result;
   }
-  return rv;
+
+  return bytes_written;

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

See above comment.

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

Same here.

 }
 
 int FileStream::Context::WriteSync(const char* buf, int buf_len) {
-  int rv = 0;
   DWORD bytes_written = 0;
   if (!WriteFile(file_, buf, buf_len, &bytes_written, NULL)) {
-    DWORD error = GetLastError();
-    LOG(WARNING) << "WriteFile failed: " << error;
-    rv = RecordAndMapError(error, FILE_ERROR_SOURCE_WRITE);
-  } else {
-    rv = static_cast<int>(bytes_written);
+    IOResult error = IOResult::FromOSError(GetLastError());
+    LOG(WARNING) << "WriteFile failed: " << error.os_error;
+    RecordError(error, FILE_ERROR_SOURCE_WRITE);
+    return error.result;
   }
-  return rv;
+
+  return bytes_written;
 }
 
 int FileStream::Context::Truncate(int64 bytes) {
-  BOOL result = SetEndOfFile(file_);
-  if (result)
-    return bytes;
+  if (!SetEndOfFile(file_)) {
+    IOResult error = IOResult::FromOSError(GetLastError());
+    LOG(WARNING) << "SetEndOfFile failed: " << error.os_error;
+    RecordError(error, FILE_ERROR_SOURCE_SET_EOF);
+    return error.result;
+  }
 
-  DWORD error = GetLastError();
-  LOG(WARNING) << "SetEndOfFile failed: " << error;
-  return RecordAndMapError(error, FILE_ERROR_SOURCE_SET_EOF);
+  return bytes;
 }
 
 void FileStream::Context::OnAsyncFileOpened() {
   MessageLoopForIO::current()->RegisterIOHandler(file_, this);
 }
 
-int64 FileStream::Context::SeekFileImpl(Whence whence, int64 offset) {
+FileStream::Context::IOResult FileStream::Context::SeekFileImpl(Whence whence,
+                                                                int64 offset) {
   LARGE_INTEGER distance, res;
   distance.QuadPart = offset;
   DWORD move_method = static_cast<DWORD>(whence);
   if (SetFilePointerEx(file_, distance, &res, move_method)) {
     SetOffset(&io_context_.overlapped, res);
-    return res.QuadPart;
+    return IOResult(res.QuadPart, 0);
   }
 
-  return -static_cast<int>(GetLastError());
+  return IOResult::FromOSError(GetLastError());
 }
 
-int64 FileStream::Context::FlushFileImpl() {
+FileStream::Context::IOResult FileStream::Context::FlushFileImpl() {
   if (FlushFileBuffers(file_))
-    return OK;
+    return IOResult(OK, 0);
 
-  return -static_cast<int>(GetLastError());
+  return IOResult::FromOSError(GetLastError());
 }
 
 void FileStream::Context::IOCompletionIsPending(
@@ -220,18 +213,22 @@ void FileStream::Context::OnIOCompleted(MessageLoopForIO::IOContext* context,
     return;
   }
 
-  int result = static_cast<int>(bytes_read);
-  if (error && error != ERROR_HANDLE_EOF)
-    result = RecordAndMapError(error, error_source_);
-
-  if (bytes_read)
+  IOResult result;

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

Don't think we need this here.  Can be scoped within the error block.

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

Done.

+  if (error == ERROR_HANDLE_EOF) {
+    result = IOResult(0, 0);
+  } else if (error) {
+    result = IOResult::FromOSError(error);
+    RecordError(result, error_source_);
+  } else {
+    result = IOResult(bytes_read, 0);
     IncrementOffset(&io_context_.overlapped, bytes_read);
+  }
 
   CompletionCallback temp_callback = callback_;
   callback_.Reset();
   scoped_refptr<IOBuffer> temp_buf = in_flight_buf_;
   in_flight_buf_ = NULL;
-  temp_callback.Run(result);
+  temp_callback.Run(result.result);
 }
 
 }  // namespace net