tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Cleanup utils::File API

Browse Source 
Summary:
Close the file descriptor in File destructor. This will prevent
accidental crashes during unexpected destructor calls. For example, if
an exception is thrown before the file is closed. File now takes
ownership of the descriptor. These changes now honor RAII idiom, which
should handle most of the peculiarities of C++.

Use optional value for TryOpenFile function, instead of returning a File
without a descriptor. It makes the failure state more semantically clear
to the API user.

Merge utils/filesystem with utils/file

The files aren't that big, and the naming is a bit confusing because
functions aren't really grouped for file and filesystem distinction.

Reviewers: mferencevic, mtomic

Reviewed By: mferencevic

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1111
This commit is contained in:
Teon Banek 2018-01-16 13:34:19 +01:00
parent 9e42ebbb67
commit 98cbd2b500
6 changed files with 251 additions and 269 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/CMakeLists.txt
View File

@ -47,7 +47,7 @@ set(memgraph_src_files
transactions/engine_master.cpp
transactions/engine_single_node.cpp
transactions/engine_worker.cpp
utils/filesystem.cpp
utils/file.cpp
utils/network.cpp
utils/watchdog.cpp
)

21
src/communication/raft/storage/file.hpp
View File

@ -4,7 +4,7 @@
* Raft log is stored inside a folder. Each log entry is stored in a file named
* by its index. There is a special file named "metadata" which stores Raft
* metadata and also the last log index, which is used on startup to identify
* which log entry files are valid.
* which log entry files are valid.
*/
#pragma once
@ -17,7 +17,7 @@
#include "communication/raft/raft.hpp"
#include "communication/raft/storage/memory.hpp"
#include "utils/filesystem.hpp"
#include "utils/file.hpp"
namespace communication::raft {
@ -43,14 +43,14 @@ class SimpleFileStorage : public RaftStorageInterface<State> {
}
auto md = utils::TryOpenFile(dir_, "metadata", O_RDONLY);
if (md.Empty()) {
if (!md) {
LOG(WARNING) << fmt::format("No metadata file found in directory '{}'",
parent_dir);
return;
}
boost::iostreams::file_descriptor_source src(
md.Handle(),
md->Handle(),
boost::iostreams::file_descriptor_flags::never_close_handle);
boost::iostreams::stream<boost::iostreams::file_descriptor_source> is(src);
boost::archive::binary_iarchive iar(is);
@ -96,22 +96,11 @@ class SimpleFileStorage : public RaftStorageInterface<State> {
LOG(FATAL) << fmt::format("Failed to deserialize log entry {}: {}", idx,
e.what());
}
try {
utils::Close(entry_file);
} catch (std::system_error &e) {
LOG(FATAL) << fmt::format("Failed to close entry file {}: {}", idx,
e.what());
}
}
LOG(INFO) << fmt::format("Read {} log entries", metadata.last_log_index);
utils::Close(md);
}
~SimpleFileStorage() { utils::Close(dir_); }
void WriteTermAndVotedFor(
TermId term,
const std::experimental::optional<MemberId> &voted_for) override {
@ -160,7 +149,6 @@ class SimpleFileStorage : public RaftStorageInterface<State> {
try {
utils::Fsync(entry_file);
utils::Close(entry_file);
} catch (std::system_error &e) {
LOG(FATAL) << fmt::format("Failed to write log entry file to disk: {}",
e.what());
@ -234,7 +222,6 @@ class SimpleFileStorage : public RaftStorageInterface<State> {
try {
utils::Fsync(md_tmp);
utils::Close(md_tmp);
} catch (std::system_error &e) {
LOG(FATAL) << fmt::format(
"Failed to write temporary metadata file to disk: {}", e.what());

109
src/utils/filesystem.cpp → src/utils/file.cpp
View File

@ -1,4 +1,4 @@
#include "filesystem.hpp"
#include "utils/file.hpp"
#include <fcntl.h>
#include <sys/stat.h>
@ -11,21 +11,65 @@
namespace utils {
std::vector<fs::path> LoadFilePaths(const fs::path &directory,
const std::string &extension) {
// result container
std::vector<fs::path> file_paths;
for (auto &directory_entry : fs::recursive_directory_iterator(directory)) {
auto path = directory_entry.path().string();
// skip directories
if (!fs::is_regular_file(directory_entry)) continue;
// if extension isn't defined then put all file paths from the directory
// to the result set
if (!extension.empty()) {
// skip paths that don't have appropriate extension
auto file_extension = path.substr(path.find_last_of(".") + 1);
if (file_extension != extension) continue;
}
file_paths.emplace_back(path);
// skip paths that don't have appropriate extension
auto file_extension = path.substr(path.find_last_of(".") + 1);
if (file_extension != extension) continue;
// path has the right extension and can be placed in the result
// container
file_paths.emplace_back(path);
}
return file_paths;
}
std::vector<std::string> ReadLines(const fs::path &path) {
std::vector<std::string> lines;
std::ifstream stream(path.c_str());
std::string line;
while (std::getline(stream, line)) {
lines.emplace_back(line);
}
return lines;
}
void Write(const std::string &text, const fs::path &path) {
std::ofstream stream;
stream.open(path.c_str());
stream << text;
stream.close();
}
File::File() : fd_(-1), path_() {}
File::File(int fd, fs::path path) : fd_(fd), path_(std::move(path)) {}
File::~File() {
CHECK(fd_ == -1) << fmt::format(
"Underlying file descriptor should be released or closed "
"before destructing (fd = {}, path = {})",
fd_, path_);
}
File::~File() { Close(); }
File::File(File &&rhs) : fd_(rhs.fd_), path_(rhs.path_) {
LOG(INFO) << "Move constructor";
rhs.Release();
}
File::File(File &&rhs) : fd_(rhs.fd_), path_(rhs.path_) { rhs.Release(); }
File &File::operator=(File &&rhs) {
if (this != &rhs) {
@ -45,6 +89,17 @@ void File::Release() {
path_ = fs::path();
}
void File::Close() {
if (fd_ == -1) {
return;
}
if (::close(fd_) == -1) {
throw std::system_error(errno, std::generic_category(),
fmt::format("cannot close {}", path_));
}
Release();
}
File OpenFile(const fs::path &path, int flags, ::mode_t mode) {
int fd = ::open(path.c_str(), flags, mode);
if (fd == -1) {
@ -55,9 +110,11 @@ File OpenFile(const fs::path &path, int flags, ::mode_t mode) {
return File(fd, path);
}
File TryOpenFile(const fs::path &path, int flags, ::mode_t mode) {
std::experimental::optional<File> TryOpenFile(const fs::path &path, int flags,
::mode_t mode) {
int fd = ::open(path.c_str(), flags, mode);
return fd == -1 ? File() : File(fd, path);
return fd == -1 ? std::experimental::nullopt
: std::experimental::make_optional(File(fd, path));
}
File OpenFile(const File &dir, const fs::path &path, int flags, ::mode_t mode) {
@ -69,18 +126,13 @@ File OpenFile(const File &dir, const fs::path &path, int flags, ::mode_t mode) {
return File(fd, dir.Path() / path);
}
File TryOpenFile(const File &dir, const fs::path &path, int flags,
::mode_t mode) {
std::experimental::optional<File> TryOpenFile(const File &dir,
const fs::path &path, int flags,
::mode_t mode) {
int fd = ::openat(dir.Handle(), path.c_str(), flags, mode);
return fd == -1 ? File() : File(fd, dir.Path() / path);
}
void Close(File &file) {
if (::close(file.Handle()) == -1) {
throw std::system_error(errno, std::generic_category(),
fmt::format("cannot close {}", file.Path()));
}
file.Release();
return fd == -1
? std::experimental::nullopt
: std::experimental::make_optional(File(fd, dir.Path() / path));
}
void Fsync(const File &file) {
@ -104,7 +156,6 @@ void Rename(const File &dir1, const fs::path &path1, const File &dir2,
void SyncDir(const fs::path &path) {
File dir = OpenFile(path, O_DIRECTORY | O_RDONLY);
Fsync(dir);
Close(dir);
}
File OpenDir(const fs::path &path) {
@ -113,11 +164,9 @@ File OpenDir(const fs::path &path) {
if (path.has_parent_path()) {
SyncDir(path.parent_path());
}
} else {
if (errno != EEXIST) {
throw std::system_error(errno, std::generic_category(),
fmt::format("cannot create directory {}", path));
}
} else if (errno != EEXIST) {
throw std::system_error(errno, std::generic_category(),
fmt::format("cannot create directory {}", path));
}
int fd = ::open(path.c_str(), O_RDONLY | O_DIRECTORY);

218
src/utils/file.hpp
View File

@ -1,11 +1,21 @@
/**
* @file
*
* This file contains utilities for operations with files. Other than utility
* funtions, a `File` class is provided which wraps a system file handle.
*/
#pragma once
#include <experimental/filesystem>
#include <experimental/optional>
#include <fstream>
namespace fs = std::experimental::filesystem;
namespace utils {
// Higher level utility operations follow.
/**
* Loads all file paths in the specified directory. Optionally
* the paths are filtered by extension.
@ -18,38 +28,8 @@ namespace utils {
*
* @return std::vector of paths founded in the directory
*/
inline auto LoadFilePaths(const fs::path &directory,
const std::string &extension = "") {
// result container
std::vector<fs::path> file_paths;
for (auto &directory_entry : fs::recursive_directory_iterator(directory)) {
auto path = directory_entry.path().string();
// skip directories
if (!fs::is_regular_file(directory_entry)) continue;
// if extension isn't defined then put all file paths from the directory
// to the result set
if (!extension.empty()) {
// skip paths that don't have appropriate extension
auto file_extension = path.substr(path.find_last_of(".") + 1);
if (file_extension != extension) continue;
}
file_paths.emplace_back(path);
// skip paths that don't have appropriate extension
auto file_extension = path.substr(path.find_last_of(".") + 1);
if (file_extension != extension) continue;
// path has the right extension and can be placed in the result
// container
file_paths.emplace_back(path);
}
return file_paths;
}
std::vector<fs::path> LoadFilePaths(const fs::path &directory,
const std::string &extension = "");
// TODO: add error checking
/**
@ -58,28 +38,164 @@ inline auto LoadFilePaths(const fs::path &directory,
* @param path file path.
* @return vector of all lines from the file.
*/
std::vector<std::string> ReadLines(const fs::path &path) {
std::vector<std::string> lines;
std::ifstream stream(path.c_str());
std::string line;
while (std::getline(stream, line)) {
lines.emplace_back(line);
}
return lines;
}
std::vector<std::string> ReadLines(const fs::path &path);
/**
* Writes test into the file specified by path.
* Writes text into the file specified by path.
*
* @param text content which will be written in the file.
* @param path a path to the file.
*/
void Write(const std::string &text, const fs::path &path) {
std::ofstream stream;
stream.open(path.c_str());
stream << text;
stream.close();
}
}
void Write(const std::string &text, const fs::path &path);
// End higher level operations.
// Lower level wrappers around C system calls follow.
/**
* Thin wrapper around system file handle.
*/
class File {
public:
/** Constructs an empty file handle. */
File();
/**
* Take ownership of the given system file handle.
*
* @param fd System file handle.
* @param path Pathname naming the file, used only for error handling.
*/
File(int fd, fs::path path);
File(File &&);
File &operator=(File &&);
File(const File &) = delete;
File &operator=(const File &) = delete;
/**
* Closes the underlying file handle.
*
* @throws std::system_error
*/
~File();
/** Gets the path to the underlying file. */
fs::path Path() const;
/** Gets the underlying file handle. */
int Handle() const;
/** Checks if there's an underlying file handle. */
bool Empty() const;
/**
* Closes the underlying file handle.
*
* Wrapper around `close` system call (see `man 2 close`). File will
* be empty after the call returns. You may call Close multiple times, all
* calls after the first one are no-op.
*
* @throws std::system_error
*/
void Close();
private:
int fd_;
fs::path path_;
void Release();
};
/**
* Opens a file descriptor.
* Wrapper around `open` system call (see `man 2 open`).
*
* @param path Pathname naming the file.
* @param flags Opening flags.
* @param mode File mode bits to apply for creation of new file.
* @return File descriptor referring to the opened file.
*
* @throws std::system_error
*/
File OpenFile(const fs::path &path, int flags, ::mode_t mode = 0666);
/**
* Same as OpenFile, but returns `nullopt` instead of throwing an exception.
*/
std::experimental::optional<File> TryOpenFile(const fs::path &path, int flags,
::mode_t mode = 0666);
/** Calls File::Close */
inline void Close(File &file) { file.Close(); }
/**
* Opens a file descriptor for a file inside a directory.
* Wrapper around `openat` system call (see `man 2 openat`).
*
* @param dir Directory file descriptor.
* @param path Pathname naming the file.
* @param flags Opening flags.
* @param mode File mode bits to apply for creation of new file.
* @return File descriptor referring to the opened file.
*
* @throws std::system_error
*/
File OpenFile(const File &dir, const fs::path &path, int flags,
::mode_t mode = 0666);
/**
* Same as OpenFile, but returns `nullopt` instead of throwing an exception.
*/
std::experimental::optional<File> TryOpenFile(const File &dir,
const fs::path &path, int flags,
::mode_t mode = 0666);
/**
* Synchronizes file with the underlying storage device.
* Wrapper around `fsync` system call (see `man 2 fsync`).
*
* @param file File descriptor referring to the file to be synchronized.
*
* @throws std::system_error
*/
void Fsync(const File &file);
/**
* Moves a file from one directory to another.
*
* Wrapper around `renameat` system call (see `man 2 renameat`).
*
* @param dir1 Source directory.
* @param path1 Pathname naming the source file.
* @param dir2 Destination directory.
* @param path2 Pathname naming the destination file.
*
* @throws std::system_error
*/
void Rename(const File &dir1, const fs::path &path1, const File &dir2,
const fs::path &path2);
/**
* Synchronizes directory with the underlying storage device.
*
* @param path Pathname naming the directory.
*
* @throws std::system_error
*/
void SyncDir(const fs::path &path);
/**
* Opens a directory, creating it if it doesn't exist.
*
* @param path Pathname naming the directory.
* @return File descriptor referring to the opened directory.
*
* @throws std::system_error
*/
File OpenDir(const fs::path &path);
// End lower level wrappers.
} // namespace utils

169
src/utils/filesystem.hpp
View File

@ -1,169 +0,0 @@
/**
* @file
* @brief This file contains C++ wrappers around some C system calls.
*/
#pragma once
#include <experimental/filesystem>
namespace fs = std::experimental::filesystem;
namespace utils {
/**
* @brief Thin wrapper around system file handle.
*/
class File {
public:
/**
* Constructs an empty file handle.
*/
File();
/**
* Wraps the given system file handle.
*
* This class doesn't take ownership of the file handle, it won't close it
* on destruction.
*
* @param fd System file handle.
* @param path Pathname naming the file, used only for error handling.
*/
File(int fd, fs::path path);
File(File &&);
File &operator=(File &&);
File(const File &) = delete;
File &operator=(const File &) = delete;
/***
* Destructor -- crashes the program if the underlying file descriptor is not
* released or closed.
*/
~File();
/**
* Gets the path to the underlying file.
*/
fs::path Path() const;
/**
* Gets the underlying file handle.
*/
int Handle() const;
/**
* Checks if there's an underlying file handle.
*/
bool Empty() const;
/**
* Releases the underlying file handle.
*
* File descriptor will be empty after the call returns.
*/
void Release();
private:
int fd_;
fs::path path_;
};
/**
* Opens a file descriptor.
* Wrapper around `open` system call (see `man 2 open`).
*
* @param path Pathname naming the file.
* @param flags Opening flags.
* @param mode File mode bits to apply for creation of new file.
* @return File descriptor referring to the opened file.
*
* @throws std::system_error
*/
File OpenFile(const fs::path &path, int flags, ::mode_t mode = 0666);
/**
* Same as OpenFile, but returns an empty File object instead of
* throwing an exception.
*/
File TryOpenFile(const fs::path &path, int flags, ::mode_t mode = 0666);
/**
* Opens a file descriptor for a file inside a directory.
* Wrapper around `openat` system call (see `man 2 openat`).
*
* @param dir Directory file descriptor.
* @param path Pathname naming the file.
* @param flags Opening flags.
* @param mode File mode bits to apply for creation of new file.
* @return File descriptor referring to the opened file.
*
* @throws std::system_error
*/
File OpenFile(const File &dir, const fs::path &path, int flags,
::mode_t mode = 0666);
/**
* Same as OpenFile, but returns an empty File object instead of
* throwing an exception.
*/
File TryOpenFile(const File &dir, const fs::path &path, int flags,
::mode_t mode = 0666);
/**
* Closes a file descriptor.
* Wrapper around `close` system call (see `man 2 close`). File descriptor will
* be empty after the call returns.
*
* @param file File descriptor to be closed.
*
* @throws std::system_error
*/
void Close(File &file);
/**
* Synchronizes file with the underlying storage device.
* Wrapper around `fsync` system call (see `man 2 fsync`).
*
* @param file File descriptor referring to the file to be synchronized.
*
* @throws std::system_error
*/
void Fsync(const File &file);
/**
* Moves a file from one directory to another.
*
* Wrapper around `renameat` system call (see `man 2 renameat`).
*
* @param dir1 Source directory.
* @param path1 Pathname naming the source file.
* @param dir2 Destination directory.
* @param path2 Pathname naming the destination file.
*
* @throws std::system_error
*/
void Rename(const File &dir1, const fs::path &path1, const File &dir2,
const fs::path &path2);
/**
* Synchronizes directory with the underlying storage device.
*
* @param path Pathname naming the directory.
*
* @throws std::system_error
*/
void SyncDir(const fs::path &path);
/**
* Opens a directory, creating it if it doesn't exist.
*
* @param path Pathname naming the directory.
* @return File descriptor referring to the opened directory.
*
* @throws std::system_error
*/
File OpenDir(const fs::path &path);
} // namespace utils

1
tests/unit/raft_storage.cpp
View File

@ -4,7 +4,6 @@
#include "communication/raft/storage/file.hpp"
#include "communication/raft/test_utils.hpp"
#include "utils/filesystem.hpp"
using communication::raft::LogEntry;
using communication::raft::SimpleFileStorage;