From 361e10d09c644dea5cedeb0961bb83e370835f54 Mon Sep 17 00:00:00 2001 From: Andrew Gaul Date: Tue, 26 Sep 2023 07:52:55 +0900 Subject: [PATCH] Add scope_guard for ad-hoc resource management (#2313) References #2261. Suggested by: https://stackoverflow.com/questions/10270328/the-simplest-and-neatest-c11-scopeguard --- src/fdcache.cpp | 5 +---- src/fdcache_fdinfo.cpp | 19 +++++++++++++------ src/fdcache_stat.cpp | 19 ++++++++++--------- src/s3fs_util.h | 28 ++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/src/fdcache.cpp b/src/fdcache.cpp index 52b4616..405ba17 100644 --- a/src/fdcache.cpp +++ b/src/fdcache.cpp @@ -935,6 +935,7 @@ bool FdManager::RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const S3FS_PRN_CACHE(fp, CACHEDBG_FMT_CRIT_HEAD, "Could not open cache file"); continue; } + scope_guard guard([&]() { close(cache_file_fd); }); // get inode number for cache file struct stat st; @@ -943,7 +944,6 @@ bool FdManager::RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const S3FS_PRN_CACHE(fp, CACHEDBG_FMT_FILE_PROB, object_file_path.c_str(), strOpenedWarn.c_str()); S3FS_PRN_CACHE(fp, CACHEDBG_FMT_CRIT_HEAD, "Could not get file inode number for cache file"); - close(cache_file_fd); continue; } ino_t cache_file_inode = st.st_ino; @@ -956,7 +956,6 @@ bool FdManager::RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const S3FS_PRN_CACHE(fp, CACHEDBG_FMT_FILE_PROB, object_file_path.c_str(), strOpenedWarn.c_str()); S3FS_PRN_CACHE(fp, CACHEDBG_FMT_CRIT_HEAD, "Could not load cache file stats information"); - close(cache_file_fd); continue; } cfstat.Release(); @@ -967,7 +966,6 @@ bool FdManager::RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const S3FS_PRN_CACHE(fp, CACHEDBG_FMT_FILE_PROB, object_file_path.c_str(), strOpenedWarn.c_str()); S3FS_PRN_CACHE(fp, CACHEDBG_FMT_CRIT_HEAD2 "The cache file size(%lld) and the value(%lld) from cache file stats are different", static_cast(st.st_size), static_cast(pagelist.Size())); - close(cache_file_fd); continue; } @@ -999,7 +997,6 @@ bool FdManager::RawCheckAllCache(FILE* fp, const char* cache_stat_top_dir, const } err_area_list.clear(); warn_area_list.clear(); - close(cache_file_fd); } } closedir(statsdir); diff --git a/src/fdcache_fdinfo.cpp b/src/fdcache_fdinfo.cpp index 58d7542..f313eca 100644 --- a/src/fdcache_fdinfo.cpp +++ b/src/fdcache_fdinfo.cpp @@ -28,6 +28,7 @@ #include "common.h" #include "s3fs_logger.h" +#include "s3fs_util.h" #include "fdcache_fdinfo.h" #include "fdcache_pseudofd.h" #include "fdcache_entity.h" @@ -179,19 +180,25 @@ bool PseudoFdInfo::OpenUploadFd(AutoLock::Type type) } // duplicate fd - if(-1 == (upload_fd = dup(physical_fd)) || 0 != lseek(upload_fd, 0, SEEK_SET)){ + int fd; + if(-1 == (fd = dup(physical_fd))){ S3FS_PRN_ERR("Could not duplicate physical file descriptor(errno=%d)", errno); - if(-1 != upload_fd){ - close(upload_fd); - } + return false; + } + scope_guard guard([&]() { close(fd); }); + + if(0 != lseek(fd, 0, SEEK_SET)){ + S3FS_PRN_ERR("Could not seek physical file descriptor(errno=%d)", errno); return false; } struct stat st; - if(-1 == fstat(upload_fd, &st)){ + if(-1 == fstat(fd, &st)){ S3FS_PRN_ERR("Invalid file descriptor for uploading(errno=%d)", errno); - close(upload_fd); return false; } + + guard.dismiss(); + upload_fd = fd; return true; } diff --git a/src/fdcache_stat.cpp b/src/fdcache_stat.cpp index ba1a2ee..c337409 100644 --- a/src/fdcache_stat.cpp +++ b/src/fdcache_stat.cpp @@ -208,34 +208,35 @@ bool CacheFileStat::RawOpen(bool readonly) return false; } // open + int tmpfd; if(readonly){ - if(-1 == (fd = open(sfile_path.c_str(), O_RDONLY))){ + if(-1 == (tmpfd = open(sfile_path.c_str(), O_RDONLY))){ S3FS_PRN_ERR("failed to read only open cache stat file path(%s) - errno(%d)", path.c_str(), errno); return false; } }else{ - if(-1 == (fd = open(sfile_path.c_str(), O_CREAT|O_RDWR, 0600))){ + if(-1 == (tmpfd = open(sfile_path.c_str(), O_CREAT|O_RDWR, 0600))){ S3FS_PRN_ERR("failed to open cache stat file path(%s) - errno(%d)", path.c_str(), errno); return false; } } + scope_guard guard([&]() { close(tmpfd); }); + // lock - if(-1 == flock(fd, LOCK_EX)){ + if(-1 == flock(tmpfd, LOCK_EX)){ S3FS_PRN_ERR("failed to lock cache stat file(%s) - errno(%d)", path.c_str(), errno); - close(fd); - fd = -1; return false; } // seek top - if(0 != lseek(fd, 0, SEEK_SET)){ + if(0 != lseek(tmpfd, 0, SEEK_SET)){ S3FS_PRN_ERR("failed to lseek cache stat file(%s) - errno(%d)", path.c_str(), errno); - flock(fd, LOCK_UN); - close(fd); - fd = -1; + flock(tmpfd, LOCK_UN); return false; } S3FS_PRN_DBG("file locked(%s - %s)", path.c_str(), sfile_path.c_str()); + guard.dismiss(); + fd = tmpfd; return true; } diff --git a/src/s3fs_util.h b/src/s3fs_util.h index d41111c..b83d15b 100644 --- a/src/s3fs_util.h +++ b/src/s3fs_util.h @@ -21,6 +21,8 @@ #ifndef S3FS_S3FS_UTIL_H_ #define S3FS_S3FS_UTIL_H_ +#include + #ifndef CLOCK_REALTIME #define CLOCK_REALTIME 0 #endif @@ -77,6 +79,32 @@ std::string s3fs_str_realtime(); // Wrap fclose since it is illegal to take the address of a stdlib function int s3fs_fclose(FILE* fp); +class scope_guard { +public: + template + explicit scope_guard(Callable&& undo_func) + : func(std::forward(undo_func)) + {} + + ~scope_guard() { + if(func != nullptr) { + func(); + } + } + + void dismiss() { + func = nullptr; + } + + scope_guard(const scope_guard&) = delete; + scope_guard(scope_guard&& other) = delete; + scope_guard& operator=(const scope_guard&) = delete; + scope_guard& operator=(scope_guard&&) = delete; + +private: + std::function func; +}; + #endif // S3FS_S3FS_UTIL_H_ /*