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 2022/11/28 13:38:05 UTC
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1434: MINIFICPP-1949 ConsumeWindowsEventLog precompiled regex
fgerlits commented on code in PR #1434:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1434#discussion_r1030666865
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -233,7 +231,7 @@ void ConsumeWindowsEventLog::onSchedule(const std::shared_ptr<core::ProcessConte
context->getProperty(IdentifierMatcher.getName(), regex_);
Review Comment:
this line is no longer needed, since `regex_` is set later, at lines 257-260
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -537,79 +548,71 @@ void ConsumeWindowsEventLog::substituteXMLPercentageItems(pugi::xml_document& do
doc.traverse(treeWalker);
}
-bool ConsumeWindowsEventLog::createEventRender(EVT_HANDLE hEvent, EventRender& eventRender) {
+nonstd::expected<std::string, std::string> ConsumeWindowsEventLog::renderEventAsXml(EVT_HANDLE event_handle) {
logger_->log_trace("Rendering an event");
WCHAR stackBuffer[4096];
DWORD size = sizeof(stackBuffer);
using Deleter = utils::StackAwareDeleter<WCHAR, utils::FreeDeleter>;
- std::unique_ptr<WCHAR, Deleter> buf{stackBuffer, Deleter{ stackBuffer }};
+ std::unique_ptr<WCHAR, Deleter> buf{stackBuffer, Deleter{stackBuffer}};
DWORD used = 0;
DWORD propertyCount = 0;
- if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
- if (ERROR_INSUFFICIENT_BUFFER != GetLastError()) {
- LOG_LAST_ERROR(EvtRender);
- return false;
+ if (!EvtRender(nullptr, event_handle, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
+ DWORD last_error = GetLastError();
+ if (ERROR_INSUFFICIENT_BUFFER != last_error) {
+ std::string error_message = std::format("EvtRender failed due to %s", utils::OsUtils::getWindowsErrorAsString(last_error));
+ return nonstd::make_unexpected(error_message);
}
- if (used > maxBufferSize_) {
- logger_->log_error("Dropping event because it couldn't be rendered within %" PRIu64 " bytes.", maxBufferSize_);
- return false;
+ if (used > max_buffer_size_) {
+ std::string error_message = std::format("Dropping event because it couldn't be rendered within %" PRIu64 " bytes.", max_buffer_size_);
+ return nonstd::make_unexpected(error_message);
}
size = used;
- buf.reset((LPWSTR)malloc(size));
- if (!buf) {
- return false;
- }
- if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
- LOG_LAST_ERROR(EvtRender);
- return false;
+ buf.reset((LPWSTR) malloc(size));
Review Comment:
why do we no longer check if `malloc` was successful (i.e., `buf` is non-null)?
##########
extensions/windows-event-log/wel/WindowsEventLog.cpp:
##########
@@ -25,64 +25,67 @@
#include "UnicodeConversion.h"
#include "utils/Deleters.h"
#include "utils/gsl.h"
+#include "UniqueEvtHandle.h"
namespace org::apache::nifi::minifi::wel {
+namespace {
+std::string GetEventTimestampStr(uint64_t event_timestamp) {
+ SYSTEMTIME st;
+ FILETIME ft;
+
+ ft.dwHighDateTime = (DWORD)((event_timestamp >> 32) & 0xFFFFFFFF);
+ ft.dwLowDateTime = (DWORD)(event_timestamp & 0xFFFFFFFF);
Review Comment:
very minor, but `'`s would improve the readability of these numbers: `0xFFFF'FFFF` (or even `0xFF'FF'FF'FF` maybe)
##########
extensions/windows-event-log/wel/WindowsEventLog.h:
##########
@@ -146,20 +147,19 @@ class WindowsEventLogMetadata {
class WindowsEventLogMetadataImpl : public WindowsEventLogMetadata {
public:
- WindowsEventLogMetadataImpl(EVT_HANDLE metadataProvider, EVT_HANDLE event_ptr) : metadata_ptr_(metadataProvider), event_timestamp_(0), event_ptr_(event_ptr) {
+ WindowsEventLogMetadataImpl(EVT_HANDLE metadataProvider, EVT_HANDLE event_ptr) : metadata_ptr_(metadataProvider), event_ptr_(event_ptr) {
renderMetadata();
}
- std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const override;
+ [[nodiscard]] std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const override;
- std::string getEventTimestamp() const override { return event_timestamp_str_; }
+ [[nodiscard]] std::string getEventTimestamp() const override { return event_timestamp_str_; }
short getEventTypeIndex() const override { return event_type_index_; } // NOLINT short comes from WINDOWS API
private:
void renderMetadata();
- uint64_t event_timestamp_;
std::string event_type_;
short event_type_index_; // NOLINT short comes from WINDOWS API
Review Comment:
`event_type_index_` is uninitialized
##########
extensions/windows-event-log/ConsumeWindowsEventLog.h:
##########
@@ -112,18 +109,20 @@ class ConsumeWindowsEventLog : public core::Processor {
void onSchedule(const std::shared_ptr<core::ProcessContext> &context, const std::shared_ptr<core::ProcessSessionFactory> &sessionFactory) override;
void onTrigger(const std::shared_ptr<core::ProcessContext> &context, const std::shared_ptr<core::ProcessSession> &session) override;
- void initialize(void) override;
+ void initialize() override;
void notifyStop() override;
protected:
void refreshTimeZoneData();
void putEventRenderFlowFileToSession(const EventRender& eventRender, core::ProcessSession& session) const;
- wel::WindowsEventLogHandler& getEventLogHandler(const std::string & name);
- bool insertHeaderName(wel::METADATA_NAMES &header, const std::string &key, const std::string &value) const;
- void LogWindowsError(std::string error = "Error") const;
- bool createEventRender(EVT_HANDLE eventHandle, EventRender& eventRender);
+ wel::WindowsEventLogHandler& getEventLogHandler(const std::string& name);
+ static bool insertHeaderName(wel::METADATA_NAMES& header, const std::string& key, const std::string& value);
+ void LogWindowsError(const std::string& error = "Error") const;
+ nonstd::expected<EventRender, std::string> createEventRender(EVT_HANDLE eventHandle);
void substituteXMLPercentageItems(pugi::xml_document& doc);
+ nonstd::expected<std::string, std::string> renderEventAsXml(EVT_HANDLE event_handle);
+
static constexpr const char* XML = "XML";
static constexpr const char* Both = "Both";
static constexpr const char* Plaintext = "Plaintext";
Review Comment:
I think most (all?) of these could be `private`
##########
extensions/windows-event-log/ConsumeWindowsEventLog.cpp:
##########
@@ -537,79 +548,71 @@ void ConsumeWindowsEventLog::substituteXMLPercentageItems(pugi::xml_document& do
doc.traverse(treeWalker);
}
-bool ConsumeWindowsEventLog::createEventRender(EVT_HANDLE hEvent, EventRender& eventRender) {
+nonstd::expected<std::string, std::string> ConsumeWindowsEventLog::renderEventAsXml(EVT_HANDLE event_handle) {
logger_->log_trace("Rendering an event");
WCHAR stackBuffer[4096];
DWORD size = sizeof(stackBuffer);
using Deleter = utils::StackAwareDeleter<WCHAR, utils::FreeDeleter>;
- std::unique_ptr<WCHAR, Deleter> buf{stackBuffer, Deleter{ stackBuffer }};
+ std::unique_ptr<WCHAR, Deleter> buf{stackBuffer, Deleter{stackBuffer}};
DWORD used = 0;
DWORD propertyCount = 0;
- if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
- if (ERROR_INSUFFICIENT_BUFFER != GetLastError()) {
- LOG_LAST_ERROR(EvtRender);
- return false;
+ if (!EvtRender(nullptr, event_handle, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
+ DWORD last_error = GetLastError();
+ if (ERROR_INSUFFICIENT_BUFFER != last_error) {
+ std::string error_message = std::format("EvtRender failed due to %s", utils::OsUtils::getWindowsErrorAsString(last_error));
+ return nonstd::make_unexpected(error_message);
}
- if (used > maxBufferSize_) {
- logger_->log_error("Dropping event because it couldn't be rendered within %" PRIu64 " bytes.", maxBufferSize_);
- return false;
+ if (used > max_buffer_size_) {
+ std::string error_message = std::format("Dropping event because it couldn't be rendered within %" PRIu64 " bytes.", max_buffer_size_);
+ return nonstd::make_unexpected(error_message);
}
size = used;
- buf.reset((LPWSTR)malloc(size));
- if (!buf) {
- return false;
- }
- if (!EvtRender(NULL, hEvent, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
- LOG_LAST_ERROR(EvtRender);
- return false;
+ buf.reset((LPWSTR) malloc(size));
+ if (!EvtRender(nullptr, event_handle, EvtRenderEventXml, size, buf.get(), &used, &propertyCount)) {
+ std::string error_message = std::format("EvtRender failed due to %s", utils::OsUtils::getWindowsErrorAsString(GetLastError()));
+ return nonstd::make_unexpected(error_message);
}
}
+ logger_->log_trace("Event rendered with size %" PRIu32, used);
+ return wel::to_string(buf.get());
+}
- logger_->log_debug("Event rendered with size %" PRIu32 ". Performing doc traversing...", used);
-
- std::string xml = wel::to_string(buf.get());
+nonstd::expected<EventRender, std::string> ConsumeWindowsEventLog::createEventRender(EVT_HANDLE hEvent) {
+ auto event_as_xml = renderEventAsXml(hEvent);
+ if (!event_as_xml)
+ return nonstd::make_unexpected(event_as_xml.error());
pugi::xml_document doc;
- pugi::xml_parse_result result = doc.load_string(xml.c_str());
+ if (!doc.load_string(event_as_xml->c_str()))
+ return nonstd::make_unexpected("Invalid XML produced");
- if (!result) {
- logger_->log_error("Invalid XML produced");
- return false;
- }
+ EventRender result;
// this is a well known path.
- std::string providerName = doc.child("Event").child("System").child("Provider").attribute("Name").value();
- wel::WindowsEventLogMetadataImpl metadata{getEventLogHandler(providerName).getMetadata(), hEvent};
+ std::string provider_name = doc.child("Event").child("System").child("Provider").attribute("Name").value();
+ wel::WindowsEventLogMetadataImpl metadata{getEventLogHandler(provider_name).getMetadata(), hEvent};
wel::MetadataWalker walker{metadata, channel_, !resolve_as_attributes_, apply_identifier_function_, regex_};
// resolve the event metadata
doc.traverse(walker);
- logger_->log_debug("Finish doc traversing, performing writing...");
+ logger_->log_trace("Finish doc traversing, performing writing...");
if (output_.plaintext) {
logger_->log_trace("Writing event in plain text");
- auto& handler = getEventLogHandler(providerName);
- auto message = handler.getEventMessage(hEvent);
+ auto& handler = getEventLogHandler(provider_name);
+ auto event_message = handler.getEventMessage(hEvent);
+
+ std::string_view payload_name = event_message ? "Message" : "Error";
+
+ wel::WindowsEventLogHeader log_header(header_names_, header_delimiter_, payload_name.size());
+ result.plaintext = log_header.getEventHeader([&walker](wel::METADATA metadata) { return walker.getMetadata(metadata); });
+ result.plaintext += payload_name;
+ result.plaintext += log_header.getDelimiterFor(payload_name.size());
+ result.plaintext += event_message.has_value() ? *event_message : utils::OsUtils::getWindowsErrorAsString(event_message.error());
- if (!message.empty()) {
- for (const auto &mapEntry : walker.getIdentifiers()) {
- // replace the identifiers with their translated strings.
- if (mapEntry.first.empty() || mapEntry.second.empty()) {
- continue; // This is most probably a result of a failed ID resolution
- }
- utils::StringUtils::replaceAll(message, mapEntry.first, mapEntry.second);
- }
Review Comment:
why is this no longer needed?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org