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 2020/02/10 17:34:50 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request #731:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
szaszm opened a new pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731
Squashed commit of the following:
commit b0764d9ad83dee400a0a757578f92eb0d1f37e7e
Author: Szász Márton <sz...@users.noreply.github.com>
Date: Mon Feb 3 11:08:44 2020 +0100
Check unit test failure report on appveyor
commit 8fc8f2a3336111947ab6228c6f05240ad743c91d
Author: Marton Szasz <sz...@gmail.com>
Date: Fri Jan 31 16:15:15 2020 +0000
MINIFICPP-1096 Fix TailFileTests on windows
TailFile only supports single character (line) delimiter, but Windows
uses CRLF ("\r\n") line endings. Changed the tests to write the test
file in binary mode to avoid LF -> CRLF conversion that breaks TailFile.
We should consider adapting TailFile to support CRLF as it's standard on
Windows.
commit c4a1ee6714da534f8fdea96c960a86b2f8dbdcf2
Author: Marton Szasz <sz...@gmail.com>
Date: Wed Jan 29 14:48:40 2020 +0100
MINIFICPP-1096 fix BackTrace and OOB indexing issues in streams
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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] 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)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] 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 travis-ci 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385701729
##########
File path: extensions/standard-processors/tests/unit/ProcessorTests.cpp
##########
@@ -180,19 +184,37 @@ TEST_CASE("Test GetFile Ignore", "[getfileCreate3]") {
auto records = reporter->getEvents();
record = session->get();
REQUIRE(record == nullptr);
- REQUIRE(records.size() == 0);
+ REQUIRE(records.empty());
- std::fstream file;
- std::stringstream ss;
- ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
- file.open(ss.str(), std::ios::out);
- file << "tempFile";
- file.close();
+ const std::string hidden_file_name = [&] {
+ std::stringstream ss;
+ ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
+ return ss.str();
+ }();
+ {
+ std::ofstream file{ hidden_file_name };
+ file << "tempFile";
+ }
+
+#ifdef WIN32
+ {
Review comment:
Yeah, that's 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385680531
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
It belongs to the definition, not the declaration. It doesn't matter for the caller but provides extra checks for the definition. I tend to use `const` whenever it's possible and I don't know a reason not to.
ref1: this slide: https://youtu.be/iz5Qx18H6lg?t=293
ref2: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#con1-by-default-make-objects-immutable
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385234979
##########
File path: extensions/standard-processors/tests/unit/ProcessorTests.cpp
##########
@@ -180,19 +184,37 @@ TEST_CASE("Test GetFile Ignore", "[getfileCreate3]") {
auto records = reporter->getEvents();
record = session->get();
REQUIRE(record == nullptr);
- REQUIRE(records.size() == 0);
+ REQUIRE(records.empty());
- std::fstream file;
- std::stringstream ss;
- ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
- file.open(ss.str(), std::ios::out);
- file << "tempFile";
- file.close();
+ const std::string hidden_file_name = [&] {
+ std::stringstream ss;
+ ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
+ return ss.str();
+ }();
+ {
+ std::ofstream file{ hidden_file_name };
+ file << "tempFile";
+ }
+
+#ifdef WIN32
+ {
Review comment:
I would prefer if you could add this to `utils::file::FileUtils`, we might need it again.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385257180
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -640,10 +637,10 @@ class FileUtils {
std::vector<char> buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
`readlink`'s contract is pretty clear from the POSIX specification (https://pubs.opengroup.org/onlinepubs/9699919799/): it will return -1 on error, not negative on error. This is fine the way it is.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385678369
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -121,7 +122,7 @@ int64_t ExtractText::ReadCallback::process(std::shared_ptr<io::BaseStream> strea
ctx_->getProperty(SizeLimit.getName(), sizeLimitStr);
ctx_->getProperty(RegexMode.getName(), regex_mode);
- if (sizeLimitStr == "")
+ if (sizeLimitStr.empty())
Review comment:
Normally when I touch a file, I go through the clang-tidy warnings and apply the suggested fixes. This is one of them. Not strictly necessary, but it takes close to 0 effort and looks nicer.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-587131450
The next appveyor build should be green. The issue was early `ERRORLEVEL` variable substitution, proof that appveyor is red on unit test failure with the fix: https://ci.appveyor.com/project/szaszm/nifi-minifi-cpp/builds/30810385
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-603260855
@arpadboda The branch is currently not mergeable, so after @szaszm rebased it to the current master and I have been able to successfully compile it on Windows and run some tests I would be okay with merging it with the follow-up 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385242227
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
This was messed up, but the class description says that `Backtrace is a movable vector of trace lines.`, so either we should make it noncopyable, or justify why we need to copy it and change the description.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385808318
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
Making it copyable in addition to being movable doesn't break the previous promise. We can keep it non-copyable if desired, but I didn't see the point and since both declarations were wrong I decided to rely on the implicit copy and move ctors.
Given the above, do you still want to disable copy or change the description? (I'd just remove the "movable" word.)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-584241910
backwards compatibility notes:
The OOB indexing issue resolution means a backwards compatibility breakage because previously we incorrectly expected only the read buffer `capacity()` to be sufficient (and proceeded to write to the reserved but not allocated memory) but with this PR the new requirement is sufficient `size()`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385242227
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
This was messed up, put the class description says that `Backtrace is a movable vector of trace lines.`, so either we should make it noncopyable, or justify why we need to copy it and change the description.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385771605
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
I understand your intent, but it can be confusing for users when looking at the function signature. I agree with making as many things const as possible, but this, in my opinion, reduces readability of the API without having much benefit.
None of your references specifically address the case of having const pointers in function signatures, they are just speaking generically about immutable objects - and as I said, in the generic case, I agree.
Neither do I know of any major projects that would use const pointers on their API.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-589627687
GenerateFlowFileTests is a flicker (didn't happen last time), I'm investigating.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385771963
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -121,7 +122,7 @@ int64_t ExtractText::ReadCallback::process(std::shared_ptr<io::BaseStream> strea
ctx_->getProperty(SizeLimit.getName(), sizeLimitStr);
ctx_->getProperty(RegexMode.getName(), regex_mode);
- if (sizeLimitStr == "")
+ if (sizeLimitStr.empty())
Review comment:
I don't agree with it looking nicer, but I can live with 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385257180
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -640,10 +637,10 @@ class FileUtils {
std::vector<char> buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
`readlink`'s contract is pretty clear from the POSIX specification (https://pubs.opengroup.org/onlinepubs/009695399/functions/readlink.html): it will return -1 on error, not negative on error. This is fine the way it is.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385227484
##########
File path: extensions/http-curl/client/HTTPStream.cpp
##########
@@ -90,13 +95,17 @@ inline std::vector<uint8_t> HttpStream::readBuffer(const T& t) {
}
int HttpStream::readData(std::vector<uint8_t> &buf, int buflen) {
- if ((int) buf.capacity() < buflen) {
+ if (buflen < 0) {
+ throw minifi::Exception{ExceptionType::GENERAL_EXCEPTION, "negative buflen"};
+ }
+
+ if (buf.size() < static_cast<size_t>(buflen)) {
Review comment:
If this cast is the way to silence a warning I am OK with it, but then we should do the same in `HttpStream::writeData` (and all the other `readData`/`writeData` functions modified 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385773769
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
@szaszm my bad, your second reference actually addresses the issue, and spells out the reasons I give not to use it in this case:
```Exception Function arguments are rarely mutated, but also rarely declared const. To avoid confusion and lots of false positives, don’t enforce this rule for function arguments.```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385693436
##########
File path: extensions/standard-processors/tests/unit/ProcessorTests.cpp
##########
@@ -180,19 +184,37 @@ TEST_CASE("Test GetFile Ignore", "[getfileCreate3]") {
auto records = reporter->getEvents();
record = session->get();
REQUIRE(record == nullptr);
- REQUIRE(records.size() == 0);
+ REQUIRE(records.empty());
- std::fstream file;
- std::stringstream ss;
- ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
- file.open(ss.str(), std::ios::out);
- file << "tempFile";
- file.close();
+ const std::string hidden_file_name = [&] {
+ std::stringstream ss;
+ ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
+ return ss.str();
+ }();
+ {
+ std::ofstream file{ hidden_file_name };
+ file << "tempFile";
+ }
+
+#ifdef WIN32
+ {
Review comment:
This is inherently specific to Windows, since on windows "hidden" is a file system attribute while on unix it's a filename prefix. Because of the wildly different semantics, I would only add it inside an `#ifdef WIN32`/`#endif /* WIN32 */` there as well.
Would that be good in your opinion?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-603733093
@arpadboda @szaszm Built and ran tests on macOS and Windows. `C2VerifyHeartbeatAndStopSecure` and `C2VerifyHeartbeatAndStop` fail pretty frequently for me, both on macOS and Windows.
If these - as I suspect - are part of the follow-up https://issues.apache.org/jira/browse/MINIFICPP-1172 issue, then it's OK, but then those issues should be addressed sooner than later, as they prevent successful CI runs - both Travis and AppVeyor failed on the latest version of this PR- and finally having successful CI runs would have been one of the goals for this PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-603322133
rebased to master, compiled and ran the test suite on linux and windows, removed WIP tag given the separate handling of the remaining flicker.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386438667
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
reverted
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385768427
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -640,10 +637,10 @@ class FileUtils {
std::vector<char> buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
It doesn't hurt, I just didn't understand the purpose, but I can live with 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385242526
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
Review comment:
`explicit` breaks API.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-588222761
WIP: there seems to be a SEGFAULT triggered by `ManipulateArchiveTests`, I'm looking into 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #731:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385677942
##########
File path: extensions/http-curl/client/HTTPStream.cpp
##########
@@ -90,13 +95,17 @@ inline std::vector<uint8_t> HttpStream::readBuffer(const T& t) {
}
int HttpStream::readData(std::vector<uint8_t> &buf, int buflen) {
- if ((int) buf.capacity() < buflen) {
+ if (buflen < 0) {
+ throw minifi::Exception{ExceptionType::GENERAL_EXCEPTION, "negative buflen"};
+ }
+
+ if (buf.size() < static_cast<size_t>(buflen)) {
Review comment:
Yes, the purpose is to silence the warnings about comparison between signed and unsigned. I think I've applied this universally, but I'll double check.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385238986
##########
File path: libminifi/include/core/Repository.h
##########
@@ -127,7 +127,7 @@ class Repository : public virtual core::SerializableComponent, public core::Trac
/**
* Since SerializableComponents represent a runnable object, we should return traces
*/
- virtual BackTrace &&getTraces() {
+ virtual BackTrace getTraces() {
Review 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385231359
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -121,7 +122,7 @@ int64_t ExtractText::ReadCallback::process(std::shared_ptr<io::BaseStream> strea
ctx_->getProperty(SizeLimit.getName(), sizeLimitStr);
ctx_->getProperty(RegexMode.getName(), regex_mode);
- if (sizeLimitStr == "")
+ if (sizeLimitStr.empty())
Review comment:
I don't mind `empty()` too much, but why was this necessary? `std::string` has a pretty effective `operator==` with `const CharT* rhs`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386315388
##########
File path: extensions/standard-processors/tests/unit/ProcessorTests.cpp
##########
@@ -180,19 +184,37 @@ TEST_CASE("Test GetFile Ignore", "[getfileCreate3]") {
auto records = reporter->getEvents();
record = session->get();
REQUIRE(record == nullptr);
- REQUIRE(records.size() == 0);
+ REQUIRE(records.empty());
- std::fstream file;
- std::stringstream ss;
- ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
- file.open(ss.str(), std::ios::out);
- file << "tempFile";
- file.close();
+ const std::string hidden_file_name = [&] {
+ std::stringstream ss;
+ ss << dir << utils::file::FileUtils::get_separator() << ".filewithoutanext";
+ return ss.str();
+ }();
+ {
+ std::ofstream file{ hidden_file_name };
+ file << "tempFile";
+ }
+
+#ifdef WIN32
+ {
Review comment:
I also used this Win API call in GetFileTests.cpp. In case you create an util func, please make that depend on that, 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm edited a comment on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm edited a comment on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-584340432
WIP: appveyor is still not red on test failure + the above duplicate include
The rest is ready to review if someone's interested
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386299652
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
I am OK with it being copyable, but then the description should be changed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-603936391
The issue is that the C2 heartbeat is sometimes missing agentInfo in those test cases. Since it's likely to be because of the referenced bug, I'm ok with handling it there. I'd prefer not to waste effort on debugging it in context of this PR until the referenced issue is fixed.
I'm indifferent on whether we should merge this PR on not before MINIFICPP-1172 is resolved, but I'm against closing the Jira ticket until the CI is stable. If MINIFICPP-1172 doesn't solve the issue, a followup PR/commits are the right way to fix the C2 heartbeat flickers IMO.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm edited a comment on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm edited a comment on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-589627687
GenerateFlowFileTests failure is a flicker (didn't happen last time), I'm investigating.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests,
appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r377299347
##########
File path: libminifi/src/io/DataStream.cpp
##########
@@ -17,13 +17,13 @@
*/
#include "io/DataStream.h"
#include <vector>
-#include <iostream>
#include <cstdint>
-#include <cstdio>
-#include <cstring>
#include <string>
#include <algorithm>
#include <iterator>
+#include <Exception.h>
+#include <cassert>
+#include <iterator>
Review comment:
valid linter report on travis: duplicate include
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-594662529
WIP: debugging C2VerifyHeartbeatAndStop on windows (flicker)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385805427
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
Pointers are objects. My interpretation of the exception is that the issue is too minor to be enforced by static analyzers, and is commonly not followed, hence the "false positives". In other words, my interpretation of the exception doesn't suggest that function argument objects shouldn't be declared `const`.
I can see the point behind the readability argument, since `const` is present in the argument list, yet, the string behind the pointer is mutated. I think the risk of misunderstanding is minor given that we expect people who view the source to know the language well enough to know what `const` applies to. With this assumption, IMO the value gained by extra static analysis is greater than the value of lost readability.
I couldn't find any clear guidelines on the `const`-ness of argument pointers. I'm willing to remove `const` if you insist on 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386298703
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
Our core code is expected to be written by people very familiar with the language. We strive to create an ecosystem where extensions can be written (our API can be used) safely even by people without great familiarity with the language.
This change introduces a risk of misunderstanding by someone less familiar with the language (or someone familiar with it, just running low on caffeine), without bringing comparable benefits.
Please revert 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386298703
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
Our core code is expected to be written by people very familiar with the language. We strive to create an ecosystem where extensions can be written (our API can be used) safely even by people without great familiarity with the language.
This change introduces a risk of misunderstanding by someone less familiar with a language (or someone familiar with it, just running low on caffeine), without bringing comparable benefits.
Please revert 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386438350
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
Review comment:
removed `explicit`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385254493
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
#ifdef WIN32
- std::string tempDirectory;
- char tempBuffer[MAX_PATH];
- auto ret = GetTempPath(MAX_PATH, tempBuffer);
- if (ret <= MAX_PATH && ret != 0)
- {
- static std::shared_ptr<minifi::utils::IdGenerator> generator;
- if (!generator) {
- generator = minifi::utils::IdGenerator::getIdGenerator();
- generator->initialize(std::make_shared<minifi::Properties>());
- }
- tempDirectory = tempBuffer;
- minifi::utils::Identifier id;
- generator->generate(id);
- tempDirectory += id.to_string();
- create_dir(tempDirectory);
- }
- return tempDirectory;
+ std::string tempDirectory;
+ char tempBuffer[MAX_PATH];
+ auto ret = GetTempPath(MAX_PATH, tempBuffer);
+ if (ret <= MAX_PATH && ret != 0)
+ {
+ static std::shared_ptr<minifi::utils::IdGenerator> generator;
Review comment:
I know this is old code, but it is horrifically unsafe. Since it is currently only used from tests I don't mind it so much, but:
- `minifi::utils::IdGenerator::getIdGenerator` is already a singleton getter so this does function static variable does not make much sense aside from saving a shared_ptr copy
- only the creation (and initialization to nullptr) of `generator` is protected, the check for nullptr and assignment from `minifi::utils::IdGenerator::getIdGenerator` is not, so it is a race condition
- `generator->initialize` would not be protected even if `generator` would be initially initialized to `minifi::utils::IdGenerator::getIdGenerator()`, that's why `generator->initialize` should only be called at the beginning of a main executable, when there is only a single thread (see `main/MiNiFiMain.cpp` and `libminifi/test/TestBase.h`)
This whole part should just be rewritten to `utils::IdGenerator::getIdGenerator()->generate(id)`, and if it breaks something, that's the real problem.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request
#731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests,
appveyor reporting
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r381332868
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,33 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(const char * const format) {
Review comment:
As it's test code and I think nobody has ever made anything that depended on the modified buffer, I'm fine with this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385246943
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
Review comment:
What is the purpose of using a const pointer (not pointer to const) here as a function argument? It gives no extra guarantees to the caller.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-603996363
Additional info: I could observe the assertion failure in the C2 tests occuring on a civet worker thread while the main thread tries to delete the web server. I only observed this in a Debug build, not in a RelWithDebInfo, but I assume it always happens there and the different observation with RelWithDebInfo builds were because of optimization messing with debuggability.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on issue #731: MINIFICPP-1096
fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on issue #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-603323932
@szaszm Thanks, will start the compilation and tests.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm edited a comment on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm edited a comment on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-594662529
WIP: debugging C2VerifyHeartbeatAndStop on windows (flicker)
edit: probably related to [MINIFICPP-1172](https://issues.apache.org/jira/browse/MINIFICPP-1172)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
bakaid commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385233811
##########
File path: extensions/standard-processors/processors/ExtractText.cpp
##########
@@ -212,10 +213,10 @@ int64_t ExtractText::ReadCallback::process(std::shared_ptr<io::BaseStream> strea
}
ExtractText::ReadCallback::ReadCallback(std::shared_ptr<core::FlowFile> flowFile, core::ProcessContext *ctx, std::shared_ptr<logging::Logger> lgr)
- : flowFile_(flowFile),
+ : flowFile_(std::move(flowFile)),
ctx_(ctx),
- logger_(lgr) {
- buffer_.reserve(std::min<uint64_t>(flowFile->getSize(), MAX_BUFFER_SIZE));
+ logger_(std::move(lgr)) {
+ buffer_.resize(std::min<uint64_t>(flowFile_->getSize(), MAX_BUFFER_SIZE));
Review 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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386439462
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
#ifdef WIN32
- std::string tempDirectory;
- char tempBuffer[MAX_PATH];
- auto ret = GetTempPath(MAX_PATH, tempBuffer);
- if (ret <= MAX_PATH && ret != 0)
- {
- static std::shared_ptr<minifi::utils::IdGenerator> generator;
- if (!generator) {
- generator = minifi::utils::IdGenerator::getIdGenerator();
- generator->initialize(std::make_shared<minifi::Properties>());
- }
- tempDirectory = tempBuffer;
- minifi::utils::Identifier id;
- generator->generate(id);
- tempDirectory += id.to_string();
- create_dir(tempDirectory);
- }
- return tempDirectory;
+ std::string tempDirectory;
+ char tempBuffer[MAX_PATH];
+ auto ret = GetTempPath(MAX_PATH, tempBuffer);
+ if (ret <= MAX_PATH && ret != 0)
+ {
+ static std::shared_ptr<minifi::utils::IdGenerator> generator;
Review comment:
rewritten definition and fixed a possible data race in `IdGenerator`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests,
appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r381301940
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,33 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(const char * const format) {
Review comment:
This breaks backwards compatibility on POSIX because the buffer pointed-to by the argument is no longer modified. It was never modified on Windows, so the new behavior is consistent but different.
Is this OK to change?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on issue #731: WIP:
MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on issue #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#issuecomment-584340432
WIP: appveyor is still not red on unit test failure + the above duplicate include
The rest is ready to review if someone's interested
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests,
appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: WIP: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r381359294
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,33 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(const char * const format) {
Review comment:
I bailed on the idea since too many processor tests rely on the buffer being modified and they continue to compile but fail at runtime.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386437366
##########
File path: extensions/http-curl/client/HTTPStream.cpp
##########
@@ -90,13 +95,17 @@ inline std::vector<uint8_t> HttpStream::readBuffer(const T& t) {
}
int HttpStream::readData(std::vector<uint8_t> &buf, int buflen) {
- if ((int) buf.capacity() < buflen) {
+ if (buflen < 0) {
+ throw minifi::Exception{ExceptionType::GENERAL_EXCEPTION, "negative buflen"};
+ }
+
+ if (buf.size() < static_cast<size_t>(buflen)) {
Review comment:
I've missed this and `HTTPStream::writeData`, but I've found no more occurences of missing cast.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385681572
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -640,10 +637,10 @@ class FileUtils {
std::vector<char> buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
I know, but I feel safer with a `< 0` check before making a narrowing cast (to unsigned) below. (To avoid unsigned vs signed comparison warnings.)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r386437675
##########
File path: libminifi/include/utils/BackTrace.h
##########
@@ -41,20 +42,16 @@ class TraceResolver;
*/
class BackTrace {
public:
- BackTrace() {
- }
- BackTrace(const std::string &name)
- : name_(name) {
+ BackTrace() = default;
+
+ explicit BackTrace(std::string name)
+ : name_(std::move(name)) {
}
- BackTrace(BackTrace &&) = default;
- BackTrace(BackTrace &) = delete;
Review comment:
changed description
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request
#731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #731: MINIFICPP-1096 fix BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385681572
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -640,10 +637,10 @@ class FileUtils {
std::vector<char> buf(1024U);
while (true) {
ssize_t ret = readlink("/proc/self/exe", buf.data(), buf.size());
- if (ret == -1) {
+ if (ret < 0) {
Review comment:
I know, but I feel safer with a `< 0` check before making a narrowing cast (to unsigned) below. (To avoid unsigned vs signed comparison warnings.)
I can revert if you insist on 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
With regards,
Apache Git Services