[Rework] Split locked and unlocked files, as mmap does not need flock normally

2 files changed, 127 insertions, 54 deletions

diff --git a/src/libutil/cxx/locked_file.cxx b/src/libutil/cxx/locked_file.cxx
index bd25b0fc7..b4d865626 100644
--- a/src/libutil/cxx/locked_file.cxx
+++ b/src/libutil/cxx/locked_file.cxx
@@ -24,7 +24,7 @@
 
 namespace rspamd::util {
 
-auto raii_locked_file::open(const char *fname, int flags) -> tl::expected<raii_locked_file, std::string>
+auto raii_file::open(const char *fname, int flags) -> tl::expected<raii_file, std::string>
 {
 	int oflags = flags;
 #ifdef O_CLOEXEC
@@ -46,7 +46,7 @@ auto raii_locked_file::open(const char *fname, int flags) -> tl::expected<raii_l
 		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno)));
 	}
 
-	auto ret = raii_locked_file{fname, fd, false};
+	auto ret = raii_file{fname, fd, false};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
 		return tl::make_unexpected(fmt::format("cannot stat file {}: {}", fname, ::strerror(errno)));
@@ -55,18 +55,7 @@ auto raii_locked_file::open(const char *fname, int flags) -> tl::expected<raii_l
 	return ret;
 }
 
-raii_locked_file::~raii_locked_file()
-{
-	if (fd != -1) {
-		if (temp) {
-			(void)unlink(fname.c_str());
-		}
-		(void) rspamd_file_unlock(fd, FALSE);
-		close(fd);
-	}
-}
-
-auto raii_locked_file::create(const char *fname, int flags, int perms) -> tl::expected<raii_locked_file, std::string>
+auto raii_file::create(const char *fname, int flags, int perms) -> tl::expected<raii_file, std::string>
 {
 	int oflags = flags;
 #ifdef O_CLOEXEC
@@ -88,7 +77,7 @@ auto raii_locked_file::create(const char *fname, int flags, int perms) -> tl::ex
 		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno)));
 	}
 
-	auto ret = raii_locked_file{fname, fd, false};
+	auto ret = raii_file{fname, fd, false};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
 		return tl::make_unexpected(fmt::format("cannot stat file {}: {}", fname, ::strerror(errno)));
@@ -97,7 +86,7 @@ auto raii_locked_file::create(const char *fname, int flags, int perms) -> tl::ex
 	return ret;
 }
 
-auto raii_locked_file::create_temp(const char *fname, int flags, int perms) -> tl::expected<raii_locked_file, std::string>
+auto raii_file::create_temp(const char *fname, int flags, int perms) -> tl::expected<raii_file, std::string>
 {
 	int oflags = flags;
 #ifdef O_CLOEXEC
@@ -114,11 +103,12 @@ auto raii_locked_file::create_temp(const char *fname, int flags, int perms) -> t
 	}
 
 	if (!rspamd_file_lock(fd, TRUE)) {
+		unlink(fname);
 		close(fd);
 		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno)));
 	}
 
-	auto ret = raii_locked_file{fname, fd, true};
+	auto ret = raii_file{fname, fd, true};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
 		return tl::make_unexpected(fmt::format("cannot stat file {}: {}", fname, ::strerror(errno)));
@@ -127,7 +117,7 @@ auto raii_locked_file::create_temp(const char *fname, int flags, int perms) -> t
 	return ret;
 }
 
-auto raii_locked_file::mkstemp(const char *pattern, int flags, int perms) -> tl::expected<raii_locked_file, std::string>
+auto raii_file::mkstemp(const char *pattern, int flags, int perms) -> tl::expected<raii_file, std::string>
 {
 	int oflags = flags;
 #ifdef O_CLOEXEC
@@ -151,7 +141,7 @@ auto raii_locked_file::mkstemp(const char *pattern, int flags, int perms) -> tl:
 		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", pattern, ::strerror(errno)));
 	}
 
