Add an "append flag" to base::PlatformFile.

Add an "append flag" to base::PlatformFile.

PLATFORM_FILE_APPEND is exclusive with PLATFORM_FILE_WRITE. This is because on Windows, if we were to apply both flags, the behavior would not be consistent with O_APPEND.

On Posix, PLATFORM_FILE_APPEND provides O_APPEND, and either O_WRONLY or O_RDWR, depending on other flags.

On Windows, PLATFORM_FILE_APPEND provides FILE_APPEND_DATA, and fails to create the plaform file if PLATFORM_FILE_WRITE is also passed.

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

[jar (doing other things), 2013-05-31 16:33:50]

Drive by question...

https://codereview.chromium.org/15861011/diff/6001/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/15861011/diff/6001/base/platform_file_posix.c...
base/platform_file_posix.cc:84: !(flags & PLATFORM_FILE_OPEN_ALWAYS)) {
Are some of these "effective assertions" going to pop?  Do you need to add
another exception?

[teravest, 2013-05-31 17:04:08]

https://codereview.chromium.org/15861011/diff/6001/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/15861011/diff/6001/base/platform_file_posix.c...
base/platform_file_posix.cc:84: !(flags & PLATFORM_FILE_OPEN_ALWAYS)) {
On 2013/05/31 16:33:50, jar wrote:
> Are some of these "effective assertions" going to pop?  Do you need to add
> another exception?

Nice catch! I updated the test to take this path, verified it hit the
NOTREACHED(), and added another exception.

Thanks.

[teravest, 2013-06-03 20:47:42]

Hi Jim,
Could you take another look at this?

[jar (doing other things), 2013-06-04 23:31:50]

https://codereview.chromium.org/15861011/diff/12001/base/platform_file.h
File base/platform_file.h (right):

https://codereview.chromium.org/15861011/diff/12001/base/platform_file.h#newc...
base/platform_file.h:22: // PLATFORM_FILE_(OPEN|CREATE).* are mutually
exclusive. You should specify
nit: you probably need an analogous comment about PLATFORM_FILE_WRITE and
PLATFORM_FILE_APPEND, along with your explanation.

https://codereview.chromium.org/15861011/diff/12001/base/platform_file.h#newc...
base/platform_file.h:53: PLATFORM_FILE_APPEND = 1 << 19,
nit: You *might* want to re-order these, so that WRITE and APPEND are next to
each other, now that you've made them exclusive.

Do you think there is a bunch of other code that would break?  Are there other
consumers or exporters of these flags???

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_posix....
base/platform_file_posix.cc:94: else if (flags & PLATFORM_FILE_APPEND)
nit: I guess you shouldn't change this... as it is pervasively done throughout
this file..... but I didn't like having to read the pattern of:

if (a && b)
 do_one_thing();
else if (a)  // Note re-use of a test, hopefully spelled the same :-/
  do_something_else();

and would vastly prefer:
if (a) {
  if (b)
    do_one_thing()
  else
    do_something else
}


If by any chance you too are bothered by this... you could consider renovating
this... but I can't justify insistence... and it is probably best to leave this
as is... and copy the style as you have done <sigh>.

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
File base/platform_file_unittest.cc (right):

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
base/platform_file_unittest.cc:221: base::PLATFORM_FILE_APPEND,
nit: This should fit nicely (and more readably) on line 220.

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
base/platform_file_unittest.cc:242: NULL, NULL);
nit: I can see this odd style used elsewhere in this file, where sometimes the
author put one argument per line (see line 238), sometimes they wrapped args
when not really needed (lines 239-240), and sometimes they put multiple args on
a line (here in 242).

I *think* you should go with the "Arguments may optionally all be placed on
subsequent lines, with one line per argument:" rule, and then when you're forced
to wrap (line 241?), indent that by an extra 4 spaces.

I found the current mixture (even though it is just copying local style) to be
more painful to read.

You could alternatively use the style shown on lines 38 and 39 (which could
perchance be better), but I think the direction of putting each of the 4 args at
the start of a line is better.

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
base/platform_file_unittest.cc:255: // Read the entire file
nit: Periods at end of sentences.

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_win.cc
File base/platform_file_win.cc (right):

https://codereview.chromium.org/15861011/diff/12001/base/platform_file_win.cc...
base/platform_file_win.cc:63: if (flags & PLATFORM_FILE_WRITE)  // Invalid.
Note that the corresponding check on CREATE vs OPEN was done with a DCHECK
(lines 29, 34, etc.).  You should probably follow that pattern:
DCHECK_FALSE(flags & PLATFORM_FILE_WRITE);

