[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1434: MINIFICPP-1949 ConsumeWindowsEventLog precompiled regex

[GitBox <gi...@apache.org>]

szaszm commented on code in PR #1434:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1434#discussion_r1052248325


##########
extensions/windows-event-log/wel/MetadataWalker.h:
##########
@@ -95,16 +90,12 @@ class MetadataWalker : public pugi::xml_tree_walker {
 
   const WindowsEventLogMetadata& windows_event_log_metadata_;
   std::string log_name_;
-  std::optional<utils::Regex> regex_;
+  const std::optional<utils::Regex>& regex_;

Review Comment:
   I feel like a nullable raw pointer to regex is similar enough that it probably fulfills the requirements, but it's a simpler representation, so I would rather go with that if it works. But only if this class doesn't need to react to concurrent changes of the regex data member.



##########
libminifi/src/utils/OsUtils.cpp:
##########
@@ -284,6 +284,22 @@ int64_t OsUtils::getTotalPagingFileSize() {
   DWORDLONG total_paging_file_size = memory_info.ullTotalPageFile;
   return total_paging_file_size;
 }
+
+std::string OsUtils::getWindowsErrorAsString(DWORD error_code) {
+  if (error_code == 0)
+    return "";
+
+  LPSTR message_buffer = nullptr;
+
+  size_t size = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
+                               nullptr, error_code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                               (LPSTR)&message_buffer, 0, nullptr);
+
+  std::string message(message_buffer, size);
+
+  LocalFree(message_buffer);
+  return message;
+}

Review Comment:
   It's much simpler and safer to use std::error_code and related facilities to convert windows-specific error codes to std::error_code, then use .message() (returns a string) instead of FormatMessage. The narrowing conversion from DWORD to int is the only unsafe step, but all windows error codes fit in both, so it's fine. The codes go from 0 to 16k. https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes
   
   ```suggestion
   std::string OsUtils::getWindowsErrorAsString(DWORD error_code) {
     return std::error_code{gsl::narrow<int>(error_code), std::system_category()}.message();
   }
   ```
   



##########
extensions/windows-event-log/wel/WindowsEventLog.cpp:
##########
@@ -158,51 +159,59 @@ std::string WindowsEventLogMetadataImpl::getEventData(EVT_FORMAT_MESSAGE_FLAGS f
   return event_data;
 }
 
-std::string WindowsEventLogHandler::getEventMessage(EVT_HANDLE eventHandle) const {
+nonstd::expected<std::string, DWORD> WindowsEventLogHandler::getEventMessage(EVT_HANDLE eventHandle) const {

Review Comment:
   I suggest using `std::error_code` as the unexpected type.



##########
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) & 0xFF'FF'FF'FF);
+  ft.dwLowDateTime = (DWORD)(event_timestamp & 0xFF'FF'FF'FF);
+
+  FileTimeToSystemTime(&ft, &st);
+  std::stringstream datestr;
+
+  std::string period = "AM";
+  auto hour = st.wHour;
+  if (hour >= 12 && hour < 24)
+    period = "PM";
+  if (hour > 12)
+    hour -= 12;
+  if (hour == 0)
+    hour = 12;
+  datestr << st.wMonth << "/" << st.wDay << "/" << st.wYear << " " << std::setfill('0') << std::setw(2) << hour << ":" << std::setfill('0')
+          << std::setw(2) << st.wMinute << ":" << std::setfill('0') << std::setw(2) << st.wSecond << " " << period;
+  return datestr.str();
+}
+}  // namespace

Review Comment:
   Is there any way to convert the timestamp using standard date/time formatting facilities, or date.h? Because these are complex problems with existing solutions, and I can't confidently say that this covers all edge cases. (endianness, timezones, DST and so on)



-- 
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