From af64702dd0c5be3a2434d670f9e3dbb803f3e388 Mon Sep 17 00:00:00 2001 From: Ian Lee Date: Wed, 27 Apr 2016 19:14:03 +0100 Subject: [PATCH] Simplify FileNode Cache. Remove the need for PlaceHolder. - Store std::shared_ptr's directly, no need for PlaceHolder wrapper - Use std::forward_list to store shared_ptr's, the shared_ptr is guaranteed to be unique hence a set is an unnecessary overhead - Refactor code to use FileNode * instead of PlaceHolder * --- encfs/Context.cpp | 36 ++++++++++++------------------------ encfs/Context.h | 15 +++++---------- encfs/DirNode.cpp | 12 ++++-------- encfs/FileUtils.cpp | 5 ++--- encfs/encfs.cpp | 36 +++++++++++++++++++----------------- encfs/encfsctl.cpp | 28 ++++++++++++---------------- encfs/main.cpp | 3 +-- 7 files changed, 55 insertions(+), 80 deletions(-) diff --git a/encfs/Context.cpp b/encfs/Context.cpp index 348b2a9..0a6bcd5 100644 --- a/encfs/Context.cpp +++ b/encfs/Context.cpp @@ -95,10 +95,9 @@ std::shared_ptr 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(); + return it->second.front(); } + return std::shared_ptr(); } void EncFS_Context::renameNode(const char *from, const char *to) { @@ -106,36 +105,27 @@ void EncFS_Context::renameNode(const char *from, const char *to) { FileMap::iterator it = openFiles.find(std::string(from)); if (it != openFiles.end()) { - std::set val = it->second; + auto val = it->second; openFiles.erase(it); openFiles[std::string(to)] = val; } } -std::shared_ptr EncFS_Context::getNode(void *pl) { - Placeholder *ph = (Placeholder *)pl; - return ph->node; + +FileNode *EncFS_Context::putNode(const char *path, + std::shared_ptr &&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 &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); - - rAssert(rmCount == 1); + it->second.pop_front(); // if no more references to this file, remove the record all together if (it->second.empty()) { @@ -145,8 +135,6 @@ void EncFS_Context::eraseNode(const char *path, void *pl) { openFiles.erase(it); storedName.assign(storedName.length(), '\0'); } - - delete ph; } } // namespace encfs diff --git a/encfs/Context.h b/encfs/Context.h index 1ddbfaa..5f9fab7 100644 --- a/encfs/Context.h +++ b/encfs/Context.h @@ -21,10 +21,10 @@ #ifndef _Context_incl_ #define _Context_incl_ +#include #include #include #include - #include #include @@ -42,15 +42,14 @@ class EncFS_Context { EncFS_Context(); ~EncFS_Context(); - std::shared_ptr getNode(void *ptr); std::shared_ptr lookupNode(const char *path); int getAndResetUsageCounter(); int openFileCount() const; - void *putNode(const char *path, const std::shared_ptr &node); + FileNode *putNode(const char *path, std::shared_ptr &&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 node; - Placeholder(const std::shared_ptr &ptr) : node(ptr) {} - }; - - typedef std::unordered_map > FileMap; + typedef std::unordered_map< + std::string, std::forward_list>> FileMap; mutable pthread_mutex_t contextMutex; FileMap openFiles; diff --git a/encfs/DirNode.cpp b/encfs/DirNode.cpp index c0111c7..3612d6d 100644 --- a/encfs/DirNode.cpp +++ b/encfs/DirNode.cpp @@ -633,7 +633,6 @@ std::shared_ptr DirNode::renameNode(const char *from, const char *to, std::shared_ptr DirNode::findOrCreate(const char *plainName) { std::shared_ptr 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 DirNode::findOrCreate(const char *plainName) { return node; } -std::shared_ptr DirNode::lookupNode(const char *plainName, - const char *requestor) { - (void)requestor; + +shared_ptr DirNode::lookupNode(const char *plainName, + const char * /* requestor */) { Lock _lock(mutex); - - std::shared_ptr node = findOrCreate(plainName); - - return node; + return findOrCreate(plainName); } /* diff --git a/encfs/FileUtils.cpp b/encfs/FileUtils.cpp index cbc08af..7497272 100644 --- a/encfs/FileUtils.cpp +++ b/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; } diff --git a/encfs/encfs.cpp b/encfs/encfs.cpp index 3896809..5182ee0 100644 --- a/encfs/encfs.cpp +++ b/encfs/encfs.cpp @@ -122,24 +122,25 @@ static int withFileNode(const char *opName, const char *path, if (!FSRoot) return res; try { - std::shared_ptr 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(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(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(finfo->fh)); return ESUCCESS; } catch (encfs::Error &err) { RLOG(ERROR) << "error caught in release: " << err.what(); diff --git a/encfs/encfsctl.cpp b/encfs/encfsctl.cpp index 308512c..4964d23 100644 --- a/encfs/encfsctl.cpp +++ b/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; } diff --git a/encfs/main.cpp b/encfs/main.cpp index 0abf09b..571bc9a 100644 --- a/encfs/main.cpp +++ b/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() {