On Thu, Mar 12, 2026 at 5:02 PM Daniel P. Berrangé <[email protected]> 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 <[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. > What to do with Windows? We definitely want to see a freeze/thaw event in Event Viewer, but ping is also redundant there. > > > > > @@ -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 :| > >
