Hi On Fri, Jan 25, 2013 at 2:55 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
