From 61a35bd936da8c96955c2c274c5b63dd4c6ffcdb Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Mon, 17 Dec 2018 08:33:36 -0800 Subject: [PATCH] Remove file_mode::append_new (API Change): * Tidying * Increase test coverage * Fix file_mode::append_existing API Changes: * file_mode::append_new is removed, as it makes no sense Actions Required: * Replace file_mode::append_new with file_mode::append or file_mode::append_existing instead of file_mode::append_new --- CHANGELOG.md | 4 + include/boost/beast/core/file_base.hpp | 48 ++- include/boost/beast/core/file_posix.hpp | 5 + include/boost/beast/core/impl/file_posix.hpp | 78 ++-- include/boost/beast/core/impl/file_stdio.hpp | 67 +++- include/boost/beast/core/impl/file_win32.hpp | 7 - test/beast/core/buffer_test.hpp | 4 +- test/beast/core/file_posix.cpp | 2 +- test/beast/core/file_stdio.cpp | 2 +- test/beast/core/file_test.hpp | 400 +++++++++++++++---- test/beast/core/file_win32.cpp | 4 +- 11 files changed, 458 insertions(+), 163 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c606e3d2..039ece6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ API Changes: * buffers_adaptor replaces buffers_adapter (rename) * make_printable replaces buffers (rename) +* Remove file_mode::append_new Actions Required: @@ -34,6 +35,9 @@ Actions Required: * Replace call sites to use make_printable instead of buffers, and also include make_printable.hpp instead of ostream.hpp. +* Replace file_mode::append_new with file_mode::append + or file_mode::append_existing instead of file_mode::append_new + -------------------------------------------------------------------------------- Version 199: diff --git a/include/boost/beast/core/file_base.hpp b/include/boost/beast/core/file_base.hpp index c2b3c53a..3fdb37e2 100644 --- a/include/boost/beast/core/file_base.hpp +++ b/include/boost/beast/core/file_base.hpp @@ -16,6 +16,20 @@ namespace boost { namespace beast { +/* + +file_mode acesss sharing seeking file std mode +-------------------------------------------------------------------------------------- +read read-only shared random must exist "rb" +scan read-only shared sequential must exist "rbS" +write read/write exclusive random create/truncate "wb+" +write_new read/write exclusive random must not exist "wbx" +write_existing read/write exclusive random must exist "rb+" +append write-only exclusive sequential create/truncate "ab" +append_existing write-only exclusive sequential must exist "ab" + +*/ + /** File open modes These modes are used when opening files using @@ -25,28 +39,33 @@ namespace beast { */ enum class file_mode { - /// Random reading + /// Random read-only access to an existing file read, - /// Sequential reading + /// Sequential read-only access to an existing file scan, - /** Random writing to a new or truncated file + /** Random reading and writing to a new or truncated file - @li If the file does not exist, it is created. - - @li If the file exists, it is truncated to - zero size upon opening. + This mode permits random-access reading and writing + for the specified file. If the file does not exist + prior to the function call, it is created with an + initial size of zero bytes. Otherwise if the file + already exists, the size is truncated to zero bytes. */ write, - /** Random writing to new file only + /** Random reading and writing to a new file only - If the file exists, an error is generated. + This mode permits random-access reading and writing + for the specified file. The file will be created with + an initial size of zero bytes. If the file already exists + prior to the function call, an error is returned and + no file is opened. */ write_new, - /** Random writing to existing file + /** Random write-only access to existing file If the file does not exist, an error is generated. */ @@ -64,15 +83,6 @@ enum class file_mode */ append, - /** Appending to a new file only - - The current file position shall be set to the end of - the file prior to each write. - - If the file exists, an error is generated. - */ - append_new, - /** Appending to an existing file The current file position shall be set to the end of diff --git a/include/boost/beast/core/file_posix.hpp b/include/boost/beast/core/file_posix.hpp index c0addcde..ef2bf25b 100644 --- a/include/boost/beast/core/file_posix.hpp +++ b/include/boost/beast/core/file_posix.hpp @@ -43,6 +43,11 @@ class file_posix { int fd_ = -1; + BOOST_BEAST_DECL + static + int + native_close(int& fd); + public: /** The type of the underlying file handle. diff --git a/include/boost/beast/core/impl/file_posix.hpp b/include/boost/beast/core/impl/file_posix.hpp index 2533bfe4..9ac97b52 100644 --- a/include/boost/beast/core/impl/file_posix.hpp +++ b/include/boost/beast/core/impl/file_posix.hpp @@ -36,30 +36,29 @@ namespace boost { namespace beast { -namespace detail { - -inline int -file_posix_close(int fd) +file_posix:: +native_close(native_handle_type& fd) { - for(;;) + if(fd != -1) { - if(! ::close(fd)) - break; - int const ev = errno; - if(errno != EINTR) - return ev; + for(;;) + { + if(::close(fd) == 0) + break; + int const ev = errno; + if(errno != EINTR) + return ev; + } + fd = -1; } return 0; } -} // detail - file_posix:: ~file_posix() { - if(fd_ != -1) - detail::file_posix_close(fd_); + native_close(fd_); } file_posix:: @@ -74,8 +73,7 @@ operator=(file_posix&& other) { if(&other == this) return *this; - if(fd_ != -1) - detail::file_posix_close(fd_); + native_close(fd_); fd_ = other.fd_; other.fd_ = -1; return *this; @@ -85,8 +83,7 @@ void file_posix:: native_handle(native_handle_type fd) { - if(fd_ != -1) - detail::file_posix_close(fd_); + native_close(fd_); fd_ = fd; } @@ -94,36 +91,23 @@ void file_posix:: close(error_code& ec) { - if(fd_ != -1) - { - auto const ev = - detail::file_posix_close(fd_); - if(ev) - ec.assign(ev, system_category()); - else - ec = {}; - fd_ = -1; - } + auto const ev = native_close(fd_); + if(ev) + ec.assign(ev, system_category()); else - { ec = {}; - } } void file_posix:: open(char const* path, file_mode mode, error_code& ec) { - if(fd_ != -1) - { - auto const ev = - detail::file_posix_close(fd_); - if(ev) - ec.assign(ev, system_category()); - else - ec = {}; - fd_ = -1; - } + auto const ev = native_close(fd_); + if(ev) + ec.assign(ev, system_category()); + else + ec = {}; + int f = 0; #if BOOST_BEAST_USE_POSIX_FADVISE int advise = 0; @@ -166,21 +150,14 @@ open(char const* path, file_mode mode, error_code& ec) break; case file_mode::append: - f = O_RDWR | O_CREAT | O_TRUNC; - #if BOOST_BEAST_USE_POSIX_FADVISE - advise = POSIX_FADV_SEQUENTIAL; - #endif - break; - - case file_mode::append_new: - f = O_RDWR | O_CREAT | O_EXCL; + f = O_WRONLY | O_CREAT | O_TRUNC; #if BOOST_BEAST_USE_POSIX_FADVISE advise = POSIX_FADV_SEQUENTIAL; #endif break; case file_mode::append_existing: - f = O_RDWR | O_EXCL; + f = O_WRONLY; #if BOOST_BEAST_USE_POSIX_FADVISE advise = POSIX_FADV_SEQUENTIAL; #endif @@ -202,8 +179,7 @@ open(char const* path, file_mode mode, error_code& ec) if(::posix_fadvise(fd_, 0, 0, advise)) { auto const ev = errno; - detail::file_posix_close(fd_); - fd_ = -1; + native_close(fd_); ec.assign(ev, system_category()); return; } diff --git a/include/boost/beast/core/impl/file_stdio.hpp b/include/boost/beast/core/impl/file_stdio.hpp index 339d07b3..bd7f3f7a 100644 --- a/include/boost/beast/core/impl/file_stdio.hpp +++ b/include/boost/beast/core/impl/file_stdio.hpp @@ -10,6 +10,7 @@ #ifndef BOOST_BEAST_CORE_IMPL_FILE_STDIO_HPP #define BOOST_BEAST_CORE_IMPL_FILE_STDIO_HPP +#include #include #include @@ -81,17 +82,65 @@ open(char const* path, file_mode mode, error_code& ec) switch(mode) { default: - case file_mode::read: s = "rb"; break; - case file_mode::scan: s = "rb"; break; - case file_mode::write: s = "wb"; break; - case file_mode::write_new: s = "wbx"; break; - case file_mode::write_existing: s = "wb"; break; - case file_mode::append: s = "ab"; break; - case file_mode::append_new: s = "abx"; break; - case file_mode::append_existing: s = "ab"; break; + case file_mode::read: + s = "rb"; + break; + + case file_mode::scan: + #if BOOST_MSVC + s = "rbS"; + #else + s = "rb"; + #endif + break; + + case file_mode::write: + s = "wb+"; + break; + + case file_mode::write_new: + { +#if BOOST_WORKAROUND(BOOST_MSVC, < 1910) + auto const f0 = std::fopen(path, "rb"); + if(f0) + { + std::fclose(f0); + ec = make_error_code( + errc::file_exists); + return; + } + s = "wb"; +#else + s = "wbx"; +#endif + break; } + + case file_mode::write_existing: + s = "rb+"; + break; + + case file_mode::append: + s = "ab"; + break; + + case file_mode::append_existing: + { + auto const f0 = std::fopen(path, "rb+"); + if(! f0) + { + ec = make_error_code( + errc::no_such_file_or_directory); + return; + } + std::fclose(f0); + s = "ab"; + break; + } + } + #if BOOST_MSVC - auto const ev = fopen_s(&f_, path, s); + auto const ev = ::fopen_s(&f_, path, s); if(ev) { f_ = nullptr; diff --git a/include/boost/beast/core/impl/file_win32.hpp b/include/boost/beast/core/impl/file_win32.hpp index a7ea3626..2d9631df 100644 --- a/include/boost/beast/core/impl/file_win32.hpp +++ b/include/boost/beast/core/impl/file_win32.hpp @@ -175,13 +175,6 @@ open(char const* path, file_mode mode, error_code& ec) flags_and_attributes = 0x08000000; // FILE_FLAG_SEQUENTIAL_SCAN break; - case file_mode::append_new: - desired_access = boost::winapi::GENERIC_READ_ | - boost::winapi::GENERIC_WRITE_; - creation_disposition = boost::winapi::CREATE_NEW_; - flags_and_attributes = 0x08000000; // FILE_FLAG_SEQUENTIAL_SCAN - break; - case file_mode::append_existing: desired_access = boost::winapi::GENERIC_READ_ | boost::winapi::GENERIC_WRITE_; diff --git a/test/beast/core/buffer_test.hpp b/test/beast/core/buffer_test.hpp index cdf8375f..541962ec 100644 --- a/test/beast/core/buffer_test.hpp +++ b/test/beast/core/buffer_test.hpp @@ -7,8 +7,8 @@ // Official repository: https://github.com/boostorg/beast // -#ifndef BOOST_BEAST_TEST_BUFFER_TEST_HPP -#define BOOST_BEAST_TEST_BUFFER_TEST_HPP +#ifndef BOOST_BEAST_BUFFER_TEST_HPP +#define BOOST_BEAST_BUFFER_TEST_HPP #include #include diff --git a/test/beast/core/file_posix.cpp b/test/beast/core/file_posix.cpp index cc0f4517..d314c3bd 100644 --- a/test/beast/core/file_posix.cpp +++ b/test/beast/core/file_posix.cpp @@ -29,7 +29,7 @@ public: void run() { - doTestFile(*this); + test_file(*this); } }; diff --git a/test/beast/core/file_stdio.cpp b/test/beast/core/file_stdio.cpp index d9f2a0d2..9eb2c18b 100644 --- a/test/beast/core/file_stdio.cpp +++ b/test/beast/core/file_stdio.cpp @@ -27,7 +27,7 @@ public: void run() { - doTestFile(*this); + test_file(*this); } }; diff --git a/test/beast/core/file_test.hpp b/test/beast/core/file_test.hpp index 976907f5..571d19f1 100644 --- a/test/beast/core/file_test.hpp +++ b/test/beast/core/file_test.hpp @@ -7,119 +7,379 @@ // Official repository: https://github.com/boostorg/beast // -#ifndef BOOST_BEAST_TEST_CORE_FILE_TEST_HPP -#define BOOST_BEAST_TEST_CORE_FILE_TEST_HPP +#ifndef BOOST_BEAST_FILE_TEST_HPP +#define BOOST_BEAST_FILE_TEST_HPP #include #include -#include -#include +#include +#include +#include #include +#include namespace boost { namespace beast { template void -doTestFile(beast::unit_test::suite& test) +test_file(beast::unit_test::suite& test) { - BOOST_STATIC_ASSERT(is_file::value); + BOOST_STATIC_ASSERT( + is_file::value); + BOOST_STATIC_ASSERT( + ! std::is_copy_constructible::value); + BOOST_STATIC_ASSERT( + ! std::is_copy_assignable::value); - error_code ec; - auto const temp = boost::filesystem::unique_path(); + namespace fs = boost::filesystem; + class temp_path + { + fs::path path_; + std::string str_; + + public: + temp_path() + : path_(fs::unique_path()) + , str_(path_.string()) + { + } + + operator fs::path const&() + { + return path_; + } + + operator char const*() + { + return str_.c_str(); + } + }; + + auto const create = + [&test](fs::path const& path) + { + auto const s = + path.string(); + test.BEAST_EXPECT(! fs::exists(path)); + FILE* f = ::fopen(s.c_str(), "w"); + if( test.BEAST_EXPECT(f != nullptr)) + ::fclose(f); + }; + + auto const remove = + [](fs::path const& path) + { + error_code ec; + fs::remove(path); + }; + + temp_path path; + + // bad file descriptor + { + File f; + char buf[1]; + test.BEAST_EXPECT(! f.is_open()); + test.BEAST_EXPECT(! fs::exists(path)); + { + error_code ec; + f.size(ec); + test.BEAST_EXPECT(ec == errc::bad_file_descriptor); + } + { + error_code ec; + f.pos(ec); + test.BEAST_EXPECT(ec == errc::bad_file_descriptor); + } + { + error_code ec; + f.seek(0, ec); + test.BEAST_EXPECT(ec == errc::bad_file_descriptor); + } + { + error_code ec; + f.read(buf, 0, ec); + test.BEAST_EXPECT(ec == errc::bad_file_descriptor); + } + { + error_code ec; + f.write(buf, 0, ec); + test.BEAST_EXPECT(ec == errc::bad_file_descriptor); + } + } + + // file_mode::read + { + { + File f; + error_code ec; + create(path); + f.open(path, file_mode::read, ec); + test.BEAST_EXPECT(! ec); + } + remove(path); + } + + // file_mode::scan + { + { + File f; + error_code ec; + create(path); + f.open(path, file_mode::scan, ec); + test.BEAST_EXPECT(! ec); + } + remove(path); + } + + // file_mode::write + { + { + File f; + error_code ec; + test.BEAST_EXPECT(! fs::exists(path)); + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(fs::exists(path)); + } + { + File f; + error_code ec; + test.BEAST_EXPECT(fs::exists(path)); + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(fs::exists(path)); + } + remove(path); + } + + // file_mode::write_new + { + { + File f; + error_code ec; + test.BEAST_EXPECT(! fs::exists(path)); + f.open(path, file_mode::write_new, ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(fs::exists(path)); + } + { + File f; + error_code ec; + test.BEAST_EXPECT(fs::exists(path)); + f.open(path, file_mode::write_new, ec); + test.BEAST_EXPECT(ec); + } + remove(path); + } + + // file_mode::write_existing + { + { + File f; + error_code ec; + test.BEAST_EXPECT(! fs::exists(path)); + f.open(path, file_mode::write_existing, ec); + test.BEAST_EXPECT(ec); + test.BEAST_EXPECT(! fs::exists(path)); + } + { + File f; + error_code ec; + create(path); + test.BEAST_EXPECT(fs::exists(path)); + f.open(path, file_mode::write_existing, ec); + test.BEAST_EXPECT(! ec); + } + remove(path); + } + + // file_mode::append + { + { + File f; + error_code ec; + test.BEAST_EXPECT(! fs::exists(path)); + f.open(path, file_mode::append, ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(fs::exists(path)); + } + { + File f; + error_code ec; + test.BEAST_EXPECT(fs::exists(path)); + f.open(path, file_mode::append, ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(fs::exists(path)); + } + remove(path); + } + + // file_mode::append_existing + { + { + File f; + error_code ec; + test.BEAST_EXPECT(! fs::exists(path)); + f.open(path, file_mode::append_existing, ec); + test.BEAST_EXPECT(ec); + test.BEAST_EXPECT(! fs::exists(path)); + } + remove(path); + { + File f; + error_code ec; + create(path); + test.BEAST_EXPECT(fs::exists(path)); + f.open(path, file_mode::append_existing, ec); + test.BEAST_EXPECT(! ec); + } + remove(path); + } + + // special members { { File f1; - test.BEAST_EXPECT(! f1.is_open()); - f1.open(temp.string().c_str(), file_mode::write, ec); + error_code ec; + f1.open(path, file_mode::write, ec); test.BEAST_EXPECT(! ec); - File f2{std::move(f1)}; + test.BEAST_EXPECT(f1.is_open()); + + // move constructor + File f2(std::move(f1)); test.BEAST_EXPECT(! f1.is_open()); test.BEAST_EXPECT(f2.is_open()); + + // move assignment File f3; f3 = std::move(f2); test.BEAST_EXPECT(! f2.is_open()); test.BEAST_EXPECT(f3.is_open()); - f1.close(ec); - test.BEAST_EXPECT(! ec); - auto const temp2 = boost::filesystem::unique_path(); - f3.open(temp2.string().c_str(), file_mode::read, ec); - test.BEAST_EXPECT(ec); - ec.assign(0, ec.category()); } - boost::filesystem::remove(temp, ec); - test.BEAST_EXPECT(! ec); + remove(path); } - File f; + // re-open + { + { + File f; + error_code ec; + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); + } + remove(path); + } - test.BEAST_EXPECT(! f.is_open()); + // re-assign + { + temp_path path2; + { + error_code ec; - f.size(ec); - test.BEAST_EXPECT(ec == errc::bad_file_descriptor); - ec.assign(0, ec.category()); + File f1; + f1.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); - f.pos(ec); - test.BEAST_EXPECT(ec == errc::bad_file_descriptor); - ec.assign(0, ec.category()); + File f2; + f2.open(path2, file_mode::write, ec); + test.BEAST_EXPECT(! ec); - f.seek(0, ec); - test.BEAST_EXPECT(ec == errc::bad_file_descriptor); - ec.assign(0, ec.category()); + f2 = std::move(f1); + test.BEAST_EXPECT(! f1.is_open()); + test.BEAST_EXPECT(f2.is_open()); + } + remove(path); + remove(path2); + } - char tmp[1]; + // self-move + { + { + File f; + error_code ec; + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); + f = std::move(f); + test.BEAST_EXPECT(f.is_open()); + } + remove(path); + } - f.read(tmp, 0, ec); - test.BEAST_EXPECT(ec == errc::bad_file_descriptor); - ec.assign(0, ec.category()); + // native_handle + { + { + File f; + auto none = f.native_handle(); + error_code ec; + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); + auto fd = f.native_handle(); + test.BEAST_EXPECT(fd != none); + f.native_handle(none); + test.BEAST_EXPECT(! f.is_open()); + } + remove(path); + } - f.write(tmp, 0, ec); - test.BEAST_EXPECT(ec == errc::bad_file_descriptor); - ec.assign(0, ec.category()); + // read and write + { + string_view const s = "Hello, world!"; - f.open(temp.string().c_str(), file_mode::write, ec); - test.BEAST_EXPECT(! ec); + // write + { + File f; + error_code ec; + f.open(path, file_mode::write, ec); + test.BEAST_EXPECT(! ec); - std::string const s = "Hello, world!"; - f.write(s.data(), s.size(), ec); - test.BEAST_EXPECT(! ec); + f.write(s.data(), s.size(), ec); + test.BEAST_EXPECT(! ec); - auto size = f.size(ec); - test.BEAST_EXPECT(! ec); - test.BEAST_EXPECT(size == s.size()); + auto size = f.size(ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(size == s.size()); - auto pos = f.pos(ec); - test.BEAST_EXPECT(! ec); - test.BEAST_EXPECT(pos == size); + auto pos = f.pos(ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(pos == size); - f.close(ec); - test.BEAST_EXPECT(! ec); + f.close(ec); + test.BEAST_EXPECT(! ec); + } - f.open(temp.string().c_str(), file_mode::read, ec); - test.BEAST_EXPECT(! ec); + // read + { + File f; + error_code ec; + f.open(path, file_mode::read, ec); + test.BEAST_EXPECT(! ec); - std::string buf; - buf.resize(s.size()); - f.read(&buf[0], buf.size(), ec); - test.BEAST_EXPECT(! ec); - test.BEAST_EXPECT(buf == s); + std::string buf; + buf.resize(s.size()); + f.read(&buf[0], buf.size(), ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(buf == s); - f.seek(1, ec); - test.BEAST_EXPECT(! ec); - buf.resize(3); - f.read(&buf[0], buf.size(), ec); - test.BEAST_EXPECT(! ec); - test.BEAST_EXPECT(buf == "ell"); + f.seek(1, ec); + test.BEAST_EXPECT(! ec); + buf.resize(3); + f.read(&buf[0], buf.size(), ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(buf == "ell"); - pos = f.pos(ec); - test.BEAST_EXPECT(! ec); - test.BEAST_EXPECT(pos == 4); + auto pos = f.pos(ec); + test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(pos == 4); + } + remove(path); + } - f.close(ec); - test.BEAST_EXPECT(! ec); - boost::filesystem::remove(temp, ec); - test.BEAST_EXPECT(! ec); + test.BEAST_EXPECT(! fs::exists(path)); } } // beast diff --git a/test/beast/core/file_win32.cpp b/test/beast/core/file_win32.cpp index 6be3f190..6223e64e 100644 --- a/test/beast/core/file_win32.cpp +++ b/test/beast/core/file_win32.cpp @@ -20,8 +20,6 @@ namespace boost { namespace beast { -BOOST_STATIC_ASSERT(! std::is_copy_constructible::value); - class file_win32_test : public beast::unit_test::suite { @@ -29,7 +27,7 @@ public: void run() { - doTestFile(*this); + test_file(*this); } };