[virt-tools-list] [PATCH virt-viewer] win32: use Windows Event Log for glib logging

Fri Jan 25 13:55:06 UTC 2013

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

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

> +
> +win32-messages_rc.$(OBJEXT): win32-messages.rc
> +	$(AM_V_GEN)$(WINDRES) -i $< -o $@

It seems this object is not used, is this rule needed?

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

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

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

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

> +
> +MessageId       = 1
> +SymbolicName    = CATEGORY_DUMMY
> +Severity        = Success
> +Language        = English
> +A dummy category, as a reminder

Is this needed at all?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20130125/5c837687/attachment.sig>