From: Joel Rosdahl Date: Mon, 1 Jun 2020 16:36:00 +0000 (+0200) Subject: Add and use Fd and Finalizer classes X-Git-Tag: v4.0~403 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e071bcfd37dfb02b4f1fa4b45fff8feb10d1cbd2;p=thirdparty%2Fccache.git Add and use Fd and Finalizer classes --- diff --git a/src/Fd.hpp b/src/Fd.hpp new file mode 100644 index 000000000..72583ac32 --- /dev/null +++ b/src/Fd.hpp @@ -0,0 +1,99 @@ +// Copyright (C) 2020 Joel Rosdahl and other contributors +// +// See doc/AUTHORS.adoc for a complete list of contributors. +// +// This program is free software; you can redistribute it and/or modify it +// under the terms of the GNU General Public License as published by the Free +// Software Foundation; either version 3 of the License, or (at your option) +// any later version. +// +// This program is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +// more details. +// +// You should have received a copy of the GNU General Public License along with +// this program; if not, write to the Free Software Foundation, Inc., 51 +// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +#include "system.hpp" + +#include "NonCopyable.hpp" + +class Fd : NonCopyable +{ +public: + Fd() = default; + explicit Fd(int fd); + Fd(Fd&& other_fd); + ~Fd(); + + explicit operator bool() const; + + int get() const; + int operator*() const; + + Fd& operator=(Fd&& other_fd); + + // Close wrapped fd before the lifetime of Fd has ended. + bool close(); + + // Release ownership of wrapped fd. + int release(); + +private: + int m_fd = -1; +}; + +inline Fd::Fd(int fd) : m_fd(fd) +{ +} + +inline Fd::Fd(Fd&& other_fd) : m_fd(other_fd.release()) +{ +} + +inline Fd::~Fd() +{ + close(); +} + +inline Fd::operator bool() const +{ + return m_fd != -1; +} + +inline int +Fd::get() const +{ + return m_fd; +} + +inline int +Fd::operator*() const +{ + assert(m_fd != -1); + return m_fd; +} + +inline Fd& +Fd::operator=(Fd&& other_fd) +{ + close(); + m_fd = other_fd.release(); + return *this; +} + +inline bool +Fd::close() +{ + return m_fd != -1 && ::close(release()) == 0; +} + +inline int +Fd::release() +{ + int fd = m_fd; + m_fd = -1; + return fd; +} diff --git a/src/Finalizer.hpp b/src/Finalizer.hpp new file mode 100644 index 000000000..59a427092 --- /dev/null +++ b/src/Finalizer.hpp @@ -0,0 +1,41 @@ +// Copyright (C) 2020 Joel Rosdahl and other contributors +// +// See doc/AUTHORS.adoc for a complete list of contributors. +// +// This program is free software; you can redistribute it and/or modify it +// under the terms of the GNU General Public License as published by the Free +// Software Foundation; either version 3 of the License, or (at your option) +// any later version. +// +// This program is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +// more details. +// +// You should have received a copy of the GNU General Public License along with +// this program; if not, write to the Free Software Foundation, Inc., 51 +// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +#include "system.hpp" + +#include + +class Finalizer +{ +public: + Finalizer(std::function finalizer); + ~Finalizer(); + +private: + std::function m_finalizer; +}; + +inline Finalizer::Finalizer(std::function finalizer) + : m_finalizer(finalizer) +{ +} + +inline Finalizer::~Finalizer() +{ + m_finalizer(); +} diff --git a/src/InodeCache.cpp b/src/InodeCache.cpp index 004e2ccc3..420742611 100644 --- a/src/InodeCache.cpp +++ b/src/InodeCache.cpp @@ -21,6 +21,8 @@ #ifdef INODE_CACHE_SUPPORTED # include "Config.hpp" +# include "Fd.hpp" +# include "Finalizer.hpp" # include "Stat.hpp" # include "Util.hpp" # include "ccache.hpp" @@ -129,24 +131,23 @@ InodeCache::mmap_file(const std::string& inode_cache_file) munmap(m_sr, sizeof(SharedRegion)); m_sr = nullptr; } - int fd = open(inode_cache_file.c_str(), O_RDWR); - if (fd < 0) { + Fd fd(open(inode_cache_file.c_str(), O_RDWR)); + if (!fd) { cc_log("Failed to open inode cache %s: %s", inode_cache_file.c_str(), strerror(errno)); return false; } bool is_nfs; - if (Util::is_nfs_fd(fd, &is_nfs) == 0 && is_nfs) { + if (Util::is_nfs_fd(*fd, &is_nfs) == 0 && is_nfs) { cc_log( "Inode cache not supported because the cache file is located on nfs: %s", inode_cache_file.c_str()); - close(fd); return false; } SharedRegion* sr = reinterpret_cast(mmap( - nullptr, sizeof(SharedRegion), PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0)); - close(fd); + nullptr, sizeof(SharedRegion), PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0)); + fd.close(); if (sr == reinterpret_cast(-1)) { cc_log("Failed to mmap %s: %s", inode_cache_file.c_str(), strerror(errno)); return false; @@ -258,22 +259,23 @@ InodeCache::create_new_file(const std::string& filename) // Create the new file to a temporary name to prevent other processes from // mapping it before it is fully initialized. - auto temp_fd = Util::create_temp_fd(filename); + auto temp_fd_and_path = Util::create_temp_fd(filename); + const auto& temp_path = temp_fd_and_path.second; + + Fd temp_fd(temp_fd_and_path.first); + Finalizer temp_file_remover([=] { unlink(temp_path.c_str()); }); + bool is_nfs; - if (Util::is_nfs_fd(temp_fd.first, &is_nfs) == 0 && is_nfs) { + if (Util::is_nfs_fd(*temp_fd, &is_nfs) == 0 && is_nfs) { cc_log( "Inode cache not supported because the cache file would be located on" " nfs: %s", filename.c_str()); - unlink(temp_fd.second.c_str()); - close(temp_fd.first); return false; } - int err = Util::fallocate(temp_fd.first, sizeof(SharedRegion)); + int err = Util::fallocate(*temp_fd, sizeof(SharedRegion)); if (err) { cc_log("Failed to allocate file space for inode cache: %s", strerror(err)); - unlink(temp_fd.second.c_str()); - close(temp_fd.first); return false; } SharedRegion* sr = @@ -281,12 +283,10 @@ InodeCache::create_new_file(const std::string& filename) sizeof(SharedRegion), PROT_READ | PROT_WRITE, MAP_SHARED, - temp_fd.first, + *temp_fd, 0)); if (sr == reinterpret_cast(-1)) { cc_log("Failed to mmap new inode cache: %s", strerror(errno)); - unlink(temp_fd.second.c_str()); - close(temp_fd.first); return false; } @@ -303,20 +303,18 @@ InodeCache::create_new_file(const std::string& filename) } munmap(sr, sizeof(SharedRegion)); - close(temp_fd.first); + temp_fd.close(); // link() will fail silently if a file with the same name already exists. // This will be the case if two processes try to create a new file // simultaneously. Thus close the current file handle and reopen a new one, // which will make us use the first created file even if we didn't win the // race. - if (link(temp_fd.second.c_str(), filename.c_str()) != 0) { + if (link(temp_path.c_str(), filename.c_str()) != 0) { cc_log("Failed to link new inode cache: %s", strerror(errno)); - unlink(temp_fd.second.c_str()); return false; } - unlink(temp_fd.second.c_str()); return true; } diff --git a/src/ccache.cpp b/src/ccache.cpp index 22c7c287f..c1091b0c1 100644 --- a/src/ccache.cpp +++ b/src/ccache.cpp @@ -22,7 +22,9 @@ #include "Args.hpp" #include "ArgsInfo.hpp" #include "Context.hpp" +#include "Fd.hpp" #include "File.hpp" +#include "Finalizer.hpp" #include "FormatNonstdStringView.hpp" #include "MiniTrace.hpp" #include "ProgressBar.hpp" @@ -245,7 +247,6 @@ do_remember_include_file(Context& ctx, bool system, struct hash* depend_mode_hash) { - struct hash* fhash = nullptr; bool is_pch = false; if (path.length() >= 2 && path[0] == '<' && path[path.length() - 1] == '>') { @@ -321,9 +322,8 @@ do_remember_include_file(Context& ctx, } // Let's hash the include file content. - std::unique_ptr fhash_holder(hash_init(), - &hash_free); - fhash = fhash_holder.get(); + struct hash* fhash = hash_init(); + Finalizer fhash_finalizer([=] { hash_free(fhash); }); is_pch = is_precompiled_header(path.c_str()); if (is_pch) { @@ -773,10 +773,9 @@ send_cached_stderr(const std::string& path_stderr, bool strip_colors) // Fall through } } else { - int fd_stderr = open(path_stderr.c_str(), O_RDONLY | O_BINARY); - if (fd_stderr != -1) { - copy_fd(fd_stderr, STDERR_FILENO); - close(fd_stderr); + Fd fd_stderr(open(path_stderr.c_str(), O_RDONLY | O_BINARY)); + if (fd_stderr) { + copy_fd(*fd_stderr, STDERR_FILENO); } } } diff --git a/src/hash.cpp b/src/hash.cpp index 735a683c9..79a6d7088 100644 --- a/src/hash.cpp +++ b/src/hash.cpp @@ -19,6 +19,7 @@ #include "hash.hpp" +#include "Fd.hpp" #include "Util.hpp" #include "logging.hpp" @@ -180,13 +181,12 @@ hash_fd(struct hash* hash, int fd, bool fd_is_file) bool hash_file(struct hash* hash, const char* fname) { - int fd = open(fname, O_RDONLY | O_BINARY); - if (fd == -1) { + Fd fd(open(fname, O_RDONLY | O_BINARY)); + if (!fd) { cc_log("Failed to open %s: %s", fname, strerror(errno)); return false; } - bool ret = hash_fd(hash, fd, true); - close(fd); + bool ret = hash_fd(hash, *fd, true); return ret; } diff --git a/src/legacy_util.cpp b/src/legacy_util.cpp index 938493ac4..d4c88652d 100644 --- a/src/legacy_util.cpp +++ b/src/legacy_util.cpp @@ -19,6 +19,7 @@ #include "legacy_util.hpp" +#include "Fd.hpp" #include "Util.hpp" #include "exceptions.hpp" #include "logging.hpp" @@ -151,35 +152,33 @@ clone_file(const char* src, const char* dest, bool via_tmp_file) bool result; # if defined(__linux__) - int src_fd = open(src, O_RDONLY); - if (src_fd == -1) { + Fd src_fd(open(src, O_RDONLY)); + if (!src_fd) { return false; } - int dest_fd; + Fd dest_fd; char* tmp_file = nullptr; if (via_tmp_file) { tmp_file = x_strdup(dest); - dest_fd = create_tmp_fd(&tmp_file); + dest_fd = Fd(create_tmp_fd(&tmp_file)); } else { - dest_fd = open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666); - if (dest_fd == -1) { - close(dest_fd); - close(src_fd); + dest_fd = Fd(open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666)); + if (!dest_fd) { return false; } } int saved_errno = 0; - if (ioctl(dest_fd, FICLONE, src_fd) == 0) { + if (ioctl(*dest_fd, FICLONE, *src_fd) == 0) { result = true; } else { result = false; saved_errno = errno; } - close(dest_fd); - close(src_fd); + dest_fd.close(); + src_fd.close(); if (via_tmp_file) { if (x_rename(tmp_file, dest) != 0) { @@ -214,31 +213,29 @@ copy_file(const char* src, const char* dest, bool via_tmp_file) { bool result = false; - int src_fd = open(src, O_RDONLY); - if (src_fd == -1) { + Fd src_fd(open(src, O_RDONLY)); + if (!src_fd) { return false; } - int dest_fd; + Fd dest_fd; char* tmp_file = nullptr; if (via_tmp_file) { tmp_file = x_strdup(dest); - dest_fd = create_tmp_fd(&tmp_file); + dest_fd = Fd(create_tmp_fd(&tmp_file)); } else { - dest_fd = open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666); - if (dest_fd == -1) { - close(dest_fd); - close(src_fd); + dest_fd = Fd(open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666)); + if (!dest_fd) { return false; } } - if (copy_fd(src_fd, dest_fd, true)) { + if (copy_fd(*src_fd, *dest_fd, true)) { result = true; } - close(dest_fd); - close(src_fd); + dest_fd.close(); + src_fd.close(); if (via_tmp_file) { if (x_rename(tmp_file, dest) != 0) { @@ -710,8 +707,8 @@ read_file(const char* path, size_t size_hint, char** data, size_t* size) // +1 to be able to detect EOF in the first read call size_hint = (size_hint < 1024) ? 1024 : size_hint + 1; - int fd = open(path, O_RDONLY | O_BINARY); - if (fd == -1) { + Fd fd(open(path, O_RDONLY | O_BINARY)); + if (!fd) { return false; } size_t allocated = size_hint; @@ -724,7 +721,7 @@ read_file(const char* path, size_t size_hint, char** data, size_t* size) *data = static_cast(x_realloc(*data, allocated)); } const size_t max_read = allocated - pos; - ret = read(fd, *data + pos, max_read); + ret = read(*fd, *data + pos, max_read); if (ret == 0 || (ret == -1 && errno != EINTR)) { break; } @@ -735,7 +732,7 @@ read_file(const char* path, size_t size_hint, char** data, size_t* size) } } } - close(fd); + if (ret == -1) { cc_log("Failed reading %s", path); free(*data); diff --git a/unittest/test_Util.cpp b/unittest/test_Util.cpp index d8cd6401b..95870a0bf 100644 --- a/unittest/test_Util.cpp +++ b/unittest/test_Util.cpp @@ -17,6 +17,7 @@ // Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA #include "../src/Config.hpp" +#include "../src/Fd.hpp" #include "../src/Util.hpp" #include "TestUtil.hpp" @@ -205,18 +206,20 @@ TEST_CASE("Util::fallocate") { TestContext test_context; - const char* filename = "test-file"; - int fd = creat(filename, S_IRUSR | S_IWUSR); - CHECK(Util::fallocate(fd, 10000) == 0); - close(fd); - fd = open(filename, O_RDWR, S_IRUSR | S_IWUSR); + const char filename[] = "test-file"; + + CHECK(Util::fallocate(Fd(creat(filename, S_IRUSR | S_IWUSR)).get(), 10000) + == 0); CHECK(Stat::stat(filename).size() == 10000); - CHECK(Util::fallocate(fd, 5000) == 0); - close(fd); - fd = open(filename, O_RDWR, S_IRUSR | S_IWUSR); + + CHECK( + Util::fallocate(Fd(open(filename, O_RDWR, S_IRUSR | S_IWUSR)).get(), 5000) + == 0); CHECK(Stat::stat(filename).size() == 10000); - CHECK(Util::fallocate(fd, 20000) == 0); - close(fd); + + CHECK( + Util::fallocate(Fd(open(filename, O_RDWR, S_IRUSR | S_IWUSR)).get(), 20000) + == 0); CHECK(Stat::stat(filename).size() == 20000); }