You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/03/12 13:13:40 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
szaszm opened a new pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028
Converting `write` will be done in a followup PR to keep the PR size managable. Errors are now reported as `static_cast<size_t>(-1)`.
_______
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595304012
##########
File path: libminifi/include/io/InputStream.h
##########
@@ -39,43 +39,43 @@ class InputStream : public virtual Stream {
* reads a byte array from the stream
* @param value reference in which will set the result
* @param len length to read
- * @return resulting read size
+ * @return resulting read size or static_cast<size_t>(-1) on error or static_cast<size_t>(-2) for ClientSocket EAGAIN
Review comment:
yup, sounds good
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595866093
##########
File path: libminifi/include/sitetosite/SiteToSiteClient.h
##########
@@ -65,19 +67,14 @@ class SiteToSiteClient : public core::Connectable {
_batchSendNanos(5000000000),
ssl_context_service_(nullptr),
logger_(logging::LoggerFactory<SiteToSiteClient>::getLogger()) {
- _supportedVersion[0] = 5;
- _supportedVersion[1] = 4;
- _supportedVersion[2] = 3;
- _supportedVersion[3] = 2;
- _supportedVersion[4] = 1;
_currentVersion = _supportedVersion[0];
_currentVersionIndex = 0;
_supportedCodecVersion[0] = 1;
Review comment:
done in 34b60f0
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595866436
##########
File path: libminifi/src/FlowFileRecord.cpp
##########
@@ -181,12 +179,12 @@ std::shared_ptr<FlowFileRecord> FlowFileRecord::DeSerialize(io::InputStream& inS
}
ret = inStream.read(file->uuid_);
- if (ret <= 0) {
+ if (ret == static_cast<size_t>(-1)) {
Review comment:
resolved in 34b60f0
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm removed a comment on pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm removed a comment on pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#issuecomment-849744380
Manually cancelled github actions CI runs triggered by the push event after every job was successful from the pull_request event.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595277807
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -126,22 +125,22 @@ int64_t ExtractText::ReadCallback::process(const std::shared_ptr<io::BaseStream>
if (sizeLimitStr.empty())
size_limit = DEFAULT_SIZE_LIMIT;
else if (sizeLimitStr != "0")
- size_limit = std::stoi(sizeLimitStr);
+ size_limit = gsl::narrow_cast<size_t>(std::stoi(sizeLimitStr));
Review comment:
It comes from the user and negative numbers shouldn't crash the program, but they are not "valid" either.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601538011
##########
File path: libminifi/test/archive-tests/CompressContentTests.cpp
##########
@@ -41,29 +41,23 @@
#include "processors/PutFile.h"
#include "utils/file/FileUtils.h"
#include "../Utils.h"
+#include "utils/gsl.h"
class ReadCallback: public minifi::InputStreamCallback {
public:
- explicit ReadCallback(size_t size) :
- read_size_(0) {
- buffer_size_ = size;
- buffer_ = new uint8_t[buffer_size_];
- archive_buffer_ = nullptr;
- archive_buffer_size_ = 0;
- }
- ~ReadCallback() {
- if (buffer_)
- delete[] buffer_;
- if (archive_buffer_)
- delete[] archive_buffer_;
- }
- int64_t process(const std::shared_ptr<minifi::io::BaseStream>& stream) {
+ explicit ReadCallback(size_t size)
+ :buffer_size_{size}
Review comment:
minor: a comment warning us that some default member initializers depend on `buffer_size_` might help future readers
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits closed pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596134706
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -30,42 +30,43 @@ namespace nifi {
namespace minifi {
namespace io {
-int InputStream::read(std::vector<uint8_t>& buffer, int len) {
- if (buffer.size() < gsl::narrow<size_t>(len)) {
+size_t InputStream::read(std::vector<uint8_t>& buffer, size_t len) {
+ if (buffer.size() < len) {
buffer.resize(len);
}
- int ret = read(buffer.data(), len);
- buffer.resize((std::max)(ret, 0));
+ const auto ret = read(buffer.data(), len);
+ if (ret == static_cast<size_t>(-1)) return ret;
+ buffer.resize((std::max)(ret, size_t{0}));
return ret;
}
-int InputStream::read(bool &value) {
+size_t InputStream::read(bool &value) {
uint8_t buf = 0;
if (read(&buf, 1) != 1) {
- return -1;
+ return static_cast<size_t>(-1);
}
value = buf;
return 1;
}
-int InputStream::read(utils::Identifier &value) {
+size_t InputStream::read(utils::Identifier &value) {
std::string uuidStr;
- int ret = read(uuidStr);
+ const auto ret = read(uuidStr);
if (ret < 0) {
return ret;
}
auto optional_uuid = utils::Identifier::parse(uuidStr);
if (!optional_uuid) {
- return -1;
+ return static_cast<size_t>(-1);
}
value = optional_uuid.value();
return ret;
}
-int InputStream::read(std::string &str, bool widen) {
+size_t InputStream::read(std::string &str, bool widen) {
uint32_t len = 0;
- int ret = 0;
+ size_t ret = 0;
if (!widen) {
uint16_t shortLength = 0;
ret = read(shortLength);
Review comment:
fixed in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601315940
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -190,16 +189,11 @@ class CompressContent: public core::Processor {
return ret;
}
- static la_ssize_t archive_read(struct archive *arch, void *context, const void **buff) {
- WriteCallback *callback = (WriteCallback *) context;
+ static la_ssize_t archive_read(struct archive*, void *context, const void **buff) {
+ auto *callback = (WriteCallback *) context;
callback->session_->read(callback->flow_, &callback->readDecompressCb_);
- if (callback->readDecompressCb_.read_size_ >= 0) {
- *buff = callback->readDecompressCb_.buffer_;
- return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
- } else {
- archive_set_error(arch, EIO, "Error reading flowfile");
- return -1;
- }
+ *buff = callback->readDecompressCb_.buffer_;
+ return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
Review comment:
Good catch. The site lists the official man page contents in a web format, so I would say the contents are valid. I've mistakenly removed the error branch because after converting `read_size_` to `size_t`, it was a dead branch and I didn't realize `read_size_` comes from a stream read return value. Will fix and rename `read_size_` to `stream_read_result`.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595288401
##########
File path: libminifi/src/io/tls/SecureDescriptorStream.cpp
##########
@@ -71,34 +71,29 @@ int SecureDescriptorStream::write(const uint8_t *value, int size) {
}
}
-int SecureDescriptorStream::read(uint8_t *buf, int buflen) {
- gsl_Expects(buflen >= 0);
+size_t SecureDescriptorStream::read(uint8_t * const buf, const size_t buflen) {
if (buflen == 0) {
return 0;
}
- if (!IsNullOrEmpty(buf)) {
- int total_read = 0;
- int status = 0;
- while (buflen) {
- int sslStatus;
- do {
- status = SSL_read(ssl_, buf, buflen);
- sslStatus = SSL_get_error(ssl_, status);
- } while (status < 0 && sslStatus == SSL_ERROR_WANT_READ);
+ if (IsNullOrEmpty(buf)) return static_cast<size_t>(-1);
+ size_t total_read = 0;
+ uint8_t* writepos = buf;
+ while (buflen > total_read) {
+ int status;
+ int sslStatus;
+ do {
+ status = SSL_read(ssl_, writepos, gsl::narrow<int>(std::min(buflen - total_read, gsl::narrow<size_t>(std::numeric_limits<int>::max()))));
Review comment:
The code is not prepared to handle exceptions and I don't want to rely on `gsl::narrow` throwing instead of calling `terminate`.
If there is any code that relies on `gsl::narrow` throwing vs terminating, that's an error.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596129303
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -126,22 +125,22 @@ int64_t ExtractText::ReadCallback::process(const std::shared_ptr<io::BaseStream>
if (sizeLimitStr.empty())
size_limit = DEFAULT_SIZE_LIMIT;
else if (sizeLimitStr != "0")
- size_limit = std::stoi(sizeLimitStr);
+ size_limit = gsl::narrow_cast<size_t>(std::stoi(sizeLimitStr));
Review comment:
changed to `static_cast` in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595819327
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -126,22 +125,22 @@ int64_t ExtractText::ReadCallback::process(const std::shared_ptr<io::BaseStream>
if (sizeLimitStr.empty())
size_limit = DEFAULT_SIZE_LIMIT;
else if (sizeLimitStr != "0")
- size_limit = std::stoi(sizeLimitStr);
+ size_limit = gsl::narrow_cast<size_t>(std::stoi(sizeLimitStr));
Review comment:
Actually it would make sense to cast them to `size_t` and treat `-1` as "essentially no limit" instead of throwing. If you think this is a good idea, I'll change it to `static_cast<size_t>(...)`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595820660
##########
File path: libminifi/src/FlowFileRecord.cpp
##########
@@ -181,12 +179,12 @@ std::shared_ptr<FlowFileRecord> FlowFileRecord::DeSerialize(io::InputStream& inS
}
ret = inStream.read(file->uuid_);
- if (ret <= 0) {
+ if (ret == static_cast<size_t>(-1)) {
Review comment:
Definitely an oversight, will fix. If you find other suspicious places, feel free to point out. I might have missed some in the 100+ "Find usages" results.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596199210
##########
File path: libminifi/test/rocksdb-tests/ContentSessionTests.cpp
##########
@@ -56,11 +56,10 @@ const std::shared_ptr<minifi::io::BaseStream>& operator>>(const std::shared_ptr<
str = "";
uint8_t buffer[4096]{};
while (true) {
- size_t ret = stream->read(buffer, sizeof(buffer));
+ const auto ret = stream->read(buffer, sizeof(buffer));
REQUIRE(ret >= 0);
Review comment:
We need both checks, and the new one is one line below. Maybe I misunderstood your point?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595929512
##########
File path: thirdparty/google-styleguide/run_linter.sh
##########
@@ -40,5 +40,7 @@ done
HEADERS=`find $INCLUDE_DIRS -name '*.h' | sort | uniq | tr '\n' ' '`
SOURCES=`find $SOURCE_DIRS -name '*.cpp' | sort | uniq | tr '\n' ' '`
-REPOSITORY="$(realpath --physical "$(dirname "$0")/../..")"
+# this realpath alternative should work on mac
+alias prealpath="python -c 'import os, sys; print(os.path.realpath(sys.argv[1]))'"
Review comment:
In the end I incorporated it to the below command, because the alias didn't work in subshells. I forgot to remove this one.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r639710821
##########
File path: libminifi/include/io/Stream.h
##########
@@ -24,14 +24,23 @@ namespace nifi {
namespace minifi {
namespace io {
+inline bool isError(const size_t read_return) noexcept {
+ return read_return == static_cast<size_t>(-1) // general error
+ || read_return == static_cast<size_t>(-2); // Socket EAGAIN, to be refactored to eliminate this error condition
Review comment:
`static_cast<size_t>(-2)` is a special case, I don't want to touch that. I'm open to extracting `static_cast<size_t>(-1)` to a symbolic constant, will probably do it later.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r640755520
##########
File path: libminifi/test/archive-tests/CompressContentTests.cpp
##########
@@ -41,29 +41,23 @@
#include "processors/PutFile.h"
#include "utils/file/FileUtils.h"
#include "../Utils.h"
+#include "utils/gsl.h"
class ReadCallback: public minifi::InputStreamCallback {
public:
- explicit ReadCallback(size_t size) :
- read_size_(0) {
- buffer_size_ = size;
- buffer_ = new uint8_t[buffer_size_];
- archive_buffer_ = nullptr;
- archive_buffer_size_ = 0;
- }
- ~ReadCallback() {
- if (buffer_)
- delete[] buffer_;
- if (archive_buffer_)
- delete[] archive_buffer_;
- }
- int64_t process(const std::shared_ptr<minifi::io::BaseStream>& stream) {
+ explicit ReadCallback(size_t size)
+ :buffer_size_{size}
Review comment:
added comment in 47358cb
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596134134
##########
File path: libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp
##########
@@ -140,7 +140,7 @@ TEST_CASE("Test Empty Claim", "[TestDBCR3]") {
std::string readstr;
// -1 tell us we have an invalid stream
Review comment:
changed in 46f3b0e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r594995982
##########
File path: extensions/script/python/PyBaseStream.cpp
##########
@@ -49,13 +49,11 @@ py::bytes PyBaseStream::read(size_t len) {
std::vector<uint8_t> buffer(len);
- auto read = stream_->read(buffer.data(), static_cast<int>(len));
- auto result = py::bytes(reinterpret_cast<char *>(buffer.data()), static_cast<size_t>(read));
-
- return result;
+ const auto read = stream_->read(buffer.data(), len);
+ return py::bytes(reinterpret_cast<char *>(buffer.data()), static_cast<size_t>(read));
Review comment:
this `static_cast` is no longer needed
##########
File path: extensions/sftp/client/SFTPClient.cpp
##########
@@ -577,20 +577,20 @@ bool SFTPClient::putFile(const std::string& path, io::BaseStream& input, bool ov
}
logger_->log_trace("Read %d bytes", read_ret);
total_read += read_ret;
- ssize_t remaining = read_ret;
+ auto remaining = gsl::narrow<size_t>(read_ret);
Review comment:
why do we need this `gsl::narrow`? `read_ret` is already of type `size_t`
##########
File path: libminifi/include/sitetosite/SiteToSiteClient.h
##########
@@ -65,19 +67,14 @@ class SiteToSiteClient : public core::Connectable {
_batchSendNanos(5000000000),
ssl_context_service_(nullptr),
logger_(logging::LoggerFactory<SiteToSiteClient>::getLogger()) {
- _supportedVersion[0] = 5;
- _supportedVersion[1] = 4;
- _supportedVersion[2] = 3;
- _supportedVersion[3] = 2;
- _supportedVersion[4] = 1;
_currentVersion = _supportedVersion[0];
_currentVersionIndex = 0;
_supportedCodecVersion[0] = 1;
Review comment:
this can be removed, too
##########
File path: libminifi/test/archive-tests/MergeFileTests.cpp
##########
@@ -111,8 +111,8 @@ std::vector<FixedBuffer> read_archives(const FixedBuffer& input) {
class ArchiveEntryReader {
public:
explicit ArchiveEntryReader(archive* arch) : arch(arch) {}
- int read(uint8_t* out, std::size_t len) {
- return gsl::narrow<int>(archive_read_data(arch, out, len));
+ size_t read(uint8_t* out, std::size_t len) {
+ return gsl::narrow_cast<size_t>(archive_read_data(arch, out, len));
Review comment:
I know this is just a test file helper, but it would be safer to explicitly test for -1 and then use `gsl::narrow`, instead of using `gsl::narrow_cast` and risk that the return value will be something like `static_cast<size_t>(-3)`, which is not handled by `FixedBuffer::write()`.
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -126,22 +125,22 @@ int64_t ExtractText::ReadCallback::process(const std::shared_ptr<io::BaseStream>
if (sizeLimitStr.empty())
size_limit = DEFAULT_SIZE_LIMIT;
else if (sizeLimitStr != "0")
- size_limit = std::stoi(sizeLimitStr);
+ size_limit = gsl::narrow_cast<size_t>(std::stoi(sizeLimitStr));
Review comment:
Why is this `narrow_cast` instead of `narrow`?
##########
File path: libminifi/src/FlowFileRecord.cpp
##########
@@ -181,12 +179,12 @@ std::shared_ptr<FlowFileRecord> FlowFileRecord::DeSerialize(io::InputStream& inS
}
ret = inStream.read(file->uuid_);
- if (ret <= 0) {
+ if (ret == static_cast<size_t>(-1)) {
Review comment:
here, and everywhere below in this file: we used to return `{}` when `read()` returns 0, too
```suggestion
if (ret == 0 || ret == static_cast<size_t>(-1)) {
```
(also in Provenance.cpp, RawSocketProtocol.cpp and SiteToSiteClient.cpp)
EDIT: this has changed in a few places in https://github.com/apache/nifi-minifi-cpp/pull/1028/commits/30f59be7b4ba2316c61bf3c1225b535dd0678f17 but not in other places; are the remaining places intentional?
##########
File path: libminifi/src/io/tls/SecureDescriptorStream.cpp
##########
@@ -71,34 +71,29 @@ int SecureDescriptorStream::write(const uint8_t *value, int size) {
}
}
-int SecureDescriptorStream::read(uint8_t *buf, int buflen) {
- gsl_Expects(buflen >= 0);
+size_t SecureDescriptorStream::read(uint8_t * const buf, const size_t buflen) {
if (buflen == 0) {
return 0;
}
- if (!IsNullOrEmpty(buf)) {
- int total_read = 0;
- int status = 0;
- while (buflen) {
- int sslStatus;
- do {
- status = SSL_read(ssl_, buf, buflen);
- sslStatus = SSL_get_error(ssl_, status);
- } while (status < 0 && sslStatus == SSL_ERROR_WANT_READ);
+ if (IsNullOrEmpty(buf)) return static_cast<size_t>(-1);
+ size_t total_read = 0;
+ uint8_t* writepos = buf;
+ while (buflen > total_read) {
+ int status;
+ int sslStatus;
+ do {
+ status = SSL_read(ssl_, writepos, gsl::narrow<int>(std::min(buflen - total_read, gsl::narrow<size_t>(std::numeric_limits<int>::max()))));
Review comment:
Do we need this `std::min`? `gsl_narrow` will throw if `buflen - total_read` is larger than `std::numeric_limits<int>::max()`.
(also in two places in TLSSocket.cpp)
##########
File path: libminifi/include/io/InputStream.h
##########
@@ -39,43 +39,43 @@ class InputStream : public virtual Stream {
* reads a byte array from the stream
* @param value reference in which will set the result
* @param len length to read
- * @return resulting read size
+ * @return resulting read size or static_cast<size_t>(-1) on error or static_cast<size_t>(-2) for ClientSocket EAGAIN
Review comment:
Returning `static_cast<size_t>(-2)` feels dangerous, as in most cases we only test the return value for `== static_cast<size_t>(-1)` and assume that it is a valid stream length otherwise.
We could either only return `static_cast<size_t>(-1)` on errors, or always test for both -1 and -2, maybe using someting like `Stream::isError(size_t)`.
##########
File path: libminifi/include/provenance/Provenance.h
##########
@@ -363,25 +363,23 @@ class ProvenanceEventRecord : public core::SerializableComponent {
uint64_t getEventTime(const uint8_t *buffer, const size_t bufferSize) {
const auto size = std::min<size_t>(72, bufferSize);
- org::apache::nifi::minifi::io::BufferStream outStream(buffer, gsl::narrow<int>(size));
+ org::apache::nifi::minifi::io::BufferStream outStream(buffer, size);
std::string uuid;
- int ret = outStream.read(uuid);
-
- if (ret <= 0) {
+ const auto uuidret = outStream.read(uuid);
+ if (uuidret <= 0) {
Review comment:
```suggestion
if (uuidret == 0 || uuidret == static_cast<size_t>(-1)) {
```
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -248,11 +248,11 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
void ControllerSocketProtocol::parse_content(const std::vector<C2ContentResponse> &content) {
for (const auto &payload_content : content) {
if (payload_content.name == "Components") {
- for (auto content : payload_content.operation_arguments) {
+ for (const auto& content_ : payload_content.operation_arguments) {
Review comment:
why `content_`? the `_` postfix usually marks a member variable
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601384653
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -190,16 +189,11 @@ class CompressContent: public core::Processor {
return ret;
}
- static la_ssize_t archive_read(struct archive *arch, void *context, const void **buff) {
- WriteCallback *callback = (WriteCallback *) context;
+ static la_ssize_t archive_read(struct archive*, void *context, const void **buff) {
+ auto *callback = (WriteCallback *) context;
callback->session_->read(callback->flow_, &callback->readDecompressCb_);
- if (callback->readDecompressCb_.read_size_ >= 0) {
- *buff = callback->readDecompressCb_.buffer_;
- return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
- } else {
- archive_set_error(arch, EIO, "Error reading flowfile");
- return -1;
- }
+ *buff = callback->readDecompressCb_.buffer_;
+ return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
Review comment:
fixed in 7955d00
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596129693
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -248,11 +248,11 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
void ControllerSocketProtocol::parse_content(const std::vector<C2ContentResponse> &content) {
for (const auto &payload_content : content) {
if (payload_content.name == "Components") {
- for (auto content : payload_content.operation_arguments) {
+ for (const auto& content_ : payload_content.operation_arguments) {
Review comment:
renamed to `operation_argument` in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601279395
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -190,16 +189,11 @@ class CompressContent: public core::Processor {
return ret;
}
- static la_ssize_t archive_read(struct archive *arch, void *context, const void **buff) {
- WriteCallback *callback = (WriteCallback *) context;
+ static la_ssize_t archive_read(struct archive*, void *context, const void **buff) {
+ auto *callback = (WriteCallback *) context;
callback->session_->read(callback->flow_, &callback->readDecompressCb_);
- if (callback->readDecompressCb_.read_size_ >= 0) {
- *buff = callback->readDecompressCb_.buffer_;
- return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
- } else {
- archive_set_error(arch, EIO, "Error reading flowfile");
- return -1;
- }
+ *buff = callback->readDecompressCb_.buffer_;
+ return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
Review comment:
[this](http://manpages.org/archive_read_open2/3) I-don't-know-how-trustworthy site says:
> On error, the read callback should invoke Fn archive_set_error to register an error code and message and return -1.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595284802
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -248,11 +248,11 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
void ControllerSocketProtocol::parse_content(const std::vector<C2ContentResponse> &content) {
for (const auto &payload_content : content) {
if (payload_content.name == "Components") {
- for (auto content : payload_content.operation_arguments) {
+ for (const auto& content_ : payload_content.operation_arguments) {
Review comment:
A minimal change to avoid shadowing the parameter. I'm open to alternatives.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601313766
##########
File path: extensions/mqtt/processors/PublishMQTT.h
##########
@@ -74,19 +77,21 @@ class PublishMQTT : public processors::AbstractMQTTProcessor {
status_ = 0;
read_size_ = 0;
}
- ~ReadCallback() = default;
- int64_t process(const std::shared_ptr<io::BaseStream>& stream) {
+ ~ReadCallback() override = default;
+ int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
if (flow_size_ < max_seg_size_)
max_seg_size_ = flow_size_;
+ gsl_Expects(max_seg_size_ < gsl::narrow<uint64_t>(std::numeric_limits<int>::max()));
std::vector<unsigned char> buffer;
buffer.reserve(max_seg_size_);
read_size_ = 0;
status_ = 0;
while (read_size_ < flow_size_) {
- int readRet = stream->read(&buffer[0], max_seg_size_);
+ // MQTTClient_message::payloadlen is int, so we can't handle 2GB+
+ int readRet = static_cast<int>(stream->read(&buffer[0], max_seg_size_));
Review comment:
we could check the return value of `stream->read(...)` with `io::isError` before casting to `int`
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601282015
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -383,13 +377,14 @@ class CompressContent: public core::Processor {
std::vector<uint8_t> buffer(16 * 1024U);
int64_t read_size = 0;
while (read_size < gsl::narrow<int64_t>(writer_.flow_->getSize())) {
- int ret = inputStream->read(buffer.data(), gsl::narrow<int>(buffer.size()));
- if (ret < 0) {
+ const auto ret = inputStream->read(buffer.data(), buffer.size());
+ if (io::isError(ret)) {
return -1;
} else if (ret == 0) {
break;
} else {
- if (outputStream_->write(buffer.data(), ret) != ret) {
+ const auto writeret = outputStream_->write(buffer.data(), ret);
+ if (!io::isError(writeret) && gsl::narrow<size_t>(writeret) != ret) {
Review comment:
previously this returned -1 on error, shouldn't it be?
```suggestion
if (io::isError(writeret) || gsl::narrow<size_t>(writeret) != ret) {
```
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595282360
##########
File path: libminifi/include/io/InputStream.h
##########
@@ -39,43 +39,43 @@ class InputStream : public virtual Stream {
* reads a byte array from the stream
* @param value reference in which will set the result
* @param len length to read
- * @return resulting read size
+ * @return resulting read size or static_cast<size_t>(-1) on error or static_cast<size_t>(-2) for ClientSocket EAGAIN
Review comment:
I felt the same, but didn't have a good idea how to resolve it. Refactoring Socket classes to use some other form of handling EAGAIN would be preferred, but that seems too out of scope here, and this change set feels dangerous enough already.
I like the idea of `Stream::isError`, I'll change the checks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595865232
##########
File path: extensions/script/python/PyBaseStream.cpp
##########
@@ -49,13 +49,11 @@ py::bytes PyBaseStream::read(size_t len) {
std::vector<uint8_t> buffer(len);
- auto read = stream_->read(buffer.data(), static_cast<int>(len));
- auto result = py::bytes(reinterpret_cast<char *>(buffer.data()), static_cast<size_t>(read));
-
- return result;
+ const auto read = stream_->read(buffer.data(), len);
+ return py::bytes(reinterpret_cast<char *>(buffer.data()), static_cast<size_t>(read));
Review comment:
resolved in 34b60f0
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r647219084
##########
File path: extensions/standard-processors/processors/PutFile.cpp
##########
@@ -304,9 +306,9 @@ void PutFile::getDirectoryPermissions(core::ProcessContext *context) {
}
#endif
-PutFile::ReadCallback::ReadCallback(const std::string &tmp_file, const std::string &dest_file)
- : tmp_file_(tmp_file),
- dest_file_(dest_file) {
+PutFile::ReadCallback::ReadCallback(std::string tmp_file, std::string dest_file)
+ : tmp_file_(std::move(tmp_file)),
+ dest_file_(std::move(dest_file)) {
Review comment:
same question here
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r647218577
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -133,28 +134,24 @@ class CompressContent: public core::Processor {
std::shared_ptr<logging::Logger> logger_;
};
// Nest Callback Class for read stream from flow for decompress
- class ReadCallbackDecompress: public InputStreamCallback {
- public:
- ReadCallbackDecompress(const std::shared_ptr<core::FlowFile> &flow) :
- read_size_(0), offset_(0), flow_(flow) {
- origin_offset_ = flow_->getOffset();
+ struct ReadCallbackDecompress : InputStreamCallback {
+ explicit ReadCallbackDecompress(std::shared_ptr<core::FlowFile> flow) :
+ flow_file(std::move(flow)) {
Review comment:
same question
##########
File path: extensions/standard-processors/processors/PutFile.cpp
##########
@@ -304,9 +306,9 @@ void PutFile::getDirectoryPermissions(core::ProcessContext *context) {
}
#endif
-PutFile::ReadCallback::ReadCallback(const std::string &tmp_file, const std::string &dest_file)
- : tmp_file_(tmp_file),
- dest_file_(dest_file) {
+PutFile::ReadCallback::ReadCallback(std::string tmp_file, std::string dest_file)
+ : tmp_file_(std::move(tmp_file)),
+ dest_file_(std::move(dest_file)) {
Review comment:
same question here
##########
File path: libminifi/include/sitetosite/SiteToSiteClient.h
##########
@@ -43,15 +45,14 @@ namespace sitetosite {
*/
class DataPacket {
public:
- DataPacket(const std::shared_ptr<logging::Logger> &logger, const std::shared_ptr<Transaction> &transaction, std::map<std::string, std::string> attributes, const std::string &payload)
- : payload_(payload),
- logger_reference_(logger) {
- _size = 0;
- transaction_ = transaction;
- _attributes = attributes;
+ DataPacket(std::shared_ptr<logging::Logger> logger, std::shared_ptr<Transaction> transaction, std::map<std::string, std::string> attributes, const std::string &payload)
+ : _attributes{std::move(attributes)},
+ transaction_{std::move(transaction)},
+ payload_{payload},
+ logger_reference_{std::move(logger)} {
Review comment:
why is this better than the const& and copy?
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596138026
##########
File path: libminifi/src/sitetosite/SiteToSiteClient.cpp
##########
@@ -31,47 +31,37 @@ namespace sitetosite {
int SiteToSiteClient::readResponse(const std::shared_ptr<Transaction>& /*transaction*/, RespondCode &code, std::string &message) {
uint8_t firstByte;
-
- int ret = peer_->read(firstByte);
-
- if (ret <= 0 || firstByte != CODE_SEQUENCE_VALUE_1)
+ auto ret = peer_->read(firstByte);
+ if (ret == 0 || ret == static_cast<size_t>(-1) || firstByte != CODE_SEQUENCE_VALUE_1)
return -1;
uint8_t secondByte;
-
ret = peer_->read(secondByte);
-
- if (ret <= 0 || secondByte != CODE_SEQUENCE_VALUE_2)
+ if (ret == 0 || ret == static_cast<size_t>(-1) || secondByte != CODE_SEQUENCE_VALUE_2)
return -1;
uint8_t thirdByte;
-
ret = peer_->read(thirdByte);
-
- if (ret <= 0)
- return ret;
+ if (ret == 0 || ret == static_cast<size_t>(-1))
+ return gsl::narrow_cast<int>(ret);
code = (RespondCode) thirdByte;
-
RespondCodeContext *resCode = this->getRespondCodeContext(code);
-
- if (resCode == NULL) {
- // Not a valid respond code
+ if (!resCode) {
return -1;
}
if (resCode->hasDescription) {
ret = peer_->read(message);
- if (ret <= 0)
+ if (ret != static_cast<size_t>(-1))
Review comment:
fixed partially in 0c23d4e and finally in 46f3b0e after https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596108625
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596861816
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
We used to throw an exception (in an obfuscated way) in the error case, now we return an empty string. This is probably good, but I don't know if we want to make a change like this in a refactoring PR. If we do want to make this change, we should probably change line 42, too.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596133924
##########
File path: libminifi/src/sitetosite/SiteToSiteClient.cpp
##########
@@ -31,47 +31,43 @@ namespace sitetosite {
int SiteToSiteClient::readResponse(const std::shared_ptr<Transaction>& /*transaction*/, RespondCode &code, std::string &message) {
uint8_t firstByte;
-
- int ret = peer_->read(firstByte);
-
- if (ret <= 0 || firstByte != CODE_SEQUENCE_VALUE_1)
- return -1;
+ {
+ const auto ret = peer_->read(firstByte);
+ if (ret == 0 || io::isError(ret) || firstByte != CODE_SEQUENCE_VALUE_1)
+ return -1;
+ }
uint8_t secondByte;
-
- ret = peer_->read(secondByte);
-
- if (ret <= 0 || secondByte != CODE_SEQUENCE_VALUE_2)
- return -1;
+ {
+ const auto ret = peer_->read(secondByte);
+ if (ret == 0 || io::isError(ret) || secondByte != CODE_SEQUENCE_VALUE_2)
+ return -1;
+ }
uint8_t thirdByte;
-
- ret = peer_->read(thirdByte);
-
- if (ret <= 0)
- return ret;
+ {
+ const auto ret = peer_->read(thirdByte);
+ if (ret == 0 || io::isError(ret))
+ return static_cast<int>(ret);
+ }
code = (RespondCode) thirdByte;
-
RespondCodeContext *resCode = this->getRespondCodeContext(code);
-
- if (resCode == NULL) {
- // Not a valid respond code
+ if (!resCode) {
return -1;
}
if (resCode->hasDescription) {
- ret = peer_->read(message);
- if (ret <= 0)
+ const auto ret = peer_->read(message);
+ if (ret == 0 || !io::isError(ret))
Review comment:
Thanks, fixed in 46f3b0e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595291226
##########
File path: libminifi/include/io/InputStream.h
##########
@@ -39,43 +39,43 @@ class InputStream : public virtual Stream {
* reads a byte array from the stream
* @param value reference in which will set the result
* @param len length to read
- * @return resulting read size
+ * @return resulting read size or static_cast<size_t>(-1) on error or static_cast<size_t>(-2) for ClientSocket EAGAIN
Review comment:
What do you think about `io::isError`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r640755701
##########
File path: libminifi/test/BufferReader.h
##########
@@ -44,7 +44,7 @@ class BufferReader : public org::apache::nifi::minifi::InputStreamCallback {
}
int64_t process(const std::shared_ptr<org::apache::nifi::minifi::io::BaseStream>& stream) {
- return write(*stream.get(), stream->size());
+ return static_cast<int64_t>(write(*stream.get(), stream->size()));
Review comment:
changed in 47358cb
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596871602
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
My understanding is that in case of an error the old version returned `nullptr` as `std::string`, invoking the `std::string::string(const char*)` constructor which led to undefined behavior. I thought replacing this with a defined empty string would be better, but I might be missing something.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596846495
##########
File path: libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp
##########
@@ -179,7 +179,7 @@ TEST_CASE("Delete NonExistent Claim", "[TestDBCR4]") {
std::string readstr;
- // -1 tell us we have an invalid stream
+ // error tells us we have an invalid stream
Review comment:
done in 141917b
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601535334
##########
File path: libminifi/test/BufferReader.h
##########
@@ -44,7 +44,7 @@ class BufferReader : public org::apache::nifi::minifi::InputStreamCallback {
}
int64_t process(const std::shared_ptr<org::apache::nifi::minifi::io::BaseStream>& stream) {
- return write(*stream.get(), stream->size());
+ return static_cast<int64_t>(write(*stream.get(), stream->size()));
Review comment:
why not `gsl::narrow`?
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596876156
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
This snippet was changed during the rebase, btw. Previously it was a simple comparison + resize without the `narrow` and the `if (read < 0)` condition
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r640755085
##########
File path: libminifi/include/io/Stream.h
##########
@@ -24,14 +24,23 @@ namespace nifi {
namespace minifi {
namespace io {
+inline bool isError(const size_t read_return) noexcept {
+ return read_return == static_cast<size_t>(-1) // general error
+ || read_return == static_cast<size_t>(-2); // Socket EAGAIN, to be refactored to eliminate this error condition
Review comment:
Introduced `constexpr auto minifi::io::STREAM_ERROR = static_cast<size_t>(-1);` in 47358cb
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595306129
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -248,11 +248,11 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
void ControllerSocketProtocol::parse_content(const std::vector<C2ContentResponse> &content) {
for (const auto &payload_content : content) {
if (payload_content.name == "Components") {
- for (auto content : payload_content.operation_arguments) {
+ for (const auto& content_ : payload_content.operation_arguments) {
Review comment:
Ah OK, I didn't see the name of the argument. We could rename `content_` to `operation_argument`, but it's not vital.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596166085
##########
File path: libminifi/test/rocksdb-tests/ContentSessionTests.cpp
##########
@@ -56,11 +56,10 @@ const std::shared_ptr<minifi::io::BaseStream>& operator>>(const std::shared_ptr<
str = "";
uint8_t buffer[4096]{};
while (true) {
- size_t ret = stream->read(buffer, sizeof(buffer));
+ const auto ret = stream->read(buffer, sizeof(buffer));
REQUIRE(ret >= 0);
Review comment:
```suggestion
REQUIRE(!minifi::io::isError(ret));
```
##########
File path: libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp
##########
@@ -179,7 +179,7 @@ TEST_CASE("Delete NonExistent Claim", "[TestDBCR4]") {
std::string readstr;
- // -1 tell us we have an invalid stream
+ // error tells us we have an invalid stream
Review comment:
it would be better to remove this comment, as we don't check for error here (also at line 230)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r647235884
##########
File path: libminifi/include/sitetosite/SiteToSiteClient.h
##########
@@ -43,15 +45,14 @@ namespace sitetosite {
*/
class DataPacket {
public:
- DataPacket(const std::shared_ptr<logging::Logger> &logger, const std::shared_ptr<Transaction> &transaction, std::map<std::string, std::string> attributes, const std::string &payload)
- : payload_(payload),
- logger_reference_(logger) {
- _size = 0;
- transaction_ = transaction;
- _attributes = attributes;
+ DataPacket(std::shared_ptr<logging::Logger> logger, std::shared_ptr<Transaction> transaction, std::map<std::string, std::string> attributes, const std::string &payload)
+ : _attributes{std::move(attributes)},
+ transaction_{std::move(transaction)},
+ payload_{payload},
+ logger_reference_{std::move(logger)} {
Review comment:
looked more into it, this might be better actually so consider my comments obsolete
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601385022
##########
File path: extensions/mqtt/processors/PublishMQTT.h
##########
@@ -74,19 +77,21 @@ class PublishMQTT : public processors::AbstractMQTTProcessor {
status_ = 0;
read_size_ = 0;
}
- ~ReadCallback() = default;
- int64_t process(const std::shared_ptr<io::BaseStream>& stream) {
+ ~ReadCallback() override = default;
+ int64_t process(const std::shared_ptr<io::BaseStream>& stream) override {
if (flow_size_ < max_seg_size_)
max_seg_size_ = flow_size_;
+ gsl_Expects(max_seg_size_ < gsl::narrow<uint64_t>(std::numeric_limits<int>::max()));
std::vector<unsigned char> buffer;
buffer.reserve(max_seg_size_);
read_size_ = 0;
status_ = 0;
while (read_size_ < flow_size_) {
- int readRet = stream->read(&buffer[0], max_seg_size_);
+ // MQTTClient_message::payloadlen is int, so we can't handle 2GB+
+ int readRet = static_cast<int>(stream->read(&buffer[0], max_seg_size_));
Review comment:
done in 7ef4b05
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596130219
##########
File path: libminifi/src/io/tls/SecureDescriptorStream.cpp
##########
@@ -71,34 +71,29 @@ int SecureDescriptorStream::write(const uint8_t *value, int size) {
}
}
-int SecureDescriptorStream::read(uint8_t *buf, int buflen) {
- gsl_Expects(buflen >= 0);
+size_t SecureDescriptorStream::read(uint8_t * const buf, const size_t buflen) {
if (buflen == 0) {
return 0;
}
- if (!IsNullOrEmpty(buf)) {
- int total_read = 0;
- int status = 0;
- while (buflen) {
- int sslStatus;
- do {
- status = SSL_read(ssl_, buf, buflen);
- sslStatus = SSL_get_error(ssl_, status);
- } while (status < 0 && sslStatus == SSL_ERROR_WANT_READ);
+ if (IsNullOrEmpty(buf)) return static_cast<size_t>(-1);
+ size_t total_read = 0;
+ uint8_t* writepos = buf;
+ while (buflen > total_read) {
+ int status;
+ int sslStatus;
+ do {
+ status = SSL_read(ssl_, writepos, gsl::narrow<int>(std::min(buflen - total_read, gsl::narrow<size_t>(std::numeric_limits<int>::max()))));
Review comment:
named the third argument in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r640755398
##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -214,7 +214,7 @@ class FlowFileResponder : public ServerAwareHandler {
minifi::io::CivetStream civet_stream(conn);
minifi::io::CRCStream < minifi::io::CivetStream > stream(gsl::make_not_null(&civet_stream));
uint32_t num_attributes;
- int read;
+ size_t read;
Review comment:
refactored the function in 47358cb
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596129303
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -126,22 +125,22 @@ int64_t ExtractText::ReadCallback::process(const std::shared_ptr<io::BaseStream>
if (sizeLimitStr.empty())
size_limit = DEFAULT_SIZE_LIMIT;
else if (sizeLimitStr != "0")
- size_limit = std::stoi(sizeLimitStr);
+ size_limit = gsl::narrow_cast<size_t>(std::stoi(sizeLimitStr));
Review comment:
changed to `static_cast` in 46f3b0e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595274724
##########
File path: extensions/sftp/client/SFTPClient.cpp
##########
@@ -577,20 +577,20 @@ bool SFTPClient::putFile(const std::string& path, io::BaseStream& input, bool ov
}
logger_->log_trace("Read %d bytes", read_ret);
total_read += read_ret;
- ssize_t remaining = read_ret;
+ auto remaining = gsl::narrow<size_t>(read_ret);
Review comment:
Indeed, it was an oversight. Fixing in the next commit.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r639708731
##########
File path: extensions/tensorflow/TFExtractTopLabels.cpp
##########
@@ -134,9 +136,9 @@ int64_t TFExtractTopLabels::LabelsReadCallback::process(const std::shared_ptr<io
buf.resize(buf_size);
while (total_read < stream->size()) {
- auto read = stream->read(reinterpret_cast<uint8_t *>(&buf[0]), static_cast<int>(buf_size));
-
- for (auto i = 0; i < read; i++) {
+ const auto read = stream->read(reinterpret_cast<uint8_t *>(&buf[0]), buf_size);
+ if (io::isError(read)) break;
Review comment:
read/write usually signal an error, but it's not consistent. I think Callback::process just forwards these results.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596108625
##########
File path: libminifi/src/sitetosite/SiteToSiteClient.cpp
##########
@@ -31,47 +31,43 @@ namespace sitetosite {
int SiteToSiteClient::readResponse(const std::shared_ptr<Transaction>& /*transaction*/, RespondCode &code, std::string &message) {
uint8_t firstByte;
-
- int ret = peer_->read(firstByte);
-
- if (ret <= 0 || firstByte != CODE_SEQUENCE_VALUE_1)
- return -1;
+ {
+ const auto ret = peer_->read(firstByte);
+ if (ret == 0 || io::isError(ret) || firstByte != CODE_SEQUENCE_VALUE_1)
+ return -1;
+ }
uint8_t secondByte;
-
- ret = peer_->read(secondByte);
-
- if (ret <= 0 || secondByte != CODE_SEQUENCE_VALUE_2)
- return -1;
+ {
+ const auto ret = peer_->read(secondByte);
+ if (ret == 0 || io::isError(ret) || secondByte != CODE_SEQUENCE_VALUE_2)
+ return -1;
+ }
uint8_t thirdByte;
-
- ret = peer_->read(thirdByte);
-
- if (ret <= 0)
- return ret;
+ {
+ const auto ret = peer_->read(thirdByte);
+ if (ret == 0 || io::isError(ret))
+ return static_cast<int>(ret);
+ }
code = (RespondCode) thirdByte;
-
RespondCodeContext *resCode = this->getRespondCodeContext(code);
-
- if (resCode == NULL) {
- // Not a valid respond code
+ if (!resCode) {
return -1;
}
if (resCode->hasDescription) {
- ret = peer_->read(message);
- if (ret <= 0)
+ const auto ret = peer_->read(message);
+ if (ret == 0 || !io::isError(ret))
Review comment:
```suggestion
if (ret == 0 || io::isError(ret))
```
##########
File path: libminifi/test/rocksdb-tests/DBContentRepositoryTests.cpp
##########
@@ -140,7 +140,7 @@ TEST_CASE("Test Empty Claim", "[TestDBCR3]") {
std::string readstr;
// -1 tell us we have an invalid stream
Review comment:
minor, but you changed "-1" to "error" in the other place -- should be changed here, too
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596129693
##########
File path: libminifi/src/c2/ControllerSocketProtocol.cpp
##########
@@ -248,11 +248,11 @@ void ControllerSocketProtocol::initialize(core::controller::ControllerServicePro
void ControllerSocketProtocol::parse_content(const std::vector<C2ContentResponse> &content) {
for (const auto &payload_content : content) {
if (payload_content.name == "Components") {
- for (auto content : payload_content.operation_arguments) {
+ for (const auto& content_ : payload_content.operation_arguments) {
Review comment:
renamed to `operation_argument` in 46f3b0e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596871602
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
My understanding is that the old version returned `nullptr` as `std::string`, invoking the `std::string::string(const char*)` constructor which led to undefined behavior in case of an error. I thought replacing this with a defined empty string would be better, but I might be missing something.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596218670
##########
File path: libminifi/test/rocksdb-tests/ContentSessionTests.cpp
##########
@@ -56,11 +56,10 @@ const std::shared_ptr<minifi::io::BaseStream>& operator>>(const std::shared_ptr<
str = "";
uint8_t buffer[4096]{};
while (true) {
- size_t ret = stream->read(buffer, sizeof(buffer));
+ const auto ret = stream->read(buffer, sizeof(buffer));
REQUIRE(ret >= 0);
Review comment:
somehow I didn't see the other check below it -- but yes, this can be removed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601513048
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -84,9 +85,9 @@ int InputStream::read(std::string &str, bool widen) {
}
std::vector<uint8_t> buffer(len);
- uint32_t bytes_read = gsl::narrow<uint32_t>(read(buffer.data(), len));
+ const auto bytes_read = gsl::narrow<uint32_t>(read(buffer.data(), len));
if (bytes_read != len) {
- return -1;
+ return static_cast<size_t>(-1);
Review comment:
as classes generally only override `read(uint8_t*, size_t)` won't using this swallow meaningful error codes coming from a base implementation? e.g. as I understand `Socket::read` might return `size_t{-2}`
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601506029
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -30,42 +30,43 @@ namespace nifi {
namespace minifi {
namespace io {
-int InputStream::read(std::vector<uint8_t>& buffer, int len) {
- if (buffer.size() < gsl::narrow<size_t>(len)) {
+size_t InputStream::read(std::vector<uint8_t>& buffer, size_t len) {
+ if (buffer.size() < len) {
buffer.resize(len);
}
- int ret = read(buffer.data(), len);
- buffer.resize((std::max)(ret, 0));
+ const auto ret = read(buffer.data(), len);
+ if (io::isError(ret)) return ret;
+ buffer.resize((std::max)(ret, size_t{0}));
Review comment:
is the `std::max` still necessary?
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595891399
##########
File path: libminifi/src/sitetosite/SiteToSiteClient.cpp
##########
@@ -31,47 +31,37 @@ namespace sitetosite {
int SiteToSiteClient::readResponse(const std::shared_ptr<Transaction>& /*transaction*/, RespondCode &code, std::string &message) {
uint8_t firstByte;
-
- int ret = peer_->read(firstByte);
-
- if (ret <= 0 || firstByte != CODE_SEQUENCE_VALUE_1)
+ auto ret = peer_->read(firstByte);
+ if (ret == 0 || ret == static_cast<size_t>(-1) || firstByte != CODE_SEQUENCE_VALUE_1)
return -1;
uint8_t secondByte;
-
ret = peer_->read(secondByte);
-
- if (ret <= 0 || secondByte != CODE_SEQUENCE_VALUE_2)
+ if (ret == 0 || ret == static_cast<size_t>(-1) || secondByte != CODE_SEQUENCE_VALUE_2)
return -1;
uint8_t thirdByte;
-
ret = peer_->read(thirdByte);
-
- if (ret <= 0)
- return ret;
+ if (ret == 0 || ret == static_cast<size_t>(-1))
+ return gsl::narrow_cast<int>(ret);
code = (RespondCode) thirdByte;
-
RespondCodeContext *resCode = this->getRespondCodeContext(code);
-
- if (resCode == NULL) {
- // Not a valid respond code
+ if (!resCode) {
return -1;
}
if (resCode->hasDescription) {
ret = peer_->read(message);
- if (ret <= 0)
+ if (ret != static_cast<size_t>(-1))
Review comment:
missing `== 0 ||`
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -30,42 +30,43 @@ namespace nifi {
namespace minifi {
namespace io {
-int InputStream::read(std::vector<uint8_t>& buffer, int len) {
- if (buffer.size() < gsl::narrow<size_t>(len)) {
+size_t InputStream::read(std::vector<uint8_t>& buffer, size_t len) {
+ if (buffer.size() < len) {
buffer.resize(len);
}
- int ret = read(buffer.data(), len);
- buffer.resize((std::max)(ret, 0));
+ const auto ret = read(buffer.data(), len);
+ if (ret == static_cast<size_t>(-1)) return ret;
+ buffer.resize((std::max)(ret, size_t{0}));
return ret;
}
-int InputStream::read(bool &value) {
+size_t InputStream::read(bool &value) {
uint8_t buf = 0;
if (read(&buf, 1) != 1) {
- return -1;
+ return static_cast<size_t>(-1);
}
value = buf;
return 1;
}
-int InputStream::read(utils::Identifier &value) {
+size_t InputStream::read(utils::Identifier &value) {
std::string uuidStr;
- int ret = read(uuidStr);
+ const auto ret = read(uuidStr);
if (ret < 0) {
return ret;
}
auto optional_uuid = utils::Identifier::parse(uuidStr);
if (!optional_uuid) {
- return -1;
+ return static_cast<size_t>(-1);
}
value = optional_uuid.value();
return ret;
}
-int InputStream::read(std::string &str, bool widen) {
+size_t InputStream::read(std::string &str, bool widen) {
uint32_t len = 0;
- int ret = 0;
+ size_t ret = 0;
if (!widen) {
uint16_t shortLength = 0;
ret = read(shortLength);
Review comment:
I can't comment at line 78, but it needs to be updated from `<= 0` to `== 0 || == static_cast(-1)`, too
##########
File path: thirdparty/google-styleguide/run_linter.sh
##########
@@ -40,5 +40,7 @@ done
HEADERS=`find $INCLUDE_DIRS -name '*.h' | sort | uniq | tr '\n' ' '`
SOURCES=`find $SOURCE_DIRS -name '*.cpp' | sort | uniq | tr '\n' ' '`
-REPOSITORY="$(realpath --physical "$(dirname "$0")/../..")"
+# this realpath alternative should work on mac
+alias prealpath="python -c 'import os, sys; print(os.path.realpath(sys.argv[1]))'"
Review comment:
where is this alias used?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596846391
##########
File path: libminifi/test/rocksdb-tests/ContentSessionTests.cpp
##########
@@ -56,11 +56,10 @@ const std::shared_ptr<minifi::io::BaseStream>& operator>>(const std::shared_ptr<
str = "";
uint8_t buffer[4096]{};
while (true) {
- size_t ret = stream->read(buffer, sizeof(buffer));
+ const auto ret = stream->read(buffer, sizeof(buffer));
REQUIRE(ret >= 0);
Review comment:
done in 141917b
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601538011
##########
File path: libminifi/test/archive-tests/CompressContentTests.cpp
##########
@@ -41,29 +41,23 @@
#include "processors/PutFile.h"
#include "utils/file/FileUtils.h"
#include "../Utils.h"
+#include "utils/gsl.h"
class ReadCallback: public minifi::InputStreamCallback {
public:
- explicit ReadCallback(size_t size) :
- read_size_(0) {
- buffer_size_ = size;
- buffer_ = new uint8_t[buffer_size_];
- archive_buffer_ = nullptr;
- archive_buffer_size_ = 0;
- }
- ~ReadCallback() {
- if (buffer_)
- delete[] buffer_;
- if (archive_buffer_)
- delete[] archive_buffer_;
- }
- int64_t process(const std::shared_ptr<minifi::io::BaseStream>& stream) {
+ explicit ReadCallback(size_t size)
+ :buffer_size_{size}
Review comment:
minor: a comment warning us that other member initializers depend on `buffer_size_` might help future readers
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596136746
##########
File path: thirdparty/google-styleguide/run_linter.sh
##########
@@ -40,5 +40,7 @@ done
HEADERS=`find $INCLUDE_DIRS -name '*.h' | sort | uniq | tr '\n' ' '`
SOURCES=`find $SOURCE_DIRS -name '*.cpp' | sort | uniq | tr '\n' ' '`
-REPOSITORY="$(realpath --physical "$(dirname "$0")/../..")"
+# this realpath alternative should work on mac
+alias prealpath="python -c 'import os, sys; print(os.path.realpath(sys.argv[1]))'"
Review comment:
removed the leftover line in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596915019
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
Yes, I think returning an empty string at line 42 would be fine.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601553300
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -190,16 +189,11 @@ class CompressContent: public core::Processor {
return ret;
}
- static la_ssize_t archive_read(struct archive *arch, void *context, const void **buff) {
- WriteCallback *callback = (WriteCallback *) context;
+ static la_ssize_t archive_read(struct archive*, void *context, const void **buff) {
+ auto *callback = (WriteCallback *) context;
callback->session_->read(callback->flow_, &callback->readDecompressCb_);
- if (callback->readDecompressCb_.read_size_ >= 0) {
- *buff = callback->readDecompressCb_.buffer_;
- return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
- } else {
- archive_set_error(arch, EIO, "Error reading flowfile");
- return -1;
- }
+ *buff = callback->readDecompressCb_.buffer_;
+ return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
Review comment:
`archive_set_error` should also be restored according to the link
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596888000
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
I didn't know that libstdc++ throws there. Despite being an unrelated change, I really don't want to rely on undefined behavior there, so I insist on a change.
In the old main version (that this PR was originally based on), there was no string construction from nullptr here (but the one on line 42 was already present). I believe that change on main to be an error, so by moving to returning an empty string, we basically revert to the old, correct version, the only change being that we no longer return an empty string of size `len`.
If you insist on throwing, I can change this to throw something explicitly, but I prefer the old behavior of returning an empty string. I want to change the occurrence on line 42 to be consistent with this one, too.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595865441
##########
File path: extensions/sftp/client/SFTPClient.cpp
##########
@@ -577,20 +577,20 @@ bool SFTPClient::putFile(const std::string& path, io::BaseStream& input, bool ov
}
logger_->log_trace("Read %d bytes", read_ret);
total_read += read_ret;
- ssize_t remaining = read_ret;
+ auto remaining = gsl::narrow<size_t>(read_ret);
Review comment:
resolved in 34b60f0
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#issuecomment-849744380
Manually cancelled github actions CI runs triggered by the push event after every job was successful from the pull_request event.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596202026
##########
File path: libminifi/test/rocksdb-tests/ContentSessionTests.cpp
##########
@@ -56,11 +56,10 @@ const std::shared_ptr<minifi::io::BaseStream>& operator>>(const std::shared_ptr<
str = "";
uint8_t buffer[4096]{};
while (true) {
- size_t ret = stream->read(buffer, sizeof(buffer));
+ const auto ret = stream->read(buffer, sizeof(buffer));
REQUIRE(ret >= 0);
Review comment:
I think I get it: `ret >= 0` is always true, I'll remove it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596073337
##########
File path: libminifi/include/io/InputStream.h
##########
@@ -39,43 +39,43 @@ class InputStream : public virtual Stream {
* reads a byte array from the stream
* @param value reference in which will set the result
* @param len length to read
- * @return resulting read size
+ * @return resulting read size or static_cast<size_t>(-1) on error or static_cast<size_t>(-2) for ClientSocket EAGAIN
Review comment:
done in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r640767159
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -190,16 +189,11 @@ class CompressContent: public core::Processor {
return ret;
}
- static la_ssize_t archive_read(struct archive *arch, void *context, const void **buff) {
- WriteCallback *callback = (WriteCallback *) context;
+ static la_ssize_t archive_read(struct archive*, void *context, const void **buff) {
+ auto *callback = (WriteCallback *) context;
callback->session_->read(callback->flow_, &callback->readDecompressCb_);
- if (callback->readDecompressCb_.read_size_ >= 0) {
- *buff = callback->readDecompressCb_.buffer_;
- return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
- } else {
- archive_set_error(arch, EIO, "Error reading flowfile");
- return -1;
- }
+ *buff = callback->readDecompressCb_.buffer_;
+ return gsl::narrow<la_ssize_t>(callback->readDecompressCb_.read_size_);
Review comment:
restored `archive_set_error` in e8a0a33
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r601414700
##########
File path: extensions/tensorflow/TFExtractTopLabels.cpp
##########
@@ -134,9 +136,9 @@ int64_t TFExtractTopLabels::LabelsReadCallback::process(const std::shared_ptr<io
buf.resize(buf_size);
while (total_read < stream->size()) {
- auto read = stream->read(reinterpret_cast<uint8_t *>(&buf[0]), static_cast<int>(buf_size));
-
- for (auto i = 0; i < read; i++) {
+ const auto read = stream->read(reinterpret_cast<uint8_t *>(&buf[0]), buf_size);
+ if (io::isError(read)) break;
Review comment:
not sure how other `Callback::process` methods work when they encounter an error, do they generally return what they managed to process or do they signal an error?
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596879885
##########
File path: extensions/script/lua/LuaBaseStream.cpp
##########
@@ -52,16 +52,11 @@ std::string LuaBaseStream::read(size_t len) {
// 0 <= n < s.size()."
//
// http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
- auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), static_cast<int>(len));
- if (read < 0) {
- return nullptr;
- }
-
- if (gsl::narrow<size_t>(read) != len) {
- buffer.resize(gsl::narrow<size_t>(read));
+ const auto read = stream_->read(reinterpret_cast<uint8_t *>(&buffer[0]), len);
+ if (!io::isError(read) && read != len) {
+ buffer.resize(read);
}
-
- return buffer;
+ return io::isError(read) ? std::string{} : buffer;
Review comment:
yup, I said "probably good", you said "would be better", there is no disagreement there :)
libstdc++ throws an exception, but you're right that other implementations may behave differently
my point is that the same thing happens at line 42
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596131314
##########
File path: libminifi/test/archive-tests/MergeFileTests.cpp
##########
@@ -111,8 +111,8 @@ std::vector<FixedBuffer> read_archives(const FixedBuffer& input) {
class ArchiveEntryReader {
public:
explicit ArchiveEntryReader(archive* arch) : arch(arch) {}
- int read(uint8_t* out, std::size_t len) {
- return gsl::narrow<int>(archive_read_data(arch, out, len));
+ size_t read(uint8_t* out, std::size_t len) {
+ return gsl::narrow_cast<size_t>(archive_read_data(arch, out, len));
Review comment:
Introduced a check for negative return values in 0c23d4e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596130219
##########
File path: libminifi/src/io/tls/SecureDescriptorStream.cpp
##########
@@ -71,34 +71,29 @@ int SecureDescriptorStream::write(const uint8_t *value, int size) {
}
}
-int SecureDescriptorStream::read(uint8_t *buf, int buflen) {
- gsl_Expects(buflen >= 0);
+size_t SecureDescriptorStream::read(uint8_t * const buf, const size_t buflen) {
if (buflen == 0) {
return 0;
}
- if (!IsNullOrEmpty(buf)) {
- int total_read = 0;
- int status = 0;
- while (buflen) {
- int sslStatus;
- do {
- status = SSL_read(ssl_, buf, buflen);
- sslStatus = SSL_get_error(ssl_, status);
- } while (status < 0 && sslStatus == SSL_ERROR_WANT_READ);
+ if (IsNullOrEmpty(buf)) return static_cast<size_t>(-1);
+ size_t total_read = 0;
+ uint8_t* writepos = buf;
+ while (buflen > total_read) {
+ int status;
+ int sslStatus;
+ do {
+ status = SSL_read(ssl_, writepos, gsl::narrow<int>(std::min(buflen - total_read, gsl::narrow<size_t>(std::numeric_limits<int>::max()))));
Review comment:
named the third argument in 46f3b0e
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595309149
##########
File path: libminifi/src/io/tls/SecureDescriptorStream.cpp
##########
@@ -71,34 +71,29 @@ int SecureDescriptorStream::write(const uint8_t *value, int size) {
}
}
-int SecureDescriptorStream::read(uint8_t *buf, int buflen) {
- gsl_Expects(buflen >= 0);
+size_t SecureDescriptorStream::read(uint8_t * const buf, const size_t buflen) {
if (buflen == 0) {
return 0;
}
- if (!IsNullOrEmpty(buf)) {
- int total_read = 0;
- int status = 0;
- while (buflen) {
- int sslStatus;
- do {
- status = SSL_read(ssl_, buf, buflen);
- sslStatus = SSL_get_error(ssl_, status);
- } while (status < 0 && sslStatus == SSL_ERROR_WANT_READ);
+ if (IsNullOrEmpty(buf)) return static_cast<size_t>(-1);
+ size_t total_read = 0;
+ uint8_t* writepos = buf;
+ while (buflen > total_read) {
+ int status;
+ int sslStatus;
+ do {
+ status = SSL_read(ssl_, writepos, gsl::narrow<int>(std::min(buflen - total_read, gsl::narrow<size_t>(std::numeric_limits<int>::max()))));
Review comment:
OK, that makes sense. This line is quite long and hard to read, though; pulling out the 3rd argument (with or without the `gsl::narrow`) and giving it a name could make it more readable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r595303389
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -126,22 +125,22 @@ int64_t ExtractText::ReadCallback::process(const std::shared_ptr<io::BaseStream>
if (sizeLimitStr.empty())
size_limit = DEFAULT_SIZE_LIMIT;
else if (sizeLimitStr != "0")
- size_limit = std::stoi(sizeLimitStr);
+ size_limit = gsl::narrow_cast<size_t>(std::stoi(sizeLimitStr));
Review comment:
I think throwing an exception in `onSchedule()` would be better than treating a limit of "-2" as a limit of 18.4 exabytes, but this is how the code was working before this PR, and I agree that changing it would be out of scope.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r596134706
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -30,42 +30,43 @@ namespace nifi {
namespace minifi {
namespace io {
-int InputStream::read(std::vector<uint8_t>& buffer, int len) {
- if (buffer.size() < gsl::narrow<size_t>(len)) {
+size_t InputStream::read(std::vector<uint8_t>& buffer, size_t len) {
+ if (buffer.size() < len) {
buffer.resize(len);
}
- int ret = read(buffer.data(), len);
- buffer.resize((std::max)(ret, 0));
+ const auto ret = read(buffer.data(), len);
+ if (ret == static_cast<size_t>(-1)) return ret;
+ buffer.resize((std::max)(ret, size_t{0}));
return ret;
}
-int InputStream::read(bool &value) {
+size_t InputStream::read(bool &value) {
uint8_t buf = 0;
if (read(&buf, 1) != 1) {
- return -1;
+ return static_cast<size_t>(-1);
}
value = buf;
return 1;
}
-int InputStream::read(utils::Identifier &value) {
+size_t InputStream::read(utils::Identifier &value) {
std::string uuidStr;
- int ret = read(uuidStr);
+ const auto ret = read(uuidStr);
if (ret < 0) {
return ret;
}
auto optional_uuid = utils::Identifier::parse(uuidStr);
if (!optional_uuid) {
- return -1;
+ return static_cast<size_t>(-1);
}
value = optional_uuid.value();
return ret;
}
-int InputStream::read(std::string &str, bool widen) {
+size_t InputStream::read(std::string &str, bool widen) {
uint32_t len = 0;
- int ret = 0;
+ size_t ret = 0;
if (!widen) {
uint16_t shortLength = 0;
ret = read(shortLength);
Review comment:
partially fixed in 0c23d4e and properly fixed in 46f3b0e after your new comment
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r640756106
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -84,9 +85,9 @@ int InputStream::read(std::string &str, bool widen) {
}
std::vector<uint8_t> buffer(len);
- uint32_t bytes_read = gsl::narrow<uint32_t>(read(buffer.data(), len));
+ const auto bytes_read = gsl::narrow<uint32_t>(read(buffer.data(), len));
if (bytes_read != len) {
- return -1;
+ return static_cast<size_t>(-1);
Review comment:
changed to forward errors in 47358cb
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -30,42 +30,43 @@ namespace nifi {
namespace minifi {
namespace io {
-int InputStream::read(std::vector<uint8_t>& buffer, int len) {
- if (buffer.size() < gsl::narrow<size_t>(len)) {
+size_t InputStream::read(std::vector<uint8_t>& buffer, size_t len) {
+ if (buffer.size() < len) {
buffer.resize(len);
}
- int ret = read(buffer.data(), len);
- buffer.resize((std::max)(ret, 0));
+ const auto ret = read(buffer.data(), len);
+ if (io::isError(ret)) return ret;
+ buffer.resize((std::max)(ret, size_t{0}));
Review comment:
no, removed in 47358cb
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r605454800
##########
File path: extensions/http-curl/tests/HTTPHandlers.h
##########
@@ -214,7 +214,7 @@ class FlowFileResponder : public ServerAwareHandler {
minifi::io::CivetStream civet_stream(conn);
minifi::io::CRCStream < minifi::io::CivetStream > stream(gsl::make_not_null(&civet_stream));
uint32_t num_attributes;
- int read;
+ size_t read;
Review comment:
This declaration could be merged with the definition
##########
File path: libminifi/include/io/Stream.h
##########
@@ -24,14 +24,23 @@ namespace nifi {
namespace minifi {
namespace io {
+inline bool isError(const size_t read_return) noexcept {
+ return read_return == static_cast<size_t>(-1) // general error
+ || read_return == static_cast<size_t>(-2); // Socket EAGAIN, to be refactored to eliminate this error condition
Review comment:
It may be more readable to name `static_cast<size_t>(-1)` and `static_cast<size_t>(-2)` as constants or as enums and replace them in the codebase.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r639766408
##########
File path: libminifi/src/io/InputStream.cpp
##########
@@ -84,9 +85,9 @@ int InputStream::read(std::string &str, bool widen) {
}
std::vector<uint8_t> buffer(len);
- uint32_t bytes_read = gsl::narrow<uint32_t>(read(buffer.data(), len));
+ const auto bytes_read = gsl::narrow<uint32_t>(read(buffer.data(), len));
if (bytes_read != len) {
- return -1;
+ return static_cast<size_t>(-1);
Review comment:
It would, but there is only one occurrence of returning -2 and I think it doesn't hit this path. That one occurrence should be cleaned up in the future anyway, so it's not an important issue IMO.
I will change it to forward the original return value, but if it mattered, we would already have an issue.
--
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:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1028: MINIFICPP-1507 convert InputStream::read to size_t
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1028:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1028#discussion_r639767608
##########
File path: libminifi/test/BufferReader.h
##########
@@ -44,7 +44,7 @@ class BufferReader : public org::apache::nifi::minifi::InputStreamCallback {
}
int64_t process(const std::shared_ptr<org::apache::nifi::minifi::io::BaseStream>& stream) {
- return write(*stream.get(), stream->size());
+ return static_cast<int64_t>(write(*stream.get(), stream->size()));
Review comment:
Because I wanted `static_cast<size_t>(-1)` to become `-1`. I will change it to explicitly check for error.
--
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:
users@infra.apache.org