phrocker commented on a change in pull request #596: MINIFICPP-925 - Fix
TailFile hang on long lines
URL: https://github.com/apache/nifi-minifi-cpp/pull/596#discussion_r297783964
##########
File path: extensions/standard-processors/processors/LogAttribute.h
##########
@@ -90,26 +90,19 @@ class LogAttribute : public core::Processor {
class ReadCallback : public InputStreamCallback {
public:
ReadCallback(uint64_t size)
- : read_size_(0) {
- buffer_size_ = size;
- buffer_ = new uint8_t[buffer_size_];
- }
- ~ReadCallback() {
- if (buffer_)
- delete[] buffer_;
+ : buffer_(size) {
}
int64_t process(std::shared_ptr<io::BaseStream> stream) {
- int64_t ret = 0;
- ret = stream->read(buffer_, buffer_size_);
- if (!stream)
- read_size_ = stream->getSize();
- else
- read_size_ = buffer_size_;
- return ret;
+ if (buffer_.size() == 0U) {
+ return 0U;
+ }
+ int ret = stream->read(buffer_.data(), buffer_.size());
Review comment:
I don't think that's a correct interpretation of the contract. The null
dereference is bad, but my focus is continuation of the same positive path
behavior. My opinion is that this should be chunking and checking return size.
The fact that the constructor accepts the size doesn't make a lot of sense. We
should choose a smaller size and chunk it ( which the contract does support
even if it isn't evident ) or be safe and continue reading until -1 or 0 are
met.
-1 returning rolls back, but when that exception is caught the same will be
done so the change isn't great. The only change is that a partial read ( which
the contract doesn't prohibit ). I'll have to think about this general change
and the impact and realistic possibility it may occur.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
