Cross platform low level file IO wrappers

Cross platform low level file IO wrappers

Rename fd_io to file_io, and ReadFD to ReadFile, etc.

file_io.cc is the higher level versions that call the basic ReadFile/WriteFile
and then file_io_posix.cc and file_io_win.cc are the implementations of
those functions.

The Windows path is as yet untested, lacking the ability to link the test binary.

R=cpu@chromium.org, mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/10165ce449849509b71687890e4ac32b1e5366af

[scottmg, 2014-12-16 19:22:17]

Patchset #6 (id:100001) has been deleted

[scottmg, 2014-12-16 20:28:23]

Patchset #7 (id:140001) has been deleted

[scottmg, 2014-12-16 20:32:14]

Patchset #10 (id:220001) has been deleted

[scottmg, 2014-12-16 20:32:18]

Patchset #9 (id:200001) has been deleted

[scottmg, 2014-12-16 20:32:21]

Patchset #8 (id:180001) has been deleted

[scottmg, 2014-12-16 21:20:07]

Patchset #8 (id:240001) has been deleted

[scottmg, 2014-12-16 21:29:45]

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io.cc
File util/file/file_io.cc (right):

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io.cc#ne...
util/file/file_io.cc:26: PLOG(ERROR) << "read";
The error messages being "read" is a small drawback, but seems better than
duplication.

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io_win.c...
util/file/file_io_win.cc:1: // Copyright 2014 The Crashpad Authors. All rights
reserved.
I couldn't find a --similarity that got this to choose the correct base.

[Mark Mentovai, 2014-12-16 22:40:26]

Cool!

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io.cc
File util/file/file_io.cc (right):

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io.cc#ne...
util/file/file_io.cc:26: PLOG(ERROR) << "read";
scottmg wrote:
> The error messages being "read" is a small drawback, but seems better than
> duplication.

One thing we could do is have something like const char* read_call_name and
const char* write_call_name. Those could either be local to this file with
#ifdefs or they could be declared (in namespace crashpad::internal) in file_io.h
with the actual definitions in the .cc files.

But since this is just for logging, and “read” and “write” aren’t really even
that wrong for the Windows case, I’m also OK just leaving the names alone.

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io_win.c...
util/file/file_io_win.cc:1: // Copyright 2014 The Crashpad Authors. All rights
reserved.
scottmg wrote:
> I couldn't find a --similarity that got this to choose the correct base.

