From aa02cbd0908f2dd06fde4fbe287d8540424f6464 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 8 May 2021 13:51:09 -0700 Subject: [PATCH] Wrap up history's mmap logic into its own type This improves the factoring a bit. --- src/history_file.cpp | 82 ++++++++++++++++++++++++++++---------------- src/history_file.h | 11 +++--- 2 files changed, 60 insertions(+), 33 deletions(-) diff --git a/src/history_file.cpp b/src/history_file.cpp index 9a741c39e..3d232e505 100644 --- a/src/history_file.cpp +++ b/src/history_file.cpp @@ -95,16 +95,56 @@ static void unescape_yaml_fish_2_0(std::string *str) { } } -history_file_contents_t::~history_file_contents_t() { munmap(const_cast(start_), length_); } +// A type wrapping up a region allocated via mmap(). +struct history_file_contents_t::mmap_region_t { + void *const ptr; + const size_t len; -history_file_contents_t::history_file_contents_t(const char *mmap_start, size_t mmap_length) - : start_(mmap_start), length_(mmap_length) { - assert(mmap_start != MAP_FAILED && mmap_length > 0 && "Invalid mmap params"); + mmap_region_t(void *ptr, size_t len) : ptr(ptr), len(len) { + assert(ptr != MAP_FAILED && len > 0 && "Invalid params"); + } + + ~mmap_region_t() { (void)munmap(ptr, len); } + + /// Map a region [0, len) from an fd. + /// \return nullptr on failure. + static std::unique_ptr map_file(int fd, size_t len) { + if (len == 0) return nullptr; + void *ptr = mmap(nullptr, size_t(len), PROT_READ, MAP_PRIVATE, fd, 0); + if (ptr == MAP_FAILED) return nullptr; + return make_unique(ptr, len); + } + + /// Map anonymous memory of a given length. + /// \return nullptr on failure. + static std::unique_ptr map_anon(size_t len) { + if (len == 0) return nullptr; + void *ptr = +#ifdef MAP_ANON + mmap(nullptr, size_t(len), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); +#else + mmap(0, size_t(len), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); +#endif + if (ptr == MAP_FAILED) return nullptr; + return make_unique(ptr, len); + } + + mmap_region_t(mmap_region_t &&rhs) = delete; + void operator=(mmap_region_t &&rhs) = delete; + mmap_region_t(const mmap_region_t &) = delete; + void operator=(const mmap_region_t &) = delete; +}; + +history_file_contents_t::~history_file_contents_t() = default; + +history_file_contents_t::history_file_contents_t(std::unique_ptr region) + : region_(std::move(region)), start_(static_cast(region_->ptr)), length_(region_->len) { + assert(region_ && start_ && length_ > 0 && "Invalid params"); } /// Try to infer the history file type based on inspecting the data. bool history_file_contents_t::infer_file_type() { - assert(length_ >= 0 && "File shoudl never be empty"); + assert(length_ > 0 && "File should never be empty"); if (start_[0] == '#') { this->type_ = history_type_fish_1_x; } else { // assume new fish @@ -117,36 +157,20 @@ std::unique_ptr history_file_contents_t::create(int fd) // Check that the file is seekable, and its size. off_t len = lseek(fd, 0, SEEK_END); if (len <= 0 || static_cast(len) >= SIZE_MAX) return nullptr; - if (lseek(fd, 0, SEEK_SET) != 0) return nullptr; - // Read the file, possibly using mmap. - void *mmap_start = nullptr; bool mmap_file_directly = should_mmap(fd); - if (mmap_file_directly) { - // We feel confident to map the file directly. Note this is still risky: if another - // process truncates the file we risk SIGBUS. - mmap_start = mmap(nullptr, size_t(len), PROT_READ, MAP_PRIVATE, fd, 0); - } else { - // We don't want to map the file. mmap some private memory and then read into it. We use - // mmap instead of malloc so that the destructor can always munmap(). - mmap_start = -#ifdef MAP_ANON - mmap(nullptr, size_t(len), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); -#else - mmap(0, size_t(len), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); -#endif - } - if (mmap_start == MAP_FAILED) return nullptr; - - // Construct the contents right away so it can clean up on error. - std::unique_ptr result( - new history_file_contents_t(static_cast(mmap_start), len)); + std::unique_ptr region = + mmap_file_directly ? mmap_region_t::map_file(fd, len) : mmap_region_t::map_anon(len); + if (!region) return nullptr; // If we mapped anonymous memory, we have to read from the file. - if (!mmap_file_directly && !read_from_fd(fd, mmap_start, len)) { - return nullptr; + if (!mmap_file_directly) { + if (lseek(fd, 0, SEEK_SET) != 0) return nullptr; + if (!read_from_fd(fd, region->ptr, region->len)) return nullptr; } + std::unique_ptr result(new history_file_contents_t(std::move(region))); + // Check the file type. if (!result->infer_file_type()) return nullptr; return result; diff --git a/src/history_file.h b/src/history_file.h index 53f1257fd..29bc57442 100644 --- a/src/history_file.h +++ b/src/history_file.h @@ -52,10 +52,13 @@ class history_file_contents_t { ~history_file_contents_t(); private: - // The memory mapped pointer. - const char *const start_; + // A type wrapping up the logic around mmap and munmap. + struct mmap_region_t; + const std::unique_ptr region_{}; - // The mapped length. + // The memory mapped pointer and length. + // These just alias the mapped region. + const char *const start_; const size_t length_; // The type of the mapped file. @@ -63,7 +66,7 @@ class history_file_contents_t { history_file_type_t type_{}; // Private constructor; use the static create() function. - history_file_contents_t(const char *mmap_start, size_t mmap_length); + explicit history_file_contents_t(std::unique_ptr region); // Try to infer the file type to populate type_. // \return true on success, false on error.