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

[Elizabeth Ashurov <eashurov@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5211 bytes --]

Hi,

Thanks for the review and suggestions.
I updated the patch based on the discussion and sent v2.

Best regards,

On Thu, Mar 12, 2026 at 5:36 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

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

[-- Attachment #2: Type: text/html, Size: 7789 bytes --]