From 076a96e7420b11b1e9f1934e12e78fea76aae040 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Tue, 15 Aug 2023 09:14:23 +0000 Subject: GC unused File code and other fallout. --- vespalib/src/tests/guard/guard_test.cpp | 152 +------------- vespalib/src/tests/io/fileutil/fileutiltest.cpp | 41 ---- vespalib/src/vespa/vespalib/io/fileutil.cpp | 102 ++++------ vespalib/src/vespa/vespalib/io/fileutil.h | 43 ++-- vespalib/src/vespa/vespalib/util/guard.h | 250 ++---------------------- 5 files changed, 75 insertions(+), 513 deletions(-) diff --git a/vespalib/src/tests/guard/guard_test.cpp b/vespalib/src/tests/guard/guard_test.cpp index 9e5e7e55cc6..c61c4874eff 100644 --- a/vespalib/src/tests/guard/guard_test.cpp +++ b/vespalib/src/tests/guard/guard_test.cpp @@ -7,20 +7,7 @@ using namespace vespalib; -class Test : public TestApp -{ -public: - void testFilePointer(); - void testFileDescriptor(); - void testDirPointer(); - void testValueGuard(); - void testMaxValueGuard(); - void testCounterGuard(); - int Main() override; -}; - -void -Test::testFilePointer() +TEST("testFilePointer") { { FilePointer file(fopen("bogus", "r")); @@ -72,8 +59,7 @@ Test::testFilePointer() } } -void -Test::testFileDescriptor() +TEST("testFileDescriptor") { { FileDescriptor file(open("bogus", O_RDONLY)); @@ -126,124 +112,7 @@ Test::testFileDescriptor() } } -void -Test::testDirPointer() -{ - { - DirPointer dir(opendir("bogus")); - EXPECT_TRUE(!dir.valid()); - } - { - DirPointer dir(opendir(TEST_PATH("").c_str())); - EXPECT_TRUE(dir.valid()); - - dirent *de; - bool foundGuardCpp = false; - while ((de = readdir(dir)) != NULL) { - if (strcmp(de->d_name, "guard_test.cpp") == 0) { - foundGuardCpp = true; - } - } - EXPECT_TRUE(foundGuardCpp); - } - { - DIR *dp = NULL; - { - DirPointer dir(opendir(".")); - EXPECT_TRUE(dir.valid()); - dp = dir; - } - EXPECT_TRUE(dp != NULL); - // EXPECT_TRUE(readdir(dp) == NULL); - } - { - DirPointer dir(opendir(".")); - EXPECT_TRUE(dir.valid()); - dir.reset(opendir(".")); - EXPECT_TRUE(dir.valid()); - - DIR *ref = dir.dp(); - DIR *dp = dir.release(); - EXPECT_TRUE(dp != NULL); - EXPECT_TRUE(dp == ref); - EXPECT_TRUE(!dir.valid()); - EXPECT_TRUE(dir.dp() == NULL); - closedir(dp); - } -} - -void -Test::testValueGuard() -{ - int value = 10; - { - ValueGuard guard(value); - value = 20; - EXPECT_TRUE(value == 20); - } - EXPECT_TRUE(value == 10); - { - ValueGuard guard(value, 50); - value = 20; - EXPECT_TRUE(value == 20); - } - EXPECT_TRUE(value == 50); - { - ValueGuard guard(value); - value = 20; - guard.update(100); - EXPECT_TRUE(value == 20); - } - EXPECT_TRUE(value == 100); - { - ValueGuard guard(value); - value = 20; - guard.dismiss(); - EXPECT_TRUE(value == 20); - } - EXPECT_TRUE(value == 20); -} - -void -Test::testMaxValueGuard() -{ - int value = 10; - { - MaxValueGuard guard(value); - value = 20; - EXPECT_TRUE(value == 20); - } - EXPECT_TRUE(value == 10); - { - MaxValueGuard guard(value); - value = 5; - EXPECT_TRUE(value == 5); - } - EXPECT_TRUE(value == 5); - { - MaxValueGuard guard(value, 50); - value = 100; - EXPECT_TRUE(value == 100); - } - EXPECT_TRUE(value == 50); - { - MaxValueGuard guard(value); - value = 200; - guard.update(100); - EXPECT_TRUE(value == 200); - } - EXPECT_TRUE(value == 100); - { - MaxValueGuard guard(value); - value = 200; - guard.dismiss(); - EXPECT_TRUE(value == 200); - } - EXPECT_TRUE(value == 200); -} - -void -Test::testCounterGuard() +TEST("testCounterGuard") { int cnt = 10; { @@ -254,17 +123,4 @@ Test::testCounterGuard() EXPECT_TRUE(cnt == 10); } -int -Test::Main() -{ - TEST_INIT("guard_test"); - testFilePointer(); - testFileDescriptor(); - testDirPointer(); - testValueGuard(); - testMaxValueGuard(); - testCounterGuard(); - TEST_DONE(); -} - -TEST_APPHOOK(Test) +TEST_MAIN() { TEST_RUN_ALL(); } \ No newline at end of file diff --git a/vespalib/src/tests/io/fileutil/fileutiltest.cpp b/vespalib/src/tests/io/fileutil/fileutiltest.cpp index 0948d18304e..5d1d1a14475 100644 --- a/vespalib/src/tests/io/fileutil/fileutiltest.cpp +++ b/vespalib/src/tests/io/fileutil/fileutiltest.cpp @@ -204,47 +204,6 @@ TEST("require that vespalib::File::resize works") EXPECT_EQUAL(std::string("foo"), std::string(&vec[0], 3)); } -TEST("require that copy constructor and assignment for vespalib::File works") -{ - // Copy file not opened. - { - File f("myfile"); - File f2(f); - EXPECT_EQUAL(f.getFilename(), f2.getFilename()); - } - // Copy file opened - { - File f("myfile"); - f.open(File::CREATE); - File f2(f); - EXPECT_EQUAL(f.getFilename(), f2.getFilename()); - ASSERT_TRUE(f2.isOpen()); - ASSERT_TRUE(!f.isOpen()); - } - // Assign file opened to another file opened - { - File f("myfile"); - f.open(File::CREATE); - int fd = f.getFileDescriptor(); - File f2("targetfile"); - f2.open(File::CREATE); - f = f2; - EXPECT_EQUAL(std::string("targetfile"), f2.getFilename()); - EXPECT_EQUAL(f.getFilename(), f2.getFilename()); - ASSERT_TRUE(!f2.isOpen()); - ASSERT_TRUE(f.isOpen()); - try{ - File f3(fd, "myfile"); - f3.closeFileWhenDestructed(false); // Already closed - f3.write("foo", 3, 0); - TEST_FATAL("This file descriptor should have been closed"); - } catch (IoException& e) { - //std::cerr << e.what() << "\n"; - EXPECT_EQUAL(IoException::INTERNAL_FAILURE, e.getType()); - } - } -} - TEST("require that we can read all data written to file") { // Write text into a file. diff --git a/vespalib/src/vespa/vespalib/io/fileutil.cpp b/vespalib/src/vespa/vespalib/io/fileutil.cpp index 6c169ab8d98..0a70b343b13 100644 --- a/vespalib/src/vespa/vespalib/io/fileutil.cpp +++ b/vespalib/src/vespa/vespalib/io/fileutil.cpp @@ -20,31 +20,29 @@ namespace vespalib { namespace { - FileInfo::UP - processStat(struct stat& filestats, bool result, stringref path) { - FileInfo::UP resval; - if (result) { - resval.reset(new FileInfo); - resval->_plainfile = S_ISREG(filestats.st_mode); - resval->_directory = S_ISDIR(filestats.st_mode); - resval->_symlink = S_ISLNK(filestats.st_mode); - resval->_size = filestats.st_size; - } else if (errno != ENOENT) { - asciistream ost; - ost << "An IO error occured while statting '" << path << "'. " - << "errno(" << errno << "): " << getErrorString(errno); - throw IoException(ost.str(), IoException::getErrorType(errno), - VESPA_STRLOC); - } - LOG(debug, "stat(%s): Existed? %s, Plain file? %s, Directory? %s, " - "Size: %" PRIu64, - string(path).c_str(), - resval.get() ? "true" : "false", - resval.get() && resval->_plainfile ? "true" : "false", - resval.get() && resval->_directory ? "true" : "false", - resval.get() ? resval->_size : 0); - return resval; +FileInfo::UP +processStat(struct stat& filestats, bool result, stringref path) { + FileInfo::UP resval; + if (result) { + resval = std::make_unique(); + resval->_plainfile = S_ISREG(filestats.st_mode); + resval->_directory = S_ISDIR(filestats.st_mode); + resval->_symlink = S_ISLNK(filestats.st_mode); + resval->_size = filestats.st_size; + } else if (errno != ENOENT) { + asciistream ost; + ost << "An IO error occured while statting '" << path << "'. " + << "errno(" << errno << "): " << getErrorString(errno); + throw IoException(ost.str(), IoException::getErrorType(errno), VESPA_STRLOC); } + LOG(debug, "stat(%s): Existed? %s, Plain file? %s, Directory? %s, Size: %" PRIu64, + string(path).c_str(), + resval.get() ? "true" : "false", + resval.get() && resval->_plainfile ? "true" : "false", + resval.get() && resval->_directory ? "true" : "false", + resval.get() ? resval->_size : 0); + return resval; +} string safeStrerror(int errnum) @@ -129,36 +127,24 @@ File::operator=(File& f) return *this; } -void -File::setFilename(stringref filename) -{ - if (_filename == filename) return; - if (_close && _fd != -1) close(); - _filename = filename; - _fd = -1; - _flags = 0; - _close = true; -} - namespace { - int openAndCreateDirsIfMissing(const string & filename, int flags, - bool createDirsIfMissing) +int openAndCreateDirsIfMissing(const string & filename, int flags, bool createDirsIfMissing) +{ + int fd = ::open(filename.c_str(), flags, 0644); + if (fd < 0 && errno == ENOENT && ((flags & O_CREAT) != 0) + && createDirsIfMissing) { - int fd = ::open(filename.c_str(), flags, 0644); - if (fd < 0 && errno == ENOENT && ((flags & O_CREAT) != 0) - && createDirsIfMissing) - { - auto pos = filename.rfind('/'); - if (pos != string::npos) { - string path(filename.substr(0, pos)); - std::filesystem::create_directories(std::filesystem::path(path)); - LOG(spam, "open(%s, %d): Retrying open after creating parent " - "directories.", filename.c_str(), flags); - fd = ::open(filename.c_str(), flags, 0644); - } + auto pos = filename.rfind('/'); + if (pos != string::npos) { + string path(filename.substr(0, pos)); + std::filesystem::create_directories(std::filesystem::path(path)); + LOG(spam, "open(%s, %d): Retrying open after creating parent " + "directories.", filename.c_str(), flags); + fd = ::open(filename.c_str(), flags, 0644); } - return fd; } + return fd; +} } void @@ -436,12 +422,6 @@ File::unlink() return std::filesystem::remove(std::filesystem::path(_filename)); } -namespace { - - uint32_t diskAlignmentSize = 4_Ki; - -} - DirectoryList listDirectory(const string & path) { @@ -465,16 +445,6 @@ listDirectory(const string & path) return result; } -MallocAutoPtr -getAlignedBuffer(size_t size) -{ - void *ptr; - int result = posix_memalign(&ptr, diskAlignmentSize, size); - assert(result == 0); - (void)result; - return MallocAutoPtr(ptr); -} - string dirname(stringref name) { size_t found = name.rfind('/'); diff --git a/vespalib/src/vespa/vespalib/io/fileutil.h b/vespalib/src/vespa/vespalib/io/fileutil.h index 4de36daa85f..29e9bd66918 100644 --- a/vespalib/src/vespa/vespalib/io/fileutil.h +++ b/vespalib/src/vespa/vespalib/io/fileutil.h @@ -61,10 +61,10 @@ std::ostream& operator<<(std::ostream&, const FileInfo&); */ class File { private: - int _fd; - int _flags; - vespalib::string _filename; - bool _close; + int _fd; + int _flags; + string _filename; + bool _close; mutable int _fileReads; // Tracks number of file reads done on this file mutable int _fileWrites; // Tracks number of file writes done in this file @@ -74,6 +74,9 @@ private: */ void verifyDirectIO(uint64_t buf, size_t bufsize, off_t offset) const; + /** Copying a file instance, moves any open file descriptor. */ + File(File& f); + File& operator=(File& f); public: using UP = std::unique_ptr; @@ -83,25 +86,15 @@ public: enum Flag { READONLY = 1, CREATE = 2, DIRECTIO = 4, TRUNC = 8 }; /** Create a file instance, without opening the file. */ - File(vespalib::stringref filename); + File(stringref filename); /** Create a file instance of an already open file. */ - File(int fileDescriptor, vespalib::stringref filename); - - /** Copying a file instance, moves any open file descriptor. */ - File(File& f); - File& operator=(File& f); + File(int fileDescriptor, stringref filename); /** Closes the file if not instructed to do otherwise. */ virtual ~File(); - /** - * Make this instance point at another file. - * Closes the old file it it was open. - */ - void setFilename(vespalib::stringref filename); - - const vespalib::string& getFilename() const { return _filename; } + const string& getFilename() const { return _filename; } virtual void open(int flags, bool autoCreateDirectories = false); @@ -177,7 +170,7 @@ public: * @throw IoException If we failed to read from file. * @return The content of the file. */ - vespalib::string readAll() const; + string readAll() const; /** * Read a file into a string. @@ -188,7 +181,7 @@ public: * @throw IoException If we failed to read from file. * @return The content of the file. */ - static vespalib::string readAll(vespalib::stringref path); + static string readAll(stringref path); /** * Sync file or directory. @@ -198,24 +191,18 @@ public: * * @throw IoException If we failed to sync the file. */ - static void sync(vespalib::stringref path); + static void sync(stringref path); virtual void sync(); virtual bool close(); virtual bool unlink(); - - int getFileReadCount() const { return _fileReads; } - int getFileWriteCount() const { return _fileWrites; } }; /** * List the contents of the given directory. */ -using DirectoryList = std::vector; -extern DirectoryList listDirectory(const vespalib::string & path); - -extern MallocAutoPtr getAlignedBuffer(size_t size); - +using DirectoryList = std::vector; +extern DirectoryList listDirectory(const string & path); string dirname(stringref name); string getOpenErrorString(const int osError, stringref name); diff --git a/vespalib/src/vespa/vespalib/util/guard.h b/vespalib/src/vespa/vespalib/util/guard.h index 32237a59d9a..efd7b8345c9 100644 --- a/vespalib/src/vespa/vespalib/util/guard.h +++ b/vespalib/src/vespa/vespalib/util/guard.h @@ -2,8 +2,7 @@ #pragma once -#include -#include +#include #include namespace vespalib { @@ -19,43 +18,43 @@ class FilePointer { private: FILE *_fp; - FilePointer(const FilePointer &); - FilePointer &operator=(const FilePointer &); public: /** * @brief Create a FilePointer from a FILE pointer. * * @param file the underlying FILE pointer **/ - explicit FilePointer(FILE *file = NULL) : _fp(file) {} + explicit FilePointer(FILE *file = nullptr) noexcept : _fp(file) {} + FilePointer(const FilePointer &) = delete; + FilePointer &operator=(const FilePointer &) = delete; /** * @brief Close the file if it is still open. **/ ~FilePointer() { reset(); } /** - * @brief Check whether we have a FILE pointer (not NULL) + * @brief Check whether we have a FILE pointer (not nullptr) * * @return true if we have an underlying FILE pointer **/ - bool valid() const { return (_fp != NULL); } + bool valid() const noexcept { return (_fp != nullptr); } /** * @brief Obtain the internal FILE pointer * * @return internal FILE pointer **/ - FILE *fp() const { return _fp; } + FILE *fp() const noexcept { return _fp; } /** * @brief Implicit cast to obtain internal FILE pointer * * @return internal FILE pointer **/ - operator FILE*() { return _fp; } + operator FILE*() noexcept { return _fp; } /** * @brief Take ownership of a new FILE pointer. * * The previously owned FILE pointer is closed, if present. **/ - void reset(FILE *file = NULL) { + void reset(FILE *file = nullptr) { if (valid()) { fclose(_fp); } @@ -68,81 +67,13 @@ public: * * @return the released FILE pointer **/ - FILE *release() { + FILE *release() noexcept { FILE *tmp = _fp; - _fp = NULL; + _fp = nullptr; return tmp; } }; - -/** - * @brief A DirPointer wraps a bald DIR pointer inside a guarding object. - * - * The underlying directory is closed when the DirPointer object is - * destructed. - **/ -class DirPointer -{ -private: - DIR *_dp; - DirPointer(const DirPointer &); - DirPointer &operator=(const DirPointer &); -public: - /** - * @brief Create a DirPointer from a DIR pointer. - * - * @param dir the underlying DIR pointer - **/ - explicit DirPointer(DIR *dir = NULL) : _dp(dir) {} - /** - * Close the directory if it is still open. - **/ - ~DirPointer() { reset(); } - /** - * @brief Check whether we have a DIR pointer (not NULL) - * - * @return true if we have an underlying DIR pointer - **/ - bool valid() const { return (_dp != NULL); } - /** - * @brief Obtain the internal DIR pointer - * - * @return internal DIR pointer - **/ - DIR *dp() const { return _dp; } - /** - * @brief Implicit cast to obtain internal DIR pointer - * - * @return internal DIR pointer - **/ - operator DIR*() { return _dp; } - /** - * @brief Take ownership of a new DIR pointer. - * - * The previously owned DIR pointer is closed, if present. - **/ - void reset(DIR *dir = NULL) { - if (valid()) { - closedir(_dp); - } - _dp = dir; - } - /** - * @brief Release ownership of the current DIR pointer. - * - * The directory will no longer be closed by the destructor. - * - * @return the released DIR pointer - **/ - DIR *release() { - DIR *tmp = _dp; - _dp = NULL; - return tmp; - } -}; - - /** * @brief A FileDescriptor wraps a file descriptor inside a guarding object. * @@ -153,15 +84,15 @@ class FileDescriptor { private: int _fd; - FileDescriptor(const FileDescriptor &); - FileDescriptor &operator=(const FileDescriptor &); public: /** * @brief Create a FileDescriptor from a file descriptor. * * @param file the underlying file descriptor **/ - explicit FileDescriptor(int file = -1) : _fd(file) {} + explicit FileDescriptor(int file = -1) noexcept : _fd(file) {} + FileDescriptor(const FileDescriptor &) = delete; + FileDescriptor &operator=(const FileDescriptor &) = delete; /** * @brief Close the file if it is still open. **/ @@ -171,13 +102,13 @@ public: * * @return true if we have an underlying file descriptor **/ - bool valid() const { return (_fd >= 0); } + bool valid() const noexcept { return (_fd >= 0); } /** * @brief Obtain the internal file descriptor * * @return internal file descriptor **/ - int fd() const { return _fd; } + int fd() const noexcept { return _fd; } /** * @brief Take ownership of a new file descriptor. * @@ -196,7 +127,7 @@ public: * * @return the released file descriptor **/ - int release() { + int release() noexcept { int tmp = _fd; _fd = -1; return tmp; @@ -216,161 +147,20 @@ class CounterGuard { private: int &_cnt; - CounterGuard(const CounterGuard &); - CounterGuard &operator=(const CounterGuard &); public: /** * @brief Increase the value * * @param cnt a reference to the value that will be modified **/ - explicit CounterGuard(int &cnt) : _cnt(cnt) { ++cnt; } + explicit CounterGuard(int &cnt) noexcept : _cnt(cnt) { ++cnt; } + CounterGuard(const CounterGuard &) = delete; + CounterGuard &operator=(const CounterGuard &) = delete; /** * @brief Decrease the value **/ ~CounterGuard() { --_cnt; } }; - -/** - * @brief A ValueGuard is used to set a variable to a specific value - * when the ValueGuard is destructed. - * - * This can be used to revert a variable if an exception is thrown. - * However, you must remember to dismiss the guard if you don't want - * it to set the value when it goes out of scope. - **/ -template -class ValueGuard -{ -private: - bool _active; - T &_ref; - T _value; - - ValueGuard(const ValueGuard &); - ValueGuard &operator=(const ValueGuard &); -public: - /** - * @brief Create a ValueGuard for the given variable. - * - * The variable will be reverted to its original value in the destructor. - * - * @param ref the variable that will be modified - **/ - explicit ValueGuard(T &ref) : _active(true), _ref(ref), _value(ref) {} - /** - * @brief Create a ValueGuard for the given variable. - * - * The variable will be set to the given value in the destructor. - * - * @param ref the variable that will be modified - * @param val the value it will be set to - **/ - ValueGuard(T &ref, const T &val) : _active(true), _ref(ref), _value(val) {} - /** - * @brief Reset the variable. - * - * Set the variable to the value defined in the constructor or the - * update method. If dismiss has been invoked, the variable is not - * modified. - **/ - ~ValueGuard() { - if (_active) { - _ref = _value; - } - } - /** - * @brief Dismiss this guard. - * - * When a guard has been dismissed, the destructor will not modify - * the variable. The dismiss method is typically used to indicate - * that everything went ok, and that we no longer need to protect - * the variable from exceptions. - **/ - void dismiss() { _active = false; } - /// @brief See dismiss - void deactivate() { dismiss(); } - /** - * @brief Update the value the variable will be set to in the - * destructor. - * - * This can be used to set revert points during execution. - **/ - void update(const T &val) { _value = val; } - void operator=(const T& val) { update(val); } -}; - - -/** - * @brief A MaxValueGuard is used to enfore an upper bound on the - * value of a variable when the MaxValueGuard is destructed. - * - * This can be used to revert a variable if an exception is thrown. - * However, you must remember to dismiss the guard if you don't want - * it to set the value when it goes out of scope. - **/ -template -class MaxValueGuard { - bool _active; - T &_ref; - T _value; - - MaxValueGuard(const MaxValueGuard &); - MaxValueGuard &operator=(const MaxValueGuard &); -public: - /** - * @brief Create a MaxValueGuard for the given variable. - * - * The variable will be reverted back to its original value in the - * destructor if it has increased. - * - * @param ref the variable that will be modified - **/ - explicit MaxValueGuard(T &ref) : _active(true), _ref(ref), _value(ref) {} - /** - * @brief Create a ValueGuard for the given variable. - * - * The given upper bound will be enforced in the destructor. - * - * @param ref the variable that will be modified - * @param val upper bound for the variable - **/ - MaxValueGuard(T& ref, const T& val) : _active(true), _ref(ref), _value(val) {} - /** - * @brief Enforce the upper bound. - * - * If the current value of the variable is greater than the upper - * bound, it is set to the upper bound as defined in the - * constructor or the update method. If dismiss has been invoked, - * the variable is not modified. - **/ - ~MaxValueGuard() { - if (_active && _ref > _value) { - _ref = _value; - } - } - /** - * @brief Dismiss this guard. - * - * When a guard is dismissed, the destructor will not modify the - * variable. The dismiss method is typically used to indicate that - * everything went ok, and that we no longer need to protect the - * variable from exceptions. - **/ - void dismiss() { _active = false; } - /// @brief See dismiss - void deactivate() { dismiss(); } - /** - * @brief Update the upper bound that will be enforced in the - * destructor. - * - * This can be used to set revert points during execution. - **/ - void update(const T &val) { _value = val; } - /// @brief See update. - void operator=(const T& val) { update(val); } -}; - } // namespace vespalib -- cgit v1.2.3