Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869:
POSIX/Linux related changes to file_util.... (Closed)
|
Created:
12 years, 3 months ago by reima Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionPOSIX/Linux related changes to file_util.
1. Replaced mktemp by mkstemp as mktemp should never be used (see
http://linux.die.net/man/3/mktemp for instance).
2. Implemented file_util::CopyFile for the Linux platform. This also made
the FileUtilTest pass, so I updated the scons file as well.
3. Cleaned up some invalid uses of c_str() on a temporary std::string.
4. Changed file_util::WriteFile to allow for partial writes.
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Messages
Total messages: 20 (0 generated)
A few minor nits. Otherwise, Looks Good To Me. http://codereview.chromium.org/1869/diff/1/4 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/1/4#newcode29 Line 29: const size_t kBufferSize = 32768; Declare this down near where it's used. http://codereview.chromium.org/1869/diff/1/4#newcode31 Line 31: std::string to = WideToUTF8(to_path); Same here. http://codereview.chromium.org/1869/diff/1/4#newcode42 Line 42: from_stat.st_mode); In other places in file_util_posix.cc, we use 0666 as the mode for files to be good citizens and to let umask do its job.
Question. http://codereview.chromium.org/1869/diff/1/3 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/1/3#newcode149 Line 149: int fd = mkstemp(buffer); This function's name is confusing. If the Windows function actually creates the file too, then we should rename this to CreateTemporaryFile. If the Windows function doesn't actually create the file but just makes a new temp filename, then we should consider changing it (along with the name), because it's bad behavior.
http://codereview.chromium.org/1869/diff/1/3 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/1/3#newcode146 Line 146: tmpdir.append(L"/com.google.chrome.XXXXXX"); I didn't notice this the first time. Please either fix file_util_linux.cc GetTempDir to have a trailing / or fix file_util_mac.mm to not have one. http://codereview.chromium.org/1869/diff/1/3#newcode149 Line 149: int fd = mkstemp(buffer); The Windows code does in fact create the file. Agree that the name is bad (in fact it caused me to implement this incorrectly the first time). There's already a TODO in file_util.h to rename the function.
http://codereview.chromium.org/1869/diff/1/3 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/1/3#newcode146 Line 146: tmpdir.append(L"/com.google.chrome.XXXXXX"); I just found out there's AppendToPath. I'll use it here and in line 162 as well. Question: should the string constant "com.google.chrome.XXXXXX" be factored out? If so, where to put it?
Thank you for your comments so far. I just attached a revised patch set.
http://codereview.chromium.org/1869/diff/8/210 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/8/210#newcode36 Line 36: O_WRONLY | O_CREAT | O_EXCL, This should not be O_EXCL. The Windows version of CopyFile passes false for bFailIfExists. On the Mac, we use copyfile, which is incompletely defined but is currently implemented without opening the target file with O_EXCL. Bet you can fit this whole call on one line, especially when you get rid of O_EXCL. http://codereview.chromium.org/1869/diff/8/210#newcode39 Line 39: close(infile); An auto-closer to own the two FDs would be awesome here, but is not strictly necessary. http://codereview.chromium.org/1869/diff/8/210#newcode44 Line 44: scoped_array<char> buffer(new char[kBufferSize]); This is better as: vector<char> buffer(kBufferSize); or even as a std::string. http://codereview.chromium.org/1869/diff/8/210#newcode45 Line 45: ssize_t bytes_read, bytes_written_partial, bytes_written_total; Don't declare things until you use them. It looks like everything can be moved into the loop here. http://codereview.chromium.org/1869/diff/8/210#newcode49 Line 49: bytes_read = read(infile, buffer.get(), kBufferSize); With the vector, this would be &buffer[0], buffer.size() http://codereview.chromium.org/1869/diff/8/210#newcode56 Line 56: bytes_written_total = 0; bytes_written_total sounds like the total number of bytes written for the entire file, but that's not what it is. Can you rename it? bytes_written_per_read? http://codereview.chromium.org/1869/diff/8/210#newcode57 Line 57: // Allow for partial writes for syscalls interrupted by signals, I guess? OK. http://codereview.chromium.org/1869/diff/8/210#newcode58 Line 58: do { If you moved the while condition up top here, you could get rid of the bytes_read == 0 check above: while (bytes_written_per_read < bytes_read) { } http://codereview.chromium.org/1869/diff/8/210#newcode60 Line 60: buffer.get() + bytes_written_total, and this could simply be &buffer[bytes_written_per_read] http://codereview.chromium.org/1869/diff/8/209 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/8/209#newcode149 Line 149: int fd = mkstemp(buffer); buffer is already toast by the time you do this. This was a problem in the existing code, let's take the opportunity to fix it: std::string tmpdir_string = WideToUTF8(tmpdir); // This should be OK since mkstemp just replaces characters in place. char* buffer = const_cast<char*>(tmpdir_string.c_str()); int fd = mkstemp(buffer); The difference between this and the existing code is that tmpdir_string is now an object in the same scope as the buffer that we take from it, so buffer is still valid by the time we use it. In the existing code, WideToUTF8(tmpdir) is destroyed as soon as we take its c_str. Depending on the string implementation, it may actually be safer to do: std::string tmpdir_string = WideToUTF8(tmpdir); // This should be OK since mkstemp just replaces characters in place. int fd = mkstemp(&tmpdir_string[0]); because string storage is contiguous and '\0'-terminated in all std::string implementations we care about, and this behavior will actually guaranteed by C++0x so it's unlikely to change. The difference here is that the second way accesses the string as non-const, which forces copy-on-write implementations to copy. Although in this case, it's unlikely, as used here, that we'd be starting out with shared string storage.
http://codereview.chromium.org/1869/diff/8/210 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/8/210#newcode36 Line 36: O_WRONLY | O_CREAT | O_EXCL, P.S. When you drop O_EXCL, this needs to have O_TRUNC, obviously.
http://codereview.chromium.org/1869/diff/1/3 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/1/3#newcode146 Line 146: tmpdir.append(L"/com.google.chrome.XXXXXX"); On 2008/09/10 08:46:18, reima wrote: > I just found out there's AppendToPath. I'll use it here and in line 162 as > well. Question: should the string constant "com.google.chrome.XXXXXX" be > factored out? If so, where to put it? Yes. AppendToPath is better. You can pull out the constant if you'd like. If so, put it at the top of the file (but within the namespace) and give it a name like kTempFileName. See the C++ coding style doc if you have more questions about this. http://codereview.chromium.org/1869/diff/8/210 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/8/210#newcode44 Line 44: scoped_array<char> buffer(new char[kBufferSize]); On 2008/09/10 21:20:24, Mark Mentovai wrote: > This is better as: > > vector<char> buffer(kBufferSize); > > or even as a std::string. Mark, just curious, what's your take on the max size you'd allow on the stack? I almost made the same comment as you, but figured at 32K, I was OK having this be a heap variable.
http://codereview.chromium.org/1869/diff/8/210 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/8/210#newcode36 Line 36: O_WRONLY | O_CREAT | O_EXCL, On 2008/09/10 21:25:04, Mark Mentovai wrote: > P.S. When you drop O_EXCL, this needs to have O_TRUNC, obviously. Yes, you are right. In this case we could simply use creat() instead of open() (that's open with O_WRONLY | O_CREAT | O_TRUNC). http://codereview.chromium.org/1869/diff/8/210#newcode39 Line 39: close(infile); On 2008/09/10 21:20:24, Mark Mentovai wrote: > An auto-closer to own the two FDs would be awesome here, but is not strictly > necessary. By auto-closer you mean a wrapper class which closes the FDs when going out of scope? Is there something like that in the code base already? http://codereview.chromium.org/1869/diff/8/210#newcode44 Line 44: scoped_array<char> buffer(new char[kBufferSize]); On 2008/09/10 21:52:24, Erik Kay wrote: > On 2008/09/10 21:20:24, Mark Mentovai wrote: > > This is better as: > > > > vector<char> buffer(kBufferSize); > > > > or even as a std::string. > > Mark, just curious, what's your take on the max size you'd allow on the stack? > I almost made the same comment as you, but figured at 32K, I was OK having this > be a heap variable. Not sure what you mean--both vector and string store their payload on the heap (AFAIK). http://codereview.chromium.org/1869/diff/8/210#newcode56 Line 56: bytes_written_total = 0; On 2008/09/10 21:20:24, Mark Mentovai wrote: > bytes_written_total sounds like the total number of bytes written for the entire > file, but that's not what it is. Can you rename it? > > bytes_written_per_read? Agreed and done. http://codereview.chromium.org/1869/diff/8/210#newcode57 Line 57: // Allow for partial writes On 2008/09/10 21:20:24, Mark Mentovai wrote: > for syscalls interrupted by signals, I guess? OK. Yes, as described in the manpage. http://codereview.chromium.org/1869/diff/8/210#newcode58 Line 58: do { On 2008/09/10 21:20:24, Mark Mentovai wrote: > If you moved the while condition up top here, you could get rid of the > bytes_read == 0 check above: > > while (bytes_written_per_read < bytes_read) { > } I'm not sure what you mean. Wouldn't removing the bytes_read == 0 check lead to an infinite loop? Either way, moving the condition up here shouldn't break anything, so it's just a matter of taste where it is. http://codereview.chromium.org/1869/diff/8/209 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/8/209#newcode149 Line 149: int fd = mkstemp(buffer); On 2008/09/10 21:20:24, Mark Mentovai wrote: > buffer is already toast by the time you do this. This was a problem in the > existing code, let's take the opportunity to fix it: [...] Nice find! I'll change it right away (also in CreateNewTempDirectory).
http://codereview.chromium.org/1869/diff/8/209 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/8/209#newcode149 Line 149: int fd = mkstemp(buffer); On 2008/09/10 22:39:48, reima wrote: > Nice find! I'll change it right away (also in CreateNewTempDirectory). I just noticed there's WideToUTF8(path).c_str() all over the place in this file. I suspect all occurences should be corrected then?
http://codereview.chromium.org/1869/diff/8/210 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/8/210#newcode44 Line 44: scoped_array<char> buffer(new char[kBufferSize]); On 2008/09/10 22:39:48, reima wrote: > On 2008/09/10 21:52:24, Erik Kay wrote: > > On 2008/09/10 21:20:24, Mark Mentovai wrote: > > > This is better as: > > > > > > vector<char> buffer(kBufferSize); > > > > > > or even as a std::string. > > > > Mark, just curious, what's your take on the max size you'd allow on the stack? > > > I almost made the same comment as you, but figured at 32K, I was OK having > this > > be a heap variable. > > Not sure what you mean--both vector and string store their payload on the heap > (AFAIK). yes, igore me. I was just being stupid. http://codereview.chromium.org/1869/diff/8/209 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/8/209#newcode149 Line 149: int fd = mkstemp(buffer); On 2008/09/10 22:45:49, reima wrote: > On 2008/09/10 22:39:48, reima wrote: > > Nice find! I'll change it right away (also in CreateNewTempDirectory). > > I just noticed there's WideToUTF8(path).c_str() all over the place in this file. > I suspect all occurences should be corrected then? Ouch. Yes, these should all be fixed. Thanks for checking them out. I'll take a look in other parts in the source.
http://codereview.chromium.org/1869/diff/8/209 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/8/209#newcode48 Line 48: const char* utf8_path = WideToUTF8(path).c_str(); Possibly invalid use of c_str() on a temporary std::string. http://codereview.chromium.org/1869/diff/8/209#newcode149 Line 149: int fd = mkstemp(buffer); On 2008/09/10 22:55:14, Erik Kay wrote: > On 2008/09/10 22:45:49, reima wrote: > > I just noticed there's WideToUTF8(path).c_str() all over the place in this > file. > > I suspect all occurences should be corrected then? > > Ouch. Yes, these should all be fixed. Thanks for checking them out. I'll take > a look in other parts in the source. > Note, however, that most uses of c_str() are valid in this file: ----- <evmar> reima: fwiw, it's valid in many cases. e.g. printf("%s\n", WideToUTF8(foo).c_str()); <reima> evmar: Is that really valid? [...] <jamessan> reima: yes. the temporary std::string (and therefore the result of .c_str()) is valid for the scope of the call ----- I've marked the remaining uses of c_str() which I suspect to be invalid in this review. http://codereview.chromium.org/1869/diff/8/209#newcode164 Line 164: char* buffer = const_cast<char*>(WideToUTF8(tmpdir).c_str()); Possibly invalid use of c_str() on a temporary std::string. http://codereview.chromium.org/1869/diff/8/209#newcode304 Line 304: const char* utf8_pattern = WideToUTF8(pattern_).c_str(); Possibly invalid use of c_str() on a temporary std::string.
Next round (updated patch set). Tell me what you think now.
http://codereview.chromium.org/1869/diff/402/17 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/402/17#newcode23 Line 23: const wchar_t* kTempFileName = L"com.google.chrome.XXXXXX"; Can you make this static too? Avoid symbol table pollution. http://codereview.chromium.org/1869/diff/402/17#newcode218 Line 218: int ret_value = write(fd, data, size); Should we loop in case of interrupt here too?
Thanks again for your comments, revised patch set is already up. http://codereview.chromium.org/1869/diff/402/17 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/402/17#newcode23 Line 23: const wchar_t* kTempFileName = L"com.google.chrome.XXXXXX"; On 2008/09/11 14:35:21, Mark Mentovai wrote: > Can you make this static too? Avoid symbol table pollution. Done. http://codereview.chromium.org/1869/diff/402/17#newcode218 Line 218: int ret_value = write(fd, data, size); On 2008/09/11 14:35:21, Mark Mentovai wrote: > Should we loop in case of interrupt here too? Yes, absolutely.
LGTM Thanks for doing the c_str() cleanup along with this checkin!
Committed r2070, feel free to close this out now. Thanks for the patch, for sticking through the review, and for all of the follow-up fixes.
Realize this is already checked in, I'm slow, apparently. http://codereview.chromium.org/1869/diff/407/25 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/407/25#newcode33 Line 33: int outfile = creat(WideToUTF8(to_path).c_str(), 0666); Is this the right thing to do? I would think we should either take the mode from the file we're copying, or to use the umask, not to hardcode 0666. http://codereview.chromium.org/1869/diff/407/25#newcode39 Line 39: const size_t kBufferSize = 32768; If this is 32k, it would be nice as 32 * 1024; http://codereview.chromium.org/1869/diff/407/24 File base/file_util_posix.cc (right): http://codereview.chromium.org/1869/diff/407/24#newcode23 Line 23: static const wchar_t* kTempFileName = L"com.google.chrome.XXXXXX"; Seems a waste to make this wide, just when we're going to convert it back down again.
http://codereview.chromium.org/1869/diff/407/25 File base/file_util_linux.cc (right): http://codereview.chromium.org/1869/diff/407/25#newcode33 Line 33: int outfile = creat(WideToUTF8(to_path).c_str(), 0666); On 2008/09/12 13:59:46, deanm wrote: > Is this the right thing to do? I would think we should either take the mode > from the file we're copying, or to use the umask, not to hardcode 0666. My understanding is that both creat() and open() use the umask for you. That's why we're passing in 0666, so that we give full control to the user's umask. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
