Bug: OS::MemoryMappedFile::open() should not truncate a pre-existing file.

Bug: OS::MemoryMappedFile::open() should not truncate a pre-existing file.

Committed: http://code.google.com/p/v8/source/detail?r=6872

[marklam, 2011-02-20 09:07:47]

While testing LOL code, I see that OS::MemoryMappedFile::open() which I
introduced previously had a bug.  In the unix ports, it was truncating the file
that it opens.  In the win32 port, it was opening the file only if it
pre-existed.  The actual behavior of open() is actually expected to open the
file if it exists, and to create the file if it doesn't already exists.

Hence, for the unix ports, the fopen mode has been corrected to be "a+" instead
of "w+".  For the win32 port, the dwCreationDisposition arg has been changed
from OPEN_EXISTING to OPEN_ALWAYS (which is equivalent to a+ according to msdn
docs).

Please take a look.  Thanks.

[mnaganov (inactive), 2011-02-21 10:37:56]

http://codereview.chromium.org/6543039/diff/1/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/6543039/diff/1/src/platform-linux.cc#newcode330
src/platform-linux.cc:330: FILE* file = fopen(name, "a+");
I don't think "a+" is the right flag. If you plan to read and write to the file
from the beginning, and avoid truncating it, then you should use "r+". This
concern has arisen to me because in 'mmap' you pass 0 as the offset inside the
file, which contradicts with the "a" flag semantics that means appending data to
a file. Thus, using both "a" and "0" offset seems like a contradiction. Also,
you don't pass 'FILE_APPEND_DATA' to the Win32 function, so I'm concluding you
really don't want to append to the file.

[marklam, 2011-02-21 15:05:59]

On 2011/02/21 10:37:56, Mikhail Naganov (Chromium) wrote:
> http://codereview.chromium.org/6543039/diff/1/src/platform-linux.cc
> File src/platform-linux.cc (right):
> 
> http://codereview.chromium.org/6543039/diff/1/src/platform-linux.cc#newcode330
> src/platform-linux.cc:330: FILE* file = fopen(name, "a+");
> I don't think "a+" is the right flag. If you plan to read and write to the
file
> from the beginning, and avoid truncating it, then you should use "r+". This
> concern has arisen to me because in 'mmap' you pass 0 as the offset inside the
> file, which contradicts with the "a" flag semantics that means appending data
to
> a file. Thus, using both "a" and "0" offset seems like a contradiction. Also,
> you don't pass 'FILE_APPEND_DATA' to the Win32 function, so I'm concluding you
> really don't want to append to the file.

Thanks for catching this.

At first, I was thinking that I would want the file to be created if it does not
already exists.  That's why I chose "a+". 
http://www.cplusplus.com/reference/clibrary/cstdio/fopen/ indicates that it will
simply fail if the file does not exist.  But you are right about the append
write issue.  On further consideration, "r+" is the right mode to use.  I
doubled check that the error handling does cover the case of the file not
already existing.

Hence, I reverted the win32 case to OPEN_EXISTING, and corrected the return
value check to check for INVALID_HANDLE_VALUE instead of NULL (according to msdn
specs).

[mnaganov (inactive), 2011-02-21 17:10:05]

I see a wrong change in platform-freebsd, but it's trivial to fix.

LGTM, I will land a corrected change.

http://codereview.chromium.org/6543039/diff/1010/src/platform-freebsd.cc
File src/platform-freebsd.cc (right):

http://codereview.chromium.org/6543039/diff/1010/src/platform-freebsd.cc#newc...
src/platform-freebsd.cc:241: FILE* file = fopen(name, "r+");
Looks like you've changed this in a wrong place.