[PATCH v3 0/2] qga: Open channel before going daemon

[PATCH v3 0/2] qga: Open channel before going daemon

[Michal Privoznik]

v3 of:

https://lists.nongnu.org/archive/html/qemu-devel/2024-12/msg01073.html

diff to v2:
- Patch 1/4 from v2 was merged already,
- Patch 4/4 from v2 is now dropped because it's no longed applicable
  (run_agent_once() and subsequently can return two different values).

Michal Privoznik (2):
  qga: Invert logic on return value in main()
  qga: Don't daemonize before channel is initialized

 qga/main.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

-- 
2.45.2

[PATCH v3 1/2] qga: Invert logic on return value in main()

[Michal Privoznik]

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 <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 qga/main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 4a695235f0..68ea7f275a 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);
     }
-- 
2.45.2

Re: [PATCH v3 1/2] qga: Invert logic on return value in main()

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>

On Tue, Jan 7, 2025 at 4:52 PM Michal Privoznik <mprivozn@redhat.com> wrote:

> 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 <mprivozn@redhat.com>
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> ---
>  qga/main.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 4a695235f0..68ea7f275a 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);
>      }
> --
> 2.45.2
>
>

[-- Attachment #2: Type: text/html, Size: 3647 bytes --]

[PATCH v3 2/2] qga: Don't daemonize before channel is initialized

[Michal Privoznik]

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

Re: [PATCH v3 2/2] qga: Don't daemonize before channel is initialized

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 4587 bytes --]