Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7484052:
Return true when file size is zero. (Closed)
|
Created:
9 years, 5 months ago by Huyen Modified:
9 years, 4 months ago CC:
chromium-reviews, brettw-cc_chromium.org Visibility:
Public. |
DescriptionIn test_file_util_mac.cc, when evicting file from cache, do not map a file when it is empty.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95334
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix according to code review. #
Total comments: 3
Patch Set 3 : Change comment to make it clear evicting an empty file will result in error #
Total comments: 8
Patch Set 4 : ... #
Total comments: 2
Patch Set 5 : ... #Messages
Total messages: 19 (0 generated)
I observed a bug on the Mac where CopyRecursiveDirNoCache() would fail when trying to mmap an empty file. This should fix it but not sure if it will affect anything else.
write a better CL description http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc... base/test/test_file_util_mac.cc:20: int64 length; add a comment why this is needed http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc... base/test/test_file_util_mac.cc:21: file_util::GetFileSize(file, &length); This could fail. log a warning and return false. http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc... base/test/test_file_util_mac.cc:22: if (length == 0) { if (!length) return true;
http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc... base/test/test_file_util_mac.cc:20: int64 length; On 2011/07/22 23:48:58, kkania wrote: > add a comment why this is needed Done. http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc... base/test/test_file_util_mac.cc:21: file_util::GetFileSize(file, &length); On 2011/07/22 23:48:58, kkania wrote: > This could fail. log a warning and return false. Done. http://codereview.chromium.org/7484052/diff/1/base/test/test_file_util_mac.cc... base/test/test_file_util_mac.cc:22: if (length == 0) { On 2011/07/22 23:48:58, kkania wrote: > if (!length) > return true; Done.
http://codereview.chromium.org/7484052/diff/4001/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/4001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:26: // When a file is empty, we do not need to purge it. maybe include that mapping it would cause an error?
http://codereview.chromium.org/7484052/diff/4001/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/4001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:21: if (!file_util::GetFileSize(file, &length)) { now that I think about it, I'm not sure this is correct. If you don't evict from the cache and the file changes, will later reads possibly still see the empty file? Or is an empty file not in the cache to begin with?
lgtm http://codereview.chromium.org/7484052/diff/4001/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/4001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:21: if (!file_util::GetFileSize(file, &length)) { On 2011/07/23 00:27:02, kkania wrote: > now that I think about it, I'm not sure this is correct. If you don't evict from > the cache and the file changes, will later reads possibly still see the empty > file? Or is an empty file not in the cache to begin with? If the OS cache had that sort of bugs/behaviors, you can assume all sorts of things would probably go wrong. In this case, since the file is zero sized, there really isn't anything for it to have cached.
LGTM
Presubmit check for 7484052-7001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: base/test/test_file_util_mac.cc
Presubmit check for 7484052-7001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: base/test/test_file_util_mac.cc
TVL, kkania, I think my latest patch needs LGTM in order to use commit bot. I've only updated the comments in the latest patch. Please send an LGTM when you have time. Thanks!
You need an LGTM from someone in base/OWNERS See http://www.chromium.org/developers/owners-files-1
Presubmit check for 7484052-7001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: base/test/test_file_util_mac.cc
Hi guys, it seems like I still don't have the owner's LGTM for this change. Know who haz teh power?
+Pawel Pawel, can you take a look? I need your approval as an OWNER.
LGTM with comments. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:22: DLOG(WARNING) << "failed to get size of " << file.value(); LOG(ERROR) here please. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:28: if (!length) LOG(WARNING) here please. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:28: if (!length) Please explicitly compare with 0 instead of an implicit bool conversion. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:33: DLOG(WARNING) << "failed to memory map " << file.value(); Oh, and also change those DLOG -> LOG.
http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac.cc File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:22: DLOG(WARNING) << "failed to get size of " << file.value(); On 2011/07/30 00:05:09, Paweł Hajdan Jr. wrote: > LOG(ERROR) here please. Done. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:28: if (!length) On 2011/07/30 00:05:09, Paweł Hajdan Jr. wrote: > LOG(WARNING) here please. Done. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:28: if (!length) On 2011/07/30 00:05:09, Paweł Hajdan Jr. wrote: > Please explicitly compare with 0 instead of an implicit bool conversion. Done. http://codereview.chromium.org/7484052/diff/7001/base/test/test_file_util_mac... base/test/test_file_util_mac.cc:33: DLOG(WARNING) << "failed to memory map " << file.value(); On 2011/07/30 00:05:09, Paweł Hajdan Jr. wrote: > Oh, and also change those DLOG -> LOG. Done.
LGTM with a comment. http://codereview.chromium.org/7484052/diff/15001/base/test/test_file_util_ma... File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/15001/base/test/test_file_util_ma... base/test/test_file_util_mac.cc:28: if (length == 0) I think you really want braces {} here. Otherwise it'll always return true!
http://codereview.chromium.org/7484052/diff/15001/base/test/test_file_util_ma... File base/test/test_file_util_mac.cc (right): http://codereview.chromium.org/7484052/diff/15001/base/test/test_file_util_ma... base/test/test_file_util_mac.cc:28: if (length == 0) On 2011/08/02 22:22:20, Paweł Hajdan Jr. wrote: > I think you really want braces {} here. Otherwise it'll always return true! Good catch!
Change committed as 95334 |
