Mark removable Drive caches for cryptohome.

components/drive/chromeos/file_cache.cc

Index: components/drive/chromeos/file_cache.cc
diff --git a/components/drive/chromeos/file_cache.cc b/components/drive/chromeos/file_cache.cc
index a2eb6eab02abacd59ef7fc3b50e6c9869f1bf6d2..3ec6e2b55c9f41b0f41ee52d2198a684964858ec 100644
--- a/components/drive/chromeos/file_cache.cc
+++ b/components/drive/chromeos/file_cache.cc
@@ -4,6 +4,12 @@
 
 #include "components/drive/chromeos/file_cache.h"
 
+#include <fcntl.h>
+#include <linux/fs.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/xattr.h>
 #include <unistd.h>
 
 #include <queue>
@@ -17,6 +23,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_info.h"
@@ -45,6 +52,95 @@ base::FilePath GetPathForId(const base::FilePath& cache_directory,
       base::FilePath::FromUTF8Unsafe(util::EscapeCacheFileName(id)));
 }
 
+// Set extended file attribute as |name| |value| pair.

[hashimoto, 2016/05/02 08:29:20]

nit: "Set" -> "Sets"
The same goes for others.

[oka, 2016/05/09 14:27:39]

Done.

+bool Setxattr(const std::string& path, const std::string& name,

[hashimoto, 2016/05/02 08:29:20]

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.

[oka, 2016/05/09 14:27:39]

Done.

+    const std::string& value) {
+  return setxattr(path.c_str(), name.c_str(), value.c_str(), value.size() + 1,
+      0) == 0;
+}
+
+// Get extended file attribute associated with |name| and set it to |value|.
+// If nothing is associated with |name|, set "" to |value|.
+bool Getxattr(const std::string& path, const std::string& name,
+    std::string* value, size_t size) {

[hashimoto, 2016/05/02 08:29:20]

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?

[oka, 2016/05/09 14:27:40]

That's a good idea. Though I removed the func, I'll do it in cryptohome code.
Thanks.

+  value->resize(size - 1);
+  char* v = string_as_array(value);
+  ssize_t ret_size = getxattr(path.c_str(), name.c_str(), v, size);
+  if (ret_size < 0) {
+    if (errno == ENODATA) {
+      value->assign("");
+      return true;
+    } else {
+      PLOG(ERROR) << "getxattr: " << name;
+      return false;
+    }
+  }
+  value->resize(ret_size - 1);
+  return true;
+}
+
+// Change attributes of the file with |flags|,
+// e.g. FS_UNRM_FL | FS_IMMUTABLE_FL. See linux/fs.h for available flags.

[hashimoto, 2016/05/02 08:29:20]

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?

[oka, 2016/05/09 14:27:40]

Done.

+// Returns whether operation succeeded.
+bool Chattr(const std::string& path, int64_t flags) {

[hashimoto, 2016/05/02 08:29:20]

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().

[oka, 2016/05/09 14:27:40]

Done.

+  int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY));

[hashimoto, 2016/05/02 08:29:20]

Why don't you use base::File with which you don't need to call close()
everywhere?

[oka, 2016/05/09 14:27:40]

Done.

