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

@@ -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:

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.
+        This mode permits random-access reading and writing
-
+        for the specified file. If the file does not exist
-        @li If the file exists, it is truncated to
+        prior to the function call, it is created with an
-        zero size upon opening.
+        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

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.
 

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)
 {
+    if(fd != -1)
+    {
         for(;;)
         {
-        if(! ::close(fd))
+            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)
+    native_close(fd_);
-        detail::file_posix_close(fd_);
 }
 
 file_posix::
@@ -74,8 +73,7 @@ operator=(file_posix&& other)
 {
     if(&other == this)
         return *this;
-    if(fd_ != -1)
+    native_close(fd_);
-        detail::file_posix_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)
+    native_close(fd_);
-         detail::file_posix_close(fd_);
     fd_ = fd;
 }
 
@@ -94,36 +91,23 @@ void
 file_posix::
 close(error_code& ec)
 {
-    if(fd_ != -1)
+    auto const ev = native_close(fd_);
-    {
-        auto const ev =
-            detail::file_posix_close(fd_);
     if(ev)
         ec.assign(ev, system_category());
     else
         ec = {};
-        fd_ = -1;
-    }
-    else
-    {
-        ec = {};
-    }
 }
 
 void
 file_posix::
 open(char const* path, file_mode mode, error_code& ec)
 {
-    if(fd_ != -1)
+    auto const ev = native_close(fd_);
-    {
-        auto const ev =
-            detail::file_posix_close(fd_);
     if(ev)
         ec.assign(ev, system_category());
     else
         ec = {};
-        fd_ = -1;
+
-    }
     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;
+        f = O_WRONLY | 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;
     #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_);
+        native_close(fd_);
-        fd_ = -1;
         ec.assign(ev, system_category());
         return;
     }

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 <boost/config/workaround.hpp>
 #include <boost/core/exchange.hpp>
 #include <limits>
 
@@ -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::read:
-    case file_mode::scan:               s = "rb"; break;
+        s = "rb";
-    case file_mode::write:              s = "wb"; break;
+        break;
-    case file_mode::write_new:          s = "wbx"; break;
+
-    case file_mode::write_existing:     s = "wb"; break;
+    case file_mode::scan:
-    case file_mode::append:             s = "ab"; break;
+    #if BOOST_MSVC
-    case file_mode::append_new:         s = "abx"; break;
+        s = "rbS";
-    case file_mode::append_existing:    s = "ab"; break;
+    #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;

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_;

test/beast/core/buffer_test.hpp

@@ -7,8 +7,8 @@
 // Official repository: https://github.com/boostorg/beast
 //
 
-#ifndef BOOST_BEAST_TEST_BUFFER_TEST_HPP
+#ifndef BOOST_BEAST_BUFFER_TEST_HPP
-#define BOOST_BEAST_TEST_BUFFER_TEST_HPP
+#define BOOST_BEAST_BUFFER_TEST_HPP
 
 #include <boost/beast/core/detail/config.hpp>
 #include <boost/beast/_experimental/unit_test/suite.hpp>

test/beast/core/file_posix.cpp

@@ -29,7 +29,7 @@ public:
     void
     run()
     {
-        doTestFile<file_posix>(*this);
+        test_file<file_posix>(*this);
     }
 };
 

test/beast/core/file_stdio.cpp

@@ -27,7 +27,7 @@ public:
     void
     run()
     {
-        doTestFile<file_stdio>(*this);
+        test_file<file_stdio>(*this);
     }
 };
 

test/beast/core/file_test.hpp

@@ -7,81 +7,336 @@
 // Official repository: https://github.com/boostorg/beast
 //
 
-#ifndef BOOST_BEAST_TEST_CORE_FILE_TEST_HPP
+#ifndef BOOST_BEAST_FILE_TEST_HPP
-#define BOOST_BEAST_TEST_CORE_FILE_TEST_HPP
+#define BOOST_BEAST_FILE_TEST_HPP
 
 #include <boost/beast/core/type_traits.hpp>
 #include <boost/beast/_experimental/unit_test/suite.hpp>
-#include <boost/config.hpp>
+#include <boost/filesystem/path.hpp>
-#include <boost/filesystem.hpp>
+#include <boost/filesystem/operations.hpp>
+#include <cstdio>
 #include <string>
