ImportantFileWriter: Flush the data before closing the

ImportantFileWriter: Flush the data before closing the
temporary file so that the subsequent rename always operates
with data in disk.

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

[Scott Hess - ex-Googler, 2011-08-23 00:54:38]

I think it does the right thing, though.

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:46:
base::CreatePlatformFile(tmp_file_path, flags, NULL, NULL);
I would prefer if you could not introduce a race condition on POSIX platforms
(if someone replaced the file with a symlink between the create and the open). 
That's probably why CreateAndOpenTemporaryFile*() was originally created.

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:55: base::FlushPlatformFile(tmp_file); 
// Ignore return value.
Would it be reasonable to wire up something like file_util::FlushFile()?  On
POSIX it would be something like fflush() followed by
FlushPlatformFile(fileno(file)), but I don't know if it would be so easy on
Windows.  Or possible file_util::FlushAndCloseFile() would make sense.

[rvargas (doing something else), 2011-08-23 01:41:52]

Thanks.

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:46:
base::CreatePlatformFile(tmp_file_path, flags, NULL, NULL);
On 2011/08/23 00:54:38, shess wrote:
> I would prefer if you could not introduce a race condition on POSIX platforms
> (if someone replaced the file with a symlink between the create and the open).

> That's probably why CreateAndOpenTemporaryFile*() was originally created.

I was writing the code with some posix specific version to keep the same
behavior but decided against that because a) the code that returns a a file
descriptor directly is not currently exported by file_util and b) if we really
care about someone doing something with the file when we don't hold a reference
to it, the code is still broken by the write/close -- rename sequence.

In other words, I don't think that is a use case that we should worry about.

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:55: base::FlushPlatformFile(tmp_file); 
// Ignore return value.
On 2011/08/23 00:54:38, shess wrote:
> Would it be reasonable to wire up something like file_util::FlushFile()?  On
> POSIX it would be something like fflush() followed by
> FlushPlatformFile(fileno(file)), but I don't know if it would be so easy on
> Windows.  Or possible file_util::FlushAndCloseFile() would make sense.

Not sure. I can make fflush do the right thing on Windows, but I don't think we
need two versions of the code around: one dealing with the CRT and another one
for native file implementations. The way I see this is that given that we have a
cross-platform native file abstraction, we should only use the CRT version for
really trivial stuff, so building FILE* helpers on top of fd seems wrong.

[Scott Hess - ex-Googler, 2011-08-23 19:23:57]

LGTM

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:46:
base::CreatePlatformFile(tmp_file_path, flags, NULL, NULL);
On 2011/08/23 01:41:52, rvargas wrote:
> On 2011/08/23 00:54:38, shess wrote:
> > I would prefer if you could not introduce a race condition on POSIX
platforms
> > (if someone replaced the file with a symlink between the create and the
open).
> 
> > That's probably why CreateAndOpenTemporaryFile*() was originally created.
> 
> I was writing the code with some posix specific version to keep the same
> behavior but decided against that because a) the code that returns a a file
> descriptor directly is not currently exported by file_util and b) if we really
> care about someone doing something with the file when we don't hold a
reference
> to it, the code is still broken by the write/close -- rename sequence.
> 
> In other words, I don't think that is a use case that we should worry about.

The rename case is a very good point!  I won't dig deeper on this review :-).

[Anyhow, AFAICT, if someone can inject into path_, the user is already owned. 
So calling it "target name _suffix" rather than using a temp file would be
sufficient, and wouldn't leave garbage on crashes.]

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:55: base::FlushPlatformFile(tmp_file); 
// Ignore return value.
On 2011/08/23 01:41:52, rvargas wrote:
> On 2011/08/23 00:54:38, shess wrote:
> > Would it be reasonable to wire up something like file_util::FlushFile()?  On
> > POSIX it would be something like fflush() followed by
> > FlushPlatformFile(fileno(file)), but I don't know if it would be so easy on
> > Windows.  Or possible file_util::FlushAndCloseFile() would make sense.
> 
> Not sure. I can make fflush do the right thing on Windows, but I don't think
we
> need two versions of the code around: one dealing with the CRT and another one
> for native file implementations. The way I see this is that given that we have
a
> cross-platform native file abstraction, we should only use the CRT version for
> really trivial stuff, so building FILE* helpers on top of fd seems wrong.

OK, that's all I wanted to hear - I hate having multiple half-assed file
abstractions, because I have no idea which one I'm supposed to champion in
code-reviews like this.  We should just build a full-assed abstraction of our
own and use that everywhere.  I'll get right on it.

[Scott Hess - ex-Googler, 2011-08-23 19:27:17]

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:54: tmp_file, 0, data_.data(),
static_cast<int>(data_.length()));
BTW ... AFAICT the POSIX version of this function doesn't handle short writes in
case of signal.

[rvargas (doing something else), 2011-08-23 23:36:58]

Thanks.

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:54: tmp_file, 0, data_.data(),
static_cast<int>(data_.length()));
On 2011/08/23 19:27:17, shess wrote:
> BTW ... AFAICT the POSIX version of this function doesn't handle short writes
in
> case of signal.

In that case, line 63 should detect the error and fail, right?

[Scott Hess - ex-Googler, 2011-08-23 23:59:12]

On 2011/08/23 23:36:58, rvargas wrote:
> Thanks.
> 
>
http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
> File chrome/common/important_file_writer.cc (right):
> 
>
http://codereview.chromium.org/7695014/diff/1/chrome/common/important_file_wr...
> chrome/common/important_file_writer.cc:54: tmp_file, 0, data_.data(),
> static_cast<int>(data_.length()));
> On 2011/08/23 19:27:17, shess wrote:
> > BTW ... AFAICT the POSIX version of this function doesn't handle short
writes
> in
> > case of signal.
> 
> In that case, line 63 should detect the error and fail, right?

Sure, but is that the right thing to do?  The world is full of signals, after
all.  If this is mostly writing small files, it should be fine, I suppose.

[Nico, 2011-09-10 05:45:10]

Note from the peanut gallery: Would it help if ReplaceFile() used MoveFileEx()
with MOVEFILE_WRITE_THROUGH on win?

http://msdn.microsoft.com/en-us/library/aa365240(v=vs.85).aspx

[Scott Hess - ex-Googler, 2011-09-10 05:49:31]

On 2011/09/10 05:45:10, Nico wrote:
> Note from the peanut gallery: Would it help if ReplaceFile() used MoveFileEx()
> with MOVEFILE_WRITE_THROUGH on win?
> 
> http://msdn.microsoft.com/en-us/library/aa365240%28v=vs.85%29.aspx

The part about it applying to "copy and delete" probably means that this applies
to moves between partitions.  This file is explicitly stored on the target
partition, so should be a straight rename.