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 <[email protected]>
> ---
> 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.
> @@ -56,7 +74,7 @@ int64_t qmp_guest_sync(int64_t id, Error **errp)
>
> void qmp_guest_ping(Error **errp)
> {
> - slog("guest-ping called");
> + slog_trace("guest-ping called");
> }
>
> static void qmp_command_info(const QmpCommand *cmd, void *opaque)
> @@ -285,8 +303,8 @@ static void guest_exec_task_setup(gpointer data)
> * inside the parent, not the child.
> */
> if (dup2(STDOUT_FILENO, STDERR_FILENO) != 0) {
> - slog("dup2() failed to merge stderr into stdout: %s",
> - strerror(errno));
> + slog_error("dup2() failed to merge stderr into stdout: %s",
> + strerror(errno));
> }
> }
>
> @@ -295,8 +313,8 @@ static void guest_exec_task_setup(gpointer data)
> sigact.sa_handler = SIG_DFL;
>
> if (sigaction(SIGPIPE, &sigact, NULL) != 0) {
> - slog("sigaction() failed to reset child process's SIGPIPE: %s",
> - strerror(errno));
> + slog_error("sigaction() failed to reset child process's SIGPIPE: %s",
> + strerror(errno));
> }
> #endif
> }
> @@ -626,7 +644,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool
> has_count,
>
> read_data = guest_file_read_unsafe(gfh, count, errp);
> if (!read_data) {
> - slog("guest-file-write failed, handle: %" PRId64, handle);
> + slog_error("guest-file-write failed, handle: %" PRId64, handle);
> }
>
> return read_data;
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index d9f3922adf..7c50e43dfc 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -41,6 +41,8 @@ bool ga_logging_enabled(GAState *s);
> void ga_disable_logging(GAState *s);
> void ga_enable_logging(GAState *s);
> void G_GNUC_PRINTF(1, 2) slog(const gchar *fmt, ...);
> +void G_GNUC_PRINTF(1, 2) slog_error(const gchar *fmt, ...);
> +void G_GNUC_PRINTF(1, 2) slog_trace(const gchar *fmt, ...);
> void ga_set_response_delimited(GAState *s);
> bool ga_is_frozen(GAState *s);
> void ga_set_frozen(GAState *s);
> diff --git a/qga/main.c b/qga/main.c
> index fd19c7037d..bc786593fd 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -392,6 +392,9 @@ static void ga_log(const gchar *domain, GLogLevelFlags
> level,
>
> level &= G_LOG_LEVEL_MASK;
> if (g_strcmp0(domain, "syslog") == 0) {
> + if (!(level & s->log_level)) {
> + return;
> + }
> #ifndef _WIN32
> syslog(glib_log_level_to_system(level), "%s: %s", level_str, msg);
> #else
> @@ -1673,7 +1676,8 @@ int main(int argc, char **argv)
> GAConfig *config = g_new0(GAConfig, 1);
> int socket_activation;
>
> - config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> + config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL
> + | G_LOG_LEVEL_WARNING | G_LOG_LEVEL_MESSAGE;
>
> qemu_init_exec_dir(argv[0]);
> qga_qmp_init_marshal(&ga_commands);
> --
> 2.51.0
>
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 :|