+#include <type_traits>
 
 namespace boost {
 namespace beast {
 
 template<class File>
 void
-doTestFile(beast::unit_test::suite& test)
+test_file(beast::unit_test::suite& test)
 {
-    BOOST_STATIC_ASSERT(is_file<File>::value);
+    BOOST_STATIC_ASSERT(
+        is_file<File>::value);
+    BOOST_STATIC_ASSERT(
+        ! std::is_copy_constructible<File>::value);
+    BOOST_STATIC_ASSERT(
+        ! std::is_copy_assignable<File>::value);
 
+    namespace fs = boost::filesystem;
+
+    class temp_path
+    {
+        fs::path path_;
+        std::string str_;
+
+    public:
+        temp_path()
+            : path_(fs::unique_path())
+            , str_(path_.string<std::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<std::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;
-    auto const temp = boost::filesystem::unique_path();
+            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());
+            error_code ec;
-            f1.open(temp.string<std::string>().c_str(), file_mode::write, 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<std::string>().c_str(), file_mode::read, ec);
-            test.BEAST_EXPECT(ec);
-            ec.assign(0, ec.category());
         }
-        boost::filesystem::remove(temp, ec);
+        remove(path);
-        test.BEAST_EXPECT(! ec);
     }
 
+    // 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);
+            File f1;
-    test.BEAST_EXPECT(ec == errc::bad_file_descriptor);
+            f1.open(path, file_mode::write, ec);
-    ec.assign(0, ec.category());
+            test.BEAST_EXPECT(! ec);
-
+
-    f.pos(ec);
+            File f2;
-    test.BEAST_EXPECT(ec == errc::bad_file_descriptor);
+            f2.open(path2, file_mode::write, ec);
-    ec.assign(0, ec.category());
+            test.BEAST_EXPECT(! ec);
-
+
-    f.seek(0, ec);
+            f2 = std::move(f1);
-    test.BEAST_EXPECT(ec == errc::bad_file_descriptor);
+            test.BEAST_EXPECT(! f1.is_open());
-    ec.assign(0, ec.category());
+            test.BEAST_EXPECT(f2.is_open());
-
+        }
-    char tmp[1];
+        remove(path);
-
+        remove(path2);
-    f.read(tmp, 0, ec);
+    }
-    test.BEAST_EXPECT(ec == errc::bad_file_descriptor);
+
-    ec.assign(0, ec.category());
+    // self-move
-
+    {
-    f.write(tmp, 0, ec);
+        {
-    test.BEAST_EXPECT(ec == errc::bad_file_descriptor);
+            File f;
-    ec.assign(0, ec.category());
+            error_code ec;
-
+            f.open(path, file_mode::write, ec);
-    f.open(temp.string<std::string>().c_str(), file_mode::write, ec);
+            test.BEAST_EXPECT(! ec);
+            f = std::move(f);
+            test.BEAST_EXPECT(f.is_open());
+        }
+        remove(path);
+    }
+
+    // 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);
+    }
+
+    // read and write
+    {
+        string_view const s = "Hello, world!";
+
+        // 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);
 
@@ -95,8 +350,13 @@ doTestFile(beast::unit_test::suite& test)
 
             f.close(ec);
             test.BEAST_EXPECT(! ec);
+        }
 
-    f.open(temp.string<std::string>().c_str(), file_mode::read, ec);
+        // read
+        {
+            File f;
+            error_code ec;
+            f.open(path, file_mode::read, ec);
             test.BEAST_EXPECT(! ec);
 
             std::string buf;
@@ -112,14 +372,14 @@ doTestFile(beast::unit_test::suite& test)
             test.BEAST_EXPECT(! ec);
             test.BEAST_EXPECT(buf == "ell");
 
-    pos = f.pos(ec);
+            auto pos = f.pos(ec);
             test.BEAST_EXPECT(! ec);
             test.BEAST_EXPECT(pos == 4);
+        }
+        remove(path);
+    }
 
-    f.close(ec);
+    test.BEAST_EXPECT(! fs::exists(path));
-    test.BEAST_EXPECT(! ec);
-    boost::filesystem::remove(temp, ec);
-    test.BEAST_EXPECT(! ec);
 }
 
 } // beast

test/beast/core/file_win32.cpp

@@ -20,8 +20,6 @@
 namespace boost {
 namespace beast {
 
-BOOST_STATIC_ASSERT(! std::is_copy_constructible<file_win32>::value);
-
 class file_win32_test
     : public beast::unit_test::suite
 {
@@ -29,7 +27,7 @@ public:
     void
     run()
     {
-        doTestFile<file_win32>(*this);
+        test_file<file_win32>(*this);
     }
 };