-	auto ret = raii_locked_file{mutable_pattern.c_str(), fd, true};
+	auto ret = raii_file{mutable_pattern.c_str(), fd, true};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
 		return tl::make_unexpected(fmt::format("cannot stat file {}: {}",
@@ -161,13 +151,41 @@ auto raii_locked_file::mkstemp(const char *pattern, int flags, int perms) -> tl:
 	return ret;
 }
 
-raii_mmaped_locked_file::raii_mmaped_locked_file(raii_locked_file &&_file, void *_map)
+raii_file::~raii_file() noexcept
+{
+	if (fd != -1) {
+		if (temp) {
+			(void)unlink(fname.c_str());
+		}
+		(void) rspamd_file_unlock(fd, FALSE);
+		close(fd);
+	}
+}
+
+
+raii_locked_file::~raii_locked_file() noexcept
+{
+	if (fd != -1) {
+		(void) rspamd_file_unlock(fd, FALSE);
+	}
+}
+
+auto raii_locked_file::lock_raii_file(raii_file &&unlocked) -> tl::expected<raii_locked_file, std::string>
+{
+	if (!rspamd_file_lock(unlocked.get_fd(), TRUE)) {
+		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", unlocked.get_name(), ::strerror(errno)));
+	}
+
+	return raii_locked_file{std::move(unlocked)};
+}
+
+raii_mmaped_file::raii_mmaped_file(raii_file &&_file, void *_map)
 		: file(std::move(_file)), map(_map)
 {
 }
 
-auto raii_mmaped_locked_file::mmap_shared(raii_locked_file &&file,
-										  int flags) -> tl::expected<raii_mmaped_locked_file, std::string>
+auto raii_mmaped_file::mmap_shared(raii_file &&file,
+								   int flags) -> tl::expected<raii_mmaped_file, std::string>
 {
 	void *map;
 
@@ -179,29 +197,29 @@ auto raii_mmaped_locked_file::mmap_shared(raii_locked_file &&file,
 
 	}
 
-	return raii_mmaped_locked_file{std::move(file), map};
+	return raii_mmaped_file{std::move(file), map};
 }
 
-auto raii_mmaped_locked_file::mmap_shared(const char *fname, int open_flags,
-										  int mmap_flags) -> tl::expected<raii_mmaped_locked_file, std::string>
+auto raii_mmaped_file::mmap_shared(const char *fname, int open_flags,
+								   int mmap_flags) -> tl::expected<raii_mmaped_file, std::string>
 {
-	auto file = raii_locked_file::open(fname, open_flags);
+	auto file = raii_file::open(fname, open_flags);
 
 	if (!file.has_value()) {
 		return tl::make_unexpected(file.error());
 	}
 
-	return raii_mmaped_locked_file::mmap_shared(std::move(file.value()), mmap_flags);
+	return raii_mmaped_file::mmap_shared(std::move(file.value()), mmap_flags);
 }
 
-raii_mmaped_locked_file::~raii_mmaped_locked_file()
+raii_mmaped_file::~raii_mmaped_file()
 {
 	if (map != nullptr) {
 		munmap(map, file.get_stat().st_size);
 	}
 }
 
-raii_mmaped_locked_file::raii_mmaped_locked_file(raii_mmaped_locked_file &&other) noexcept
+raii_mmaped_file::raii_mmaped_file(raii_mmaped_file &&other) noexcept
 		: file(std::move(other.file))
 {
 	std::swap(map, other.map);
diff --git a/src/libutil/cxx/locked_file.hxx b/src/libutil/cxx/locked_file.hxx
index 94561a2d4..8690cfb64 100644
--- a/src/libutil/cxx/locked_file.hxx
+++ b/src/libutil/cxx/locked_file.hxx
@@ -24,16 +24,16 @@
 
 namespace rspamd::util {
 /**
- * A simple RAII object to contain a file descriptor with an flock wrap
+ * A simple RAII object to contain a move only file descriptor
  * A file is unlocked and closed when not needed
  */
-struct raii_locked_file final {
-	~raii_locked_file();
+struct raii_file {
+	virtual ~raii_file() noexcept;
 
-	static auto open(const char *fname, int flags) -> tl::expected<raii_locked_file, std::string>;
-	static auto create(const char *fname, int flags, int perms) -> tl::expected<raii_locked_file, std::string>;
-	static auto create_temp(const char *fname, int flags, int perms) -> tl::expected<raii_locked_file, std::string>;
-	static auto mkstemp(const char *pattern, int flags, int perms) -> tl::expected<raii_locked_file, std::string>;
+	static auto open(const char *fname, int flags) -> tl::expected<raii_file, std::string>;
+	static auto create(const char *fname, int flags, int perms) -> tl::expected<raii_file, std::string>;
+	static auto create_temp(const char *fname, int flags, int perms) -> tl::expected<raii_file, std::string>;
+	static auto mkstemp(const char *pattern, int flags, int perms) -> tl::expected<raii_file, std::string>;
 
 	auto get_fd() const -> int {
 		return fd;
@@ -79,7 +79,7 @@ struct raii_locked_file final {
 		}
 	}
 
-	raii_locked_file& operator=(raii_locked_file &&other) noexcept {
+	raii_file& operator=(raii_file &&other) noexcept {
 		std::swap(fd, other.fd);
 		std::swap(temp, other.temp);
 		std::swap(fname, other.fname);
@@ -88,30 +88,85 @@ struct raii_locked_file final {
 		return *this;
 	}
 
-	raii_locked_file(raii_locked_file &&other) noexcept {
+	raii_file(raii_file &&other) noexcept {
 		*this = std::move(other);
 	}
 
 	/* Do not allow copy/default ctor */
-	const raii_locked_file& operator=(const raii_locked_file &other) = delete;
-	raii_locked_file() = delete;
-	raii_locked_file(const raii_locked_file &other) = delete;
-private:
+	const raii_file& operator=(const raii_file &other) = delete;
+	raii_file() = delete;
+	raii_file(const raii_file &other) = delete;
+protected:
 	int fd = -1;
 	bool temp;
 	std::string fname;
 	struct stat st;
 
-	explicit raii_locked_file(const char *_fname, int _fd, bool _temp) : fd(_fd), temp(_temp), fname(_fname) {}
+	explicit raii_file(const char *fname, int fd, bool temp) : fd(fd), temp(temp), fname(fname) {}
+};
+/**
+ * A simple RAII object to contain a file descriptor with an flock wrap
+ * A file is unlocked and closed when not needed
+ */
+struct raii_locked_file final : public raii_file {
+	~raii_locked_file() noexcept override;
+
+	static auto open(const char *fname, int flags) -> tl::expected<raii_locked_file, std::string> {
+		auto locked = raii_file::open(fname, flags).and_then([]<class T>(T &&file) {
+			return lock_raii_file(std::forward<T>(file));
+		});
+
+		return locked;
+	}
+	static auto create(const char *fname, int flags, int perms) -> tl::expected<raii_locked_file, std::string> {
+		auto locked = raii_file::create(fname, flags, perms).and_then([]<class T>(T &&file) {
+			return lock_raii_file(std::forward<T>(file));
+		});
+
+		return locked;
+	}
+	static auto create_temp(const char *fname, int flags, int perms) -> tl::expected<raii_locked_file, std::string> {
+		auto locked = raii_file::create_temp(fname, flags, perms).and_then([]<class T>(T &&file) {
+			return lock_raii_file(std::forward<T>(file));
+		});
+
+		return locked;
+	}
+	static auto mkstemp(const char *pattern, int flags, int perms) -> tl::expected<raii_locked_file, std::string> {
+		auto locked = raii_file::mkstemp(pattern, flags, perms).and_then([]<class T>(T &&file) {
+			return lock_raii_file(std::forward<T>(file));
+		});
+
+		return locked;
+	}
+
+	raii_locked_file& operator=(raii_locked_file &&other) noexcept {
+		std::swap(fd, other.fd);
+		std::swap(temp, other.temp);
+		std::swap(fname, other.fname);
+		std::swap(st, other.st);
+
+		return *this;
+	}
+
+	raii_locked_file(raii_locked_file &&other) noexcept : raii_file(static_cast<raii_file &&>(std::move(other))) {}
+	/* Do not allow copy/default ctor */
+	const raii_locked_file& operator=(const raii_locked_file &other) = delete;
+	raii_locked_file() = delete;
+	raii_locked_file(const raii_locked_file &other) = delete;
+private:
+	static auto lock_raii_file(raii_file &&unlocked) -> tl::expected<raii_locked_file, std::string>;
+	raii_locked_file(raii_file &&other) noexcept : raii_file(std::move(other)) {}
+	explicit raii_locked_file(const char *fname, int fd, bool temp) : raii_file(fname, fd, temp) {}
 };
 
 /**
  * A mmap wrapper on top of a locked file
  */
-struct raii_mmaped_locked_file final {
-	~raii_mmaped_locked_file();
-	static auto mmap_shared(raii_locked_file &&file, int flags) -> tl::expected<raii_mmaped_locked_file, std::string>;
-	static auto mmap_shared(const char *fname, int open_flags, int mmap_flags) -> tl::expected<raii_mmaped_locked_file, std::string>;
+struct raii_mmaped_file final {
+	~raii_mmaped_file();
+	static auto mmap_shared(raii_file &&file, int flags) -> tl::expected<raii_mmaped_file, std::string>;
+	static auto mmap_shared(const char *fname, int open_flags, int mmap_flags) -> tl::expected<raii_mmaped_file, std::string>;
 	// Returns a constant pointer to the underlying map
 	auto get_map() const -> void* {return map;}
 	// Passes the ownership of the mmaped memory to the callee
@@ -123,23 +178,23 @@ struct raii_mmaped_locked_file final {
 
 	auto get_size() const -> std::size_t { return file.get_stat().st_size; }
 
-	raii_mmaped_locked_file& operator=(raii_mmaped_locked_file &&other) noexcept {
+	raii_mmaped_file& operator=(raii_mmaped_file &&other) noexcept {
 		std::swap(map, other.map);
 		file = std::move(other.file);
 
 		return *this;
 	}
 
-	raii_mmaped_locked_file(raii_mmaped_locked_file &&other) noexcept;
+	raii_mmaped_file(raii_mmaped_file &&other) noexcept;
 
 	/* Do not allow copy/default ctor */
-	const raii_mmaped_locked_file& operator=(const raii_mmaped_locked_file &other) = delete;
-	raii_mmaped_locked_file() = delete;
-	raii_mmaped_locked_file(const raii_mmaped_locked_file &other) = delete;
+	const raii_mmaped_file& operator=(const raii_mmaped_file &other) = delete;
+	raii_mmaped_file() = delete;
+	raii_mmaped_file(const raii_mmaped_file &other) = delete;
 private:
 	/* Is intended to be used with map_shared */
-	explicit raii_mmaped_locked_file(raii_locked_file &&_file, void *_map);
-	raii_locked_file file;
+	explicit raii_mmaped_file(raii_file &&_file, void *_map);
+	raii_file file;
 	void *map = nullptr;
 };