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:
us...@infra.apache.org
