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

[Christophe Fergeau]

On Fri, Jan 25, 2013 at 04:58:02PM +0100, Marc-André Lureau wrote:
> 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

Oh, forgot to ask about this registry path in the commit log, is it a bad
c&p?

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

Is the '?' intentional?

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

Sorry, I realized that afterwards, and forgot to remove that comment.

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

Ah ok, if there's already a way to differentiate between virtviewer and
remoteviewer, and since that makes more work, I'm fine with keeping things
as is.

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

It feels cleaner to me to avoid having global variables at all.

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

Ok, good for me if it has no side-effects that you noticed.

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/b26003f8/attachment.sig>