[virt-tools-list] [PATCH virt-viewer] win32: use Windows Event Log for glib logging
Marc-André Lureau
marcandre.lureau at gmail.com
Fri Jan 25 15:58:02 UTC 2013
Hi
On Fri, Jan 25, 2013 at 2:55 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> Hey,
>
> On Wed, Jan 23, 2013 at 12:05:29AM +0100, Marc-André Lureau wrote:
>> Windows has a proper service for logging events and messages in a
>> structured way. It does many nice things, and "Event Viewer" allows
>> UI browsing / filtering of messages etc..
>>
>> Note we don't really use any category or event ID but solely log level
>> and string. To make the Event Viewer happy, we still register a string
>> for our event. And MinGW doesn't seem to like linking to multiple
>> resource objects (apparently takes the first one and ignores the
>> rest?)
>>
>> The installer should add a registry key for the event source, however
>> it's not possible as a user, and the NSIS script is kinda user only
>> atm... The MSI will support those entries:
>> http://msdn.microsoft.com/en-us/library/windows/desktop/aa363661%28v=vs.85%29.aspx
>>
>> HKLM\SYSTEM\CurrentControlSet\services\eventlog\Application\VirtViewer
>> EventMessage $prefix\bin\remote-viewer.exe
>>
>> It's a minor annoyance if those entries are not in the registry
>> (basically, Event Viewer will complain a little, and it will be
>> impossible? to do create application filters)
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=895919
>> ---
>> configure.ac | 6 ++++++
>> src/Makefile.am | 23 +++++++++++++++++------
>> src/virt-viewer-util.c | 38 ++++++++++++++++++++++++++++++++++++++
>> src/win32-messages.mc | 36 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 97 insertions(+), 6 deletions(-)
>> create mode 100644 src/win32-messages.mc
>>
>> diff --git a/configure.ac b/configure.ac
>> index 339acbe..8419e85 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -46,6 +46,12 @@ AS_IF([test "x$os_win32" = "xyes"], [
>> if test -z "$WINDRES" ; then
>> AC_MSG_ERROR("windres is required to compile virt-viewer on this platform")
>> fi
>> +
>> + AC_CHECK_TOOL(WINDMC, [windmc])
>> +
>> + if test -z "$WINDMC" ; then
>> + AC_MSG_ERROR("windmc is required to compile virt-viewer on this platform")
>> + fi
>> ])
>>
>> AC_CONFIG_LIBOBJ_DIR([src])
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 05e20b2..d5e98b1 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -1,5 +1,6 @@
>> NULL =
>> LDADD =
>> +CLEANFILES =
>> MAINTAINERCLEANFILES =
>
> Setting MAINTAINERCLEANFILES is no longer needed here
ok
>> bin_PROGRAMS =
>>
>> @@ -141,23 +142,33 @@ desktop_DATA = remote-viewer.desktop
>>
>> EXTRA_DIST += $(desktop_DATA)
>>
>> -VIRT_VIEWER_RES = virt-viewer.rc virt-viewer.manifest
>> -ICONDIR = $(top_builddir)/icons
>> -MANIFESTDIR = $(srcdir)
>> -EXTRA_DIST += $(VIRT_VIEWER_RES)
>>
>> if OS_WIN32
>> bin_PROGRAMS += windows-cmdline-wrapper
>> windows_cmdline_wrapper_SOURCES = windows-cmdline-wrapper.c
>> windows_cmdline_wrapper_LDFLAGS = -lpsapi
>>
>> -virt-viewer_rc.$(OBJEXT): $(VIRT_VIEWER_RES) $(ICONDIR)/virt-viewer.ico
>> +VIRT_VIEWER_RES = virt-viewer.rc virt-viewer.manifest
>> +EXTRA_DIST += $(VIRT_VIEWER_RES) win32-messages.mc
>> +
>> +ICONDIR = $(top_builddir)/icons
>> +MANIFESTDIR = $(srcdir)
>> +
>> +win32-messages.rc: win32-messages.mc
>> + $(AM_V_GEN)$(WINDMC) $<
>
> As I understand it, this will generate win32-message.h which is then used
> in virt-viewer-util.c, do we need to encode this dependency somehow?
Yeah, I am updating Makefile.am to use BUILT_SOURCES instead, which we have in
>
>> +
>> +win32-messages_rc.$(OBJEXT): win32-messages.rc
>> + $(AM_V_GEN)$(WINDRES) -i $< -o $@
>
> It seems this object is not used, is this rule needed?
no, removed thanks
>
>> +
>> +virt-viewer_rc.$(OBJEXT): $(VIRT_VIEWER_RES) win32-messages.rc $(ICONDIR)/virt-viewer.ico
>> $(AM_V_GEN)$(WINDRES) \
>> -DICONDIR='\"$(ICONDIR)\"' \
>> -DMANIFESTDIR='\"$(MANIFESTDIR)\"' \
>> -i $< -o $@
>> +
>> LDADD += virt-viewer_rc.$(OBJEXT)
>> -MAINTAINERCLEANFILES += virt-viewer_rc.$(OBJEXT)
>> +CLEANFILES += virt-viewer_rc.$(OBJEXT)
>> +CLEANFILES += win32-messages.rc Messages_*.bin
>>
>> bin_PROGRAMS += debug-helper
>> debug_helper_SOURCES = debug-helper.c
>> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c
>> index 48a6978..7ee5e63 100644
>> --- a/src/virt-viewer-util.c
>> +++ b/src/virt-viewer-util.c
>> @@ -30,6 +30,7 @@
>> #ifdef G_OS_WIN32
>> #include <windows.h>
>> #include <io.h>
>> +#include "win32-messages.h"
>
> Where is this file coming from? Is this autogenerated or did you forget a
> git add ?
autogenerated, updating Makefile.am for deps
>
>> #endif
>>
>> #include <sys/types.h>
>> @@ -261,9 +262,46 @@ gulong virt_viewer_signal_connect_object(gpointer instance,
>> return ctx->handler_id;
>> }
>>
>> +#ifdef G_OS_WIN32
>> +static HANDLE ms_eventlog = NULL;
>> +
>> +static void virt_viewer_log_handler (const gchar *log_domain,
>> + GLogLevelFlags log_level,
>> + const gchar *message,
>> + G_GNUC_UNUSED gpointer user_data)
>> +{
>> + WORD logtype;
>> +
>> + switch (log_level) {
>> + case G_LOG_LEVEL_ERROR:
>> + case G_LOG_LEVEL_CRITICAL:
>> + logtype = EVENTLOG_ERROR_TYPE;
>> + break;
>> + case G_LOG_LEVEL_WARNING:
>> + logtype = EVENTLOG_WARNING_TYPE;
>> + break;
>> + default:
>> + logtype = EVENTLOG_INFORMATION_TYPE;
>> + }
>> +
>> + gchar *msg = g_strdup_printf("%s: %s", log_domain, message);
>> + ReportEventA(ms_eventlog, logtype, 0, EVENT_GLOG, NULL, 1, 0,
>> + (const char **)&msg, NULL);
>> + g_free(msg);
>> +}
>> +#endif
>> +
>> void virt_viewer_util_init(const char *appname)
>> {
>> #ifdef G_OS_WIN32
>> + ms_eventlog = RegisterEventSourceA(NULL, "VirtViewer");
>
> Using PACKAGE_NAME here would give us different log domains for virt-viewer
> and remote-viewer, dunno if that's something we want or not.
Perhaps, although the event already has the executable name
associated. We will need to make sure the registry entries are created
/ duplicated..
>
>> + if (ms_eventlog == NULL)
>> + g_printerr("can't open Windows event log\n");
>> + else
>> + g_log_set_default_handler(virt_viewer_log_handler, NULL);
>
> Can you pass ms_eventlog as user_data instead of having it as a global
> variable?
I don't see the point, it is global to the app. Beside, there is no
context given to virt_viewer_util_init()
>> +
>> + g_message(PACKAGE_STRING " started on Windows");
>> +
>> /*
>> * This named mutex will be kept around by Windows until the
>> * process terminates. This allows other instances to check if it
>> diff --git a/src/win32-messages.mc b/src/win32-messages.mc
>> new file mode 100644
>> index 0000000..2c283ca
>> --- /dev/null
>> +++ b/src/win32-messages.mc
>> @@ -0,0 +1,36 @@
>> +;#ifndef __MESSAGES_H__
>> +;#define __MESSAGES_H__
>> +;
>> +
>> +LanguageNames =
>> + (
>> + English = 0x0409:Messages_ENU
>> + )
>> +
>> +
>> +;////////////////////////////////////////
>> +;// Eventlog categories
>> +;//
>> +;// Categories always have to be the first entries in a message file!
>> +;//
>
> Hmm this is contradicted by the LanguageNames entry just above this...
Not really the same thing, so no contradiction imho
>> +
>> +MessageId = 1
>> +SymbolicName = CATEGORY_DUMMY
>> +Severity = Success
>> +Language = English
>> +A dummy category, as a reminder
>
> Is this needed at all?
I am not familiar with that windows messages stuff and I was really
walking on thin ice, so I would really prefer to keep that template,
especailly if there is a big pitfall there.
--
Marc-André Lureau
More information about the virt-tools-list
mailing list