[teravest, 2013-06-05 15:43:43]

On Tue, Jun 4, 2013 at 5:31 PM,  <jar@chromium.org> wrote:
>
> https://codereview.chromium.org/15861011/diff/12001/base/platform_file.h
> File base/platform_file.h (right):
>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file.h#newc...
> base/platform_file.h:22: // PLATFORM_FILE_(OPEN|CREATE).* are mutually
> exclusive. You should specify
> nit: you probably need an analogous comment about PLATFORM_FILE_WRITE
> and PLATFORM_FILE_APPEND, along with your explanation.

Done.

>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file.h#newc...
> base/platform_file.h:53: PLATFORM_FILE_APPEND = 1 << 19,
> nit: You *might* want to re-order these, so that WRITE and APPEND are
> next to each other, now that you've made them exclusive.
>
> Do you think there is a bunch of other code that would break?  Are there
> other consumers or exporters of these flags???

I reordered them. I didn't see any obvious other consumers of those flags.

>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_posix.cc
> File base/platform_file_posix.cc (right):
>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_posix....
> base/platform_file_posix.cc:94: else if (flags & PLATFORM_FILE_APPEND)
> nit: I guess you shouldn't change this... as it is pervasively done
> throughout this file..... but I didn't like having to read the pattern
> of:
>
> if (a && b)
>  do_one_thing();
> else if (a)  // Note re-use of a test, hopefully spelled the same :-/
>   do_something_else();
>
> and would vastly prefer:
> if (a) {
>   if (b)
>     do_one_thing()
>   else
>     do_something else
> }
>
>
> If by any chance you too are bothered by this... you could consider
> renovating this... but I can't justify insistence... and it is probably
> best to leave this as is... and copy the style as you have done <sigh>.

I'd be happy to do this in another change.

>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
> File base/platform_file_unittest.cc (right):
>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
> base/platform_file_unittest.cc:221: base::PLATFORM_FILE_APPEND,
> nit: This should fit nicely (and more readably) on line 220.

Done.

>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
> base/platform_file_unittest.cc:242: NULL, NULL);
> nit: I can see this odd style used elsewhere in this file, where
> sometimes the author put one argument per line (see line 238), sometimes
> they wrapped args when not really needed (lines 239-240), and sometimes
> they put multiple args on a line (here in 242).
>
> I *think* you should go with the "Arguments may optionally all be placed
> on subsequent lines, with one line per argument:" rule, and then when
> you're forced to wrap (line 241?), indent that by an extra 4 spaces.
>
> I found the current mixture (even though it is just copying local style)
> to be more painful to read.
>
> You could alternatively use the style shown on lines 38 and 39 (which
> could perchance be better), but I think the direction of putting each of
> the 4 args at the start of a line is better.

I think I reformatted this how you'd like it; let me know if you want
something different.

>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_unitte...
> base/platform_file_unittest.cc:255: // Read the entire file
> nit: Periods at end of sentences.

Done.

>
> https://codereview.chromium.org/15861011/diff/12001/base/platform_file_win.cc
> File base/platform_file_win.cc (right):
>
>
https://codereview.chromium.org/15861011/diff/12001/base/platform_file_win.cc...
> base/platform_file_win.cc:63: if (flags & PLATFORM_FILE_WRITE)  //
> Invalid.
> Note that the corresponding check on CREATE vs OPEN was done with a
> DCHECK (lines 29, 34, etc.).  You should probably follow that pattern:
> DCHECK_FALSE(flags & PLATFORM_FILE_WRITE);

Done. This section is a little strange now because the logic order
doesn't match the order in which the flags are ordered in the enum,
but it does match the CREATE and OPEN logic above.

>
> https://codereview.chromium.org/15861011/

[jar (doing other things), 2013-06-05 16:39:21]

https://codereview.chromium.org/15861011/diff/23001/base/platform_file_unitte...
File base/platform_file_unittest.cc (right):

https://codereview.chromium.org/15861011/diff/23001/base/platform_file_unitte...
base/platform_file_unittest.cc:240: base::PLATFORM_FILE_APPEND,
Yes... this is exactly what I was asking for.

nit: Can you similarly adjust the other calls in this file (such as on line
272-275, where they put one arg on multiple lines for no reason without extra
indent, and put multiple args on one line).  This way the file will have a
consistent approach (for future changes).

https://codereview.chromium.org/15861011/diff/23001/base/platform_file_win.cc
File base/platform_file_win.cc (right):

https://codereview.chromium.org/15861011/diff/23001/base/platform_file_win.cc...
base/platform_file_win.cc:58: DCHECK(!access)
missing semicolon.

[teravest, 2013-06-05 16:44:16]

