szaszm commented on code in PR #1900:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1900#discussion_r1859133332
##########
extensions/windows-event-log/wel/MetadataWalker.cpp:
##########
@@ -90,10 +86,11 @@ bool MetadataWalker::for_each(pugi::xml_node &node) {
if (it != formatFlagMap.end()) {
std::function<std::string(const std::string &)> updateFunc = [&](const
std::string &input) -> std::string {
if (resolve_) {
- auto resolved = windows_event_log_metadata_.getEventData(it->second);
- if (!resolved.empty()) {
- return resolved;
+ const auto resolved =
windows_event_log_metadata_.getEventData(it->second);
+ if (resolved && !resolved->empty()) {
+ return *resolved;
}
+ // TODO(szaszm): add error logging
Review Comment:
One behavior change in `getEventData`: allocation failure now returns an
`error_code`, instead of an empty string. Since both empty strings and errors
are ignored here, there is no change in behavior. Added a comment to do proper
error handling later, but I didn't wanna do it in the context of this change.
(`MetadataWalker` has no logger.)
##########
extensions/windows-event-log/tests/MetadataWalkerTests.cpp:
##########
@@ -74,7 +74,7 @@ const short event_type_index = 178; // NOLINT short comes
from WINDOWS API
class FakeWindowsEventLogMetadata : public WindowsEventLogMetadata {
public:
- [[nodiscard]] std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const
override { return "event_data_for_flag_" + std::to_string(flags); }
+ [[nodiscard]] nonstd::expected<std::string, std::error_code>
getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const noexcept override { return
"event_data_for_flag_" + std::to_string(flags); }
Review Comment:
Changed this to `noexcept`. There are allocations here. If they fail, the
test will terminate and report failure, which is an acceptable behavior IMO.
##########
extensions/windows-event-log/wel/WindowsEventLog.h:
##########
@@ -59,22 +59,19 @@ enum METADATA {
UNKNOWN
};
+using METADATA_NAMES = std::vector<std::pair<Metadata, std::string>>;
-// this is a continuous enum, so we can rely on the array
-
-using METADATA_NAMES = std::vector<std::pair<METADATA, std::string>>;
+nonstd::expected<std::string, std::error_code>
formatEvent(gsl::not_null<EVT_HANDLE> metadata, gsl::not_null<EVT_HANDLE>
event, EVT_FORMAT_MESSAGE_FLAGS flags) noexcept;
Review Comment:
`EVT_HANDLE` is an alias to `HANDLE` which is an alias to `void*`
##########
extensions/windows-event-log/wel/WindowsEventLog.h:
##########
@@ -83,76 +80,50 @@ class WindowsEventLogHandler {
class WindowsEventLogMetadata {
public:
virtual ~WindowsEventLogMetadata() = default;
- [[nodiscard]] virtual std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS
flags) const = 0;
+ [[nodiscard]] virtual nonstd::expected<std::string, std::error_code>
getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const noexcept = 0;
[[nodiscard]] virtual std::string getEventTimestamp() const = 0;
virtual short getEventTypeIndex() const = 0; // NOLINT short comes from
WINDOWS API
- static std::string getMetadataString(METADATA val) {
- static std::map<METADATA, std::string> map = {
- {LOG_NAME, "LOG_NAME"},
- {SOURCE, "SOURCE"},
- {TIME_CREATED, "TIME_CREATED"},
- {EVENTID, "EVENTID"},
- {OPCODE, "OPCODE"},
- {EVENT_RECORDID, "EVENT_RECORDID"},
- {EVENT_TYPE, "EVENT_TYPE"},
- {TASK_CATEGORY, "TASK_CATEGORY"},
- {LEVEL, "LEVEL"},
- {KEYWORDS, "KEYWORDS"},
- {USER, "USER"},
- {COMPUTER, "COMPUTER"}
- };
-
- return map[val];
- }
-
- static METADATA getMetadataFromString(const std::string& val) {
- static std::map<std::string, METADATA> map = {
- {"LOG_NAME", LOG_NAME},
- {"SOURCE", SOURCE},
- {"TIME_CREATED", TIME_CREATED},
- {"EVENTID", EVENTID},
- {"OPCODE", OPCODE},
- {"EVENT_RECORDID", EVENT_RECORDID},
- {"TASK_CATEGORY", TASK_CATEGORY},
- {"EVENT_TYPE", EVENT_TYPE},
- {"LEVEL", LEVEL},
- {"KEYWORDS", KEYWORDS},
- {"USER", USER},
- {"COMPUTER", COMPUTER}
- };
-
- auto enumVal = map.find(val);
- if (enumVal != std::end(map)) {
- return enumVal->second;
- } else {
- return METADATA::UNKNOWN;
- }
+ static Metadata getMetadataFromString(std::string_view val) {
Review Comment:
Changed this to be thread-safe, and avoid a static map, at the expense of
linear lookup instead of logarithmic. I think it doesn't matter at such a small
and constant size.
##########
extensions/windows-event-log/wel/WindowsEventLog.cpp:
##########
@@ -203,8 +191,4 @@ std::string
WindowsEventLogHeader::createDefaultDelimiter(size_t length) const {
}
}
-EVT_HANDLE WindowsEventLogHandler::getMetadata() const {
- return metadata_provider_.get();
-}
Review Comment:
moved inline to the header
##########
extensions/windows-event-log/wel/MetadataWalker.cpp:
##########
@@ -123,40 +120,25 @@ std::vector<std::string>
MetadataWalker::getIdentifiers(const std::string &text)
return found_strings;
}
-std::string MetadataWalker::getMetadata(METADATA metadata) const {
- switch (metadata) {
- case LOG_NAME:
- return log_name_;
- case SOURCE:
- return getString(metadata_, "Provider");
- case TIME_CREATED:
- return windows_event_log_metadata_.getEventTimestamp();
- case EVENTID:
- return getString(metadata_, "EventID");
- case EVENT_RECORDID:
- return getString(metadata_, "EventRecordID");
- case OPCODE:
- return getString(metadata_, "Opcode");
- case TASK_CATEGORY:
- return getString(metadata_, "Task");
- case LEVEL:
- return getString(metadata_, "Level");
- case KEYWORDS:
- return getString(metadata_, "Keywords");
- case EVENT_TYPE:
- return std::to_string(windows_event_log_metadata_.getEventTypeIndex());
- case COMPUTER:
- return WindowsEventLogMetadata::getComputerName();
- }
- return "N/A";
-}
-
-std::map<std::string, std::string> MetadataWalker::getFieldValues() const {
- return fields_values_;
-}
-
-std::map<std::string, std::string> MetadataWalker::getIdentifiers() const {
- return replaced_identifiers_;
Review Comment:
moved inline to the header
##########
extensions/windows-event-log/wel/MetadataWalker.cpp:
##########
@@ -123,40 +120,25 @@ std::vector<std::string>
MetadataWalker::getIdentifiers(const std::string &text)
return found_strings;
}
-std::string MetadataWalker::getMetadata(METADATA metadata) const {
- switch (metadata) {
- case LOG_NAME:
- return log_name_;
- case SOURCE:
- return getString(metadata_, "Provider");
- case TIME_CREATED:
- return windows_event_log_metadata_.getEventTimestamp();
- case EVENTID:
- return getString(metadata_, "EventID");
- case EVENT_RECORDID:
- return getString(metadata_, "EventRecordID");
- case OPCODE:
- return getString(metadata_, "Opcode");
- case TASK_CATEGORY:
- return getString(metadata_, "Task");
- case LEVEL:
- return getString(metadata_, "Level");
- case KEYWORDS:
- return getString(metadata_, "Keywords");
- case EVENT_TYPE:
- return std::to_string(windows_event_log_metadata_.getEventTypeIndex());
- case COMPUTER:
- return WindowsEventLogMetadata::getComputerName();
- }
- return "N/A";
-}
-
-std::map<std::string, std::string> MetadataWalker::getFieldValues() const {
- return fields_values_;
-}
-
-std::map<std::string, std::string> MetadataWalker::getIdentifiers() const {
- return replaced_identifiers_;
+std::string MetadataWalker::getMetadata(Metadata metadata) const {
+ using enum Metadata;
+ switch (metadata) {
+ case LOG_NAME: return log_name_;
+ case SOURCE: return getString(metadata_, "Provider");
+ case TIME_CREATED: return windows_event_log_metadata_.getEventTimestamp();
+ case EVENTID: return getString(metadata_, "EventID");
+ case EVENT_RECORDID: return getString(metadata_, "EventRecordID");
+ case OPCODE: return getString(metadata_, "Opcode");
+ case TASK_CATEGORY: return getString(metadata_, "Task");
+ case LEVEL: return getString(metadata_, "Level");
+ case KEYWORDS: return getString(metadata_, "Keywords");
+ case EVENT_TYPE: return
std::to_string(windows_event_log_metadata_.getEventTypeIndex());
+ case COMPUTER: return WindowsEventLogMetadata::getComputerName();
+ case USER: // TODO(szaszm): unhandled before refactoring
+ case UNKNOWN: // TODO(szaszm): unhandled before refactoring
Review Comment:
I'm not sure how to properly handle these, so I opted to not change the
behavior.
##########
extensions/windows-event-log/wel/WindowsEventLog.h:
##########
@@ -186,17 +157,16 @@ class WindowsEventLogHeader {
template<typename MetadataCollection>
std::string WindowsEventLogHeader::getEventHeader(const MetadataCollection&
metadata_collection) const {
std::stringstream eventHeader;
-
for (const auto& option : header_names_) {
- auto name = option.second;
+ const auto& name = option.second;
Review Comment:
`option.second` is a `std::string`
##########
extensions/windows-event-log/wel/WindowsEventLog.h:
##########
@@ -83,76 +80,50 @@ class WindowsEventLogHandler {
class WindowsEventLogMetadata {
public:
virtual ~WindowsEventLogMetadata() = default;
- [[nodiscard]] virtual std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS
flags) const = 0;
+ [[nodiscard]] virtual nonstd::expected<std::string, std::error_code>
getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const noexcept = 0;
[[nodiscard]] virtual std::string getEventTimestamp() const = 0;
virtual short getEventTypeIndex() const = 0; // NOLINT short comes from
WINDOWS API
- static std::string getMetadataString(METADATA val) {
- static std::map<METADATA, std::string> map = {
Review Comment:
this was unused
##########
libminifi/src/utils/OsUtils.cpp:
##########
@@ -296,7 +296,7 @@ int64_t OsUtils::getTotalPagingFileSize() {
return total_paging_file_size;
}
-std::error_code OsUtils::windowsErrorToErrorCode(DWORD error_code) {
+std::error_code OsUtils::windowsErrorToErrorCode(DWORD error_code) noexcept {
Review Comment:
No allocations happen here, so it might as well be `noexcept`
##########
extensions/windows-event-log/wel/WindowsEventLog.h:
##########
@@ -83,76 +80,50 @@ class WindowsEventLogHandler {
class WindowsEventLogMetadata {
public:
virtual ~WindowsEventLogMetadata() = default;
- [[nodiscard]] virtual std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS
flags) const = 0;
+ [[nodiscard]] virtual nonstd::expected<std::string, std::error_code>
getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const noexcept = 0;
[[nodiscard]] virtual std::string getEventTimestamp() const = 0;
virtual short getEventTypeIndex() const = 0; // NOLINT short comes from
WINDOWS API
- static std::string getMetadataString(METADATA val) {
- static std::map<METADATA, std::string> map = {
- {LOG_NAME, "LOG_NAME"},
- {SOURCE, "SOURCE"},
- {TIME_CREATED, "TIME_CREATED"},
- {EVENTID, "EVENTID"},
- {OPCODE, "OPCODE"},
- {EVENT_RECORDID, "EVENT_RECORDID"},
- {EVENT_TYPE, "EVENT_TYPE"},
- {TASK_CATEGORY, "TASK_CATEGORY"},
- {LEVEL, "LEVEL"},
- {KEYWORDS, "KEYWORDS"},
- {USER, "USER"},
- {COMPUTER, "COMPUTER"}
- };
-
- return map[val];
- }
-
- static METADATA getMetadataFromString(const std::string& val) {
- static std::map<std::string, METADATA> map = {
- {"LOG_NAME", LOG_NAME},
- {"SOURCE", SOURCE},
- {"TIME_CREATED", TIME_CREATED},
- {"EVENTID", EVENTID},
- {"OPCODE", OPCODE},
- {"EVENT_RECORDID", EVENT_RECORDID},
- {"TASK_CATEGORY", TASK_CATEGORY},
- {"EVENT_TYPE", EVENT_TYPE},
- {"LEVEL", LEVEL},
- {"KEYWORDS", KEYWORDS},
- {"USER", USER},
- {"COMPUTER", COMPUTER}
- };
-
- auto enumVal = map.find(val);
- if (enumVal != std::end(map)) {
- return enumVal->second;
- } else {
- return METADATA::UNKNOWN;
- }
+ static Metadata getMetadataFromString(std::string_view val) {
+ using enum Metadata;
+ if (val == "LOG_NAME") { return LOG_NAME; }
+ else if (val == "SOURCE") { return SOURCE; }
+ else if (val == "TIME_CREATED") { return TIME_CREATED; }
+ else if (val == "EVENTID") { return EVENTID; }
+ else if (val == "OPCODE") { return OPCODE; }
+ else if (val == "EVENT_RECORDID") { return EVENT_RECORDID; }
+ else if (val == "EVENT_TYPE") { return EVENT_TYPE; }
+ else if (val == "TASK_CATEGORY") { return TASK_CATEGORY; }
+ else if (val == "LEVEL") { return LEVEL; }
+ else if (val == "KEYWORDS") { return KEYWORDS; }
+ else if (val == "USER") { return USER; }
+ else if (val == "COMPUTER") { return COMPUTER; }
+ else { return UNKNOWN; }
}
static std::string getComputerName() {
- static std::string computer_name;
- if (computer_name.empty()) {
- char buff[10248];
+ static std::string computer_name = []() -> std::string {
Review Comment:
Moved this to an immediately invoked lambda: this way the thread-safe static
initialization takes care of only running the initialization once, and
subsequent calls will just copy the static string. The old version was not
thread-safe.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
