[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



Hi

On Fri, Jan 25, 2013 at 2:55 PM, Christophe Fergeau <cfergeau 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]