Merge pull request #165 from ial0/filenode_cache_refactor

Filenode cache refactor

encfs/Context.cpp

@@ -95,10 +95,9 @@ std::shared_ptr<FileNode> EncFS_Context::lookupNode(const char *path) {
   if (it != openFiles.end()) {
     // all the items in the set point to the same node.. so just use the
     // first
-    return (*it->second.begin())->node;
-  } else {
-    return std::shared_ptr<FileNode>();
+    return it->second.front();
   }
+  return std::shared_ptr<FileNode>();
 }
 
 void EncFS_Context::renameNode(const char *from, const char *to) {
@@ -106,47 +105,28 @@ void EncFS_Context::renameNode(const char *from, const char *to) {
 
   FileMap::iterator it = openFiles.find(std::string(from));
   if (it != openFiles.end()) {
-    std::set<Placeholder *> val = it->second;
+    auto val = it->second;
     openFiles.erase(it);
     openFiles[std::string(to)] = val;
   }
 }
-std::shared_ptr<FileNode> EncFS_Context::getNode(void *pl) {
-  Placeholder *ph = (Placeholder *)pl;
-  return ph->node;
+
+FileNode *EncFS_Context::putNode(const char *path,
+                                 std::shared_ptr<FileNode> &&node) {
+  Lock lock(contextMutex);
+  auto &list = openFiles[std::string(path)];
+  list.push_front(std::move(node));
+  return list.front().get();
 }
 
-void *EncFS_Context::putNode(const char *path,
-                             const std::shared_ptr<FileNode> &node) {
+void EncFS_Context::eraseNode(const char *path, FileNode *pl) {
   Lock lock(contextMutex);
-  Placeholder *pl = new Placeholder(node);
-  openFiles[std::string(path)].insert(pl);
-
-  return (void *)pl;
-}
-
-void EncFS_Context::eraseNode(const char *path, void *pl) {
-  Lock lock(contextMutex);
-
-  Placeholder *ph = (Placeholder *)pl;
 
   FileMap::iterator it = openFiles.find(std::string(path));
   rAssert(it != openFiles.end());
 
-  int rmCount = it->second.erase(ph);
+  it->second.pop_front();
 
-  rAssert(rmCount == 1);
-
-  // if no more references to this file, remove the record all together
-  if (it->second.empty()) {
-    // attempts to make use of shallow copy to clear memory used to hold
-    // unencrypted filenames.. not sure this does any good..
-    std::string storedName = it->first;
-    openFiles.erase(it);
-    storedName.assign(storedName.length(), '\0');
-  }
-
-  delete ph;
 }
 
 }  // namespace encfs

encfs/Context.h

@@ -21,10 +21,10 @@
 #ifndef _Context_incl_
 #define _Context_incl_
 
+#include <forward_list>
 #include <memory>
 #include <pthread.h>
 #include <set>
-
 #include <string>
 #include <unordered_map>
 
@@ -42,15 +42,14 @@ class EncFS_Context {
   EncFS_Context();
   ~EncFS_Context();
 
-  std::shared_ptr<FileNode> getNode(void *ptr);
   std::shared_ptr<FileNode> lookupNode(const char *path);
 
   int getAndResetUsageCounter();
   int openFileCount() const;
 
-  void *putNode(const char *path, const std::shared_ptr<FileNode> &node);
+  FileNode *putNode(const char *path, std::shared_ptr<FileNode> &&node);
 
-  void eraseNode(const char *path, void *placeholder);
+  void eraseNode(const char *path, FileNode *fnode);
 
   void renameNode(const char *oldName, const char *newName);
 
@@ -81,13 +80,9 @@ class EncFS_Context {
    * release() is called.  std::shared_ptr then does our reference counting for
    * us.
    */
-  struct Placeholder {
-    std::shared_ptr<FileNode> node;
 
-    Placeholder(const std::shared_ptr<FileNode> &ptr) : node(ptr) {}
-  };
-
-  typedef std::unordered_map<std::string, std::set<Placeholder *> > FileMap;
+  typedef std::unordered_map<
+      std::string, std::forward_list<std::shared_ptr<FileNode>>> FileMap;
 
   mutable pthread_mutex_t contextMutex;
   FileMap openFiles;

encfs/DirNode.cpp

@@ -633,7 +633,6 @@ std::shared_ptr<FileNode> DirNode::renameNode(const char *from, const char *to,
 std::shared_ptr<FileNode> DirNode::findOrCreate(const char *plainName) {
   std::shared_ptr<FileNode> node;
   if (ctx) node = ctx->lookupNode(plainName);
-
   if (!node) {
     uint64_t iv = 0;
     string cipherName = naming->encodePath(plainName, &iv);
@@ -647,14 +646,11 @@ std::shared_ptr<FileNode> DirNode::findOrCreate(const char *plainName) {
 
   return node;
 }
-std::shared_ptr<FileNode> DirNode::lookupNode(const char *plainName,
-                                              const char *requestor) {
-  (void)requestor;
+
+shared_ptr<FileNode> DirNode::lookupNode(const char *plainName,
+                                         const char * /* requestor */) {
   Lock _lock(mutex);
-
-  std::shared_ptr<FileNode> node = findOrCreate(plainName);
-
-  return node;
+  return findOrCreate(plainName);
 }
 
 /*

encfs/FileUtils.cpp

@@ -866,9 +866,8 @@ static void selectBlockMAC(int *macBytes, int *macRandBytes, bool forceMac) {
           "performance but it also means [almost] any modifications or errors\n"
           "within a block will be caught and will cause a read error."));
   } else {
-    cout << "\n\n"
-         << _("You specified --require-macs.  "
-              "Enabling block authentication code headers...")
+    cout << "\n\n" << _("You specified --require-macs.  "
+                        "Enabling block authentication code headers...")
          << "\n\n";
     addMAC = true;
   }

encfs/encfs.cpp

@@ -122,24 +122,25 @@ static int withFileNode(const char *opName, const char *path,
   if (!FSRoot) return res;
 
   try {
-    std::shared_ptr<FileNode> fnode;
 
-    if (fi != NULL)
-      fnode = GET_FN(ctx, fi);
+    auto do_op = [&FSRoot, opName, &op](FileNode *fnode) {
+      rAssert(fnode != nullptr);
+      VLOG(1) << "op: " << opName << " : " << fnode->cipherName();
+
+      // check that we're not recursing into the mount point itself
+      if (FSRoot->touchesMountpoint(fnode->cipherName())) {
+        VLOG(1) << "op: " << opName << " error: Tried to touch mountpoint: '"
+                << fnode->cipherName() << "'";
+        return -EIO;
+      }
+      return op(fnode);
+    };
+
+    if (fi != nullptr)
+      res = do_op(reinterpret_cast<FileNode *>(fi->fh));
     else
-      fnode = FSRoot->lookupNode(path, opName);
+      res = do_op(FSRoot->lookupNode(path, opName).get());
 
-    rAssert(fnode.get() != NULL);
-    VLOG(1) << "op: " << opName << " : " << fnode->cipherName();
-
-    // check that we're not recursing into the mount point itself
-    if (FSRoot->touchesMountpoint(fnode->cipherName())) {
-      VLOG(1) << "op: " << opName << " error: Tried to touch mountpoint: '"
-              << fnode->cipherName() << "'";
-      return res;  // still -EIO
-    }
-
-    res = op(fnode.get());
     if (res < 0) {
       RLOG(DEBUG) << "op: " << opName << " error: " << strerror(-res);
     }
@@ -522,7 +523,8 @@ int encfs_open(const char *path, struct fuse_file_info *file) {
               << file->flags;
 
       if (res >= 0) {
-        file->fh = (uintptr_t)ctx->putNode(path, fnode);
+        file->fh =
+            reinterpret_cast<uintptr_t>(ctx->putNode(path, std::move(fnode)));
         res = ESUCCESS;
       }
     }
@@ -571,7 +573,7 @@ int encfs_release(const char *path, struct fuse_file_info *finfo) {
   EncFS_Context *ctx = context();
 
   try {
-    ctx->eraseNode(path, (void *)(uintptr_t)finfo->fh);
+    ctx->eraseNode(path, reinterpret_cast<FileNode *>(finfo->fh));
     return ESUCCESS;
   } catch (encfs::Error &err) {
     RLOG(ERROR) << "error caught in release: " << err.what();

encfs/encfsctl.cpp

@@ -182,31 +182,27 @@ static int showInfo(int argc, char **argv) {
       return EXIT_FAILURE;
     case Config_V3:
       // xgroup(diag)
-      cout << "\n"
-           << autosprintf(_("Version 3 configuration; "
-                            "created by %s\n"),
-                          config->creator.c_str());
+      cout << "\n" << autosprintf(_("Version 3 configuration; "
+                                    "created by %s\n"),
+                                  config->creator.c_str());
       break;
     case Config_V4:
       // xgroup(diag)
-      cout << "\n"
-           << autosprintf(_("Version 4 configuration; "
-                            "created by %s\n"),
-                          config->creator.c_str());
+      cout << "\n" << autosprintf(_("Version 4 configuration; "
+                                    "created by %s\n"),
+                                  config->creator.c_str());
       break;
     case Config_V5:
       // xgroup(diag)
-      cout << "\n"
-           << autosprintf(_("Version 5 configuration; "
-                            "created by %s (revision %i)\n"),
-                          config->creator.c_str(), config->subVersion);
+      cout << "\n" << autosprintf(_("Version 5 configuration; "
+                                    "created by %s (revision %i)\n"),
+                                  config->creator.c_str(), config->subVersion);
       break;
     case Config_V6:
       // xgroup(diag)
-      cout << "\n"
-           << autosprintf(_("Version 6 configuration; "
-                            "created by %s (revision %i)\n"),
-                          config->creator.c_str(), config->subVersion);
+      cout << "\n" << autosprintf(_("Version 6 configuration; "
+                                    "created by %s (revision %i)\n"),
+                                  config->creator.c_str(), config->subVersion);
       break;
   }
 

encfs/main.cpp

@@ -155,8 +155,7 @@ static void usage(const char *name) {
             "    encfs ~/.crypt ~/crypt\n"
             "\n")
        // xgroup(usage)
-       << _("For more information, see the man page encfs(1)") << "\n"
-       << endl;
+       << _("For more information, see the man page encfs(1)") << "\n" << endl;
 }
 
 static void FuseUsage() {