Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/envoy/api/io_error.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ template <typename T> struct IoCallResult {
IoErrorPtr err_;
};

using IoCallBoolResult = IoCallResult<bool>;
using IoCallSizeResult = IoCallResult<ssize_t>;
using IoCallUintResult = IoCallResult<uint64_t>;

} // namespace Api
Expand Down
5 changes: 4 additions & 1 deletion include/envoy/api/os_sys_calls.h
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
#pragma once

#ifndef WIN32
#include <sys/ioctl.h>
#include <sys/mman.h> // for mode_t
#include <sys/socket.h> // for sockaddr
#include <sys/stat.h>
#include <sys/uio.h> // for iovec

#endif

#include <memory>
#include <string>

#include "envoy/common/pure.h"
#include "envoy/common/platform.h"

namespace Envoy {
namespace Api {
Expand All @@ -34,7 +38,6 @@ typedef SysCallResult<int> SysCallIntResult;
typedef SysCallResult<ssize_t> SysCallSizeResult;
typedef SysCallResult<void*> SysCallPtrResult;
typedef SysCallResult<std::string> SysCallStringResult;
typedef SysCallResult<bool> SysCallBoolResult;

class OsSysCalls {
public:
Expand Down
8 changes: 7 additions & 1 deletion include/envoy/common/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@

// NOLINT(namespace-envoy)
#ifdef _MSC_VER
#include <malloc.h>
#include <stdint.h>

#define PACKED_STRUCT(definition, ...) \
__pragma(pack(push, 1)) definition, ##__VA_ARGS__; \
__pragma(pack(pop))

#ifdef _M_X64
using ssize_t = int64_t;
#else
#error Envoy is not supported on 32-bit Windows
#endif

#else
#define PACKED_STRUCT(definition, ...) definition, ##__VA_ARGS__ __attribute__((packed))

Expand Down
1 change: 1 addition & 0 deletions include/envoy/filesystem/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ envoy_cc_library(
deps = [
"//include/envoy/api:os_sys_calls_interface",
"//include/envoy/event:dispatcher_interface",
"//source/common/filesystem:io_file_error_lib",
],
)

Expand Down
51 changes: 34 additions & 17 deletions include/envoy/filesystem/filesystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
#include <memory>
#include <string>

#include "envoy/api/os_sys_calls.h"
#include "envoy/api/io_error.h"
#include "envoy/common/platform.h"
#include "envoy/common/pure.h"

#include "common/filesystem/io_file_error.h"

#include "absl/strings/string_view.h"

namespace Envoy {
Expand All @@ -17,6 +20,8 @@ namespace Filesystem {
*/
class File {
public:
File(const std::string& path) : fd_(-1), path_(path) {}

virtual ~File() {}

/**
Expand All @@ -25,37 +30,55 @@ class File {
*
* @return bool whether the open succeeded
*/
virtual Api::SysCallBoolResult open() PURE;
Api::IoCallBoolResult open() {
if (isOpen()) {
return resultSuccess<bool>(true);
}

openFile();
return -1 != fd_ ? resultSuccess<bool>(true) : resultFailure<bool>(false, errno);
}
Comment thread
dnoe marked this conversation as resolved.
Outdated

/**
* Write the buffer to the file. The file must be explicitly opened before writing.
*
* @return ssize_t number of bytes written, or -1 for failure
*/
virtual Api::SysCallSizeResult write(absl::string_view buffer) PURE;
Api::IoCallSizeResult write(absl::string_view buffer) {
const ssize_t rc = writeFile(buffer);
return -1 != rc ? resultSuccess<ssize_t>(rc) : resultFailure<ssize_t>(rc, errno);
Comment thread
dnoe marked this conversation as resolved.
Outdated
};

/**
* Close the file.
*
* @return bool whether the close succeeded
*/
virtual Api::SysCallBoolResult close() PURE;
Api::IoCallBoolResult close() {
ASSERT(isOpen());

bool success = closeFile();
fd_ = -1;
return success ? resultSuccess<bool>(true) : resultFailure<bool>(false, errno);
}

/**
* @return bool is the file open
*/
virtual bool isOpen() PURE;
bool isOpen() const { return fd_ != -1; };

/**
* @return string the file path
*/
virtual std::string path() PURE;
std::string path() const { return path_; };

/**
* @return string a human-readable string describing the error code
* TODO(sesmith177) Use the IOError class after #5829 merges
*/
virtual std::string errorToString(int error) PURE;
protected:
virtual void openFile() PURE;
virtual ssize_t writeFile(absl::string_view buffer) PURE;
virtual bool closeFile() PURE;

int fd_;
const std::string path_;
};

using FilePtr = std::unique_ptr<File>;
Expand Down Expand Up @@ -97,12 +120,6 @@ class Instance {
*/
virtual std::string fileReadToEnd(const std::string& path) PURE;

/**
* @param path some filesystem path.
* @return SysCallStringResult containing the canonical path (see realpath(3)).
*/
virtual Api::SysCallStringResult canonicalPath(const std::string& path) PURE;

/**
* Determine if the path is on a list of paths Envoy will refuse to access. This
* is a basic sanity check for users, blacklisting some clearly bad paths. Paths
Expand Down
14 changes: 7 additions & 7 deletions source/common/access_log/access_log_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatch
}

void AccessLogFileImpl::open() {
const Api::SysCallBoolResult result = file_->open();
const Api::IoCallBoolResult result = file_->open();
if (!result.rc_) {
throw EnvoyException(fmt::format("unable to open file '{}': {}", file_->path(),
file_->errorToString(result.errno_)));
Api::IoError::getErrorDetails(*result.err_)));
}
}

Expand All @@ -68,9 +68,9 @@ AccessLogFileImpl::~AccessLogFileImpl() {
doWrite(flush_buffer_);
}

const Api::SysCallBoolResult result = file_->close();
const Api::IoCallBoolResult result = file_->close();
ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(),
file_->errorToString(result.errno_)));
Api::IoError::getErrorDetails(*result.err_)));
}
}

Expand All @@ -91,7 +91,7 @@ void AccessLogFileImpl::doWrite(Buffer::Instance& buffer) {
Thread::LockGuard lock(file_lock_);
for (const Buffer::RawSlice& slice : slices) {
absl::string_view data(static_cast<char*>(slice.mem_), slice.len_);
const Api::SysCallSizeResult result = file_->write(data);
const Api::IoCallSizeResult result = file_->write(data);
ASSERT(result.rc_ == static_cast<ssize_t>(slice.len_));
stats_.write_completed_.inc();
}
Expand Down Expand Up @@ -131,9 +131,9 @@ void AccessLogFileImpl::flushThreadFunc() {
try {
if (reopen_file_) {
reopen_file_ = false;
const Api::SysCallBoolResult result = file_->close();
const Api::IoCallBoolResult result = file_->close();
ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(),
file_->errorToString(result.errno_)));
Api::IoError::getErrorDetails(*result.err_)));
open();
}

Expand Down
1 change: 0 additions & 1 deletion source/common/api/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ envoy_cc_library(
"//include/envoy/api:api_interface",
"//source/common/common:thread_lib",
"//source/common/event:dispatcher_lib",
"//source/common/filesystem:filesystem_lib",
],
)

Expand Down
5 changes: 3 additions & 2 deletions source/common/api/api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
namespace Envoy {
namespace Api {

Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system)
: thread_factory_(thread_factory), time_system_(time_system) {}
Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system,
Filesystem::Instance& file_system_)
: thread_factory_(thread_factory), time_system_(time_system), file_system_(file_system_) {}
Comment thread
dnoe marked this conversation as resolved.
Outdated

Event::DispatcherPtr Impl::allocateDispatcher() {
return std::make_unique<Event::DispatcherImpl>(*this, time_system_);
Expand Down
7 changes: 3 additions & 4 deletions source/common/api/api_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
#include "envoy/filesystem/filesystem.h"
#include "envoy/thread/thread.h"

#include "common/filesystem/filesystem_impl.h"

namespace Envoy {
namespace Api {

Expand All @@ -18,7 +16,8 @@ namespace Api {
*/
class Impl : public Api {
public:
Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system);
Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system,
Filesystem::Instance& file_system_);
Comment thread
dnoe marked this conversation as resolved.
Outdated

// Api::Api
Event::DispatcherPtr allocateDispatcher() override;
Expand All @@ -29,8 +28,8 @@ class Impl : public Api {

private:
Thread::ThreadFactory& thread_factory_;
Filesystem::InstanceImpl file_system_;
Event::TimeSystem& time_system_;
Filesystem::Instance& file_system_;
};

} // namespace Api
Expand Down
30 changes: 28 additions & 2 deletions source/common/filesystem/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,39 @@ envoy_cc_posix_library(

envoy_cc_library(
name = "filesystem_lib",
srcs = ["filesystem_impl.cc"],
hdrs = ["filesystem_impl.h"],
deps = envoy_cc_platform_dep("filesystem_impl_lib"),
)

envoy_cc_posix_library(
name = "filesystem_impl_lib",
srcs = ["posix/filesystem_impl.cc"],
hdrs = ["posix/filesystem_impl.h"],
strip_include_prefix = "posix",
deps = [
"//include/envoy/filesystem:filesystem_interface",
],
)

envoy_cc_win32_library(
name = "filesystem_impl_lib",
srcs = ["win32/filesystem_impl.cc"],
hdrs = ["win32/filesystem_impl.h"],
strip_include_prefix = "win32",
deps = [
"//include/envoy/filesystem:filesystem_interface",
],
)

envoy_cc_library(
name = "io_file_error_lib",
srcs = ["io_file_error.cc"],
hdrs = ["io_file_error.h"],
deps = [
"//include/envoy/api:io_error_interface",
"//source/common/common:assert_lib",
],
)

envoy_cc_library(
name = "watcher_lib",
srcs = select({
Expand Down
20 changes: 20 additions & 0 deletions source/common/filesystem/io_file_error.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#include "common/filesystem/io_file_error.h"

#include <cstring>

namespace Envoy {
namespace Filesystem {

Api::IoError::IoErrorCode IoFileError::errorCode() const {
switch (errno_) {
case EBADF:
return IoErrorCode::BadHandle;
default:
return IoErrorCode::UnknownError;
}
}

std::string IoFileError::errorDetails() const { return ::strerror(errno_); }

} // namespace Filesystem
} // namespace Envoy
40 changes: 40 additions & 0 deletions source/common/filesystem/io_file_error.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#pragma once

#include <string>

#include "envoy/api/io_error.h"

#include "common/common/assert.h"

namespace Envoy {
namespace Filesystem {

class IoFileError : public Api::IoError {
public:
explicit IoFileError(int sys_errno) : errno_(sys_errno) {}

~IoFileError() override {}

private:
IoErrorCode errorCode() const override;
Comment thread
dnoe marked this conversation as resolved.
Outdated

std::string errorDetails() const override;

int errno_;
Comment thread
dnoe marked this conversation as resolved.
Outdated
};

using IoFileErrorPtr = std::unique_ptr<IoFileError, Api::IoErrorDeleterType>;

template <typename T> Api::IoCallResult<T> resultFailure(T result, int sys_errno) {
return {result, IoFileErrorPtr(new IoFileError(sys_errno), [](Api::IoError* err) {
ASSERT(err != nullptr);
delete err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare news and deletes are always a bit concerning. Can you elaborate on why they are needed here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementations of Api::IoError need to use a custom deleter, see

void deleteIoError(Api::IoError* err) {
for example

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see why this is necessary because of the trick that I think is being removed as part of #6037. I'm a little confused here because you're not special casing the deleter for ENVOY_ERROR_AGAIN, does it not need to be covered here? After #6037, maybe the custom deleter requirement can be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we don't need to cover the ENVOY_ERROR_AGAIN in this case.
Probably after the #6037 PR will be merged, some changes will need to be done in this code.

})};
}

template <typename T> Api::IoCallResult<T> resultSuccess(T result) {
return {result, IoFileErrorPtr(nullptr, [](Api::IoError*) { NOT_REACHED_GCOVR_EXCL_LINE; })};
}

} // namespace Filesystem
} // namespace Envoy
Loading