Don’t sweat it.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc
File util/file/file_io.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc#ne...
util/file/file_io.cc:22: bool LoggingReadFile(FileHandle fh, void* buffer,
size_t size) {
Throughout this file: the header declared the parameter name as file, not fh.
The implementation and declaration should match.

I like “file” better. Fewer uncommon abbreviations.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc#ne...
util/file/file_io.cc:22: bool LoggingReadFile(FileHandle fh, void* buffer,
size_t size) {
I’m really glad that all of these wrappers are shareable!

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc#ne...
util/file/file_io.cc:60: void CheckedReadFileAtEOF(FileHandle fh) {
Especially now that we’ve got multiple implementations, this can probably
benefit from a test to make sure that things like this function have the same
semantics.

It’s OK to skip that in this change, but one of us should circle back and add a
test afterwards.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h#new...
util/file/file_io.h:34: //! \brief Reads a file, retrying when interrupted on
POSIX.
This comment is confusing, because it sounds like it reads the entire contents
of a file. Change it to “Reads from a file”?

Same on line 49 (“Writes to a file”).

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h#new...
util/file/file_io.h:34: //! \brief Reads a file, retrying when interrupted on
POSIX.
This comment used to mention short reads. It should at least continue to mention
them for POSIX. Same on line 49 for WriteFile().

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h#new...
util/file/file_io.h:40: //!     error, with `errno` or `GetLastError` set
appropriately. On error, a
Sorry to be pedantic about silly things like this, but the Crashpad comment
style uses () on function names: `GetLastError()`. Same on line 55.

The Doxygen-generated docs show up at
http://docs.crashpad.googlecode.com/git/doxygen/index.html and it’s nice for
them to be consistent in style.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_posix.cc
File util/file/file_io_posix.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_posix...
util/file/file_io_posix.cc:69: ssize_t ReadFile(int fd, void* buffer, size_t
size) {
This is now “FileHandle file”, not “int fd”. Same on lines 73 and 77.

It’s easier to match things up between the .h and .cc when these all agree.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:24: ssize_t ReadFile(HANDLE file, void* buffer, size_t
size) {
FileHandle, not HANDLE. Same on lines 33 and 44.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:27: BOOL rv = ::ReadFile(file, buffer, size_dword,
&bytes_read, NULL);
Crashpad is 100% nullptr now. Same on line 38.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:27: BOOL rv = ::ReadFile(file, buffer, size_dword,
&bytes_read, NULL);
Do ReadFile() and WriteFile() guarantee not to do a short read or write when
doing blocking I/O and not at EOF? Even on a network stream or a networked file?

Based on the docs, I think that short reads and writes are possible at least on
pipes. You left a TODO for something similar in WriteFile(), but I think that
ReadFile() might be affected too. It at least deserves a comment, even if you
aren’t ready to implement it. (And in that case, the functions would probably
become involved enough that the traitsy thing that the POSIX implementation does
would be advantageous.)

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:45: BOOL rv = ::CloseHandle(file);
The :: aren’t necessary on this one.

https://codereview.chromium.org/811823003/diff/270001/util/mach/scoped_task_s...
File util/mach/scoped_task_suspend_test.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/mach/scoped_task_s...
util/mach/scoped_task_suspend_test.cc:20: #include "util/file/file_io.h"
Looks like this #include is vestigial, and we can just remove it.

[scottmg, 2014-12-16 23:47:17]

Patchset #10 (id:290001) has been deleted

[scottmg, 2014-12-16 23:56:37]

Patchset #10 (id:310001) has been deleted

[scottmg, 2014-12-17 00:01:42]

Patchset #10 (id:330001) has been deleted

[scottmg, 2014-12-17 00:17:24]

Patchset #10 (id:350001) has been deleted

[scottmg, 2014-12-17 00:19:30]

Patchset #10 (id:370001) has been deleted

[scottmg, 2014-12-17 00:22:24]

Patchset #10 (id:170020) has been deleted

[scottmg, 2014-12-17 00:22:58]

Thanks!

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io.cc
File util/file/file_io.cc (right):

https://codereview.chromium.org/811823003/diff/260001/util/file/file_io.cc#ne...
util/file/file_io.cc:26: PLOG(ERROR) << "read";
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> scottmg wrote:
> > The error messages being "read" is a small drawback, but seems better than
> > duplication.
> 
> One thing we could do is have something like const char* read_call_name and
> const char* write_call_name. Those could either be local to this file with
> #ifdefs or they could be declared (in namespace crashpad::internal) in
file_io.h
> with the actual definitions in the .cc files.
> 
> But since this is just for logging, and “read”&nbsp;and “write”&nbsp;aren’t
really even
> that wrong for the Windows case, I’m also OK just leaving the names alone.

I think I'll leave them, the extra complexity of another symbol doesn't seem
worthwhile.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc
File util/file/file_io.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc#ne...
util/file/file_io.cc:22: bool LoggingReadFile(FileHandle fh, void* buffer,
size_t size) {
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> Throughout this file: the header declared the parameter name as file, not fh.
> The implementation and declaration should match.
> 
> I like “file” better. Fewer uncommon abbreviations.

Done.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.cc#ne...
util/file/file_io.cc:60: void CheckedReadFileAtEOF(FileHandle fh) {
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> Especially now that we’ve got multiple implementations, this can probably
> benefit from a test to make sure that things like this function have the same
> semantics.
> 
> It’s OK to skip that in this change, but one of us should circle back and add
a
> test afterwards.

Agreed. I'll skip for now and add once I can actually test on Windows.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h#new...
util/file/file_io.h:34: //! \brief Reads a file, retrying when interrupted on
POSIX.
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> This comment used to mention short reads. It should at least continue to
mention
> them for POSIX. Same on line 49 for WriteFile().

Done.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h#new...
util/file/file_io.h:34: //! \brief Reads a file, retrying when interrupted on
POSIX.
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> This comment is confusing, because it sounds like it reads the entire contents
> of a file. Change it to “Reads from a file”?
> 
> Same on line 49 (“Writes to a file”).

Done.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io.h#new...
util/file/file_io.h:40: //!     error, with `errno` or `GetLastError` set
appropriately. On error, a
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> Sorry to be pedantic about silly things like this, but the Crashpad comment
> style uses () on function names: `GetLastError()`. Same on line 55.
> 
> The Doxygen-generated docs show up at
> http://docs.crashpad.googlecode.com/git/doxygen/index.html and it’s nice for
> them to be consistent in style.

Done.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_posix.cc
File util/file/file_io_posix.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_posix...
util/file/file_io_posix.cc:69: ssize_t ReadFile(int fd, void* buffer, size_t
size) {
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> This is now “FileHandle file”, not “int fd”. Same on lines 73 and 77.
> 
> It’s easier to match things up between the .h and .cc when these all agree.

Done. (I left the implementation ones above to emphasize their concreteness.)

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:24: ssize_t ReadFile(HANDLE file, void* buffer, size_t
size) {
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> FileHandle, not HANDLE. Same on lines 33 and 44.

Done.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:27: BOOL rv = ::ReadFile(file, buffer, size_dword,
&bytes_read, NULL);
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> Crashpad is 100% nullptr now. Same on line 38.

Done.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:27: BOOL rv = ::ReadFile(file, buffer, size_dword,
&bytes_read, NULL);
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> Do ReadFile() and WriteFile() guarantee not to do a short read or write when
> doing blocking I/O and not at EOF? Even on a network stream or a networked
file?
> 
> Based on the docs, I think that short reads and writes are possible at least
on
> pipes. You left a TODO for something similar in WriteFile(), but I think that
> ReadFile() might be affected too. It at least deserves a comment, even if you
> aren’t ready to implement it. (And in that case, the functions would probably
> become involved enough that the traitsy thing that the POSIX implementation
does
> would be advantageous.)

I think you're right. From my reading of the docs, I believe ReadFile can read
short when reading from a pipe, but WriteFile simply blocks when used in
synchronous mode. Because of this difference, I didn't switch to traits.

The comment in WriteFile is still there and refers to an unrelated limit when
writing to a networked pipe on XP -- writes must be chunked to < 32M or else
they'll just fail.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_win.c...
util/file/file_io_win.cc:45: BOOL rv = ::CloseHandle(file);
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> The :: aren’t necessary on this one.

Done.

Habit to imply "this is a Win32 function", applied inconsistently in Chromium.
Either way is fine with me.

https://codereview.chromium.org/811823003/diff/270001/util/mach/scoped_task_s...
File util/mach/scoped_task_suspend_test.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/mach/scoped_task_s...
util/mach/scoped_task_suspend_test.cc:20: #include "util/file/file_io.h"
On 2014/12/16 22:40:25, Mark Mentovai wrote:
> Looks like this #include is vestigial, and we can just remove it.

Done.

[Mark Mentovai, 2014-12-17 01:05:24]

Nice.

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_posix.cc
File util/file/file_io_posix.cc (right):

https://codereview.chromium.org/811823003/diff/270001/util/file/file_io_posix...
util/file/file_io_posix.cc:69: ssize_t ReadFile(int fd, void* buffer, size_t
size) {
scottmg wrote:
> On 2014/12/16 22:40:25, Mark Mentovai wrote:
> > This is now “FileHandle file”, not “int fd”. Same on lines 73 and 77.
> > 
> > It’s easier to match things up between the .h and .cc when these all agree.
> 
> Done. (I left the implementation ones above to emphasize their concreteness.)

Perfect.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io.h#new...
util/file/file_io.h:68: //!     `ReadFile` return values, if the underlying
ReadFile() fails, or if
ReadFile(), not `ReadFile`. Same on lines 81, 92, 104.

The guideline with the documentation is to use `backticks` for things outside of
Crashpad, because Doxygen can’t generate docs for those, and they wouldn’t
otherwise be set off in the generated docs. For things inside Crashpad, the docs
will automatically link correctly, so we don’t also need backticks to force a
monospaced display.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io.h#new...
util/file/file_io.h:123: //! \brief Wraps `close()`, logging an error if the
operation fails.
“`close()` or `CloseHandle()`”? Line 129 too, and maybe clean up line 131 by
just saying “the underlying function.”

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:30: success = ::ReadFile(file, buffer, size_dword,
&bytes_read, nullptr);
On the second pass, buffer needs to point beyond its beginning by total_read
bytes, and size_dword needs to be decreased by total_read bytes. Take a look at
the trio of increments/decrements that the POSIX implementation does.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:31: if (!success && GetLastError() != ERROR_MORE_DATA)
ReadFile()’s docs say that you’ll get ERROR_MORE_DATA for a pipe read in message
mode. What if it’s a stream-type pipe? I’m not sure if you’ll get an error or if
you’ll get success with fewer bytes_read than you asked for. I haven’t found
anything online that really answers the question. We should probably just test
it and see, that should be easy enough.

Learning a whole lot about Windows IPC today!

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:43: BOOL rv = ::WriteFile(file, buffer, size_dword,
&bytes_written, nullptr);
WriteFile()’s docs aren’t 100% convincing about short writes. They say that it
can return when “A read operation releases buffer space on the read end of the
pipe (if the write was blocked).” It’s not very explicit about what happens if
the pipe is full and the reader frees up space by reading less than the writer
is trying to write.

http://blogs.msdn.com/b/oldnewthing/archive/2011/07/07/10183884.aspx clears it
up, and it says that WriteFile() will keep on blocking until it’s able to put
everything into the pipe. Partial blocking writes can’t happen. I think we can
trust Raymond Chen on this…

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:46: return bytes_written;
…so let’s make it explicit and [D]CHECK_EQ(bytes_written, size_dword) before
returning here.

[scottmg, 2014-12-17 01:50:28]

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io.h#new...
util/file/file_io.h:68: //!     `ReadFile` return values, if the underlying
ReadFile() fails, or if
On 2014/12/17 01:05:24, Mark Mentovai wrote:
> ReadFile(), not `ReadFile`. Same on lines 81, 92, 104.
> 
> The guideline with the documentation is to use `backticks` for things outside
of
> Crashpad, because Doxygen can’t generate docs for those, and they wouldn’t
> otherwise be set off in the generated docs. For things inside Crashpad, the
docs
> will automatically link correctly, so we don’t also need backticks to force a
> monospaced display.

Ah, I see. Done.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io.h#new...
util/file/file_io.h:123: //! \brief Wraps `close()`, logging an error if the
operation fails.
On 2014/12/17 01:05:24, Mark Mentovai wrote:
> “`close()` or `CloseHandle()`”? Line 129 too, and maybe clean up line 131 by
> just saying “the underlying function.”

Done.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:30: success = ::ReadFile(file, buffer, size_dword,
&bytes_read, nullptr);
On 2014/12/17 01:05:24, Mark Mentovai wrote:
> On the second pass, buffer needs to point beyond its beginning by total_read
> bytes, and size_dword needs to be decreased by total_read bytes. Take a look
at
> the trio of increments/decrements that the POSIX implementation does.

Oh boy! Hopefully tests would have saved me eventually. Done.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:31: if (!success && GetLastError() != ERROR_MORE_DATA)
On 2014/12/17 01:05:24, Mark Mentovai wrote:
> ReadFile()’s docs say that you’ll get ERROR_MORE_DATA for a pipe read in
message
> mode. What if it’s a stream-type pipe? I’m not sure if you’ll get an error or
if
> you’ll get success with fewer bytes_read than you asked for. I haven’t found
> anything online that really answers the question. We should probably just test
> it and see, that should be easy enough.

Raymond gets upset when Windows programmers poke and infer as "documented
behaviour". :) More seriously, it's the sort of thing that tends to vary from
XP-to-recent, so testing can be tricky.

From
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365605%28v=vs.85%29...

"""
To create the pipe handle in byte-read mode, specify PIPE_READMODE_BYTE or use
the default value. Data is read from the pipe as a stream of bytes. A read
operation is completed successfully when all available bytes in the pipe are
read or when the specified number of bytes is read.
"""

So in that mode, "all available bytes in the pipe are read" sounds like success
with fewer bytes than requested. So, the modified loop style that matches the
POSIX one should handle this case.

ReadFile docs mention a success case for 0 bytes read:

"""
If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a
pipe, the other end of the pipe called the WriteFile function with
nNumberOfBytesToWrite set to zero.
"""

That doesn't correspond to a complete read or EOF, so I'll leave that as
continuing to loop for now, rather than returning.


> 
> Learning a whole lot about Windows IPC today!

Maybe we can do some COM tomorrow!

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:43: BOOL rv = ::WriteFile(file, buffer, size_dword,
&bytes_written, nullptr);
On 2014/12/17 01:05:24, Mark Mentovai wrote:
> WriteFile()’s docs aren’t 100% convincing about short writes. They say that it
> can return when “A read operation releases buffer space on the read end of the
> pipe (if the write was blocked).” It’s not very explicit about what happens if
> the pipe is full and the reader frees up space by reading less than the writer
> is trying to write.
> 
> http://blogs.msdn.com/b/oldnewthing/archive/2011/07/07/10183884.aspx clears it
> up, and it says that WriteFile() will keep on blocking until it’s able to put
> everything into the pipe. Partial blocking writes can’t happen. I think we can
> trust Raymond Chen on this…

SG. The link above is also clear that it cannot succeed until all bytes are
written.

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:46: return bytes_written;
On 2014/12/17 01:05:24, Mark Mentovai wrote:
> …so let’s make it explicit and [D]CHECK_EQ(bytes_written, size_dword) before
> returning here.

Done.

[Mark Mentovai, 2014-12-17 05:39:16]

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/400001/util/file/file_io_win.c...
util/file/file_io_win.cc:31: if (!success && GetLastError() != ERROR_MORE_DATA)
scottmg wrote:
> Maybe we can do some COM tomorrow!

I can’t wait!

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io.h#new...
util/file/file_io.h:132: //! If the underlying functions fails, this function
causes execution to
“function fails” or “functions fail”

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io_win.c...
util/file/file_io_win.cc:30: BOOL success = ::ReadFile(file, buffer_c,
size_dword, &bytes_read, nullptr);
If EOF is supposed to be signaled by ReadFile() returning true and setting
bytes_read to 0, then this loop won’t ever terminate when encountering EOF
before the buffer is filled. It seems like that could happen when trying to read
a 0-byte or 1-byte file with a 4096-byte buffer. Maybe it can also happen when
reading from a named pipe and the writer closes it? The doc says that you get
ERROR_BROKEN_PIPE when the writer closes an anonymous pipe, but it doesn’t call
out named pipes for this. And it looks like ERROR_HANDLE_EOF only happens with
an asynchronous read.

I don’t know if that can be distinguished from the case you mentioned from the
doc, reading from a pipe that had a 0-byte write done to it.

[scottmg, 2014-12-17 20:10:18]

Patchset #12 (id:440001) has been deleted

[scottmg, 2014-12-17 20:18:33]

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io.h#new...
util/file/file_io.h:132: //! If the underlying functions fails, this function
causes execution to
On 2014/12/17 05:39:16, Mark Mentovai wrote:
> “function fails” or “functions fail”

Done.

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io_win.cc
File util/file/file_io_win.cc (right):

https://codereview.chromium.org/811823003/diff/420001/util/file/file_io_win.c...
util/file/file_io_win.cc:30: BOOL success = ::ReadFile(file, buffer_c,
size_dword, &bytes_read, nullptr);
On 2014/12/17 05:39:16, Mark Mentovai wrote:
> If EOF is supposed to be signaled by ReadFile() returning true and setting
> bytes_read to 0, then this loop won’t ever terminate when encountering EOF
> before the buffer is filled. It seems like that could happen when trying to
read
> a 0-byte or 1-byte file with a 4096-byte buffer.

Yes, you're right. (I confirmed this, which I should have done before.) So a 0
byte read success on a file will break now.

> Maybe it can also happen when
> reading from a named pipe and the writer closes it? The doc says that you get
> ERROR_BROKEN_PIPE when the writer closes an anonymous pipe, but it doesn’t
call
> out named pipes for this.

I can't find documentation that explicitly says that, but the example here for a
named pipe does
http://msdn.microsoft.com/en-ca/library/windows/desktop/aa365588(v=vs.85).aspx
handle ERROR_BROKEN_PIPE. Also, from
http://msdn.microsoft.com/en-ca/library/windows/desktop/aa365141(v=vs.85).aspx

"""
Anonymous pipes are implemented using a named pipe with a unique name.
Therefore, you can often pass a handle to an anonymous pipe to a function that
requires a handle to a named pipe.
"""

So I'm fairly confident it will cause a legitimate failure.

> And it looks like ERROR_HANDLE_EOF only happens with
> an asynchronous read.
> 
> I don’t know if that can be distinguished from the case you mentioned from the
> doc, reading from a pipe that had a 0-byte write done to it.

It doesn't seem like it based on just the return values. Making 0 bytes == EOF
only when GetFileType(file) == FILE_TYPE_PIPE seems close, but the documentation
for that http://msdn.microsoft.com/en-us/library/aa364960%28VS.85%29.aspx
includes "socket" in the list of things that are FILE_TYPE_PIPE. Looking at the
WinSock docs, I'm starting to think that maybe a fully generic Read isn't a
great idea on Windows!
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740522(v=vs.85).aspx .
I'm also not sure what 0 bytes read successfully from a socket should mean.

Another option would be to call GetNamedPipeInfo
http://msdn.microsoft.com/en-ca/library/windows/desktop/aa365445%28v=vs.85%29...
which should fail for non-pipes.

Given that the socket docs suggest that it is a poor idea to use
ReadFile/WriteFile on sockets in preference for WSARecv/WSASend, and that
passing a SOCKET to a function requiring a HANDLE requires a reinterpret_cast,
I've added the GetFileType(file) == FILE_TYPE_PIPE to the EOF case, as well as a
DCHECK that the handle in question is not a socket based on the above checks.

[scottmg, 2014-12-17 20:22:45]

scottmg@chromium.org changed reviewers:
+ cpu@chromium.org

[scottmg, 2014-12-17 20:22:46]

+cpu too. Trying to write a equivalent to POSIX "read from fd" in
util/file/file_io_win.cc ReadFile. Would you mind taking a look? This function
only does synchronous reads.

It will primarily be for files and pipes, I guess. Is there a better way to do a
generic read? Are there other HANDLE types/situations that you know need
additional cases?

[Mark Mentovai, 2014-12-17 20:24:47]

Wow. I definitely learned a lot here. Thanks for sticking with it, and for all
of the explanations.

LGTM. This seems right now according to all of the documentation.

[scottmg, 2014-12-17 20:37:30]

On 2014/12/17 20:24:47, Mark Mentovai wrote:
> Wow. I definitely learned a lot here.

Me too!

> Thanks for sticking with it, and for all
> of the explanations.
> 
> LGTM. This seems right now according to all of the documentation.

Hopefully it works when we get to actually running it too.

I'll wait for Carlos to weigh in on our sanity before landing.

[cpu_(ooo_6.6-7.5), 2014-12-17 21:58:10]

I haven't read the code yet so I don't know how relevant is trivia below but
here it goes:

we can't (in general) use anonymous pipes because they don't set the QoS flags
appropriately so from one of the two sides you can slurp the token of the other
side which is devastating security wise.

The other comment is that message mode is superior to byte mode but we not use
it because other platforms seem not to have it.

[cpu_(ooo_6.6-7.5), 2014-12-17 22:10:17]

looks fine, I only looked at the new files (A+)

My big question is don't we have read / write files in the base/ we have in
crashpad?

Anyhow lgtm.

https://codereview.chromium.org/811823003/diff/460001/util/file/file_io.h
File util/file/file_io.h (right):

https://codereview.chromium.org/811823003/diff/460001/util/file/file_io.h#new...
util/file/file_io.h:17: 
does windows have <sys/types.h> ?

[scottmg, 2014-12-17 22:28:59]

Thanks

On 2014/12/17 22:10:17, cpu wrote:
> looks fine, I only looked at the new files (A+)
> 
> My big question is don't we have read / write files in the base/ we have in
> crashpad?

We do, the idea here was mostly to mirror the POSIX version that's already in
crashpad. It's possible we should consider pulling base/files/ and writing to
that API instead.

> 
> Anyhow lgtm.
> 
> https://codereview.chromium.org/811823003/diff/460001/util/file/file_io.h
> File util/file/file_io.h (right):
> 
>
https://codereview.chromium.org/811823003/diff/460001/util/file/file_io.h#new...
> util/file/file_io.h:17: 
> does windows have <sys/types.h> ?

crashpad fakes it in compat/win.
https://chromium.googlesource.com/crashpad/crashpad/+/master/compat/win/sys

[scottmg, 2014-12-17 22:35:23]

Committed patchset #12 (id:460001) manually as
10165ce449849509b71687890e4ac32b1e5366af (presubmit successful).

[Mark Mentovai, 2014-12-17 22:39:38]

base/files didn’t have an interface that fit what I needed too well. That’s how
we wound up with file_io in Crashpad.