Base: Change ReadPlatformFile to perform a best

Base: Change ReadPlatformFile to perform a best
effort read of the requested data, and add another
function (ReadPlatformFileNoBestEffort) that just
forwards the call to the OS.

Now ReadPlatformFile and WritePlatformFile behave
the same way.

BUG=none
TEST=base_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99264

[darin (slow to review), 2011-09-01 05:46:30]

Why is it valuable to make ReadPlatformFile loop?  Are you worried about
cases where someone allocates a buffer the size of the file, and then calls
read once, expecting everything to be read?

Ordinarily, I would expect that that is not such a common case since the
buffer could be large, but maybe we have code like that?  I would instead
expect that people would use a temp buffer of fixed size and read into that
until they hit EOF.  This means that they would not likely be bothered by a
call to ReadPlatformFile returning a partial result.

-Darin


On Wed, Aug 31, 2011 at 7:39 PM, <rvargas@chromium.org> wrote:

> Reviewers: darin,
>
> Description:
> Base: Change ReadPlatformFile to perform a best
> effort read of the requested data, and add another
> function (ReadPlatformFileNoBestEffort) that just
> forwards the call to the OS.
>
> Now ReadPlatformFile and WritePlatformFile behave
> the same way.
>
> BUG=none
> TEST=base_unittests
>
> Please review this at
http://codereview.chromium.**org/7821013/<http://codereview.chromium.org/7821...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     base/platform_file.h
>  M     base/platform_file_posix.cc
>  M     base/platform_file_unittest.cc
>  M     base/platform_file_win.cc
>
>
> Index: base/platform_file.h
> ==============================**==============================**=======
> --- base/platform_file.h        (revision 99047)
> +++ base/platform_file.h        (working copy)
> @@ -117,10 +117,19 @@
>  BASE_EXPORT bool ClosePlatformFile(PlatformFile file);
>
>  // Reads the given number of bytes (or until EOF is reached) starting with
> the
> -// given offset. Returns the number of bytes read, or -1 on error.
> +// given offset. Returns the number of bytes read, or -1 on error. Note
> that
> +// this function makes a best effort to read all data on all platforms, so
> it is
> +// not intended for stream oriented files but instead for cases when the
> normal
> +// expectation is that actually |size| bytes are read unless there is an
> error.
>  BASE_EXPORT int ReadPlatformFile(PlatformFile file, int64 offset,
>                                  char* data, int size);
>
> +// Reads the given number of bytes (or until EOF is reached) starting with
> the
> +// given offset, but does not make any effort to read all data on all
> platforms.
> +// Returns the number of bytes read, or -1 on error.
> +BASE_EXPORT int ReadPlatformFileNoBestEffort(**PlatformFile file, int64
> offset,
> +                                             char* data, int size);
> +
>  // Writes the given buffer into the file at the given offset, overwritting
> any
>  // data that was previously there. Returns the number of bytes written, or
> -1
>  // on error. Note that this function makes a best effort to write all data
> on
> Index: base/platform_file_posix.cc
> ==============================**==============================**=======
> --- base/platform_file_posix.cc (revision 99047)
> +++ base/platform_file_posix.cc (working copy)
> @@ -147,6 +147,25 @@
>  }
>
>  int ReadPlatformFile(PlatformFile file, int64 offset, char* data, int
> size) {
> +  if (file < 0 || size < 0)
> +    return -1;
> +
> +  int bytes_read = 0;
> +  int rv;
> +  do {
> +    rv = HANDLE_EINTR(pread(file, data + bytes_read,
> +                            size - bytes_read, offset + bytes_read));
> +    if (rv <= 0)
> +      break;
> +
> +    bytes_read += rv;
> +  } while (bytes_read < size);
> +
> +  return bytes_read ? bytes_read : rv;
> +}
> +
> +int ReadPlatformFileNoBestEffort(**PlatformFile file, int64 offset,
> +                                 char* data, int size) {
>   if (file < 0)
>     return -1;
>
> Index: base/platform_file_unittest.cc
> ==============================**==============================**=======
> --- base/platform_file_unittest.cc      (revision 99047)
> +++ base/platform_file_unittest.cc      (working copy)
> @@ -13,24 +13,7 @@
>  // Reads from a file the given number of bytes, or until EOF is reached.
>  // Returns the number of bytes read.
>  int ReadFully(base::PlatformFile file, int64 offset, char* data, int size)
> {
> -  int total_bytes_read = 0;
> -  int bytes_read;
> -  while (total_bytes_read < size) {
> -    bytes_read = base::ReadPlatformFile(
> -        file, offset + total_bytes_read, &data[total_bytes_read],
> -        size - total_bytes_read);
> -
> -    // If we reached EOF, bytes_read will be 0.
> -    if (bytes_read == 0)
> -      return total_bytes_read;
> -
> -    if ((bytes_read < 0) || (bytes_read > size - total_bytes_read))
> -      return -1;
> -
> -    total_bytes_read += bytes_read;
> -  }
> -
> -  return total_bytes_read;
> +  return base::ReadPlatformFile(file, offset, data, size);
>  }
>
>  // Writes the given number of bytes to a file.
> @@ -192,6 +175,13 @@
>   for (int i = 0; i < bytes_read; i++)
>     EXPECT_EQ(data_to_write[i], data_read_1[i]);
>
> +  // Read again, but using the trivial native wrapper.
> +  bytes_read = base::**ReadPlatformFileNoBestEffort(**file, 0,
> data_read_1,
> +                                                  kTestDataSize);
> +  EXPECT_LE(bytes_read, kTestDataSize);
> +  for (int i = 0; i < bytes_read; i++)
> +    EXPECT_EQ(data_to_write[i], data_read_1[i]);
> +
>   // Write past the end of the file.
>   const int kOffsetBeyondEndOfFile = 10;
>   const int kPartialWriteLength = 2;
> Index: base/platform_file_win.cc
> ==============================**==============================**=======
> --- base/platform_file_win.cc   (revision 99047)
> +++ base/platform_file_win.cc   (working copy)
> @@ -134,6 +134,11 @@
>   return -1;
>  }
>
> +int ReadPlatformFileNoBestEffort(**PlatformFile file, int64 offset,
> +                                 char* data, int size) {
> +  return ReadPlatformFile(file, offset, data, size);
> +}
> +
>  int WritePlatformFile(PlatformFile file, int64 offset,
>                       const char* data, int size) {
>   base::ThreadRestrictions::**AssertIOAllowed();
>
>
>

[rvargas (doing something else), 2011-09-01 17:35:58]

On 2011/09/01 05:46:30, darin wrote:
> Why is it valuable to make ReadPlatformFile loop?  Are you worried about
> cases where someone allocates a buffer the size of the file, and then calls
> read once, expecting everything to be read?

I think it is a matter of consistency between Read and Write methods.

I guess if someone is reading a whole file of arbitrary length, then a loop in
the caller is the expected pattern, but I don't see why a base function like
this one should require that pattern for everyone.

If the purpose of the call is to read a fixed portion of a file, or even to read
a whole file of a known small size, looping at the caller side is not what I
would expect.

I consider more natural for this interface (especially given that it has an
explicit offset argument) to actually return what it was asked for instead of
just a few bytes.

I would also fix the current callers, but none of them (except a unit test) is
performing a loop directly... that's why I decided to change the current
behavior instead of having the new method implement the new behavior.

> 
> Ordinarily, I would expect that that is not such a common case since the
> buffer could be large, but maybe we have code like that?  I would instead
> expect that people would use a temp buffer of fixed size and read into that
> until they hit EOF.  This means that they would not likely be bothered by a
> call to ReadPlatformFile returning a partial result.
> 
> -Darin
>

[darin (slow to review), 2011-09-01 19:36:53]

On Thu, Sep 1, 2011 at 10:35 AM, <rvargas@chromium.org> wrote:

> On 2011/09/01 05:46:30, darin wrote:
>
>> Why is it valuable to make ReadPlatformFile loop?  Are you worried about
>> cases where someone allocates a buffer the size of the file, and then
>> calls
>> read once, expecting everything to be read?
>>
>
> I think it is a matter of consistency between Read and Write methods.
>
> I guess if someone is reading a whole file of arbitrary length, then a loop
> in
> the caller is the expected pattern, but I don't see why a base function
> like
> this one should require that pattern for everyone.
>
> If the purpose of the call is to read a fixed portion of a file, or even to
> read
> a whole file of a known small size, looping at the caller side is not what
> I
> would expect.
>
> I consider more natural for this interface (especially given that it has an
> explicit offset argument) to actually return what it was asked for instead
> of
> just a few bytes.
>
> I would also fix the current callers, but none of them (except a unit test)
> is
> performing a loop directly... that's why I decided to change the current
> behavior instead of having the new method implement the new behavior.
>
>
OK, LGTM



>
>
>  Ordinarily, I would expect that that is not such a common case since the
>> buffer could be large, but maybe we have code like that?  I would instead
>> expect that people would use a temp buffer of fixed size and read into
>> that
>> until they hit EOF.  This means that they would not likely be bothered by
>> a
>> call to ReadPlatformFile returning a partial result.
>>
>
>  -Darin
>>
>
>
>
>
http://codereview.chromium.**org/7821013/<http://codereview.chromium.org/7821...
>