Re: [PATCH v1] qga: rework slog to support multiple severity levels

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Mar 12, 2026 at 05:15:05PM +0200, Kostiantyn Kostiuk wrote:
> On Thu, Mar 12, 2026 at 5:02 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Thu, Mar 12, 2026 at 04:16:57PM +0200, Elizabeth Ashurov wrote:
> > > ga_log() forwarded all "syslog" domain messages unconditionally,
> > > bypassing the log level filter. Furthermore, slog() hardcoded
> > > all messages to G_LOG_LEVEL_INFO, causing log spam from frequent
> > > guest-ping calls.
> > >
> > > - Split slog() into slog_error() (WARNING), slog() (MESSAGE), and
> > slog_trace() (INFO)
> > > - Apply s->log_level filtering to "syslog" in ga_log()
> > > - Move guest-ping to slog_trace()
> > > - Move error messages to slog_error()
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3214
> > >
> > > Signed-off-by: Elizabeth Ashurov <eashurov@redhat.com>
> > > ---
> > >  qga/commands-linux.c   |  6 ++++--
> > >  qga/commands-posix.c   | 13 +++++++------
> > >  qga/commands-win32.c   | 34 +++++++++++++++++-----------------
> > >  qga/commands.c         | 30 ++++++++++++++++++++++++------
> > >  qga/guest-agent-core.h |  2 ++
> > >  qga/main.c             |  6 +++++-
> > >  6 files changed, 59 insertions(+), 32 deletions(-)
> > >
> >
> > > diff --git a/qga/commands.c b/qga/commands.c
> > > index 5f20af25d3..ed52a7d3c2 100644
> > > --- a/qga/commands.c
> > > +++ b/qga/commands.c
> > > @@ -38,6 +38,24 @@ void slog(const gchar *fmt, ...)
> > >  {
> > >      va_list ap;
> > >
> > > +    va_start(ap, fmt);
> > > +    g_logv("syslog", G_LOG_LEVEL_MESSAGE, fmt, ap);
> > > +    va_end(ap);
> > > +}
> > > +
> > > +void slog_error(const gchar *fmt, ...)
> > > +{
> > > +    va_list ap;
> > > +
> > > +    va_start(ap, fmt);
> > > +    g_logv("syslog", G_LOG_LEVEL_WARNING, fmt, ap);
> > > +    va_end(ap);
> > > +}
> > > +
> > > +void slog_trace(const gchar *fmt, ...)
> > > +{
> > > +    va_list ap;
> > > +
> > >      va_start(ap, fmt);
> > >      g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
> > >      va_end(ap);
> >
> > This is really the wrong approach IMHO.
> >
> > It is bad practice for the call sites to be declaring what log
> > output they are targetting.
> >
> > Also the first parameter of g_log call is supposed to be the
> > "domain" which is a name that identicates the *source* of the
> > error message. For code in a library it would be the library
> > name, or a module name within the library. For applications
> > it would usually be the application name.
> >
> > Passing the name of a desired log target "syslog" is really
> > a misuse of the logging framework.
> >
> > IMHO the existing usage of "slog" should be replaced by
> > calls to g_error() (for the cases which are reporting error
> > messages) and g_info() (for the cases which are reporting the
> > names of commands being executed).
> >
> > Then, we should offer better command line control over the
> > usage of logging
> >
> >  "verbose" should enable g_info() or higher
> >  "debug" should enable g_debug() or higher
> >
> > The log file (if configured) should receive *all* messages
> > per the verbose/debug flag settings. (currently it misses
> > any messages that get sent to syslog which is bad as it
> > makes the logfile incomplete.
> >
> > The syslog output should also receive all messages at
> > "info" level of higher (provided the verbose flag is
> > set). It shouldn't get debug messages, even if 'debug'
> > flag is set.
> >
> > We should not specialcase the "ping" command either IMHO,
> > it should remain "info" like the other commands. Per that
> > bug, users can exclude ping messages via a log filter on
> > the systemd service config.
> >
> 
> What to do with Windows? We definitely want to see a freeze/thaw event in
> Event Viewer, but ping is also redundant there.

If we think of the recording of commands as more of an "audit" task,
we could expose an "audit" argument to control this. It would take
a list of command names, optionally with wildcards, and optionally
with negation

   audit = *

    => current default - all commands at "info" level

   audit = "!guest-ping,*"

    => a new default - all commands at "info" level, except ping

   audit = "!guest-ssh-get*,guest-ssh*"

    => a restrictive audit, only commands which modify ssh
       setup

anything not matched by the audit setting, would be logged at
g_debug instead of g_info


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|