From d8939fa24cea9f5edfa782a13345d502e82cee3e Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 7 May 2016 11:04:49 +0900 Subject: [PATCH 1/6] Allow read-only memory mapped source. --- cpp/src/arrow/ipc/ipc-memory-test.cc | 36 ++++++++++++++++++++++++++++ cpp/src/arrow/ipc/memory.cc | 5 ++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/ipc/ipc-memory-test.cc b/cpp/src/arrow/ipc/ipc-memory-test.cc index 19339212225..b248b7b43cd 100644 --- a/cpp/src/arrow/ipc/ipc-memory-test.cc +++ b/cpp/src/arrow/ipc/ipc-memory-test.cc @@ -67,6 +67,42 @@ TEST_F(TestMemoryMappedSource, WriteRead) { } } +TEST_F(TestMemoryMappedSource, ReadOnly) { + const int64_t buffer_size = 1024; + std::vector buffer(buffer_size); + + test::random_bytes(1024, 0, buffer.data()); + + const int reps = 5; + + std::string path = "ipc-read-only-test"; + CreateFile(path, reps * buffer_size); + + std::shared_ptr rwmmap; + ASSERT_OK(MemoryMappedSource::Open(path, MemorySource::READ_WRITE, &rwmmap)); + + int64_t position = 0; + for (int i = 0; i < reps; ++i) { + ASSERT_OK(rwmmap->Write(position, buffer.data(), buffer_size)); + + position += buffer_size; + } + rwmmap->Close(); + + std::shared_ptr rommap; + ASSERT_OK(MemoryMappedSource::Open(path, MemorySource::READ_WRITE, &rommap)); + + position = 0; + std::shared_ptr out_buffer; + for (int i = 0; i < reps; ++i) { + ASSERT_OK(rommap->ReadAt(position, buffer_size, &out_buffer)); + + ASSERT_EQ(0, memcmp(out_buffer->data(), buffer.data(), buffer_size)); + position += buffer_size; + } + rommap->Close(); +} + TEST_F(TestMemoryMappedSource, InvalidFile) { std::string non_existent_path = "invalid-file-name-asfd"; diff --git a/cpp/src/arrow/ipc/memory.cc b/cpp/src/arrow/ipc/memory.cc index caff2c610b9..7ce818e0d13 100644 --- a/cpp/src/arrow/ipc/memory.cc +++ b/cpp/src/arrow/ipc/memory.cc @@ -54,9 +54,11 @@ class MemoryMappedSource::Impl { if (is_open_) { return Status::IOError("A file is already open"); } path_ = path; + int prot_flag = PROT_READ; if (mode == MemorySource::READ_WRITE) { file_ = fopen(path.c_str(), "r+b"); + prot_flag |= PROT_WRITE; } else { file_ = fopen(path.c_str(), "rb"); } @@ -73,9 +75,8 @@ class MemoryMappedSource::Impl { fseek(file_, 0L, SEEK_SET); is_open_ = true; - // TODO(wesm): Add read-only version of this data_ = reinterpret_cast( - mmap(nullptr, size_, PROT_READ | PROT_WRITE, MAP_SHARED, fileno(file_), 0)); + mmap(nullptr, size_, prot_flag, MAP_SHARED, fileno(file_), 0)); if (data_ == nullptr) { std::stringstream ss; ss << "Memory mapping file failed, errno: " << errno; From 5559b8d67e8472808da3bdce046bf99bf76356e4 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 8 May 2016 11:14:46 +0900 Subject: [PATCH 2/6] - Fixed a wrong protection flag in a test - Added a routine to check the protection flag before writing - Added a unit test to check the error status for protection mode - Improved failure check for mmap() --- cpp/src/arrow/ipc/ipc-memory-test.cc | 20 ++++++++++++++++---- cpp/src/arrow/ipc/memory.cc | 20 ++++++++++++-------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/cpp/src/arrow/ipc/ipc-memory-test.cc b/cpp/src/arrow/ipc/ipc-memory-test.cc index b248b7b43cd..a2dbd35728c 100644 --- a/cpp/src/arrow/ipc/ipc-memory-test.cc +++ b/cpp/src/arrow/ipc/ipc-memory-test.cc @@ -26,9 +26,6 @@ #include "arrow/ipc/memory.h" #include "arrow/ipc/test-common.h" -#include "arrow/test-util.h" -#include "arrow/util/buffer.h" -#include "arrow/util/status.h" namespace arrow { namespace ipc { @@ -90,7 +87,7 @@ TEST_F(TestMemoryMappedSource, ReadOnly) { rwmmap->Close(); std::shared_ptr rommap; - ASSERT_OK(MemoryMappedSource::Open(path, MemorySource::READ_WRITE, &rommap)); + ASSERT_OK(MemoryMappedSource::Open(path, MemorySource::READ_ONLY, &rommap)); position = 0; std::shared_ptr out_buffer; @@ -103,6 +100,21 @@ TEST_F(TestMemoryMappedSource, ReadOnly) { rommap->Close(); } +TEST_F(TestMemoryMappedSource, InvalidMode) { + const int64_t buffer_size = 1024; + std::vector buffer(buffer_size); + + test::random_bytes(1024, 0, buffer.data()); + + std::string path = "ipc-invalid-mode-test"; + CreateFile(path, buffer_size); + + std::shared_ptr rommap; + ASSERT_OK(MemoryMappedSource::Open(path, MemorySource::READ_ONLY, &rommap)); + + ASSERT_RAISES(IOError, rommap->Write(0, buffer.data(), buffer_size)); +} + TEST_F(TestMemoryMappedSource, InvalidFile) { std::string non_existent_path = "invalid-file-name-asfd"; diff --git a/cpp/src/arrow/ipc/memory.cc b/cpp/src/arrow/ipc/memory.cc index 7ce818e0d13..2fa670f01f4 100644 --- a/cpp/src/arrow/ipc/memory.cc +++ b/cpp/src/arrow/ipc/memory.cc @@ -53,12 +53,11 @@ class MemoryMappedSource::Impl { Status Open(const std::string& path, MemorySource::AccessMode mode) { if (is_open_) { return Status::IOError("A file is already open"); } - path_ = path; - int prot_flag = PROT_READ; + prot_flags = PROT_READ; if (mode == MemorySource::READ_WRITE) { file_ = fopen(path.c_str(), "r+b"); - prot_flag |= PROT_WRITE; + prot_flags |= PROT_WRITE; } else { file_ = fopen(path.c_str(), "rb"); } @@ -75,13 +74,13 @@ class MemoryMappedSource::Impl { fseek(file_, 0L, SEEK_SET); is_open_ = true; - data_ = reinterpret_cast( - mmap(nullptr, size_, prot_flag, MAP_SHARED, fileno(file_), 0)); - if (data_ == nullptr) { - std::stringstream ss; + void* result = mmap(nullptr, size_, prot_flags, MAP_SHARED, fileno(file_), 0); + if (result == MAP_FAILED) { + std::stringstream ss ; ss << "Memory mapping file failed, errno: " << errno; return Status::IOError(ss.str()); } + data_ = reinterpret_cast(result); return Status::OK(); } @@ -90,11 +89,13 @@ class MemoryMappedSource::Impl { uint8_t* data() { return data_; } + bool writable() { return (prot_flags & PROT_WRITE) == PROT_WRITE; } + private: - std::string path_; FILE* file_; int64_t size_; bool is_open_; + uint8_t prot_flags; // The memory map uint8_t* data_; @@ -135,6 +136,9 @@ Status MemoryMappedSource::ReadAt( } Status MemoryMappedSource::Write(int64_t position, const uint8_t* data, int64_t nbytes) { + if (!impl_->writable()) { + return Status::IOError("Unable to write"); + } if (position < 0 || position >= impl_->size()) { return Status::Invalid("position is out of bounds"); } From 22e61289a5307d83e421fabcc9bb31d13b378dfe Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 8 May 2016 11:24:50 +0900 Subject: [PATCH 3/6] Simplify error check --- cpp/src/arrow/ipc/memory.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/ipc/memory.cc b/cpp/src/arrow/ipc/memory.cc index 2fa670f01f4..60ffb0d0239 100644 --- a/cpp/src/arrow/ipc/memory.cc +++ b/cpp/src/arrow/ipc/memory.cc @@ -53,11 +53,12 @@ class MemoryMappedSource::Impl { Status Open(const std::string& path, MemorySource::AccessMode mode) { if (is_open_) { return Status::IOError("A file is already open"); } - prot_flags = PROT_READ; + uint8_t prot_flags = PROT_READ; if (mode == MemorySource::READ_WRITE) { file_ = fopen(path.c_str(), "r+b"); prot_flags |= PROT_WRITE; + is_writable_ = true; } else { file_ = fopen(path.c_str(), "rb"); } @@ -89,13 +90,15 @@ class MemoryMappedSource::Impl { uint8_t* data() { return data_; } - bool writable() { return (prot_flags & PROT_WRITE) == PROT_WRITE; } + bool writable() { return is_writable_; } + + bool opened() { return is_open_; } private: FILE* file_; int64_t size_; bool is_open_; - uint8_t prot_flags; + bool is_writable_; // The memory map uint8_t* data_; @@ -136,7 +139,7 @@ Status MemoryMappedSource::ReadAt( } Status MemoryMappedSource::Write(int64_t position, const uint8_t* data, int64_t nbytes) { - if (!impl_->writable()) { + if (!impl_->opened() || !impl_->writable()) { return Status::IOError("Unable to write"); } if (position < 0 || position >= impl_->size()) { From 63b99c55fdff4b4ca36a856042dc7002e8aee7ae Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 8 May 2016 14:32:43 +0900 Subject: [PATCH 4/6] Remove unintended whitespace --- cpp/src/arrow/ipc/memory.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/ipc/memory.cc b/cpp/src/arrow/ipc/memory.cc index 60ffb0d0239..70064b2eb36 100644 --- a/cpp/src/arrow/ipc/memory.cc +++ b/cpp/src/arrow/ipc/memory.cc @@ -53,7 +53,7 @@ class MemoryMappedSource::Impl { Status Open(const std::string& path, MemorySource::AccessMode mode) { if (is_open_) { return Status::IOError("A file is already open"); } - uint8_t prot_flags = PROT_READ; + int8_t prot_flags = PROT_READ; if (mode == MemorySource::READ_WRITE) { file_ = fopen(path.c_str(), "r+b"); @@ -77,7 +77,7 @@ class MemoryMappedSource::Impl { void* result = mmap(nullptr, size_, prot_flags, MAP_SHARED, fileno(file_), 0); if (result == MAP_FAILED) { - std::stringstream ss ; + std::stringstream ss; ss << "Memory mapping file failed, errno: " << errno; return Status::IOError(ss.str()); } From b928031f753bfc9a5f84ff200cc9ed61f28552c5 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 8 May 2016 17:53:28 +0900 Subject: [PATCH 5/6] Add missing initialization --- cpp/src/arrow/ipc/memory.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/ipc/memory.cc b/cpp/src/arrow/ipc/memory.cc index 70064b2eb36..12915ed42e9 100644 --- a/cpp/src/arrow/ipc/memory.cc +++ b/cpp/src/arrow/ipc/memory.cc @@ -41,7 +41,7 @@ MemorySource::~MemorySource() {} class MemoryMappedSource::Impl { public: - Impl() : file_(nullptr), is_open_(false), data_(nullptr) {} + Impl() : file_(nullptr), is_open_(false), is_writable_(false), data_(nullptr) {} ~Impl() { if (is_open_) { From f55dd22ed702ef72b27b9472127a5298fb39a1ff Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Mon, 9 May 2016 10:27:12 +0900 Subject: [PATCH 6/6] Change the type of protection flag from int8_t to int --- cpp/src/arrow/ipc/memory.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/ipc/memory.cc b/cpp/src/arrow/ipc/memory.cc index 12915ed42e9..a6c56d64f4a 100644 --- a/cpp/src/arrow/ipc/memory.cc +++ b/cpp/src/arrow/ipc/memory.cc @@ -53,7 +53,7 @@ class MemoryMappedSource::Impl { Status Open(const std::string& path, MemorySource::AccessMode mode) { if (is_open_) { return Status::IOError("A file is already open"); } - int8_t prot_flags = PROT_READ; + int prot_flags = PROT_READ; if (mode == MemorySource::READ_WRITE) { file_ = fopen(path.c_str(), "r+b");