szaszm commented on a change in pull request #1057:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1057#discussion_r633702768
##########
File path: libminifi/include/core/logging/Logger.h
##########
@@ -68,13 +69,32 @@ inline T conditional_conversion(T t) {
}
template<typename ... Args>
-inline std::string format_string(char const* format_str, Args&&... args) {
- char buf[LOG_BUFFER_SIZE];
- std::snprintf(buf, LOG_BUFFER_SIZE, format_str,
conditional_conversion(std::forward<Args>(args))...);
- return std::string(buf);
+inline std::string format_string(int max_size, char const* format_str,
Args&&... args) {
+ if (0 <= max_size && max_size <= LOG_BUFFER_SIZE) {
+ // use static buffer
+ char buf[LOG_BUFFER_SIZE + 1];
+ std::snprintf(buf, max_size + 1, format_str,
conditional_conversion(std::forward<Args>(args))...);
+ return std::string(buf);
+ }
+ // use dynamically allocated buffer
+ if (max_size < 0) {
+ // query what buffer size we need
+ int size_needed = std::snprintf(nullptr, 0, format_str,
conditional_conversion(std::forward<Args>(args))...);
+ if (size_needed < 0) {
+ // error
+ return std::string("Error while formatting log message");
+ }
+ std::vector<char> buffer(size_needed + 1); // extra '\0' character
+ std::snprintf(buffer.data(), buffer.size(), format_str,
conditional_conversion(std::forward<Args>(args))...);
+ return std::string(buffer.data());
+ }
+ // use dynamic but fix-sized buffer
+ std::vector<char> buffer(max_size);
+ std::snprintf(buffer.data(), buffer.size(), format_str,
conditional_conversion(std::forward<Args>(args))...);
+ return std::string(buffer.data());
Review comment:
`std::string` has an iterator pair constructor that can be optimized to
a `memcpy`. I would prefer using that one if the bounds are known (returned by
`snprintf` in this case).
##########
File path: libminifi/src/Configure.cpp
##########
@@ -37,6 +37,10 @@ bool Configure::get(const std::string& key, std::string&
value) const {
bool Configure::get(const std::string& key, const std::string& alternate_key,
std::string& value) const {
if (get(key, value)) {
+ if (get(alternate_key)) {
+ const auto logger = logging::LoggerFactory<Configure>::getLogger();
+ logger->log_warn("Both the property '%s' and an alternate property '%s'
are set. Using '%s'.", key, alternate_key, key);
Review comment:
I think this is expected behavior, not something that requires extra
attention, so I feel like the `warn` level is too high. Maybe debug?
##########
File path: libminifi/src/c2/HeartbeatJsonSerializer.cpp
##########
@@ -0,0 +1,333 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "c2/HeartbeatJsonSerializer.h"
+#include "rapidjson/document.h"
+#include "rapidjson/writer.h"
+#include "rapidjson/stringbuffer.h"
+#include "rapidjson/prettywriter.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace c2 {
+
+std::string HeartbeatJsonSerializer::getOperation(const C2Payload& payload) {
+ switch (payload.getOperation()) {
+ case Operation::ACKNOWLEDGE:
+ return "acknowledge";
+ case Operation::HEARTBEAT:
+ return "heartbeat";
+ case Operation::RESTART:
+ return "restart";
+ case Operation::DESCRIBE:
+ return "describe";
+ case Operation::STOP:
+ return "stop";
+ case Operation::START:
+ return "start";
+ case Operation::UPDATE:
+ return "update";
+ case Operation::PAUSE:
+ return "pause";
+ case Operation::RESUME:
+ return "resume";
+ default:
+ return "heartbeat";
+ }
+}
+
+static void serializeOperationInfo(rapidjson::Value& target, const C2Payload&
payload, rapidjson::Document::AllocatorType& alloc) {
+ gsl_Expects(target.IsObject());
+
+ target.AddMember("operation",
HeartbeatJsonSerializer::getOperation(payload), alloc);
+
+ std::string id = payload.getIdentifier();
+ if (id.empty()) {
+ return;
+ }
+
+ target.AddMember("operationId", id, alloc);
+ std::string state_str;
+ switch (payload.getStatus().getState()) {
+ case state::UpdateState::FULLY_APPLIED:
+ state_str = "FULLY_APPLIED";
+ break;
+ case state::UpdateState::PARTIALLY_APPLIED:
+ state_str = "PARTIALLY_APPLIED";
+ break;
+ case state::UpdateState::READ_ERROR:
+ state_str = "OPERATION_NOT_UNDERSTOOD";
+ break;
+ case state::UpdateState::SET_ERROR:
+ default:
+ state_str = "NOT_APPLIED";
+ }
Review comment:
100% optional, just a comment. These are one of the cases that I
normally write as immediately invoked lambdas, because that allows me to
initialize in one expression and avoid assignment later.
some related guidelines:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-lambda-init
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es20-always-initialize-an-object
(conceptually those values are the initial value of `state_str`)
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es22-dont-declare-a-variable-until-you-have-a-value-to-initialize-it-with
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on
and visual demonstration:
```suggestion
const std::string state_str = [] {
switch (payload.getStatus().getState()) {
case state::UpdateState::FULLY_APPLIED:
return "FULLY_APPLIED";
case state::UpdateState::PARTIALLY_APPLIED:
return "PARTIALLY_APPLIED";
case state::UpdateState::READ_ERROR:
return "OPERATION_NOT_UNDERSTOOD";
case state::UpdateState::SET_ERROR:
default:
return "NOT_APPLIED";
}
}();
```
##########
File path: libminifi/include/utils/Enum.h
##########
@@ -60,6 +63,22 @@ namespace utils {
#define FOR_EACH_8(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8) \
fn(_1) delim() fn(_2) delim() fn(_3) delim() fn(_4) delim() \
fn(_5) delim() fn(_6) delim() fn(_7) delim() fn(_8)
+#define FOR_EACH_9(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8, _9) \
+ fn(_1) delim() fn(_2) delim() fn(_3) delim() fn(_4) delim() \
+ fn(_5) delim() fn(_6) delim() fn(_7) delim() fn(_8) delim() \
+ fn(_9)
+#define FOR_EACH_10(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10) \
+ fn(_1) delim() fn(_2) delim() fn(_3) delim() fn(_4) delim() \
+ fn(_5) delim() fn(_6) delim() fn(_7) delim() fn(_8) delim() \
+ fn(_9) delim() fn(_10)
+#define FOR_EACH_11(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11) \
+ fn(_1) delim() fn(_2) delim() fn(_3) delim() fn(_4) delim() \
+ fn(_5) delim() fn(_6) delim() fn(_7) delim() fn(_8) delim() \
+ fn(_9) delim() fn(_10) delim() fn(_11)
+#define FOR_EACH_12(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
_12) \
+ fn(_1) delim() fn(_2) delim() fn(_3) delim() fn(_4) delim() \
+ fn(_5) delim() fn(_6) delim() fn(_7) delim() fn(_8) delim() \
+ fn(_9) delim() fn(_10) delim() fn(_11) delim() fn(_12)
Review comment:
Could you reference the previous macros to simplify these?
Boost.preprocessor does this:
https://github.com/boostorg/preprocessor/blob/develop/include/boost/preprocessor/repetition/limits/repeat_256.hpp
Not a big deal, I'm fine with no change as well.
```suggestion
#define FOR_EACH_11(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11)
\
fn(_1) delim() FOR_EACH_10(fn, delim, _2, _3, _4, _5, _6, _7, _8, _9, _10,
_11)
#define FOR_EACH_12(fn, delim, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
_12) \
fn(_1) delim() FOR_EACH_11(fn, delim, _2, _3, _4, _5, _6, _7, _8, _9, _10,
_11, _12)
```
##########
File path: libminifi/include/core/logging/Logger.h
##########
@@ -68,13 +69,32 @@ inline T conditional_conversion(T t) {
}
template<typename ... Args>
-inline std::string format_string(char const* format_str, Args&&... args) {
- char buf[LOG_BUFFER_SIZE];
- std::snprintf(buf, LOG_BUFFER_SIZE, format_str,
conditional_conversion(std::forward<Args>(args))...);
- return std::string(buf);
+inline std::string format_string(int max_size, char const* format_str,
Args&&... args) {
+ if (0 <= max_size && max_size <= LOG_BUFFER_SIZE) {
+ // use static buffer
+ char buf[LOG_BUFFER_SIZE + 1];
+ std::snprintf(buf, max_size + 1, format_str,
conditional_conversion(std::forward<Args>(args))...);
+ return std::string(buf);
+ }
+ // use dynamically allocated buffer
+ if (max_size < 0) {
+ // query what buffer size we need
+ int size_needed = std::snprintf(nullptr, 0, format_str,
conditional_conversion(std::forward<Args>(args))...);
Review comment:
`snprintf` incurs the full cost of formatting even if the buffer is
`nullptr` AFAIK. I would prefer a conservative sized buffer with fallback to
allocation, so that at least sometimes it can be faster.
--
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
