[3/4] qga: Don't daemonize before channel is initialized

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>
---
 qga/main.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

On Mon, Nov 4, 2024 at 11:54 AM 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>
> ---
>  qga/main.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index c003aacbe0..6240845f39 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,12 +1523,6 @@ 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)) {
> -        g_critical("failed to initialize guest agent channel");
> -        return EXIT_FAILURE;
> -    }
> -


The old flow:
run_agent call run_agent_once in loop
run_agent_once initialize channel for every run

after your changes
you expect to initialize the channel only once
this can cause issues on Windows during driver update
the default configuration on Windows is QGA + VirtioSerial and in CLI
--retry-path
during driver update (that can happen via Windows Update) the channel will
be closed
so QGA must reinitialize the channel

Theoretically, the same can happen on Linux with a UNIX socket



>      g_main_loop_run(s->main_loop);
>
>      if (s->channel) {
> --
> 2.45.2
>
>

On 12/4/24 10:44, Konstantin Kostiuk wrote:

> 
> The old flow:
> run_agent call run_agent_once in loop
> run_agent_once initialize channel for every run
> 
> after your changes
> you expect to initialize the channel only once
> this can cause issues on Windows during driver update
> the default configuration on Windows is QGA + VirtioSerial and in CLI --
> retry-path
> during driver update (that can happen via Windows Update) the channel
> will be closed
> so QGA must reinitialize the channel

Ah, I had no idea. Alright, so what I can do is:

1) keep channel_init() in initialize_agent() and become_daemon() after
that, and
2) make run_agent_once() call channel_init() if s->channel is NULL (and
also set it to NULL ...

> 
> Theoretically, the same can happen on Linux with a UNIX socket
> 
>  
> 
>          g_main_loop_run(s->main_loop);
> 
>          if (s->channel) {

... here.

V2 coming up.

Michal