base::ReadFile() should return the number of read bytes on Windows.

base::ReadFile() should return the number of read bytes on Windows.

base::ReadFile() has inconsistent behavior between Linux/Mac and Windows.
When the passed |size| argument is larger than file's size, ReadFile() on Linux/Mac returns the number of read bytes, but ReadFile() on Windows returns -1.

We can workaround this behavior by calling GetFileSize() first, but there are already many clients which treat |size| as max size (They're passing the size of buffer).
I think we should make behavior on Windows be consistent with Linux/Mac.

BUG=243885
TEST=run base_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264768

[fukino, 2014-04-17 13:12:18]

Hi Mark,
Could you take a look?

[Mark Mentovai, 2014-04-17 16:21:57]

LGTM to your change, but consider this additional improvement.

https://codereview.chromium.org/240893002/diff/40001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/240893002/diff/40001/base/file_util_posix.cc#...
base/file_util_posix.cc:660: ssize_t bytes_read = HANDLE_EINTR(read(fd, data,
max_size));
I kinda feel like if this was a short read but EOF is not indicated, the read
should be retried until the buffer is full or the file is at EOF. We could do
this with ReadFromFD, but ReadFromFD doesn’t distinguish errors from EOF in its
return value.

I know it seems silly because this function looks like it works with regular
files (it accepts a filename argument that it opens, not an FD), and regular
files don’t usually result in short reads. Be mindful that the path may actually
name a special file (like a named pipe or a device file) that read short, and
even something that appears as a regular file may be on a filesystem that can
read short, such as a network filesystem or something like procfs.

https://codereview.chromium.org/240893002/diff/40001/base/file_util_posix.cc#...
base/file_util_posix.cc:661: if (int ret = IGNORE_EINTR(close(fd)) < 0)
You don’t need to track |ret| here, you can just

  if (IGNORE_EINTR(close(fd)) < 0)
    return -1;

https://codereview.chromium.org/240893002/diff/40001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/240893002/diff/40001/base/file_util_unittest....
base/file_util_unittest.cc:1964: TEST_F(FileUtilTest, ReadFile) {
Good test!

https://codereview.chromium.org/240893002/diff/40001/base/file_util_unittest....
base/file_util_unittest.cc:1973: // Make buffer with various size.
buffers, plural.

https://codereview.chromium.org/240893002/diff/40001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/240893002/diff/40001/base/file_util_win.cc#ne...
base/file_util_win.cc:587: if (::ReadFile(file, data, max_size, &read, NULL))
The same concern about short non-EOF reads that I mentioned in the POSIX file
applies here.

[fukino, 2014-04-18 02:22:12]

https://codereview.chromium.org/240893002/diff/40001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

https://codereview.chromium.org/240893002/diff/40001/base/file_util_posix.cc#...
base/file_util_posix.cc:660: ssize_t bytes_read = HANDLE_EINTR(read(fd, data,
max_size));
On 2014/04/17 16:21:57, Mark Mentovai wrote:
> I kinda feel like if this was a short read but EOF is not indicated, the read
> should be retried until the buffer is full or the file is at EOF. We could do
> this with ReadFromFD, but ReadFromFD doesn’t distinguish errors from EOF in
its
> return value.
> 
> I know it seems silly because this function looks like it works with regular
> files (it accepts a filename argument that it opens, not an FD), and regular
> files don’t usually result in short reads. Be mindful that the path may
actually
> name a special file (like a named pipe or a device file) that read short, and
> even something that appears as a regular file may be on a filesystem that can
> read short, such as a network filesystem or something like procfs.

Thank you for suggestion!
I didn't realize any cases you mentioned.
It's good lesson to me, but I can't start implementing because I don't have
Windows environment for now.
I'm going to commit this CL without improvement, so I filed this improvement as
http://crbug.com/364762 not to forget it.

https://codereview.chromium.org/240893002/diff/40001/base/file_util_posix.cc#...
base/file_util_posix.cc:661: if (int ret = IGNORE_EINTR(close(fd)) < 0)
On 2014/04/17 16:21:57, Mark Mentovai wrote:
> You don’t need to track |ret| here, you can just
> 
>   if (IGNORE_EINTR(close(fd)) < 0)
>     return -1;

Done for ReadFile(), WriteFile() and AppendToFile()

https://codereview.chromium.org/240893002/diff/40001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/240893002/diff/40001/base/file_util_unittest....
base/file_util_unittest.cc:1964: TEST_F(FileUtilTest, ReadFile) {
On 2014/04/17 16:21:57, Mark Mentovai wrote:
> Good test!
Thanks!

https://codereview.chromium.org/240893002/diff/40001/base/file_util_unittest....
base/file_util_unittest.cc:1973: // Make buffer with various size.
On 2014/04/17 16:21:57, Mark Mentovai wrote:
> buffers, plural.

Done.

https://codereview.chromium.org/240893002/diff/40001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/240893002/diff/40001/base/file_util_win.cc#ne...
base/file_util_win.cc:587: if (::ReadFile(file, data, max_size, &read, NULL))
On 2014/04/17 16:21:57, Mark Mentovai wrote:
> The same concern about short non-EOF reads that I mentioned in the POSIX file
> applies here.

Yes. filed as http://crbug.com/364762.

[fukino, 2014-04-18 03:23:08]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-04-18 03:23:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fukino@chromium.org/240893002/60001

[commit-bot: I haz the power, 2014-04-18 10:34:19]

Change committed as 264768