From ad1e6843632555c771dda6a9425930fa25b71fb3 Mon Sep 17 00:00:00 2001 From: Konstantin Kostiuk Date: Mon, 16 Dec 2024 17:45:52 +0200 Subject: [PATCH 1/3] qga: Add log to guest-fsfreeze-thaw command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed-by: Daniel P. Berrangé Message-ID: <20241216154552.213961-2-kkostiuk@redhat.com> Signed-off-by: Konstantin Kostiuk --- qga/commands-posix.c | 2 ++ qga/commands-win32.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 6e3c15f539..12bc086d79 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -805,8 +805,10 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp) int ret; ret = qmp_guest_fsfreeze_do_thaw(errp); + if (ret >= 0) { ga_unset_frozen(ga_state); + slog("guest-fsthaw called"); execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp); } else { ret = 0; diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 99c026c0a0..749fdf8895 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1273,6 +1273,9 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp) qga_vss_fsfreeze(&i, false, NULL, errp); ga_unset_frozen(ga_state); + + slog("guest-fsthaw called"); + return i; } From 5b567c21c6d517beeb1087399f733662d7e8ff62 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Tue, 7 Jan 2025 15:52:06 +0100 Subject: [PATCH 2/3] qga: Invert logic on return value in main() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Current logic on return value ('ret' variable) in main() is error prone. The variable is initialized to EXIT_SUCCESS and then set to EXIT_FAILURE on error paths. This makes it very easy to forget to set the variable to indicate error when adding new error path, as is demonstrated by handling of initialize_agent() failure. It's simply lacking setting of the variable. There's just one case where success should be indicated: when dumping the config ('-D' cmd line argument). To resolve this, initialize the variable to failure value and set it explicitly to success value in that one specific case. Signed-off-by: Michal Privoznik Reviewed-by: Ján Tomko Reviewed-by: Konstantin Kostiuk Message-ID: <8a28265f50177a8dc4c10fcf4146e85a7fd748ee.1736261360.git.mprivozn@redhat.com> Signed-off-by: Konstantin Kostiuk --- qga/main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/qga/main.c b/qga/main.c index 531853e13d..eccfa33871 100644 --- a/qga/main.c +++ b/qga/main.c @@ -1579,7 +1579,7 @@ static void stop_agent(GAState *s, bool requested) int main(int argc, char **argv) { - int ret = EXIT_SUCCESS; + int ret = EXIT_FAILURE; GAState *s; GAConfig *config = g_new0(GAConfig, 1); int socket_activation; @@ -1607,7 +1607,6 @@ int main(int argc, char **argv) socket_activation = check_socket_activation(); if (socket_activation > 1) { g_critical("qemu-ga only supports listening on one socket"); - ret = EXIT_FAILURE; goto end; } if (socket_activation) { @@ -1631,7 +1630,6 @@ int main(int argc, char **argv) if (!config->method) { g_critical("unsupported listen fd type"); - ret = EXIT_FAILURE; goto end; } } else if (config->channel_path == NULL) { @@ -1643,13 +1641,13 @@ int main(int argc, char **argv) config->channel_path = g_strdup(QGA_SERIAL_PATH_DEFAULT); } else { g_critical("must specify a path for this channel"); - ret = EXIT_FAILURE; goto end; } } if (config->dumpconf) { config_dump(config); + ret = EXIT_SUCCESS; goto end; } @@ -1664,6 +1662,7 @@ int main(int argc, char **argv) SERVICE_TABLE_ENTRY service_table[] = { { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } }; StartServiceCtrlDispatcher(service_table); + ret = EXIT_SUCCESS; } else { ret = run_agent(s); } From c6f5dd7ac8ef62dcdec4cdeda1467c658161afff Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Tue, 7 Jan 2025 15:52:07 +0100 Subject: [PATCH 3/3] qga: Don't daemonize before channel is initialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the agent is set to daemonize but for whatever reason fails to init the channel, the error message is lost. Worse, the agent daemonizes needlessly and returns success. For instance: # qemu-ga -m virtio-serial \ -p /dev/nonexistent_device \ -f /run/qemu-ga.pid \ -t /run \ -d # echo $? 0 This makes it needlessly hard for init scripts to detect a failure in qemu-ga startup. Though, they shouldn't pass '-d' in the first place. Let's open the channel first and only after that become a daemon. Related bug: https://bugs.gentoo.org/810628 Signed-off-by: Michal Privoznik Reviewed-by: Ján Tomko Reviewed-by: Konstantin Kostiuk Message-ID: <7a42b0cbda5c7e01cf76bc1b29a1210cd018fa78.1736261360.git.mprivozn@redhat.com> Signed-off-by: Konstantin Kostiuk --- qga/main.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/qga/main.c b/qga/main.c index eccfa33871..72c39b042f 100644 --- a/qga/main.c +++ b/qga/main.c @@ -1430,7 +1430,6 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation) if (config->daemonize) { /* delay opening/locking of pidfile till filesystems are unfrozen */ s->deferred_options.pid_filepath = config->pid_filepath; - become_daemon(NULL); } if (config->log_filepath) { /* delay opening the log file till filesystems are unfrozen */ @@ -1438,9 +1437,6 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation) } ga_disable_logging(s); } else { - if (config->daemonize) { - become_daemon(config->pid_filepath); - } if (config->log_filepath) { FILE *log_file = ga_open_logfile(config->log_filepath); if (!log_file) { @@ -1487,6 +1483,20 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation) ga_apply_command_filters(s); + if (!channel_init(s, s->config->method, s->config->channel_path, + s->socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) { + g_critical("failed to initialize guest agent channel"); + return NULL; + } + + if (config->daemonize) { + if (ga_is_frozen(s)) { + become_daemon(NULL); + } else { + become_daemon(config->pid_filepath); + } + } + ga_state = s; return s; } @@ -1513,8 +1523,9 @@ static void cleanup_agent(GAState *s) static int run_agent_once(GAState *s) { - if (!channel_init(s, s->config->method, s->config->channel_path, - s->socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) { + if (!s->channel && + channel_init(s, s->config->method, s->config->channel_path, + s->socket_activation ? FIRST_SOCKET_ACTIVATION_FD : -1)) { g_critical("failed to initialize guest agent channel"); return EXIT_FAILURE; } @@ -1523,6 +1534,7 @@ static int run_agent_once(GAState *s) if (s->channel) { ga_channel_free(s->channel); + s->channel = NULL; } return EXIT_SUCCESS;