martinzink commented on a change in pull request #987:
URL: https://github.com/apache/nifi-minifi-cpp/pull/987#discussion_r570167902
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -31,21 +31,41 @@ namespace nifi {
namespace minifi {
namespace io {
+constexpr const char *file_opening_error_msg = "Error opening file: ";
+constexpr const char *read_error_msg = "Error reading from file: ";
+constexpr const char *write_error_msg = "Error writing to file: ";
+constexpr const char *seek_error = "Error seeking in file: ";
+constexpr const char *invalid_file_stream_error_msg = "invalid file stream";
+constexpr const char *tellg_call_error_msg = "tellg call on file stream
failed";
+constexpr const char *invalid_buffer_error_msg = "invalid buffer";
+constexpr const char *flush_call_error_msg = "flush call on file stream
failed";
+constexpr const char *write_call_error_msg = "write call on file stream
failed";
+constexpr const char *empty_message_error_msg = "empty message";
+constexpr const char *seekg_call_error_msg = "seekg call on file stream
failed";
+constexpr const char *seekp_call_error_msg = "seekp call on file stream
failed";
Review comment:
fixed
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -31,21 +31,41 @@ namespace nifi {
namespace minifi {
namespace io {
+constexpr const char *file_opening_error_msg = "Error opening file: ";
+constexpr const char *read_error_msg = "Error reading from file: ";
+constexpr const char *write_error_msg = "Error writing to file: ";
+constexpr const char *seek_error = "Error seeking in file: ";
+constexpr const char *invalid_file_stream_error_msg = "invalid file stream";
+constexpr const char *tellg_call_error_msg = "tellg call on file stream
failed";
+constexpr const char *invalid_buffer_error_msg = "invalid buffer";
+constexpr const char *flush_call_error_msg = "flush call on file stream
failed";
+constexpr const char *write_call_error_msg = "write call on file stream
failed";
+constexpr const char *empty_message_error_msg = "empty message";
+constexpr const char *seekg_call_error_msg = "seekg call on file stream
failed";
+constexpr const char *seekp_call_error_msg = "seekp call on file stream
failed";
+
FileStream::FileStream(const std::string &path, bool append)
: logger_(logging::LoggerFactory<FileStream>::getLogger()),
path_(path),
offset_(0) {
file_stream_ = std::unique_ptr<std::fstream>(new std::fstream());
if (append) {
file_stream_->open(path.c_str(), std::fstream::in | std::fstream::out |
std::fstream::app | std::fstream::binary);
- file_stream_->seekg(0, file_stream_->end);
- file_stream_->seekp(0, file_stream_->end);
- std::streamoff len = file_stream_->tellg();
- length_ = len > 0 ? gsl::narrow<size_t>(len) : 0;
- seek(offset_);
+ if (file_stream_->is_open()) {
+ file_stream_->seekg(0, file_stream_->end);
+ file_stream_->seekp(0, file_stream_->end);
+ std::streamoff len = file_stream_->tellg();
Review comment:
agreed, and since seeking to the end of the file is used multiple times,
I moved them to a private function (with error logging)
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -59,15 +79,19 @@ FileStream::FileStream(const std::string &path, uint32_t
offset, bool write_enab
} else {
file_stream_->open(path.c_str(), std::fstream::in | std::fstream::binary);
}
- file_stream_->seekg(0, file_stream_->end);
- file_stream_->seekp(0, file_stream_->end);
- std::streamoff len = file_stream_->tellg();
- if (len > 0) {
- length_ = gsl::narrow<size_t>(len);
+ if (file_stream_->is_open()) {
+ file_stream_->seekg(0, file_stream_->end);
+ file_stream_->seekp(0, file_stream_->end);
+ std::streamoff len = file_stream_->tellg();
+ if (len > 0) {
+ length_ = gsl::narrow<size_t>(len);
+ } else {
+ length_ = 0;
+ }
Review comment:
done
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -77,10 +101,16 @@ void FileStream::close() {
void FileStream::seek(uint64_t offset) {
std::lock_guard<std::mutex> lock(file_lock_);
+ if (file_stream_ == nullptr || !file_stream_->is_open()) {
+ logging::LOG_ERROR(logger_) << seek_error << invalid_file_stream_error_msg;
+ return;
+ }
offset_ = gsl::narrow<size_t>(offset);
file_stream_->clear();
- file_stream_->seekg(offset_);
- file_stream_->seekp(offset_);
+ if (!file_stream_->seekg(offset_))
+ logging::LOG_ERROR(logger_) << seek_error << seekg_call_error_msg;
Review comment:
That is a good question. I felt the same at first, but since the read
and write pointers should be pointing to the same position, I think it would be
better to call them both regardless of errors we encounter in the first call.
##########
File path: libminifi/src/io/FileStream.cpp
##########
@@ -114,17 +151,18 @@ int FileStream::read(uint8_t *buf, int buflen) {
}
if (!IsNullOrEmpty(buf)) {
std::lock_guard<std::mutex> lock(file_lock_);
- if (!file_stream_) {
+ if (file_stream_ == nullptr || !file_stream_->is_open()) {
+ logging::LOG_ERROR(logger_) << read_error_msg <<
invalid_file_stream_error_msg;
return -1;
}
file_stream_->read(reinterpret_cast<char*>(buf), buflen);
- if ((file_stream_->rdstate() & (file_stream_->eofbit |
file_stream_->failbit)) != 0) {
+ if (file_stream_->eof() || file_stream_->fail()) {
file_stream_->clear();
file_stream_->seekg(0, file_stream_->end);
file_stream_->seekp(0, file_stream_->end);
Review comment:
done
##########
File path: libminifi/test/unit/FileStreamTests.cpp
##########
@@ -263,3 +267,75 @@ TEST_CASE("Read zero bytes") {
minifi::io::FileStream stream(utils::file::concat_path(dir, "test.txt"), 0,
true);
REQUIRE(stream.read(nullptr, 0) == 0);
}
+
+TEST_CASE("Non-existing file read/write test") {
+ TestController test_controller;
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = test_controller.createTempDirectory(format);
+ minifi::io::FileStream stream(utils::file::concat_path(dir,
"non_existing_file.txt"), 0, true);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error opening
file", std::chrono::seconds(0)));
+ REQUIRE(test_controller.getLog().getInstance().contains("No such file or
directory", std::chrono::seconds(0)));
+ REQUIRE(stream.write("lorem ipsum", false) == -1);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error writing to
file: invalid file stream", std::chrono::seconds(0)));
+ std::vector<uint8_t> readBuffer;
+ stream.seek(0);
+ REQUIRE(stream.read(readBuffer, 1) == -1);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error reading from
file: invalid file stream", std::chrono::seconds(0)));
+}
+
+TEST_CASE("Existing file read/write test") {
+ TestController test_controller;
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = test_controller.createTempDirectory(format);
+ std::string path_to_existing_file(utils::file::concat_path(dir,
"existing_file.txt"));
+ {
+ std::ofstream outfile(path_to_existing_file);
+ outfile << "lorem ipsum" << std::endl;
+ outfile.close();
+ }
+ minifi::io::FileStream stream(path_to_existing_file, 0, true);
+ REQUIRE_FALSE(test_controller.getLog().getInstance().contains("Error opening
file", std::chrono::seconds(0)));
+ REQUIRE_FALSE(stream.write("dolor sit amet", false) == -1);
+ REQUIRE_FALSE(test_controller.getLog().getInstance().contains("Error writing
to file", std::chrono::seconds(0)));
+ std::vector<uint8_t> readBuffer;
+ stream.seek(0);
+ REQUIRE_FALSE(stream.read(readBuffer, 11) == -1);
+ REQUIRE_FALSE(test_controller.getLog().getInstance().contains("Error reading
from file", std::chrono::seconds(0)));
+ stream.seek(0);
+ REQUIRE(stream.read(nullptr, 11) == -1);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error reading from
file: invalid buffer", std::chrono::seconds(0)));
+}
+
+#ifdef USE_BOOST
+TEST_CASE("Opening file without permission creates error logs") {
+ TestController test_controller;
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = test_controller.createTempDirectory(format);
+ std::string path_to_permissionless_file(utils::file::concat_path(dir,
"permissionless_file.txt"));
+ {
+ std::ofstream outfile(path_to_permissionless_file);
+ outfile << "this file has been just created" << std::endl;
+ outfile.close();
+ // This could be done with C++17 std::filesystem
+ boost::filesystem::permissions(path_to_permissionless_file,
boost::filesystem::no_perms);
Review comment:
I feel like its useful to run this on windows as well (if there is boost
support), because it verifies that the error messages provided by strerror()
are the same on all platforms. (I wasn't sure about it previously given how
differently windows and unix handles file permissions).
Maybe we should do both, if there is boost use this if not and we are not on
windows use the FileUtils one (which would then use chmod)?
##########
File path: libminifi/test/unit/FileStreamTests.cpp
##########
@@ -263,3 +267,75 @@ TEST_CASE("Read zero bytes") {
minifi::io::FileStream stream(utils::file::concat_path(dir, "test.txt"), 0,
true);
REQUIRE(stream.read(nullptr, 0) == 0);
}
+
+TEST_CASE("Non-existing file read/write test") {
+ TestController test_controller;
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = test_controller.createTempDirectory(format);
+ minifi::io::FileStream stream(utils::file::concat_path(dir,
"non_existing_file.txt"), 0, true);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error opening
file", std::chrono::seconds(0)));
+ REQUIRE(test_controller.getLog().getInstance().contains("No such file or
directory", std::chrono::seconds(0)));
+ REQUIRE(stream.write("lorem ipsum", false) == -1);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error writing to
file: invalid file stream", std::chrono::seconds(0)));
+ std::vector<uint8_t> readBuffer;
+ stream.seek(0);
+ REQUIRE(stream.read(readBuffer, 1) == -1);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error reading from
file: invalid file stream", std::chrono::seconds(0)));
+}
+
+TEST_CASE("Existing file read/write test") {
+ TestController test_controller;
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = test_controller.createTempDirectory(format);
+ std::string path_to_existing_file(utils::file::concat_path(dir,
"existing_file.txt"));
+ {
+ std::ofstream outfile(path_to_existing_file);
+ outfile << "lorem ipsum" << std::endl;
+ outfile.close();
+ }
+ minifi::io::FileStream stream(path_to_existing_file, 0, true);
+ REQUIRE_FALSE(test_controller.getLog().getInstance().contains("Error opening
file", std::chrono::seconds(0)));
+ REQUIRE_FALSE(stream.write("dolor sit amet", false) == -1);
+ REQUIRE_FALSE(test_controller.getLog().getInstance().contains("Error writing
to file", std::chrono::seconds(0)));
+ std::vector<uint8_t> readBuffer;
+ stream.seek(0);
+ REQUIRE_FALSE(stream.read(readBuffer, 11) == -1);
+ REQUIRE_FALSE(test_controller.getLog().getInstance().contains("Error reading
from file", std::chrono::seconds(0)));
+ stream.seek(0);
+ REQUIRE(stream.read(nullptr, 11) == -1);
+ REQUIRE(test_controller.getLog().getInstance().contains("Error reading from
file: invalid buffer", std::chrono::seconds(0)));
+}
+
+#ifdef USE_BOOST
+TEST_CASE("Opening file without permission creates error logs") {
+ TestController test_controller;
+ char format[] = "/tmp/gt.XXXXXX";
+ auto dir = test_controller.createTempDirectory(format);
+ std::string path_to_permissionless_file(utils::file::concat_path(dir,
"permissionless_file.txt"));
+ {
+ std::ofstream outfile(path_to_permissionless_file);
+ outfile << "this file has been just created" << std::endl;
+ outfile.close();
+ // This could be done with C++17 std::filesystem
+ boost::filesystem::permissions(path_to_permissionless_file,
boost::filesystem::no_perms);
Review comment:
changed it in c4f9a4e
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
