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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
