Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1918243004:
Mark removable Drive caches for cryptohome. (Closed)
DescriptionMark removable Drive caches for cryptohome.
Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH4U/edit#heading=h.sa3sh9r88p48
- Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d.
- Toggle the status of metadata if cache files have been removed by
cryptohome.
- Remove a file if it exsits but metadata says not (inconsistency possibly happens on abrupt shutdown).
- Toggle +d as necessary when pinned/dirty status are changed.
Committed: https://crrev.com/1d5bdf653f3859d29ae3b6ad7535cefc3702f45d
Cr-Commit-Position: refs/heads/master@{#393229}
Patch Set 1 #Patch Set 2 : Mark removable drive caches for cryptohome. #Patch Set 3 : Add a test: MigrationHappensOnlyOnce #Patch Set 4 : Nit #
Total comments: 52
Patch Set 5 : Resovle comments. #Patch Set 6 : Nit #Patch Set 7 : Nit #Patch Set 8 : Nit #
Total comments: 14
Patch Set 9 : Resolve comments #Patch Set 10 : Resolve comments. #Patch Set 11 : Nit #
Total comments: 7
Patch Set 12 : Resolve comments. #Patch Set 13 : Resolve comments. #
Total comments: 8
Patch Set 14 : Resolve comments. #Patch Set 15 : Resolve comments. #Patch Set 16 : Nit #Patch Set 17 : Nit #
Dependent Patchsets: Messages
Total messages: 32 (10 generated)
Description was changed from ========== WIP: Cryptohome. Completed the code, but failes with segmentation fault. BUG=533750 ========== to ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... 1. If migration has not happen, do it. Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. 2. Toggle +d as necessary when pinned/dirty status are changed. BUG=533750 TEST=out/Release/unit_tests --gtest_filter=FileCacheTest* ==========
Description was changed from ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... 1. If migration has not happen, do it. Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. 2. Toggle +d as necessary when pinned/dirty status are changed. BUG=533750 TEST=out/Release/unit_tests --gtest_filter=FileCacheTest* ========== to ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - If migration has not happen, do it; mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Toggle +d as necessary when pinned/dirty status are changed. ==========
Description was changed from
==========
Mark removable Drive caches for cryptohome.
Design doc:
https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH...
- If migration has not happen, do it; mark the Drive cache dir with +d (chattr)
and user.GCacheFiles (setxattr), and mark removable files in it with +d.
- Toggle the status of metadata if cache files have been removed by
cryptohome.
- Toggle +d as necessary when pinned/dirty status are changed.
==========
to
==========
Mark removable Drive caches for cryptohome.
Design doc:
https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH...
- If migration has not happen, do it; mark the Drive cache dir with +d (chattr)
and user.GCacheFiles (setxattr), and mark removable files in it with +d.
- Toggle the status of metadata if cache files have been removed by
cryptohome.
- Toggle +d as necessary when pinned/dirty status are changed.
==========
oka@chromium.org changed reviewers: + hashimoto@chromium.org
PTAL
Description was changed from ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - If migration has not happen, do it; mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Toggle +d as necessary when pinned/dirty status are changed. ========== to ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - If migration has not happened, do it; mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Toggle +d as necessary when pinned/dirty status are changed. ==========
https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:55: // Set extended file attribute as |name| |value| pair. nit: "Set" -> "Sets" The same goes for others. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:56: bool Setxattr(const std::string& path, const std::string& name, How about making the name more descriptive (e.g. SetExtendedFileAttribute)? Our style guide says we should "eschew abbreviation". (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) The same goes for others. Also, please use base::FilePath for file paths. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:65: std::string* value, size_t size) { Do we actually care about the contents of the attribute? Can't we just call getxattr(path, name, nullptr, 0) and check that it doesn't fail with ENOATTR to know if it's marked or not? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:83: // e.g. FS_UNRM_FL | FS_IMMUTABLE_FL. See linux/fs.h for available flags. nit: FS_UNRM_FL and FS_IMMUTABLE_FL are totally unrelated to this code. How about using FS_NODUMP_FL as an example here, with a description about what it means, and how cryptohome handles it? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:85: bool Chattr(const std::string& path, int64_t flags) { Could you give this function a more readable name (e.g. SetFileAttributes)? In comment you can write that this function does the same thing as chattr(1). The same goes for Lsattr(). https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:86: int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY)); Why don't you use base::File with which you don't need to call close() everywhere? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:100: int64_t Lsattr(const std::string& path) { Please add a comment. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:128: if (!base::PathExists(path)) return true; This behavior is surprising, and not symmetric with SetRemovable(). How about checking cache_entry.is_present() in the caller code when appropriate? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:141: && Setxattr(path.value(), "user.GCacheFiles", "true"); What is the point of using "true" instead of an empty string? Also, please make "user.GCacheFiles" a constant with a name. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:651: bool FileCache::ShouldStartDriveCacheMigration() { IIUC, because of IO scheduling, a system crash or a system power loss can result in a situation where /files has attributes, but files under that directory don't on the next boot. Can't we just run the attribute setting code on every run? How much does it affect the performance? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:844: PLOG(WARNING) << "Cache and metadata are inconsistent."; It's strange to abort the initialization with a warning. If we actually want to disable Drive here (this is what aborting the initialization means), we should emit an ERROR. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:867: !entry.file_specific_info().cache_state().is_present()) { Shouldn't we handle a case where !is_present but the file exists? Anything can happen because of IO scheduling in case of a system crash (e.g. kernel panic, power lost). https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.h:147: // Returns if Drive cache migration for crbug/533750 should be invoked. nit: crbug/533750 ".com" missing. The same goes for others https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.h:190: // Add appropriate file attributes to the Drive cache directory and files in nit: "Add" -> "Adds". https://google.github.io/styleguide/cppguide.html#Function_Comments The same goes for others. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.h:192: // Inconsistency between cache and metadata should be resolved before this nit: How about "cache" -> "cache files" to avoid confusion, as everything is related to cache in this class? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... File components/drive/chromeos/file_cache_unittest.cc (right): https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:49: int64_t Lsattr(const base::FilePath& file_path) { Please give this function a more readable name. BTW, instead of having this function which does the same thing as lsattr(1), how about having a function which returns if the file has FS_NODUMP_FL or not? I think it should make the code simpler and more readable. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:50: int fd = open(file_path.value().c_str(), O_RDONLY); HANDLE_EINTR should be used. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:53: if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0) return -1; Please use base::File. This line leaks fd. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:119: ASSERT_TRUE(cache_->Initialize()); Instead of calling Initialize() everywhere, how about resetting cache_ in the beginning of the migration test instead? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:434: base::FilePath dest_file_path = cache_files_dir_.AppendASCII(id); This path construction is an internal logic of FileCache, and it can change in future. Could you use FileCache::GetCacheFilePath()? FileCacheTest is marked as a friend of FileCache so you can add a wrapper function of GetCacheFilePath() to use it from tests. The existing migration test did this because GetCacheFilePath doesn't work with old_cache_dir. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:436: EXPECT_GE(flags, 0); What is the point of this check? You want to see if the top bit is set or not? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:821: // Set removable flag. How about reusing the function in file_cache.cc? (e.g. adding a new static public method to FileCache, or adding a new .cc file which hosts utility functions like this.) https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:825: ASSERT_EQ(ioctl(fd, FS_IOC_GETFLAGS, &flags), 0); Why aren't you using the lsattr function above? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:828: close(fd); Please use base::File (or base::ScopedFD) instead of manually calling close(). https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:850: EXPECT_GE(flags_a, 0); What is the point of these >=0 checks? https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:854: EXPECT_EQ(flags_a & FS_NODUMP_FL, 0); nit: Why not EXPECT_TRUE/EXPECT_FALSE?
PTAL https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:55: // Set extended file attribute as |name| |value| pair. On 2016/05/02 08:29:20, hashimoto wrote: > nit: "Set" -> "Sets" > The same goes for others. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:56: bool Setxattr(const std::string& path, const std::string& name, On 2016/05/02 08:29:20, hashimoto wrote: > How about making the name more descriptive (e.g. SetExtendedFileAttribute)? > > Our style guide says we should "eschew abbreviation". > (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) > The same goes for others. > > Also, please use base::FilePath for file paths. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:65: std::string* value, size_t size) { On 2016/05/02 08:29:20, hashimoto wrote: > Do we actually care about the contents of the attribute? > Can't we just call getxattr(path, name, nullptr, 0) and check that it doesn't > fail with ENOATTR to know if it's marked or not? That's a good idea. Though I removed the func, I'll do it in cryptohome code. Thanks. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:83: // e.g. FS_UNRM_FL | FS_IMMUTABLE_FL. See linux/fs.h for available flags. On 2016/05/02 08:29:20, hashimoto wrote: > nit: FS_UNRM_FL and FS_IMMUTABLE_FL are totally unrelated to this code. > How about using FS_NODUMP_FL as an example here, with a description about what > it means, and how cryptohome handles it? Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:85: bool Chattr(const std::string& path, int64_t flags) { On 2016/05/02 08:29:20, hashimoto wrote: > Could you give this function a more readable name (e.g. SetFileAttributes)? > In comment you can write that this function does the same thing as chattr(1). > The same goes for Lsattr(). Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:86: int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY)); On 2016/05/02 08:29:20, hashimoto wrote: > Why don't you use base::File with which you don't need to call close() > everywhere? Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:100: int64_t Lsattr(const std::string& path) { On 2016/05/02 08:29:20, hashimoto wrote: > Please add a comment. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:128: if (!base::PathExists(path)) return true; On 2016/05/02 08:29:20, hashimoto wrote: > This behavior is surprising, and not symmetric with SetRemovable(). > How about checking cache_entry.is_present() in the caller code when appropriate? Done. Added existence check to Pin. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:141: && Setxattr(path.value(), "user.GCacheFiles", "true"); On 2016/05/02 08:29:20, hashimoto wrote: > What is the point of using "true" instead of an empty string? > > Also, please make "user.GCacheFiles" a constant with a name. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:651: bool FileCache::ShouldStartDriveCacheMigration() { On 2016/05/02 08:29:19, hashimoto wrote: > IIUC, because of IO scheduling, a system crash or a system power loss can result > in a situation where /files has attributes, but files under that directory don't > on the next boot. > > Can't we just run the attribute setting code on every run? > How much does it affect the performance? I experimentally confirmed setting attributes is not that slow (0.5s for 1000 files). Let's run the attribute setting code on every run. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:844: PLOG(WARNING) << "Cache and metadata are inconsistent."; On 2016/05/02 08:29:19, hashimoto wrote: > It's strange to abort the initialization with a warning. > If we actually want to disable Drive here (this is what aborting the > initialization means), we should emit an ERROR. Merged two funcs, and removed this warning. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.cc:867: !entry.file_specific_info().cache_state().is_present()) { On 2016/05/02 08:29:20, hashimoto wrote: > Shouldn't we handle a case where !is_present but the file exists? > Anything can happen because of IO scheduling in case of a system crash (e.g. > kernel panic, power lost). Done. I merged ResolveMetadataInconsistency and MigrateCacheFiles, and named it MigrateCacheFilesResolvingInconsistency. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.h:147: // Returns if Drive cache migration for crbug/533750 should be invoked. On 2016/05/02 08:29:20, hashimoto wrote: > nit: crbug/533750 ".com" missing. > The same goes for others Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.h:190: // Add appropriate file attributes to the Drive cache directory and files in On 2016/05/02 08:29:20, hashimoto wrote: > nit: "Add" -> "Adds". > https://google.github.io/styleguide/cppguide.html#Function_Comments > The same goes for others. Done. Merged MigrateCacheFiles and ResolveMetadataInconsistency. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache.h:192: // Inconsistency between cache and metadata should be resolved before this On 2016/05/02 08:29:20, hashimoto wrote: > nit: How about "cache" -> "cache files" to avoid confusion, as everything is > related to cache in this class? Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... File components/drive/chromeos/file_cache_unittest.cc (right): https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:49: int64_t Lsattr(const base::FilePath& file_path) { On 2016/05/02 08:29:20, hashimoto wrote: > Please give this function a more readable name. > > BTW, instead of having this function which does the same thing as lsattr(1), how > about having a function which returns if the file has FS_NODUMP_FL or not? > I think it should make the code simpler and more readable. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:50: int fd = open(file_path.value().c_str(), O_RDONLY); On 2016/05/02 08:29:21, hashimoto wrote: > HANDLE_EINTR should be used. Removed. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:53: if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0) return -1; On 2016/05/02 08:29:20, hashimoto wrote: > Please use base::File. > This line leaks fd. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:119: ASSERT_TRUE(cache_->Initialize()); On 2016/05/02 08:29:20, hashimoto wrote: > Instead of calling Initialize() everywhere, how about resetting cache_ in the > beginning of the migration test instead? Done. Given we run migration everytime, moving this to Setup is fine. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:434: base::FilePath dest_file_path = cache_files_dir_.AppendASCII(id); On 2016/05/02 08:29:20, hashimoto wrote: > This path construction is an internal logic of FileCache, and it can change in > future. > Could you use FileCache::GetCacheFilePath()? > FileCacheTest is marked as a friend of FileCache so you can add a wrapper > function of GetCacheFilePath() to use it from tests. > > The existing migration test did this because GetCacheFilePath doesn't work with > old_cache_dir. Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:436: EXPECT_GE(flags, 0); On 2016/05/02 08:29:21, hashimoto wrote: > What is the point of this check? > You want to see if the top bit is set or not? Done. Used ExpectIsRemovable. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:821: // Set removable flag. On 2016/05/02 08:29:21, hashimoto wrote: > How about reusing the function in file_cache.cc? > (e.g. adding a new static public method to FileCache, or adding a new .cc file > which hosts utility functions like this.) This is the only usage of FS_IOC_SETFLAGS in this unit test. I think extracting a file or making the method public is overkill for now. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:825: ASSERT_EQ(ioctl(fd, FS_IOC_GETFLAGS, &flags), 0); On 2016/05/02 08:29:20, hashimoto wrote: > Why aren't you using the lsattr function above? Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:828: close(fd); On 2016/05/02 08:29:21, hashimoto wrote: > Please use base::File (or base::ScopedFD) instead of manually calling close(). Done. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:850: EXPECT_GE(flags_a, 0); On 2016/05/02 08:29:20, hashimoto wrote: > What is the point of these >=0 checks? Replaced with ExpectIs*. https://codereview.chromium.org/1918243004/diff/60001/components/drive/chrome... components/drive/chromeos/file_cache_unittest.cc:854: EXPECT_EQ(flags_a & FS_NODUMP_FL, 0); On 2016/05/02 08:29:20, hashimoto wrote: > nit: Why not EXPECT_TRUE/EXPECT_FALSE? Removed.
https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:63: bool SetFileAttributes(const base::FilePath& path, int64_t flags) { Does this code work on a 32-bit device (e.g. arm)? |flags|' type is always 64-bit here, but the kernel header says it's long which can be 32-bit on a 32-bit devices. https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.... https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:378: if (base::PathExists(file_path)) { Checking is_present() is better than base::PathExists as it's consistent with other parts of this file? https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:800: const bool fileIsPresent = base::PathExists(filepath); nit: The names of variables and data members should be all lowercase. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:810: entry.mutable_file_specific_info()->mutable_cache_state()->set_is_present( !file exists && is_present case: Shouldn't we clear is_dirty and md5 of the cache entry? file exists && !is_present case: Probably we should just remove the file instead of fixing the metadata, as we have no clue about the state of the found file. (probably we shouldn't, I'll also think about this...) https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.h:57: static const std::string kGCacheFilesAttribute; Variables of class type with static storage duration are forbidden by our C++ style guide. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Please make this char[]. https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache_unittest.cc (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache_unittest.cc:55: EXPECT_EQ(static_cast<int64_t>(FS_NODUMP_FL | FS_EXTENT_FL), When something goes wrong, EXPECT_EQ here results in all error messages saying the failure is happening at this line, not at the appropriate location within the tests. Instead of these ExpectIs functions, how about having functions which just returns true or false, and using them with EXPECT_TRUE/FALSE in the tests?
PTAL. Thanks! https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:63: bool SetFileAttributes(const base::FilePath& path, int64_t flags) { On 2016/05/10 10:33:48, hashimoto wrote: > Does this code work on a 32-bit device (e.g. arm)? > > |flags|' type is always 64-bit here, but the kernel header says it's long which > can be 32-bit on a 32-bit devices. > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.... Used long. Same for GetFileAttributes. https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:378: if (base::PathExists(file_path)) { On 2016/05/10 10:33:48, hashimoto wrote: > Checking is_present() is better than base::PathExists as it's consistent with > other parts of this file? Done. https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:800: const bool fileIsPresent = base::PathExists(filepath); On 2016/05/10 10:33:48, hashimoto wrote: > nit: The names of variables and data members should be all lowercase. > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:810: entry.mutable_file_specific_info()->mutable_cache_state()->set_is_present( On 2016/05/10 10:33:48, hashimoto wrote: > !file exists && is_present case: > Shouldn't we clear is_dirty and md5 of the cache entry? > > file exists && !is_present case: > Probably we should just remove the file instead of fixing the metadata, as we > have no clue about the state of the found file. > (probably we shouldn't, I'll also think about this...) Done. I think removing the file is fine since this situation only happens when user tries to remove the file but it failed halfway (due to abrupt shutdown or something). https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.h:57: static const std::string kGCacheFilesAttribute; On 2016/05/10 10:33:49, hashimoto wrote: > Variables of class type with static storage duration are forbidden by our C++ > style guide. > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > Please make this char[]. Done. https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache_unittest.cc (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache_unittest.cc:55: EXPECT_EQ(static_cast<int64_t>(FS_NODUMP_FL | FS_EXTENT_FL), On 2016/05/10 10:33:49, hashimoto wrote: > When something goes wrong, EXPECT_EQ here results in all error messages saying > the failure is happening at this line, not at the appropriate location within > the tests. > > Instead of these ExpectIs functions, how about having functions which just > returns true or false, and using them with EXPECT_TRUE/FALSE in the tests? Done.
https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:63: bool SetFileAttributes(const base::FilePath& path, int64_t flags) { On 2016/05/11 04:24:10, oka wrote: > On 2016/05/10 10:33:48, hashimoto wrote: > > Does this code work on a 32-bit device (e.g. arm)? > > > > |flags|' type is always 64-bit here, but the kernel header says it's long > which > > can be 32-bit on a 32-bit devices. > > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.... > > Used long. Same for GetFileAttributes. Please also replace int64_t above with long. The same goes for almost all places which use int64_t. If you don't want to add NOLINT everywhere, you can avoid it by typedef'ing long (e.g. typedef long FileAttributes;) https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.cc:815: if (!base::DeleteFile(filepath, false /* recursive */)) Sorry for not saying this earlier, but I realized that this code is not needed as FreeDiskSpaceIfNeededFor() is already deleting files without corresponding metadata entries. FreeDiskSpaceIfNeededFor() uses base::FileEnumerator so it also deletes files which cannot be found with the storage iterator. https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.cc:833: // Update file attribues for cryptohome. As the proposed code is using "continue" in many places, it's a bit difficult to figure out this line can be reached only when file_is_present && file_cache_entry->is_present(). How about structuring the code like this without using "continue"? if (base::PathExists()) { if (file_cache_entry->is_present()) { (set attributes) } else { (remove file) } } else { if (file_cache_entry->is_present()) { (update metadata) } } https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.h:193: bool MigrateCacheFilesResolvingInconsistency(); "Migrate" sounds a bit misleading here as we are no longer moving files from one directory another, and this functions is run on every boot. I couldn't come up with a nice word to describe what this function is doing, but it should be something like "fix metadata and file attributes".
yawano@chromium.org changed reviewers: + yawano@chromium.org
https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.h:193: bool MigrateCacheFilesResolvingInconsistency(); How about "validate"?
validate sounds like it has no side effect. FixMetadataAndFileAttributes? On Wed, May 11, 2016 at 3:32 PM <yawano@chromium.org> wrote: > > > https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... > File components/drive/chromeos/file_cache.h (right): > > > https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... > components/drive/chromeos/file_cache.h:193: bool > MigrateCacheFilesResolvingInconsistency(); > How about "validate"? > > https://codereview.chromium.org/1918243004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
sgtm. It doesn't look pretty, but I couldn't come up with a better name :/ On 2016/05/11 06:34:59, oka wrote: > validate sounds like it has no side effect. > FixMetadataAndFileAttributes? > > On Wed, May 11, 2016 at 3:32 PM <mailto:yawano@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... > > File components/drive/chromeos/file_cache.h (right): > > > > > > > https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... > > components/drive/chromeos/file_cache.h:193: bool > > MigrateCacheFilesResolvingInconsistency(); > > How about "validate"? > > > > https://codereview.chromium.org/1918243004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Resolve comments.
PTAL https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/140001/components/drive/chrom... components/drive/chromeos/file_cache.cc:63: bool SetFileAttributes(const base::FilePath& path, int64_t flags) { On 2016/05/11 06:29:12, hashimoto wrote: > On 2016/05/11 04:24:10, oka wrote: > > On 2016/05/10 10:33:48, hashimoto wrote: > > > Does this code work on a 32-bit device (e.g. arm)? > > > > > > |flags|' type is always 64-bit here, but the kernel header says it's long > > which > > > can be 32-bit on a 32-bit devices. > > > > > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.... > > > > Used long. Same for GetFileAttributes. > > Please also replace int64_t above with long. > The same goes for almost all places which use int64_t. > > If you don't want to add NOLINT everywhere, you can avoid it by typedef'ing long > (e.g. typedef long FileAttributes;) Done. Used typedef. https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.cc:815: if (!base::DeleteFile(filepath, false /* recursive */)) On 2016/05/11 06:29:12, hashimoto wrote: > Sorry for not saying this earlier, but I realized that this code is not needed > as FreeDiskSpaceIfNeededFor() is already deleting files without corresponding > metadata entries. > FreeDiskSpaceIfNeededFor() uses base::FileEnumerator so it also deletes files > which cannot be found with the storage iterator. Discussed offline. Keeping this logic will be OK, since 1. FreeDiskSpaceIfNeededFor do nothing if there's enough space. 2. We do nothing special to detect this case. https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.cc:833: // Update file attribues for cryptohome. On 2016/05/11 06:29:12, hashimoto wrote: > As the proposed code is using "continue" in many places, it's a bit difficult to > figure out this line can be reached only when file_is_present && > file_cache_entry->is_present(). > > How about structuring the code like this without using "continue"? > > if (base::PathExists()) { > if (file_cache_entry->is_present()) { > (set attributes) > } else { > (remove file) > } > } else { > if (file_cache_entry->is_present()) { > (update metadata) > } > } Done. https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... File components/drive/chromeos/file_cache.h (right): https://codereview.chromium.org/1918243004/diff/200001/components/drive/chrom... components/drive/chromeos/file_cache.h:193: bool MigrateCacheFilesResolvingInconsistency(); On 2016/05/11 06:32:47, yawano wrote: > How about "validate"? validate sounds like it has no side effect. Named it FixMetadataAndFileAttributes.
https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache.cc:831: file_cache_entry->set_is_pinned(false); Do we really need to unpin the file here? https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... File components/drive/chromeos/file_cache_unittest.cc (right): https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache_unittest.cc:748: TEST_F(FileCacheTest, MigrateCacheFiles) { Please fix the test name and the comment. https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache_unittest.cc:842: // Initialize resolves metadta inconsistency and invokes migration. ditto.
PTAL https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache.cc:831: file_cache_entry->set_is_pinned(false); On 2016/05/12 07:12:11, hashimoto wrote: > Do we really need to unpin the file here? Cryptohome removed a file means it was not pinned nor dirty. So removing pinned might be redundant but should be safe. https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... File components/drive/chromeos/file_cache_unittest.cc (right): https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache_unittest.cc:748: TEST_F(FileCacheTest, MigrateCacheFiles) { On 2016/05/12 07:12:11, hashimoto wrote: > Please fix the test name and the comment. Done. https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache_unittest.cc:842: // Initialize resolves metadta inconsistency and invokes migration. On 2016/05/12 07:12:11, hashimoto wrote: > ditto. Done.
https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache.cc:831: file_cache_entry->set_is_pinned(false); On 2016/05/12 09:18:32, oka wrote: > On 2016/05/12 07:12:11, hashimoto wrote: > > Do we really need to unpin the file here? > > Cryptohome removed a file means it was not pinned nor dirty. So removing pinned > might be redundant but should be safe. IIUC this can happen: 1. The file was pinned but not present. 2. Then the file was downloaded and became present. 3. Unclean shutdown happens, metadata update was saved to the disk, but the file move was not. In this case, IMO we shouldn't clear is_pinned().
Nit
PTAL https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... File components/drive/chromeos/file_cache.cc (right): https://codereview.chromium.org/1918243004/diff/240001/components/drive/chrom... components/drive/chromeos/file_cache.cc:831: file_cache_entry->set_is_pinned(false); On 2016/05/12 09:24:35, hashimoto wrote: > On 2016/05/12 09:18:32, oka wrote: > > On 2016/05/12 07:12:11, hashimoto wrote: > > > Do we really need to unpin the file here? > > > > Cryptohome removed a file means it was not pinned nor dirty. So removing > pinned > > might be redundant but should be safe. > > IIUC this can happen: > 1. The file was pinned but not present. > 2. Then the file was downloaded and became present. > 3. Unclean shutdown happens, metadata update was saved to the disk, but the > file move was not. > In this case, IMO we shouldn't clear is_pinned(). Done. Thank you!
Nit
lgtm thanks!
Description was changed from ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - If migration has not happened, do it; mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Toggle +d as necessary when pinned/dirty status are changed. ========== to ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Remove a file if it exsits but metadata says not (inconsistency possibly happens on abrupt shutdown). - Toggle +d as necessary when pinned/dirty status are changed. ==========
The CQ bit was checked by oka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1918243004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1918243004/320001
Message was sent while issue was closed.
Description was changed from ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Remove a file if it exsits but metadata says not (inconsistency possibly happens on abrupt shutdown). - Toggle +d as necessary when pinned/dirty status are changed. ========== to ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Remove a file if it exsits but metadata says not (inconsistency possibly happens on abrupt shutdown). - Toggle +d as necessary when pinned/dirty status are changed. ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Remove a file if it exsits but metadata says not (inconsistency possibly happens on abrupt shutdown). - Toggle +d as necessary when pinned/dirty status are changed. ========== to ========== Mark removable Drive caches for cryptohome. Design doc: https://docs.google.com/document/d/1Kdq-b5kpudbotQ1q2GRNd52f-oZUrf7mfuZb1XUqH... - Mark the Drive cache dir with +d (chattr) and user.GCacheFiles (setxattr), and mark removable files in it with +d. - Toggle the status of metadata if cache files have been removed by cryptohome. - Remove a file if it exsits but metadata says not (inconsistency possibly happens on abrupt shutdown). - Toggle +d as necessary when pinned/dirty status are changed. Committed: https://crrev.com/1d5bdf653f3859d29ae3b6ad7535cefc3702f45d Cr-Commit-Position: refs/heads/master@{#393229} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/1d5bdf653f3859d29ae3b6ad7535cefc3702f45d Cr-Commit-Position: refs/heads/master@{#393229}
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/1989863005/ by oka@chromium.org. The reason for reverting is: Corresponding changes for Chrome OS haven't been submitted. Let's punt this change to M53.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
