fd/atomic: Better separate this orthogonal functionality
Nothing is using the old interface, so we can move it for better
separation of responisbilities.
Change-Id: I3b6e429b106aa22e58df25e0801b60af0fbd6d70
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/include-fd/meson.build b/include-fd/meson.build
index 1e85293..3e36cb8 100644
--- a/include-fd/meson.build
+++ b/include-fd/meson.build
@@ -1,6 +1,7 @@
stdplus_headers += include_directories('.')
install_headers(
+ 'stdplus/fd/atomic.hpp',
'stdplus/fd/create.hpp',
'stdplus/fd/dupable.hpp',
'stdplus/fd/fmt.hpp',
diff --git a/include-fd/stdplus/fd/atomic.hpp b/include-fd/stdplus/fd/atomic.hpp
new file mode 100644
index 0000000..904e706
--- /dev/null
+++ b/include-fd/stdplus/fd/atomic.hpp
@@ -0,0 +1,40 @@
+#pragma once
+#include <filesystem>
+#include <stdplus/fd/managed.hpp>
+#include <string>
+#include <string_view>
+
+namespace stdplus
+{
+namespace fd
+{
+
+class AtomicWriter : public stdplus::FdImpl
+{
+ public:
+ AtomicWriter(const std::filesystem::path& filename, int mode,
+ std::string_view tmpl = {});
+ AtomicWriter(AtomicWriter&& other);
+ AtomicWriter& operator=(AtomicWriter&& other);
+ ~AtomicWriter();
+
+ void commit(bool allow_copy = false);
+
+ inline const std::string& getTmpname() const
+ {
+ return tmpname;
+ }
+
+ int get() const override;
+
+ private:
+ std::filesystem::path filename;
+ int mode;
+ std::string tmpname;
+ ManagedFd fd;
+
+ void cleanup() noexcept;
+};
+
+} // namespace fd
+} // namespace stdplus
diff --git a/include-fd/stdplus/fd/fmt.hpp b/include-fd/stdplus/fd/fmt.hpp
index b0e4c11..de77b68 100644
--- a/include-fd/stdplus/fd/fmt.hpp
+++ b/include-fd/stdplus/fd/fmt.hpp
@@ -1,11 +1,9 @@
#pragma once
-#include <filesystem>
#include <fmt/format.h>
#include <functional>
#include <stdplus/fd/intf.hpp>
-#include <stdplus/fd/managed.hpp>
-#include <string_view>
#include <type_traits>
+#include <utility>
namespace fmt
{
@@ -58,41 +56,5 @@
void writeIfNeeded();
};
-class FormatToFile
-{
- public:
- explicit FormatToFile(std::string_view tmpl = "/tmp/stdplus.XXXXXX");
- ~FormatToFile();
- FormatToFile(const FormatToFile&) = delete;
- FormatToFile(FormatToFile&&) = delete;
- FormatToFile& operator=(const FormatToFile&) = delete;
- FormatToFile& operator=(FormatToFile&&) = delete;
-
- template <typename... Args>
- inline void append(fmt::format_string<Args...> fmt, Args&&... args)
- {
- buf.append(fmt, std::forward<Args>(args)...);
- }
- template <typename T, typename... Args,
- std::enable_if_t<fmt::detail::is_compiled_string<T>::value,
- bool> = true>
- inline void append(const T& t, Args&&... args)
- {
- buf.append(t, std::forward<Args>(args)...);
- }
-
- void commit(const std::filesystem::path& out, int mode = 0644);
-
- inline const std::string& getTmpname() const
- {
- return tmpname;
- }
-
- private:
- std::string tmpname;
- ManagedFd fd;
- FormatBuffer buf;
-};
-
} // namespace fd
} // namespace stdplus
diff --git a/src/fd/atomic.cpp b/src/fd/atomic.cpp
new file mode 100644
index 0000000..df200e6
--- /dev/null
+++ b/src/fd/atomic.cpp
@@ -0,0 +1,125 @@
+#include <cstdlib>
+#include <filesystem>
+#include <fmt/format.h>
+#include <stdplus/fd/atomic.hpp>
+#include <stdplus/fd/managed.hpp>
+#include <stdplus/util/cexec.hpp>
+#include <sys/stat.h>
+#include <system_error>
+#include <utility>
+
+namespace stdplus
+{
+namespace fd
+{
+
+static std::string makeTmpName(const std::filesystem::path& filename)
+{
+ auto name = filename.filename();
+ auto path =
+ filename.parent_path() / fmt::format(".{}.XXXXXX", name.native());
+ return path.native();
+}
+
+static int mktemp(std::string& tmpl)
+{
+ mode_t old = umask(0077);
+ int fd = mkstemp(tmpl.data());
+ umask(old);
+ return CHECK_ERRNO(fd, [&](int error) {
+ throw std::system_error(error, std::generic_category(),
+ fmt::format("mkstemp({})", tmpl));
+ });
+}
+
+AtomicWriter::AtomicWriter(const std::filesystem::path& filename, int mode,
+ std::string_view tmpl) :
+ filename(filename),
+ mode(mode),
+ tmpname(!tmpl.empty() ? std::string(tmpl) : makeTmpName(filename)),
+ fd(mktemp(tmpname))
+{
+}
+
+AtomicWriter::AtomicWriter(AtomicWriter&& other) :
+ filename(std::move(other.filename)), mode(other.mode),
+ tmpname(std::move(other.tmpname)), fd(std::move(other.fd))
+{
+ // We don't want to cleanup twice
+ other.tmpname.clear();
+}
+
+AtomicWriter& AtomicWriter::operator=(AtomicWriter&& other)
+{
+ if (this != &other)
+ {
+ filename = std::move(other.filename);
+ mode = other.mode;
+ tmpname = std::move(other.tmpname);
+ fd = std::move(other.fd);
+
+ // We don't want to cleanup twice
+ other.tmpname.clear();
+ }
+ return *this;
+}
+
+AtomicWriter::~AtomicWriter()
+{
+ cleanup();
+}
+
+void AtomicWriter::commit(bool allow_copy)
+{
+ try
+ {
+ CHECK_ERRNO(fsync(get()), "fsync");
+ CHECK_ERRNO(fchmod(get(), mode), "fchmod");
+ // We want the file to be closed before renaming it
+ {
+ auto ifd = std::move(fd);
+ }
+ try
+ {
+ std::filesystem::rename(tmpname, filename);
+ tmpname.clear();
+ }
+ catch (const std::filesystem::filesystem_error& e)
+ {
+ if (!allow_copy || e.code() != std::errc::cross_device_link)
+ {
+ throw;
+ }
+ std::filesystem::copy(tmpname, filename);
+ }
+ }
+ catch (...)
+ {
+ // We do this to ensure that any crashes that might happen before the
+ // destructor don't cause us to leave stale files.
+ cleanup();
+ throw;
+ }
+}
+
+int AtomicWriter::get() const
+{
+ return fd.get();
+}
+
+void AtomicWriter::cleanup() noexcept
+{
+ if (!tmpname.empty())
+ {
+ // Ensure the FD is closed prior to removing the file
+ {
+ auto ifd = std::move(fd);
+ }
+ std::error_code ec;
+ std::filesystem::remove(tmpname, ec);
+ tmpname.clear();
+ }
+}
+
+} // namespace fd
+} // namespace stdplus
diff --git a/src/fd/fmt.cpp b/src/fd/fmt.cpp
index 0eeb6ae..94d3bc1 100644
--- a/src/fd/fmt.cpp
+++ b/src/fd/fmt.cpp
@@ -1,8 +1,5 @@
-#include <cstdlib>
#include <stdplus/fd/fmt.hpp>
#include <stdplus/fd/ops.hpp>
-#include <stdplus/util/cexec.hpp>
-#include <sys/stat.h>
namespace stdplus
{
@@ -35,41 +32,5 @@
}
}
-FormatToFile::FormatToFile(std::string_view tmpl) :
- tmpname(tmpl),
- fd(CHECK_ERRNO(mkstemp(tmpname.data()),
- [&](int error) {
- auto msg = fmt::format("mkstemp({})", tmpname);
- tmpname.clear();
- throw std::system_error(error, std::generic_category(),
- msg);
- })),
- buf(fd)
-{
-}
-
-FormatToFile::~FormatToFile()
-{
- if (!tmpname.empty())
- {
- std::error_code ec;
- std::filesystem::remove(tmpname, ec);
- }
-}
-
-void FormatToFile::commit(const std::filesystem::path& out, int mode)
-{
- {
- buf.flush();
- auto ifd = std::move(fd);
- }
- CHECK_ERRNO(chmod(tmpname.c_str(), mode), [&](int error) {
- throw std::system_error(error, std::generic_category(),
- fmt::format("chmod({}, {})", tmpname, mode));
- });
- std::filesystem::rename(tmpname, out);
- tmpname.clear();
-}
-
} // namespace fd
} // namespace stdplus
diff --git a/src/meson.build b/src/meson.build
index f937dc7..91fecb5 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -24,6 +24,7 @@
if has_fd
stdplus_srcs += [
+ 'fd/atomic.cpp',
'fd/create.cpp',
'fd/dupable.cpp',
'fd/fmt.cpp',
diff --git a/test/fd/atomic.cpp b/test/fd/atomic.cpp
new file mode 100644
index 0000000..c67ce41
--- /dev/null
+++ b/test/fd/atomic.cpp
@@ -0,0 +1,71 @@
+#include <filesystem>
+#include <memory>
+#include <stdplus/fd/atomic.hpp>
+#include <stdplus/fd/ops.hpp>
+#include <stdplus/gtest/tmp.hpp>
+#include <string_view>
+
+namespace stdplus
+{
+namespace fd
+{
+
+using std::literals::string_view_literals::operator""sv;
+
+class AtomicWriterTest : public gtest::TestWithTmp
+{
+ protected:
+ std::string filename;
+ std::unique_ptr<AtomicWriter> file;
+
+ AtomicWriterTest() : filename(fmt::format("{}/out", CaseTmpDir()))
+ {
+ }
+
+ ~AtomicWriterTest() noexcept
+ {
+ if (file)
+ {
+ auto tmpname = file->getTmpname();
+ file.reset();
+ if (!tmpname.empty())
+ {
+ EXPECT_FALSE(std::filesystem::exists(tmpname));
+ }
+ }
+ }
+};
+
+TEST_F(AtomicWriterTest, NoCommit)
+{
+ ASSERT_NO_THROW(file = std::make_unique<AtomicWriter>(filename, 0644));
+ writeExact(*file, "hi\n"sv);
+ EXPECT_FALSE(std::filesystem::exists(filename));
+ EXPECT_TRUE(std::filesystem::exists(file->getTmpname()));
+}
+
+TEST_F(AtomicWriterTest, BadCommit)
+{
+ auto tmp = fmt::format("{}/tmp.XXXXXX", CaseTmpDir());
+ ASSERT_NO_THROW(file =
+ std::make_unique<AtomicWriter>("/dev/null", 0644, tmp));
+ writeExact(*file, "hi\n"sv);
+ EXPECT_TRUE(std::filesystem::exists(file->getTmpname()));
+ EXPECT_THROW(file->commit(), std::filesystem::filesystem_error);
+ EXPECT_EQ(file->getTmpname(), ""sv);
+}
+
+TEST_F(AtomicWriterTest, Basic)
+{
+ ASSERT_NO_THROW(file = std::make_unique<AtomicWriter>(filename, 0644));
+ writeExact(*file, "hi\n"sv);
+ EXPECT_TRUE(std::filesystem::exists(file->getTmpname()));
+ EXPECT_NO_THROW(file->commit());
+ EXPECT_EQ(file->getTmpname(), ""sv);
+ std::error_code ec;
+ EXPECT_EQ(3, std::filesystem::file_size(filename, ec));
+ EXPECT_EQ(std::errc{}, ec);
+}
+
+} // namespace fd
+} // namespace stdplus
diff --git a/test/fd/fmt.cpp b/test/fd/fmt.cpp
index 55f6c5d..f7ff9d8 100644
--- a/test/fd/fmt.cpp
+++ b/test/fd/fmt.cpp
@@ -1,11 +1,8 @@
#include <gtest/gtest.h>
-#include <filesystem>
#include <fmt/compile.h>
-#include <memory>
#include <stdplus/fd/fmt.hpp>
#include <stdplus/fd/managed.hpp>
-#include <stdplus/gtest/tmp.hpp>
#include <stdplus/util/cexec.hpp>
#include <sys/mman.h>
@@ -42,45 +39,5 @@
EXPECT_EQ(4109, fd.lseek(0, Whence::Cur));
}
-class FormatToFileTest : public gtest::TestWithTmp
-{
- protected:
- std::string tmpname;
- std::unique_ptr<FormatToFile> file;
-
- FormatToFileTest() :
- tmpname(fmt::format("{}/tmp.XXXXXX", CaseTmpDir())),
- file(std::make_unique<FormatToFile>(tmpname))
- {
- tmpname = file->getTmpname();
- EXPECT_TRUE(std::filesystem::exists(tmpname));
- }
-
- ~FormatToFileTest() noexcept(true)
- {
- file.reset();
- EXPECT_FALSE(std::filesystem::exists(tmpname));
- }
-};
-
-TEST_F(FormatToFileTest, NoCommit)
-{
- file->append("hi\n");
- EXPECT_EQ(0, std::filesystem::file_size(tmpname));
-}
-
-TEST_F(FormatToFileTest, Basic)
-{
- file->append("hi\n");
- file->append("hi\n"sv);
- file->append(FMT_STRING("hi\n"));
- file->append(FMT_COMPILE("hi\n"));
- EXPECT_EQ(0, std::filesystem::file_size(tmpname));
- auto filename = fmt::format("{}/out", CaseTmpDir());
- file->commit(filename);
- EXPECT_FALSE(std::filesystem::exists(tmpname));
- EXPECT_EQ(12, std::filesystem::file_size(filename));
-}
-
} // namespace fd
} // namespace stdplus
diff --git a/test/meson.build b/test/meson.build
index 7aada4a..452edae 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -23,6 +23,7 @@
gtests += {
'fd/dupable': [stdplus_fd_dep],
'fd/managed': [stdplus_fd_dep],
+ 'fd/fmt': [stdplus_fd_dep, stdplus_dep, gtest_main_dep],
'fd/intf': [stdplus_fd_dep],
'fd/impl': [stdplus_fd_dep],
'fd/line': [stdplus_fd_dep, stdplus_dep, gmock_dep, gtest_main_dep],
@@ -32,12 +33,12 @@
}
if has_gtest
gtests += {
- 'fd/fmt': [stdplus_fd_dep, stdplus_gtest_dep, stdplus_dep, gtest_main_dep],
+ 'fd/atomic': [stdplus_fd_dep, stdplus_gtest_dep, gtest_main_dep],
}
elif build_tests.enabled()
- error('Not testing fd/fmt feature')
+ error('Not testing fd/atomic feature')
else
- warning('Not testing fd/fmt feature')
+ warning('Not testing fd/atomic feature')
endif
elif build_tests.enabled()
error('Not testing file descriptor feature')