Re: [PATCH 1/2] qga-win: add logging to Windows event log

[Andrey Drobyshev]

On 11/29/22 00:36, M M wrote:
> 
> 
>> On 28. 11. 2022., at 21:54, Andrey Drobyshev via <qemu-devel@nongnu.org> 
>> wrote:
>>
>> This commit allows QGA to write to Windows event log using Win32 API's
>> ReportEvent() [1], much like syslog() under *nix guests.
>>
>> In order to generate log message definitions we use a very basic message
>> text file [2], so that every QGA's message gets ID 1.  The tools
>> "windmc" and "windres" respectively are used to generate ".rc" file and
>> COFF object file, and then the COFF file is linked into qemu-ga.exe.
>>
>> [1] 
>> https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-reporteventa
>> [2] 
>> https://learn.microsoft.com/en-us/windows/win32/eventlog/message-text-files
>>
>> Originally-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> configure                 |  3 +++
>> qga/installer/qemu-ga.wxs |  5 +++++
>> qga/main.c                | 15 ++++++++++++---
>> qga/meson.build           | 19 ++++++++++++++++++-
>> qga/messages-win32.mc     |  9 +++++++++
>> 5 files changed, 47 insertions(+), 4 deletions(-)
>> create mode 100644 qga/messages-win32.mc
>>
>> diff --git a/configure b/configure
>> index 26c7bc5154..789a4f6cc9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -372,6 +372,7 @@ smbd="$SMBD"
>> strip="${STRIP-${cross_prefix}strip}"
>> widl="${WIDL-${cross_prefix}widl}"
>> windres="${WINDRES-${cross_prefix}windres}"
>> +windmc="${WINDMC-${cross_prefix}windmc}"
>> pkg_config_exe="${PKG_CONFIG-${cross_prefix}pkg-config}"
>> query_pkg_config() {
>>     "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"
>> @@ -2561,6 +2562,7 @@ if test "$skip_meson" = no; then
>>   echo "strip = [$(meson_quote $strip)]" >> $cross
>>   echo "widl = [$(meson_quote $widl)]" >> $cross
>>   echo "windres = [$(meson_quote $windres)]" >> $cross
>> +  echo "windmc = [$(meson_quote $windmc)]" >> $cross
>>   if test "$cross_compile" = "yes"; then
>>     cross_arg="--cross-file config-meson.cross"
>>     echo "[host_machine]" >> $cross
>> @@ -2667,6 +2669,7 @@ preserve_env SMBD
>> preserve_env STRIP
>> preserve_env WIDL
>> preserve_env WINDRES
>> +preserve_env WINDMC
>>
>> printf "exec" >>config.status
>> for i in "$0" "$@"; do
>> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
>> index 73ce2c4965..d9567836f3 100644
>> --- a/qga/installer/qemu-ga.wxs
>> +++ b/qga/installer/qemu-ga.wxs
>> @@ -110,6 +110,11 @@
>>               <RegistryValue Type="string" Name="ProductID" 
>> Value="fb0a0d66-c7fb-4e2e-a16b-c4a3bfe8d13b" />
>>               <RegistryValue Type="string" Name="Version" 
>> Value="$(var.QEMU_GA_VERSION)" />
>>             </RegistryKey>
>> +            <RegistryKey Root="HKLM"
>> +                         
>> Key="System\CurrentControlSet\Services\EventLog\Application\qemu-ga">
>> +              <RegistryValue Type="integer" Name="TypesSupported" Value="7" 
>> />
>> +              <RegistryValue Type="string" Name="EventMessageFile" 
>> Value="[qemu_ga_directory]qemu-ga.exe" />
>> +            </RegistryKey>
>>           </Component>
>>         </Directory>
>>       </Directory>
>> diff --git a/qga/main.c b/qga/main.c
>> index b3580508fa..10314dfe5d 100644
>> --- a/qga/main.c
>> +++ b/qga/main.c
>> @@ -82,6 +82,7 @@ struct GAState {
>>     bool logging_enabled;
>> #ifdef _WIN32
>>     GAService service;
>> +    HANDLE event_log;
> Just for the sake of uniformity
> shouldn't we close this handle at cleanup_agent() similar to wakeup_event ?

Sure thing! Thanks for your note, will include that in v2.

> 
>>     HANDLE wakeup_event;
>> #endif
>>     bool delimit_response;
>> @@ -324,13 +325,14 @@ static void ga_log(const gchar *domain, GLogLevelFlags 
>> level,
>>     }
>>
>>     level &= G_LOG_LEVEL_MASK;
>> -#ifndef _WIN32
>>     if (g_strcmp0(domain, "syslog") == 0) {
>> +#ifndef _WIN32
>>         syslog(LOG_INFO, "%s: %s", level_str, msg);
>> -    } else if (level & s->log_level) {
>> #else
>> -    if (level & s->log_level) {
>> +        ReportEvent(s->event_log, EVENTLOG_INFORMATION_TYPE,
>> +                    0, 1, NULL, 1, 0, &msg, NULL);
>> #endif
>> +    } else if (level & s->log_level) {
>>         g_autoptr(GDateTime) now = g_date_time_new_now_utc();
>>         g_autofree char *nowstr = g_date_time_format(now, "%s.%f");
>>         fprintf(s->log_file, "%s: %s: %s\n", nowstr, level_str, msg);
>> @@ -1286,6 +1288,13 @@ static GAState *initialize_agent(GAConfig *config, 
>> int socket_activation)
>>     g_debug("Guest agent version %s started", QEMU_FULL_VERSION);
>>
>> #ifdef _WIN32
>> +    s->event_log = RegisterEventSource(NULL, "qemu-ga");
>> +    if (!s->event_log) {
>> +        g_autofree gchar *errmsg = g_win32_error_message(GetLastError());
>> +        g_critical("unable to register event source: %s", errmsg);
>> +        return NULL;
>> +    }
>> +
>>     /* On win32 the state directory is application specific (be it the 
>> default
>>      * or a user override). We got past the command line parsing; let's 
>> create
>>      * the directory (with any intermediate directories). If we run into an
>