Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157593005:
Added new ReadFileToString API with a max_size argument (Closed)
|
Created:
6 years, 10 months ago by kaliamoorthi Modified:
6 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, bartfab (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded new ReadFileToString API with a max_size argument
The CL creates a new API that takes a max_size argument. When the file size exceed the max_size, the API primes the cache and returns false.
BUG=339417
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251771
Patch Set 1 #
Total comments: 26
Patch Set 2 : Added new ReadFileToString API with a max_size argument #Patch Set 3 : Added new ReadFileToString API with a max_size argument #
Total comments: 7
Patch Set 4 : Added new ReadFileToString API with a max_size argument #
Total comments: 18
Patch Set 5 : Added new ReadFileToString API with a max_size argument #Patch Set 6 : Added new ReadFileToString API with a max_size argument #Patch Set 7 : Added new ReadFileToString API with a max_size argument #
Total comments: 4
Patch Set 8 : Added new ReadFileToString API with a max_size argument #
Total comments: 6
Patch Set 9 : Added new ReadFileToString API with a max_size argument #
Total comments: 2
Patch Set 10 : Added new ReadFileToString API with a max_size argument #
Total comments: 4
Patch Set 11 : Added new ReadFileToString API with a max_size argument #
Total comments: 6
Messages
Total messages: 28 (0 generated)
Can you please review this CL.
This is pretty close - good work! Please edit the bug description to be more descriptive - the bug entries go into the git/SVN log, so you can see some examples by typing "git log". "Solution for issue 339417" is a bit too terse and requires people to look up the bug to see what's going on. Try changing the description to something more like: Added new ReadFileToString API with a max_size argument and then below that go into details of your actual change. Oh, and then have a line like this: BUG=339417 This tells our tools what bug is associated with this CL so it can automatically update the bug. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode130 base/file_util.cc:130: bool checkSz, size_t maxsize) { nit: checkSz does not adhere to the style guidelines (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml). Should be "check_size". https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode135 base/file_util.cc:135: return false; nit: space between if and ( and also make sure indent level is 2 characters, not 3. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode137 base/file_util.cc:137: if(!contents) { space before open paren https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode138 base/file_util.cc:138: CloseFile(file); This breaks the old behavior of priming the disk cache when you pass a null "contents", as documented in the comments for ReadFileToDisk(). https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode146 base/file_util.cc:146: // hence the file is read sequentially as opposed to one-shot read nit: period at the end of this sentence. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode149 base/file_util.cc:149: if(checkSz && maxsize < sz) { space before open paren. Suggestion - instead of having a "checkSz" param, just treat maxsize = 0 as "unlimited size". Or else have people pass in SIZE_MAX if they want an unlimited size. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode167 base/file_util.cc:167: return ReadFileToStringImpl(path, contents, false, 0); Suggest passing SIZE_MAX for an unlimited read, rather than having a separate variable (see above). https://codereview.chromium.org/157593005/diff/1/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/1/base/file_util.h#newcode141 base/file_util.h:141: // This function has an additional check on the maximum size of the file nit: put a period at the end of the sentence. Also please clarify what the behavior is if the size of the file exceeds "maxsize" (do we return an error, or just truncate the amount read)? https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... File base/files/file_util_proxy_unittest.cc (right): https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... base/files/file_util_proxy_unittest.cc:220: EXPECT_FALSE(ReadFileToString(path_, &data, 0)); Can we move these to a separate test? Also, can you add a test for ReadFileToString(path, NULL) which should return true if there were no errors (since this functionality was broken in your CL)?
The CQ bit was unchecked by kaliamoorthi@chromium.org
The CQ bit was checked by kaliamoorthi@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/157593005/diff/1/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode130 base/file_util.cc:130: bool checkSz, size_t maxsize) { On 2014/02/10 14:29:46, Andrew T Wilson wrote: > nit: checkSz does not adhere to the style guidelines > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml). Should be > "check_size". Further nit, also per style guide: Either put all arguments on one line, or put each argument on a separate line. The middle ground is not allowed. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode143 base/file_util.cc:143: size_t len, sz = 0; Nit 1: Per style guide, avid abbreviations whenever possible. Thus, s/sz/size/ Nit 2: Do not declare more than one variable per line. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode148 base/file_util.cc:148: sz += len; Nit: What happens when |sz + len| overflows size_t? This will never happen in practice for now because nobody has enough memory to read a file of that size. But it is good to keep in mind that overflows can and do occur. https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... File base/files/file_util_proxy_unittest.cc (right): https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... base/files/file_util_proxy_unittest.cc:221: EXPECT_FALSE(ReadFileToString(path_, &data, 2)); Nit: Could you order these tests by increasing maxsize? They appear a bit chaotic right now.
PTAL https://codereview.chromium.org/157593005/diff/1/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode130 base/file_util.cc:130: bool checkSz, size_t maxsize) { On 2014/02/10 14:55:37, bartfab wrote: > On 2014/02/10 14:29:46, Andrew T Wilson wrote: > > nit: checkSz does not adhere to the style guidelines > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml). Should be > > "check_size". > > Further nit, also per style guide: Either put all arguments on one line, or put > each argument on a separate line. The middle ground is not allowed. Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode130 base/file_util.cc:130: bool checkSz, size_t maxsize) { On 2014/02/10 14:29:46, Andrew T Wilson wrote: > nit: checkSz does not adhere to the style guidelines > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml). Should be > "check_size". Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode135 base/file_util.cc:135: return false; On 2014/02/10 14:29:46, Andrew T Wilson wrote: > nit: space between if and ( and also make sure indent level is 2 characters, not > 3. Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode137 base/file_util.cc:137: if(!contents) { On 2014/02/10 14:29:46, Andrew T Wilson wrote: > space before open paren Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode138 base/file_util.cc:138: CloseFile(file); On 2014/02/10 14:29:46, Andrew T Wilson wrote: > This breaks the old behavior of priming the disk cache when you pass a null > "contents", as documented in the comments for ReadFileToDisk(). I changed the code to retain the old behavior of priming the disk cache. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode143 base/file_util.cc:143: size_t len, sz = 0; On 2014/02/10 14:55:37, bartfab wrote: > Nit 1: Per style guide, avid abbreviations whenever possible. Thus, s/sz/size/ > > Nit 2: Do not declare more than one variable per line. Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode146 base/file_util.cc:146: // hence the file is read sequentially as opposed to one-shot read On 2014/02/10 14:29:46, Andrew T Wilson wrote: > nit: period at the end of this sentence. Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode148 base/file_util.cc:148: sz += len; On 2014/02/10 14:55:37, bartfab wrote: > Nit: What happens when |sz + len| overflows size_t? > > This will never happen in practice for now because nobody has enough memory to > read a file of that size. But it is good to keep in mind that overflows can and > do occur. Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode149 base/file_util.cc:149: if(checkSz && maxsize < sz) { On 2014/02/10 14:29:46, Andrew T Wilson wrote: > space before open paren. > > Suggestion - instead of having a "checkSz" param, just treat maxsize = 0 as > "unlimited size". Or else have people pass in SIZE_MAX if they want an unlimited > size. Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.cc#newcode167 base/file_util.cc:167: return ReadFileToStringImpl(path, contents, false, 0); On 2014/02/10 14:29:46, Andrew T Wilson wrote: > Suggest passing SIZE_MAX for an unlimited read, rather than having a separate > variable (see above). Done. https://codereview.chromium.org/157593005/diff/1/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/1/base/file_util.h#newcode141 base/file_util.h:141: // This function has an additional check on the maximum size of the file On 2014/02/10 14:29:46, Andrew T Wilson wrote: > nit: put a period at the end of the sentence. Also please clarify what the > behavior is if the size of the file exceeds "maxsize" (do we return an error, or > just truncate the amount read)? Done. https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... File base/files/file_util_proxy_unittest.cc (right): https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... base/files/file_util_proxy_unittest.cc:220: EXPECT_FALSE(ReadFileToString(path_, &data, 0)); On 2014/02/10 14:29:46, Andrew T Wilson wrote: > Can we move these to a separate test? > > Also, can you add a test for ReadFileToString(path, NULL) which should return > true if there were no errors (since this functionality was broken in your CL)? Done. https://codereview.chromium.org/157593005/diff/1/base/files/file_util_proxy_u... base/files/file_util_proxy_unittest.cc:221: EXPECT_FALSE(ReadFileToString(path_, &data, 2)); On 2014/02/10 14:55:37, bartfab wrote: > Nit: Could you order these tests by increasing maxsize? They appear a bit > chaotic right now. Done.
https://codereview.chromium.org/157593005/diff/390001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/390001/base/file_util.cc#newco... base/file_util.cc:168: return ReadFileToStringImpl(path, contents, ~(size_t)0); Can you use SIZE_MAX here instead of ~0? I'm OK with having a separate ReadFileToStringImpl but you should do one of the following: 1) Wrap it in an anonymous namespace (since it's not intended to be exported from this file). 2) Move it into ReadFileToString(path, contents, maxsize) itself, and have ReadFileToString(path, contents) just call the 3-parameter version of the function. https://codereview.chromium.org/157593005/diff/390001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/390001/base/file_util.h#newcod... base/file_util.h:144: BASE_EXPORT bool ReadFileToString(const FilePath& path, One nit: put | around variable names in contents. So, for example: "When the file size is greater than |maxsize|, the function returns false with |contents| set to an empty string". https://codereview.chromium.org/157593005/diff/390001/base/files/file_util_pr... File base/files/file_util_proxy_unittest.cc (right): https://codereview.chromium.org/157593005/diff/390001/base/files/file_util_pr... base/files/file_util_proxy_unittest.cc:224: TEST_F(FileUtilProxyTest, ReadFileToString) { I think this code should live in base/file_util_unittest.cc, not in file_util_proxy_unittest.cc. https://codereview.chromium.org/157593005/diff/390001/base/files/file_util_pr... base/files/file_util_proxy_unittest.cc:258: EXPECT_TRUE(base::DeleteFile(test_path(), false)); Add another call to make sure trying to read a deleted file fails: EXPECT_FALSE(ReadFileToString(test_path(), NULL));
https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:130: std::string* contents, Nit: Align the arguments. https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:143: // Many files supplied in path have incorrect size (proc files, streams etc) Nit: What do you mean by incorrect size? IIUC, the size is not incorrect, it just cannot reliably be determined because it may change at any time. https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:146: if ((maxsize - size) < len) { Nit: A matter of taste but I think |len > maxsize - size| would be easier to parse: "If the amount of data read (len) is larger than the amount still permitted (maxsize - size), report an error." https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:162: return ReadFileToString(path, contents, std::numeric_limits<size_t>::max()); Nit: #include <limits> https://codereview.chromium.org/157593005/diff/460001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/460001/base/file_util.h#newcod... base/file_util.h:143: // with |contents| pointing to empty string. Nit: s/to empty/to an empty/ https://codereview.chromium.org/157593005/diff/460001/base/file_util.h#newcod... base/file_util.h:146: size_t maxsize); Nit: s/maxsize/max_size/ to be more consistent with other variable names in Chrome code https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest... base/file_util_unittest.cc:1941: EXPECT_EQ(data.length(), (size_t)0); Nit: Use C++-style casts, not C-style casts. In this case, you can avoid all casts by using 0u instead of 0. https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest... base/file_util_unittest.cc:1963: // Make sure we can & do delete the created file to prevent leaks on the bots. Nit 1: Avoid abbreviations, s/&/and/. Nit 2: The comment is unnecessary and misleading. Yes, one purpose of deleting the file is to prevent leaks on bots. But you also use the nonexistence of the file for further testing. https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest... base/file_util_unittest.cc:1964: EXPECT_TRUE(base::DeleteFile(file_path, false)); Nit: For readability, add a blank like after this one.
PTAL https://codereview.chromium.org/157593005/diff/390001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/390001/base/file_util.cc#newco... base/file_util.cc:168: return ReadFileToStringImpl(path, contents, ~(size_t)0); On 2014/02/11 10:27:12, Andrew T Wilson wrote: > Can you use SIZE_MAX here instead of ~0? > > I'm OK with having a separate ReadFileToStringImpl but you should do one of the > following: > > 1) Wrap it in an anonymous namespace (since it's not intended to be exported > from this file). > > 2) Move it into ReadFileToString(path, contents, maxsize) itself, and have > ReadFileToString(path, contents) just call the 3-parameter version of the > function. Changed to std::numeric_limits<size_t>::max() as discussed. Chose option 2 and removed ReadFileToStringImpl. https://codereview.chromium.org/157593005/diff/390001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/390001/base/file_util.h#newcod... base/file_util.h:144: BASE_EXPORT bool ReadFileToString(const FilePath& path, On 2014/02/11 10:27:12, Andrew T Wilson wrote: > One nit: put | around variable names in contents. > > So, for example: > > "When the file size is greater than |maxsize|, the function returns false with > |contents| set to an empty string". Done. https://codereview.chromium.org/157593005/diff/390001/base/files/file_util_pr... File base/files/file_util_proxy_unittest.cc (right): https://codereview.chromium.org/157593005/diff/390001/base/files/file_util_pr... base/files/file_util_proxy_unittest.cc:224: TEST_F(FileUtilProxyTest, ReadFileToString) { On 2014/02/11 10:27:12, Andrew T Wilson wrote: > I think this code should live in base/file_util_unittest.cc, not in > file_util_proxy_unittest.cc. Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:130: std::string* contents, On 2014/02/11 12:51:12, bartfab wrote: > Nit: Align the arguments. Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:143: // Many files supplied in path have incorrect size (proc files, streams etc) On 2014/02/11 12:51:12, bartfab wrote: > Nit: What do you mean by incorrect size? IIUC, the size is not incorrect, it > just cannot reliably be determined because it may change at any time. Some files especially procfs have incorrect size (reported as 0) https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:146: if ((maxsize - size) < len) { On 2014/02/11 12:51:12, bartfab wrote: > Nit: A matter of taste but I think |len > maxsize - size| would be easier to > parse: "If the amount of data read (len) is larger than the amount still > permitted (maxsize - size), report an error." Since it is not a coding guideline, I think the current one is fine https://codereview.chromium.org/157593005/diff/460001/base/file_util.cc#newco... base/file_util.cc:162: return ReadFileToString(path, contents, std::numeric_limits<size_t>::max()); On 2014/02/11 12:51:12, bartfab wrote: > Nit: #include <limits> Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/460001/base/file_util.h#newcod... base/file_util.h:143: // with |contents| pointing to empty string. On 2014/02/11 12:51:12, bartfab wrote: > Nit: s/to empty/to an empty/ Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util.h#newcod... base/file_util.h:146: size_t maxsize); On 2014/02/11 12:51:12, bartfab wrote: > Nit: s/maxsize/max_size/ to be more consistent with other variable names in > Chrome code Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest... base/file_util_unittest.cc:1941: EXPECT_EQ(data.length(), (size_t)0); On 2014/02/11 12:51:12, bartfab wrote: > Nit: Use C++-style casts, not C-style casts. In this case, you can avoid all > casts by using 0u instead of 0. Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest... base/file_util_unittest.cc:1963: // Make sure we can & do delete the created file to prevent leaks on the bots. On 2014/02/11 12:51:12, bartfab wrote: > Nit 1: Avoid abbreviations, s/&/and/. > Nit 2: The comment is unnecessary and misleading. Yes, one purpose of deleting > the file is to prevent leaks on bots. But you also use the nonexistence of the > file for further testing. Done. https://codereview.chromium.org/157593005/diff/460001/base/file_util_unittest... base/file_util_unittest.cc:1964: EXPECT_TRUE(base::DeleteFile(file_path, false)); On 2014/02/11 12:51:12, bartfab wrote: > Nit: For readability, add a blank like after this one. Done.
lgtm
Hi Brett, Can you please do a owner review.
lgtm https://codereview.chromium.org/157593005/diff/460003/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/460003/base/file_util.cc#newco... base/file_util.cc:144: // Many files supplied in path have incorrect size (proc files, streams etc) Nit 1: s/path/|path|/ Nit 2: s/etc/etc./ Nit 3: End the sentence after the bracket. Then, start a new sentence by capitalizing "Hence" on the next line. https://codereview.chromium.org/157593005/diff/460003/base/file_util.cc#newco... base/file_util.cc:145: // hence the file is read sequentially as opposed to one-shot read. Nit 1: Add comma after "hence." Nit 2: s/one-shot/a one-shot/
Can you provide more reason for wanting thsi in the CL description or bug? Like, are you actually seeing 2GB policy files that cause OOM crashes? How does that even happen? Or is this more of a theoretical problem?
On 2014/02/13 18:54:58, brettw wrote: > Can you provide more reason for wanting thsi in the CL description or bug? Like, > are you actually seeing 2GB policy files that cause OOM crashes? How does that > even happen? Or is this more of a theoretical problem? There's code in the policy framework that is manually reading in data from files using ReadFile rather than using ReadFileFromString(), precisely because it wants to enforce a size limit. Take a look here for one example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... They do these checks on ChromeOS because if for some reason a really large file is loaded, it would effectively brick the device. There's similar code executed on startup on desktop Chrome that is currently not doing this size check, because it's waiting for this API.
Looking at this function signature, I'd expect somewhat different behavior than it's doing. If the file was too big, I'd expect the buffer still to be filled, but just up to the max size, rather than have the entire read failed. Do you think this is a reasonable behavior? This is how most "read a file" functions work. If you think this is good, we can probably still return false to indicate that something bad happened, and if the calling code cares about the difference, it can check if there's any data in the buffer (we should probably be sure to clear the buffer in all other failure cases then).
Hi Bret, > Looking at this function signature, I'd expect somewhat different behavior than > it's doing. If the file was too big, I'd expect the buffer still to be filled, > but just up to the max size, rather than have the entire read failed. Do you > think this is a reasonable behavior? This is how most "read a file" functions > work. Yes this seems to be a reasonable behavior. I modified the API to reflect this behavior. > If you think this is good, we can probably still return false to indicate that > something bad happened, and if the calling code cares about the difference, it > can check if there's any data in the buffer (we should probably be sure to clear > the buffer in all other failure cases then). The current implementation clears content irrespective of the error. So the content value can help the caller to differentiate the type of error (to some extent).
PTAL https://codereview.chromium.org/157593005/diff/460003/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/460003/base/file_util.cc#newco... base/file_util.cc:144: // Many files supplied in path have incorrect size (proc files, streams etc) On 2014/02/12 16:27:11, bartfab wrote: > Nit 1: s/path/|path|/ > Nit 2: s/etc/etc./ > Nit 3: End the sentence after the bracket. Then, start a new sentence by > capitalizing "Hence" on the next line. Done. https://codereview.chromium.org/157593005/diff/460003/base/file_util.cc#newco... base/file_util.cc:145: // hence the file is read sequentially as opposed to one-shot read. On 2014/02/12 16:27:11, bartfab wrote: > Nit 1: Add comma after "hence." > Nit 2: s/one-shot/a one-shot/ Done.
I'm not sure if changing the behavior of ReadFileToString() from its current behavior of appending data to the string, to its new behavior of replacing the contents of the string, is a good idea. Perhaps Brett has some insight here (not sure if the old behavior was intentional or not). https://codereview.chromium.org/157593005/diff/640001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/640001/base/file_util.cc#newco... base/file_util.cc:133: if(contents) Two things: 1) space before the open paren. 2) this is changing the behavior of ReadFileToString(), which in its current incarnation appends to the current value of contents, and never clears it. Are we sure that nobody depends on the current implementation? https://codereview.chromium.org/157593005/diff/640001/base/file_util.cc#newco... base/file_util.cc:145: bool return_var = true; Change this name to something like "read_status". https://codereview.chromium.org/157593005/diff/640001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/640001/base/file_util.h#newcod... base/file_util.h:147: // Useful for unit tests. Not sure what "Useful for unit tests" means - perhaps you can omit this? Or are you saying that priming the disk cache is what's useful for unit tests? If so, then I'd say something like "priming the disk cache, which is useful for unit tests". Mostly I'd just omit this.
https://codereview.chromium.org/157593005/diff/710001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/157593005/diff/710001/base/file_util_unittest... base/file_util_unittest.cc:1965: EXPECT_FALSE(ReadFileToString(file_path, &data)); Nit: Check the contents of |data| using EXPECT statements here as well.
LGTM. I doubt the appending behavior of the current function was intended. I went through the callers quickly (there are <300) and didn't notice anything alarming, and actually some places where people were deliberately clearing before calling again (probably each of these took some debugging time on the part of the caller). So I think we should definitely change it. https://codereview.chromium.org/157593005/diff/750001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/750001/base/file_util.cc#newco... base/file_util.cc:150: if (contents && (max_size - size) > 0) Does the max_size - size check do anything? These are both unsigned types so the result can't be negative (I think this is OK based on your logic, but please double-check that's what you expected). But then this is just a check != 0. But there's no problem appending 0 bytes. I think I'd find it clearer if this wasn't here and we just did if (contents) ... https://codereview.chromium.org/157593005/diff/750001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/750001/base/file_util.h#newcod... base/file_util.h:136: // side effect of priming the disk cache, which is useful for unit tests. Can you document that this replaces rather than appends, and that the buffer will be cleared on failure?
Made a few final changes to address review comments. https://codereview.chromium.org/157593005/diff/640001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/640001/base/file_util.cc#newco... base/file_util.cc:133: if(contents) On 2014/02/17 14:38:22, Andrew T Wilson wrote: > Two things: > > 1) space before the open paren. > 2) this is changing the behavior of ReadFileToString(), which in its current > incarnation appends to the current value of contents, and never clears it. Are > we sure that nobody depends on the current implementation? I have fixed 1 in a new patch. https://codereview.chromium.org/157593005/diff/640001/base/file_util.cc#newco... base/file_util.cc:145: bool return_var = true; On 2014/02/17 14:38:22, Andrew T Wilson wrote: > Change this name to something like "read_status". Done. https://codereview.chromium.org/157593005/diff/640001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/640001/base/file_util.h#newcod... base/file_util.h:147: // Useful for unit tests. On 2014/02/17 14:38:22, Andrew T Wilson wrote: > Not sure what "Useful for unit tests" means - perhaps you can omit this? Or are > you saying that priming the disk cache is what's useful for unit tests? If so, > then I'd say something like "priming the disk cache, which is useful for unit > tests". > > Mostly I'd just omit this. Done. https://codereview.chromium.org/157593005/diff/710001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/157593005/diff/710001/base/file_util_unittest... base/file_util_unittest.cc:1965: EXPECT_FALSE(ReadFileToString(file_path, &data)); On 2014/02/17 15:25:55, bartfab wrote: > Nit: Check the contents of |data| using EXPECT statements here as well. Done. https://codereview.chromium.org/157593005/diff/750001/base/file_util.cc File base/file_util.cc (right): https://codereview.chromium.org/157593005/diff/750001/base/file_util.cc#newco... base/file_util.cc:150: if (contents && (max_size - size) > 0) On 2014/02/18 05:24:33, brettw wrote: > Does the max_size - size check do anything? These are both unsigned types so the > result can't be negative (I think this is OK based on your logic, but please > double-check that's what you expected). But then this is just a check != 0. But > there's no problem appending 0 bytes. I think I'd find it clearer if this wasn't > here and we just did if (contents) ... Done. https://codereview.chromium.org/157593005/diff/750001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/750001/base/file_util.h#newcod... base/file_util.h:136: // side effect of priming the disk cache, which is useful for unit tests. On 2014/02/18 05:24:33, brettw wrote: > Can you document that this replaces rather than appends, and that the buffer > will be cleared on failure? Done.
The CQ bit was checked by kaliamoorthi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/157593005/88...
https://codereview.chromium.org/157593005/diff/880001/base/file_util.h File base/file_util.h (right): https://codereview.chromium.org/157593005/diff/880001/base/file_util.h#newcod... base/file_util.h:136: // side effect of priming the disk cache, which is useful for unit tests. Nit: The sentence now says that the function is "useful for" X which is "useful for" Y. Not exactly the best wording possible :). https://codereview.chromium.org/157593005/diff/880001/base/file_util.h#newcod... base/file_util.h:137: // The function replaces rather than append to |contents|, further |contents| Nit 1: s/append/appending/ Nit 2: Document the behavior on error in a separate sentence. Nit 3: "could be cleared" leaves the actual behavior undefined. Better state when the contents is cleared and when it is not. https://codereview.chromium.org/157593005/diff/880001/base/file_util.h#newcod... base/file_util.h:146: // |contents| may be NULL, in which case this function is useful for its Nit: The sentence now says that the function is "useful for" X which is "useful for" Y. Not exactly the best wording possible :). https://codereview.chromium.org/157593005/diff/880001/base/file_util.h#newcod... base/file_util.h:148: // The function replaces rather than append to |contents|, further |contents| Nit 1: s/append/appending/ Nit 2: Document the behavior on error in a separate sentence. Nit 3: "could be cleared" leaves the actual behavior undefined. Better state when the contents is cleared and when it is not. https://codereview.chromium.org/157593005/diff/880001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/157593005/diff/880001/base/file_util_unittest... base/file_util_unittest.cc:1965: EXPECT_FALSE(ReadFileToString(file_path, &data)); Nit: You could add a data = "temp" statement before this to verify that it clears |data|. https://codereview.chromium.org/157593005/diff/880001/base/file_util_unittest... base/file_util_unittest.cc:1968: EXPECT_FALSE(ReadFileToString(file_path, &data, 6)); Nit: You could add a data = "temp" statement before this to verify that it clears |data|.
Message was sent while issue was closed.
Change committed as 251771 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
