Add eventlog_sink for logging to Windows Event Log (local only) #1418
Conversation
ban-dana
force-pushed
the
v1.x
branch
2 times, most recently
from
980ccef
to
4de1720
Compare
include/spdlog/sinks/eventlog_sink.h
Outdated
| // Copyright(c) 2015-present, Gabi Melman & spdlog contributors. | ||
| // Distributed under the MIT License (http://opensource.org/licenses/MIT) | ||
|
|
||
| // Requires the following registry entries to be present, with the following modifications: |
There was a problem hiding this comment.
I saw that the add_registry_info already creates such entry? why is it "Requires the following registry entries to be present" ?
There was a problem hiding this comment.
Registering a source is something that requires elevation (aka "running as administrator") so it's usually done during the setup. However I wanted to give the user an option to do it in code.
include/spdlog/sinks/eventlog_sink.h
Outdated
| explicit eventlog_sink( | ||
| std::string const& source, | ||
| std::string const& log = "Application", | ||
| std::string const& message_file_path = "%windir%\\System32\\mscoree.dll"); |
There was a problem hiding this comment.
It's a part of Microsoft .NET Framework that is installed on pretty much every Windows system out there, and this dll happens to contain the needed resource. I don't think this is going to change any time soon, but since it is still an undocumented hack, I also give a way to do that with a custom built dll if so desired or if Microsoft changes their old dll or whatever.
There was a problem hiding this comment.
This still a bit confusing to me. If we expect the user to set those reg entries beforehand using regedit, then there is no real benefit for all this registry code, and it can be simplified dramatically. Add to this the fact that user must run as admin for it to even work.
I suggest removing the registry code completely and let the user take care of it as written in your top comment (use regedit).
|
|
||
| struct sid | ||
| { | ||
| static SID* duplicate(SID* p_src_sid) |
There was a problem hiding this comment.
Please add some comments It is really hard to understand what is the purpose of those function
| if (!GetTokenInformation(current_process_token.hToken_, TokenUser, (LPVOID) buffer.data(), tusize, &tusize)) | ||
| SPDLOG_THROW(win32_error("GetTokenInformation")); | ||
|
|
||
| return sid::duplicate((SID *) ((TOKEN_USER*) buffer.data())->User.Sid); |
There was a problem hiding this comment.
Do we really need all those complicated sid functions? The "ReportEventA" docs state it can be NULL.
There was a problem hiding this comment.
Right, but then the user information won't be available in the event viewer, which may sometimes be useful as the event log is a global facility of multi-user environment.
| } | ||
|
|
||
| /** Reports a message to stderr */ | ||
| static void report(std::string const& message) SPDLOG_NOEXCEPT |
adressing issue #989