GNOME Bugzilla – Bug 692979
win32: add log handler to Windows event log
Last modified: 2018-05-24 15:00:24 UTC
(see last patch for more details)
Created attachment 234944 [details] [review] gmessages: seperate the default handler message formatting Split the default handler code in a new format_message() function, that can be reused by other handlers. That way they can share the same output format.
Created attachment 234945 [details] [review] win32: build a simple glib message table The message table is used to provide text and translations. It is not strictly required, but tools such as Event Viewer will warn if they don't find it. Note that GLib messages can't use much of the categories & events and those are little stub to get rid of a few warnings.
Created attachment 234946 [details] [review] win32: add simple log helpers to report Windows Events Windows Event Log provides a centralized repository for events which is, in general, always enabled and stored on disk. Administrator friendly tools, such as Event Viewer, enables reading and searching for events. In short, it is similar to syslog. Given that most applications and services using GLib may already report important messages via g_log, it would help a bunch to provide optionnal helpers to log to Windows Event by default or of certain domains / levels, hence the proposed Windows-specific functions g_win32_log_set_event_handler() and g_win32_log_as_events(). Several applications and services running on Windows would really benefit logging in production, without having to resort writting similar handlers in each projects. Note that ETW is more suited for debugging/tracing. It is also slightly more complex to use and thus to difficult to fit with glog, has two flavours (before and after Vista) and doesn't bring advantages over Event Log to do user/admin level reporting. In fact it seems Event Log is built on top of ETW nowadays. See also FAQ: Common Questions for ETW and Windows Event Log http://social.msdn.microsoft.com/Forums/en-US/etw/thread/a1aa1350-41a0-4490-9ae3-9b4520aeb9d4/
Review of attachment 234944 [details] [review]: Seems like two changes here. Did the first one (the documentation fix) sneak in accidentally?
Review of attachment 234945 [details] [review]: ::: glib/Makefile.am @@ +407,3 @@ +GMESSAGES_WIN32 = gmessages-win32.h gmessages-win32.rc gmessages-win32_*.bin +$(GMESSAGES_WIN32): gmessages-win32.mc Is it normal (or standard) to use wildcards as a make target? ::: glib/gmessages-win32.mc @@ +40,3 @@ +SymbolicName = CATEGORY_GLIB +Language = English +GLib There's got to be a better description we can put here, but can't think of one :S @@ +50,3 @@ +MessageId = 0x100 +Severity = Success +Facility = System It seems odd to have these messages go to the System eventlog. Most Glib applications would not be considered part of the system.
Review of attachment 234946 [details] [review]: This seems like a worthwhile feature to have in glib. It's the sort of thing that will make glib based services (like telepathy and so on) better citizens on that platform. However it does seem like half a solution, if the caller has to call Win32 API functions to get an event log handle with then pass it into these functions. I do agree with the approach of not having this on by default. Just like we don't log to syslog by default, you don't want every application spewing stuff into the windows eventlog. For anyone else following along, the limited set of log_type codes is a consequence of how ReportMessage makes you precompile all your various messages and codes. Often open source projects take the approach of having just a few codes, and cramming text into a %1. So I think that's fine here too. ::: glib/gmessages.c @@ +1580,3 @@ + DWORD log_type, event_id; + gchar *string; + const gchar *message, Why not use gunichar2? wchar_t varies between platforms. Even if in G_OS_WIN32 it is guaranteed to be 16 bits, its use here is confusing. @@ +1612,3 @@ + /* Prior to Windows Vista: Each string is limited to 32K characters, + let's chop the string even before that (here in bytes) */ + { The limit post-vista is: Each string is limited to 31,839 characters http://msdn.microsoft.com/en-us/library/windows/desktop/aa363679%28v=vs.85%29.aspx Also can this fail if 32767 falls on a non-utf8 char boundary?
Review of attachment 234946 [details] [review]: ::: glib/gmessages.c @@ +1580,3 @@ + DWORD log_type, event_id; + gchar *string; + wchar_t *wc_string; I used the same type as 95% of GLib code using g_utf8_to_utf16(), perhaps it maps better to the expected type of fooW() functions? @@ +1612,3 @@ + /* Prior to Windows Vista: Each string is limited to 32K characters, + let's chop the string even before that (here in bytes) */ + wc_string = g_utf8_to_utf16 (string, 32767, NULL, NULL, NULL); It can, since g_utf8_to_utf16() len argument is in bytes. It seems that this should be handled without problem, by grepping and looking around "partial utf8 character" in glib.
Review of attachment 234945 [details] [review]: ::: glib/Makefile.am @@ +407,3 @@ +GMESSAGES_WIN32 = gmessages-win32.h gmessages-win32.rc gmessages-win32_*.bin +$(GMESSAGES_WIN32): gmessages-win32.mc Right, moved to CLEANFILES. ::: glib/gmessages-win32.mc @@ +50,3 @@ +MessageId = 0x100 +Severity = Success +Facility = System It is either System or Application (supported by mingw binutils at least). Would Application be more appropriate?
(In reply to comment #6) > However it does seem like half a solution, if the caller has to call Win32 API > functions to get an event log handle with then pass it into these functions. It is not more complicated than: #ifdef G_OS_WIN32 g_win32_log_as_events (RegisterEventSource (NULL, "MyService")); #endif Since the event source can be on a remote server, and the source name has certain limitations, I think it is worth to leave that to the caller to figure out. It's Win32-specific code after all, so user of this function shouldn't mind much about digging into windows-only documentation to register it appropriately while giving more flexibility (a given specific handler could be required to use in some larger framework for example)
Created attachment 235274 [details] [review] gmessages: seperate the default handler message formatting Split the default handler code in a new format_message() function, that can be reused by other handlers. That way they can share the same output format.
Created attachment 235275 [details] [review] g_log_set_handler() can return 0 on error
Created attachment 235276 [details] [review] win32: build a simple glib message table The message table is used to provide text and translations. It is not strictly required, but tools such as Event Viewer will warn if they don't find it. Note that GLib messages can't use much of the categories & events and those are little stub to get rid of a few warnings.
Created attachment 235277 [details] [review] win32: add simple log helpers to report Windows Events Windows Event Log provides a centralized repository for events which is, in general, always enabled and stored on disk. Administrator friendly tools, such as Event Viewer, enables reading and searching for events. In short, it is similar to syslog. Given that most applications and services using GLib may already report important messages via g_log, it would help a bunch to provide optionnal helpers to log to Windows Event by default or of certain domains / levels, hence the proposed Windows-specific functions g_win32_log_set_event_handler() and g_win32_log_as_events(). Several applications and services running on Windows would really benefit logging in production, without having to resort writting similar handlers in each projects. Note that ETW is more suited for debugging/tracing. It is also slightly more complex to use and thus to difficult to fit with glog, has two flavours (before and after Vista) and doesn't bring advantages over Event Log to do user/admin level reporting. In fact it seems Event Log is built on top of ETW nowadays. See also FAQ: Common Questions for ETW and Windows Event Log http://social.msdn.microsoft.com/Forums/en-US/etw/thread/a1aa1350-41a0-4490-9ae3-9b4520aeb9d4/
(In reply to comment #8) > ::: glib/gmessages-win32.mc > @@ +50,3 @@ > +MessageId = 0x100 > +Severity = Success > +Facility = System > > It is either System or Application (supported by mingw binutils at least). > Would Application be more appropriate? Yes, Application is more appropriate, I think. (In reply to comment #9) > (In reply to comment #6) > > However it does seem like half a solution, if the caller has to call Win32 API > > functions to get an event log handle with then pass it into these functions. > > It is not more complicated than: > > #ifdef G_OS_WIN32 > g_win32_log_as_events (RegisterEventSource (NULL, "MyService")); > #endif > > Since the event source can be on a remote server, and the source name has > certain limitations, I think it is worth to leave that to the caller to figure > out. It's Win32-specific code after all, so user of this function shouldn't > mind much about digging into windows-only documentation to register it > appropriately while giving more flexibility (a given specific handler could be > required to use in some larger framework for example) Here's what I would suggest: * Make log_handle the last argument. * Make it (allow-none) ie: optional * If NULL, register an event source for the application using g_get_application_name() That way it it 'just works' without special understanding of the Win32 API for most callers, but callers with special needs can pass their own event source.
oops, I missed your last comments: (In reply to comment #14) > (In reply to comment #8) > > ::: glib/gmessages-win32.mc > > @@ +50,3 @@ > > +MessageId = 0x100 > > +Severity = Success > > +Facility = System > > > > It is either System or Application (supported by mingw binutils at least). > > Would Application be more appropriate? > > Yes, Application is more appropriate, I think. ok, already changed in last attached patch. > (In reply to comment #9) > > (In reply to comment #6) > > #ifdef G_OS_WIN32 > > g_win32_log_as_events (RegisterEventSource (NULL, "MyService")); > > #endif > > > > Since the event source can be on a remote server, and the source name has > > certain limitations, I think it is worth to leave that to the caller to figure > > out. It's Win32-specific code after all, so user of this function shouldn't > > mind much about digging into windows-only documentation to register it > > appropriately while giving more flexibility (a given specific handler could be > > required to use in some larger framework for example) > Here's what I would suggest: > > * Make log_handle the last argument. > * Make it (allow-none) ie: optional > * If NULL, register an event source for the application using > g_get_application_name() > > That way it it 'just works' without special understanding of the Win32 API for > most callers, but callers with special needs can pass their own event source. Sadly, it won't just work. First, g_get_application_name() would have to be a pre-requisite, and more importantly, as I said before, source name has some limitations. We can try to register on user behalf, but it's not guarateed to work. I will update the patch as you suggest, since it should work in most cases hopefully.
Created attachment 236129 [details] [review] win32: add simple log helpers to report Windows Events Windows Event Log provides a centralized repository for events which is, in general, always enabled and stored on disk. Administrator friendly tools, such as Event Viewer, enables reading and searching for events. In short, it is similar to syslog. Given that most applications and services using GLib may already report important messages via g_log, it would help a bunch to provide optionnal helpers to log to Windows Event by default or of certain domains / levels, hence the proposed Windows-specific functions g_win32_log_set_event_handler() and g_win32_log_as_events(). Several applications and services running on Windows would really benefit logging in production, without having to resort writting similar handlers in each projects. Note that ETW is more suited for debugging/tracing. It is also slightly more complex to use and thus to difficult to fit with glog, has two flavours (before and after Vista) and doesn't bring advantages over Event Log to do user/admin level reporting. In fact it seems Event Log is built on top of ETW nowadays. See also FAQ: Common Questions for ETW and Windows Event Log http://social.msdn.microsoft.com/Forums/en-US/etw/thread/a1aa1350-41a0-4490-9ae3-9b4520aeb9d4/
Looks fine in general, but why do we need two functions (and two log handler implementations) for this ? Do we really need the convenience function ? And if we do, shouldn't it just be a wrapper around the full function ?
(In reply to comment #17) > Looks fine in general, but why do we need two functions (and two log handler > implementations) for this ? Do we really need the convenience function ? And if > we do, shouldn't it just be a wrapper around the full function ? There is two "internal" log handler: win32_default_event_log_handler calling win32_event_log_handler, because the default log handler must filter by log level, to log only warnings and above. It is the same logic as the current GLib log handlers. Also win32_event_log_handler needs user_data to be the log handle, which we now create and set on behalf of the user. We could expose the handlers themself instead of g_win32_log_as_events() & g_win32_log_set_event_handler(), but that makes it not possible to verify or create the win32 log handle. I don't see clear advantage of exposing the log handlers themself, only some drawbacks.
I think the win32 log handlers are fine. But maybe g_win32_log_as_events would be better off as a cross-platform g_log_to_system() which would do what g_win32_log_as_events does for Windows, and log syslog/journal on Linux ?
Review of attachment 235275 [details] [review]: Not really true; g_log_set_handler[_full]() only returns 0 if its preconditions fail, and that’s something which (by convention) we don’t document in GLib (because it’s a programmer error rather than a runtime error).
Review of attachment 235274 [details] [review]: This is basically equivalent to g_log_writer_format_fields() in the new log handling code.
Review of attachment 236129 [details] [review]: This would be good to rework in the form of a (public) GLogWriterFunc to integrate it with the new logging API.
Review of attachment 235276 [details] [review]: Is this still relevant if attachment #236129 [details] is significantly reworked as a GLogWriterFunc? (Marking as needs-work on the assumption that it is.)
Since 2013 we’ve gained a new logging API (see GLogWriterFunc) which should (hopefully) allow integration with the Windows Event Log a lot more naturally. The Windows APIs for this may also have moved on in that time too.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/665.