On Wed, Jun 5, 2013 at 10:39 AM,  <jar@chromium.org> wrote:
>
>
https://codereview.chromium.org/15861011/diff/23001/base/platform_file_unitte...
> File base/platform_file_unittest.cc (right):
>
>
https://codereview.chromium.org/15861011/diff/23001/base/platform_file_unitte...
> base/platform_file_unittest.cc:240: base::PLATFORM_FILE_APPEND,
> Yes... this is exactly what I was asking for.
>
> nit: Can you similarly adjust the other calls in this file (such as on
> line 272-275, where they put one arg on multiple lines for no reason
> without extra indent, and put multiple args on one line).  This way the
> file will have a consistent approach (for future changes).

Done.

>
> https://codereview.chromium.org/15861011/diff/23001/base/platform_file_win.cc
> File base/platform_file_win.cc (right):
>
>
https://codereview.chromium.org/15861011/diff/23001/base/platform_file_win.cc...
> base/platform_file_win.cc:58: DCHECK(!access)
> missing semicolon.

Done.

>
> https://codereview.chromium.org/15861011/

[jar (doing other things), 2013-06-06 15:46:45]

Thank you for smoothing out the formatting in that file!!!

LGTM (with one missing nit)

https://codereview.chromium.org/15861011/diff/25003/base/platform_file_unitte...
File base/platform_file_unittest.cc (right):

https://codereview.chromium.org/15861011/diff/25003/base/platform_file_unitte...
base/platform_file_unittest.cc:99: &created, &error_code);
nit: separate lines

[teravest, 2013-06-06 15:50:18]

On Thu, Jun 6, 2013 at 9:46 AM,  <jar@chromium.org> wrote:
> Thank you for smoothing out the formatting in that file!!!
>
> LGTM (with one missing nit)
>
>
>
https://codereview.chromium.org/15861011/diff/25003/base/platform_file_unitte...
> File base/platform_file_unittest.cc (right):
>
>
https://codereview.chromium.org/15861011/diff/25003/base/platform_file_unitte...
> base/platform_file_unittest.cc:99: &created, &error_code);
> nit: separate lines

Nice catch, fixed.

>
> https://codereview.chromium.org/15861011/

[commit-bot: I haz the power, 2013-06-06 15:51:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/15861011/30002

[commit-bot: I haz the power, 2013-06-06 16:23:27]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[teravest, 2013-06-06 17:48:54]

I discovered one surprising behavior while testing this:

For O_APPEND files,
  on Linux, pwrite() will always write to the end, regardless of offset, but
  on Mac, pwrite() writes at the specified offset (and is POSIX-compliant).

WritePlatformFile() uses pwrite(), and exposes this differing behavior on
different platforms today.

I'm considering adding the following to WritePlatformFile to prevent this
confusion:
  DCHECK(!(fcntl(file, F_GETFL) & O_APPEND)) <<
      "WritePlatformFile is not supported with PLATFORM_FILE_APPEND.";
...and then updating the test to not call WritePlatformFile for the append
cases.

Does this sound good to you?

[teravest, 2013-06-06 18:24:27]

I spent some time trying out what I wrote above, but it made
PLATFORM_FILE_APPEND files hard to use, since WritePlatformFile is
called in a lot of places.

My preference after looking around would be to make Mac behave the
same way as Linux-- when a file is opened with PLATFORM_FILE_APPEND,
the offset is simply ignored in WritePlatformFile. I'll code that up
quickly, let me know what you think.

On Thu, Jun 6, 2013 at 11:48 AM,  <teravest@chromium.org> wrote:
> I discovered one surprising behavior while testing this:
>
> For O_APPEND files,
>   on Linux, pwrite() will always write to the end, regardless of offset, but
>   on Mac, pwrite() writes at the specified offset (and is POSIX-compliant).
>
> WritePlatformFile() uses pwrite(), and exposes this differing behavior on
> different platforms today.
>
> I'm considering adding the following to WritePlatformFile to prevent this
> confusion:
>   DCHECK(!(fcntl(file, F_GETFL) & O_APPEND)) <<
>       "WritePlatformFile is not supported with PLATFORM_FILE_APPEND.";
> ...and then updating the test to not call WritePlatformFile for the append
> cases.
>
> Does this sound good to you?
>
> https://codereview.chromium.org/15861011/

[commit-bot: I haz the power, 2013-06-07 19:42:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/15861011/50002

[commit-bot: I haz the power, 2013-06-07 23:11:54]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-10 13:49:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/15861011/50002

[commit-bot: I haz the power, 2013-06-10 15:28:16]

Change committed as 205217