+  if (fd < 0) {
+    PLOG(ERROR) << "open: " << path;
+    return false;
+  }
+  if (ioctl(fd, FS_IOC_SETFLAGS, &flags) < 0) {
+    PLOG(ERROR) << "ioctl" << path;
+    IGNORE_EINTR(close(fd));
+    return false;
+  }
+  IGNORE_EINTR(close(fd));
+  return true;
+}
+
+int64_t Lsattr(const std::string& path) {

[hashimoto, 2016/05/02 08:29:20]

Please add a comment.

[oka, 2016/05/09 14:27:40]

Done.

+  int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY));
+  if (fd < 0) {
+    PLOG(ERROR) << "open: " << path;
+    return -1;
+  }
+  int64_t flags = 0;
+  if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0) {
+    PLOG(ERROR) << "ioctl";
+    IGNORE_EINTR(close(fd));
+    return -1;
+  }
+  IGNORE_EINTR(close(fd));
+  return flags;
+}
+
+// Mark the cache file to be removable by cryptohome.
+bool SetRemovable(const base::FilePath& path) {
+  int64_t flags = Lsattr(path.value());
+  if (flags < 0) return false;
+  if ((flags & FS_NODUMP_FL) == FS_NODUMP_FL) return true;
+
+  return Chattr(path.value(), flags | FS_NODUMP_FL);
+}
+
+// Mark the cache file to be unremovable by cryptohome.
+bool UnsetRemovable(const base::FilePath& path) {
+  // File is to be created.
+  if (!base::PathExists(path)) return true;

[hashimoto, 2016/05/02 08:29:20]

This behavior is surprising, and not symmetric with SetRemovable().
How about checking cache_entry.is_present() in the caller code when appropriate?

[oka, 2016/05/09 14:27:40]

Done. Added existence check to Pin.

+
+  int64_t flags = Lsattr(path.value());
+  if (flags < 0) return false;
+  if ((flags & FS_NODUMP_FL) == 0) return true;
+
+  return Chattr(path.value(), flags & ~(static_cast<int64_t>(FS_NODUMP_FL)));
+}
+
+// Mark |path| as drive cache dir.
+// Returns if the operation succeeded.
+bool MarkAsDriveCacheDir(const base::FilePath& path) {
+  return SetRemovable(path)
+      && Setxattr(path.value(), "user.GCacheFiles", "true");

[hashimoto, 2016/05/02 08:29:20]

What is the point of using "true" instead of an empty string?

Also, please make "user.GCacheFiles" a constant with a name.

[oka, 2016/05/09 14:27:39]

Done.

+}
+
 typedef std::pair<base::File::Info, ResourceEntry> CacheInfo;
 
 class CacheInfoLatestCompare {
@@ -282,6 +378,15 @@ FileError FileCache::Store(const std::string& id,
   cache_state->set_is_present(true);
   if (md5.empty())
     cache_state->set_is_dirty(true);
+
+  if (!cache_state->is_pinned() && !cache_state->is_dirty()) {
+    if (!SetRemovable(dest_path))
+      return FILE_ERROR_FAILED;
+  } else {
+    if (!UnsetRemovable(dest_path))
+      return FILE_ERROR_FAILED;
+  }
+
   return storage_->PutEntry(entry);
 }
 
@@ -292,8 +397,12 @@ FileError FileCache::Pin(const std::string& id) {
   FileError error = storage_->GetEntry(id, &entry);
   if (error != FILE_ERROR_OK)
     return error;
+
   entry.mutable_file_specific_info()->mutable_cache_state()->set_is_pinned(
       true);
+  if (!UnsetRemovable(GetCacheFilePath(entry.local_id())))
+    return FILE_ERROR_FAILED;
+
   return storage_->PutEntry(entry);
 }
 
@@ -310,6 +419,10 @@ FileError FileCache::Unpin(const std::string& id) {
   if (entry.file_specific_info().cache_state().is_present()) {
     entry.mutable_file_specific_info()->mutable_cache_state()->set_is_pinned(
         false);
+    if (!entry.file_specific_info().cache_state().is_dirty()) {
+      if (!SetRemovable(GetCacheFilePath(entry.local_id())))
+        return FILE_ERROR_FAILED;
+    }
   } else {
     // Remove the existing entry if we are unpinning a non-present file.
     entry.mutable_file_specific_info()->clear_cache_state();
@@ -376,6 +489,9 @@ FileError FileCache::OpenForWrite(
   }
 
   entry.mutable_file_specific_info()->mutable_cache_state()->set_is_dirty(true);
+  if (!UnsetRemovable(GetCacheFilePath(entry.local_id())))
+    return FILE_ERROR_FAILED;
+
   entry.mutable_file_specific_info()->mutable_cache_state()->clear_md5();
   error = storage_->PutEntry(entry);
   if (error != FILE_ERROR_OK)
@@ -447,6 +563,11 @@ FileError FileCache::ClearDirty(const std::string& id) {
 
   entry.mutable_file_specific_info()->mutable_cache_state()->set_is_dirty(
       false);
+  if (!entry.file_specific_info().cache_state().is_pinned()) {
+    if (!SetRemovable(GetCacheFilePath(entry.local_id())))
+      return FILE_ERROR_FAILED;
+  }
+
   return storage_->PutEntry(entry);
 }
 
@@ -513,9 +634,31 @@ bool FileCache::Initialize() {
 
   if (!RenameCacheFilesToNewFormat())
     return false;
+
+  if (!ResolveMetadataInconsistency()) {
+    return false;
+  }
+
+  if (ShouldStartDriveCacheMigration()) {
+    if (!MigrateCacheFiles()) {
+      return false;
+    }
+  }
+
   return true;
 }
 
+bool FileCache::ShouldStartDriveCacheMigration() {

[hashimoto, 2016/05/02 08:29:19]

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?

[oka, 2016/05/09 14:27:39]

I experimentally confirmed setting attributes is not that slow (0.5s for 1000
files).
Let's run the attribute setting code on every run.

+  std::string value = "";
+  if (!Getxattr(cache_file_directory_.value(), "user.GCacheFiles", &value, 5)) {
+    PLOG(WARNING) << "Getxattr: " << cache_file_directory_.value();
+  }
+  int64_t flags = Lsattr(cache_file_directory_.value());
+  if (flags < 0) return false;
+
+  return value != "true" || ((flags & FS_NODUMP_FL) == 0);
+}
+
 void FileCache::Destroy() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -684,46 +827,57 @@ bool FileCache::RenameCacheFilesToNewFormat() {
   return true;
 }
 
-// static
-bool FileCache::MigrateCacheFiles(const base::FilePath& from,
-                                  const base::FilePath& to_files,
-                                  const base::FilePath& to_links,
-                                  ResourceMetadataStorage* metadata_storage) {
+bool FileCache::MigrateCacheFiles() {
   std::unique_ptr<ResourceMetadataStorage::Iterator> it =
-      metadata_storage->GetIterator();
+      storage_->GetIterator();
+
   for (; !it->IsAtEnd(); it->Advance()) {
     const ResourceEntry& entry = it->GetValue();
     if (!entry.file_specific_info().cache_state().is_present()) {
       continue;
     }
 
-    // Ignore missing cache file case since it never succeeds.
-    // TODO(yawano): handle this case at metadata validation in FileCache.
-    const base::FilePath move_from = GetPathForId(from, entry.local_id());
-    if (!base::PathExists(move_from)) {
-      continue;
+    const base::FilePath filepath = GetPathForId(cache_file_directory_,
+        entry.local_id());
+
+    if (!base::PathExists(filepath)) {
+      PLOG(WARNING) << "Cache and metadata are inconsistent.";

[hashimoto, 2016/05/02 08:29:19]

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.

[oka, 2016/05/09 14:27:39]

Merged two funcs, and removed this warning.

+      return false;
     }
 
-    // Create hard link to cache file if it's pinned or dirty. cryptohome will
-    // not delete a cache file if there is a hard link to it.
     const FileCacheEntry& file_cache_entry =
         entry.file_specific_info().cache_state();
     if (file_cache_entry.is_pinned() || file_cache_entry.is_dirty()) {
-      const base::FilePath link_path = GetPathForId(to_links, entry.local_id());
-      int link_result = link(move_from.AsUTF8Unsafe().c_str(),
-                             link_path.AsUTF8Unsafe().c_str());
-      if (link_result != 0 && errno != EEXIST) {
-        return false;
-      }
+      if (!UnsetRemovable(filepath)) return false;
+    } else {
+      if (!SetRemovable(filepath)) return false;
     }
+  }
 
-    // Move cache file.
-    const base::FilePath move_to = GetPathForId(to_files, entry.local_id());
-    if (!base::Move(move_from, move_to)) {
-      return false;
+  return MarkAsDriveCacheDir(cache_file_directory_);
+}
+
+bool FileCache::ResolveMetadataInconsistency() {
+  std::unique_ptr<ResourceMetadataStorage::Iterator>
+    it = storage_->GetIterator();
+  for (; !it->IsAtEnd(); it->Advance()) {
+    ResourceEntry entry = it->GetValue();
+
+    if (!entry.file_specific_info().has_cache_state() ||
+        !entry.file_specific_info().cache_state().is_present()) {

[hashimoto, 2016/05/02 08:29:20]

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).

[oka, 2016/05/09 14:27:40]

Done.
I merged ResolveMetadataInconsistency and MigrateCacheFiles, and named it
MigrateCacheFilesResolvingInconsistency.

+      continue;
+    }
+
+    if (base::PathExists(GetCacheFilePath(entry.local_id()))) {
+      continue;
     }
-  }
 
+    entry.mutable_file_specific_info()->mutable_cache_state()->set_is_present(
+        false);
+
+    if (storage_->PutEntry(entry) != FILE_ERROR_OK)
+      return false;
+  }
   return true;
 }