| Message ID | 7a42b0cbda5c7e01cf76bc1b29a1210cd018fa78.1736261360.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: Open channel before going daemon | expand |
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com> On Tue, Jan 7, 2025 at 4:52 PM Michal Privoznik <mprivozn@redhat.com> wrote: > 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 <mprivozn@redhat.com> > Reviewed-by: Ján Tomko <jtomko@redhat.com> > --- > qga/main.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/qga/main.c b/qga/main.c > index 68ea7f275a..35f061b5ea 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; > -- > 2.45.2 > >
diff --git a/qga/main.c b/qga/main.c index 68ea7f275a..35f061b5ea 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;
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 <mprivozn@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> --- qga/main.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)