From 1426e3fe4ca1cd248e1cf2e35a3f946000b031e2 Mon Sep 17 00:00:00 2001 From: Vsevolod Stakhov Date: Sun, 16 Oct 2022 15:22:10 +0100 Subject: [PATCH] [Minor] More tests and fixes to raii file --- src/libutil/cxx/locked_file.cxx | 60 ++++++++++++++++++++------------- src/libutil/cxx/locked_file.hxx | 19 +++++++++++ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/libutil/cxx/locked_file.cxx b/src/libutil/cxx/locked_file.cxx index 4c91f44ed..c972e1de3 100644 --- a/src/libutil/cxx/locked_file.cxx +++ b/src/libutil/cxx/locked_file.cxx @@ -41,11 +41,6 @@ auto raii_file::open(const char *fname, int flags) -> tl::expected tl::expected< return tl::make_unexpected(fmt::format("cannot create file {}: {}", fname, ::strerror(errno))); } - if (!rspamd_file_lock(fd, TRUE)) { - close(fd); - return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno))); - } - auto ret = raii_file{fname, fd, false}; if (fstat(ret.fd, &ret.st) == -1) { @@ -102,12 +92,6 @@ auto raii_file::create_temp(const char *fname, int flags, int perms) -> tl::expe return tl::make_unexpected(fmt::format("cannot create file {}: {}", fname, ::strerror(errno))); } - if (!rspamd_file_lock(fd, TRUE)) { - unlink(fname); - close(fd); - return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno))); - } - auto ret = raii_file{fname, fd, true}; if (fstat(ret.fd, &ret.st) == -1) { @@ -135,12 +119,6 @@ auto raii_file::mkstemp(const char *pattern, int flags, int perms) -> tl::expect return tl::make_unexpected(fmt::format("cannot create file {}: {}", pattern, ::strerror(errno))); } - if (!rspamd_file_lock(fd, TRUE)) { - close(fd); - (void)unlink(mutable_pattern.c_str()); - return tl::make_unexpected(fmt::format("cannot lock file {}: {}", pattern, ::strerror(errno))); - } - auto ret = raii_file{mutable_pattern.c_str(), fd, true}; if (fstat(ret.fd, &ret.st) == -1) { @@ -157,11 +135,15 @@ raii_file::~raii_file() noexcept if (temp) { (void)unlink(fname.c_str()); } - (void) rspamd_file_unlock(fd, FALSE); close(fd); } } +auto raii_file::update_stat() noexcept -> bool +{ + return fstat(fd, &st) != -1; +} + raii_locked_file::~raii_locked_file() noexcept { @@ -197,6 +179,8 @@ auto raii_mmaped_file::mmap_shared(raii_file &&file, { void *map; + /* Update stat on file to ensure it is up-to-date */ + file.update_stat(); map = mmap(NULL, file.get_stat().st_size, flags, MAP_SHARED, file.get_fd(), 0); if (map == MAP_FAILED) { @@ -386,6 +370,36 @@ TEST_CASE("tempfile") { CHECK(serrno == ENOENT); } +TEST_CASE("mmap") { + std::string tmpname; + { + auto raii_file = raii_file::mkstemp("/tmp//doctest-XXXXXXXX", + O_RDWR|O_CREAT|O_EXCL, 00600); + CHECK(raii_file.has_value()); + CHECK(raii_file->get_dir() == "/tmp"); + CHECK(access(raii_file->get_name().data(), R_OK) == 0); + tmpname = std::string{raii_file->get_name()}; + char payload[] = {'1', '2', '3'}; + CHECK(write(raii_file->get_fd(), payload, sizeof(payload)) == sizeof(payload)); + auto mmapped_file1 = raii_mmaped_file::mmap_shared(std::move(raii_file.value()), PROT_READ|PROT_WRITE); + CHECK(mmapped_file1.has_value()); + CHECK(!raii_file->is_valid()); + CHECK(mmapped_file1->get_size() == sizeof(payload)); + CHECK(memcmp(mmapped_file1->get_map(), payload, sizeof(payload)) == 0); + *(char *)mmapped_file1->get_map() = '2'; + auto mmapped_file2 = raii_mmaped_file::mmap_shared(tmpname.c_str(), O_RDONLY, PROT_READ); + CHECK(mmapped_file2.has_value()); + CHECK(mmapped_file2->get_size() == sizeof(payload)); + CHECK(memcmp(mmapped_file2->get_map(), payload, sizeof(payload)) != 0); + CHECK(memcmp(mmapped_file2->get_map(), mmapped_file1->get_map(), sizeof(payload)) == 0); + } + // File must be deleted after this call + auto ret = ::access(tmpname.c_str(), R_OK); + auto serrno = errno; + CHECK(ret == -1); + CHECK(serrno == ENOENT); +} + } // TEST_SUITE } // namespace tests diff --git a/src/libutil/cxx/locked_file.hxx b/src/libutil/cxx/locked_file.hxx index c7d286cbb..507b02762 100644 --- a/src/libutil/cxx/locked_file.hxx +++ b/src/libutil/cxx/locked_file.hxx @@ -44,6 +44,10 @@ public: return st; }; + auto get_size() const -> std::size_t { + return st.st_size; + }; + auto get_name() const -> std::string_view { return std::string_view{fname}; } @@ -93,10 +97,24 @@ public: *this = std::move(other); } + /** + * Prevent file from being deleted + * @return + */ auto make_immortal() noexcept { temp = false; } + /** + * Performs fstat on an opened file to refresh internal stat + * @return + */ + auto update_stat() noexcept -> bool; + + auto is_valid() noexcept -> bool { + return fd != -1; + } + /* Do not allow copy/default ctor */ const raii_file& operator=(const raii_file &other) = delete; raii_file() = delete; @@ -181,6 +199,7 @@ struct raii_mmaped_file final { static auto mmap_shared(const char *fname, int open_flags, int mmap_flags) -> tl::expected; // Returns a constant pointer to the underlying map auto get_map() const -> void* {return map;} + auto get_file() const -> const raii_file& { return file; } // Passes the ownership of the mmaped memory to the callee auto steal_map() -> std::tuple { auto ret = std::make_tuple(this->map, file.get_stat().st_size); -- 2.39.5