[PATCH v2 0/4] qga: Open channel before going daemon

[PATCH v2 0/4] qga: Open channel before going daemon

[Michal Privoznik]

v2 of:

https://lists.nongnu.org/archive/html/qemu-devel/2024-11/msg00460.html

diff to v1:
- In 2/4 fixed one path where EXIT_FAILURE would be returned,
- Reworked 3/4 so that channel initialization is kept in
  run_agent_once().

Michal Privoznik (4):
  qga: Don't access global variable in run_agent_once()
  qga: Invert logic on return value in main()
  qga: Don't daemonize before channel is initialized
  qga: Make run_agent() and run_agent_once() return no value

 qga/main.c | 52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

-- 
2.45.2

[PATCH v2 1/4] qga: Don't access global variable in run_agent_once()

[Michal Privoznik]

The run_agent_once() function is already given GAState via an
argument. There's no need to access the global ga_state variable
which points to the argument anyways (thanks to
initialize_agent()). Worse, some parts of the function use the
argument and the other use the global variable.  Stick with the
function argument.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index 50186760bf..4a695235f0 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1519,7 +1519,7 @@ static int run_agent_once(GAState *s)
         return EXIT_FAILURE;
     }
 
-    g_main_loop_run(ga_state->main_loop);
+    g_main_loop_run(s->main_loop);
 
     if (s->channel) {
         ga_channel_free(s->channel);
-- 
2.45.2

[PATCH v2 2/4] 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

[PATCH v2 3/4] 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

[PATCH v2 4/4] qga: Make run_agent() and run_agent_once() return no value

[Michal Privoznik]

After previous commits, run_agent_once() can't return anything
else but EXIT_SUCCESS. Transitionally, run_agent() can't return
anything else but EXIT_SUCCESS too. There's not much value in
having these function return an integer. Make them return void.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 qga/main.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 35f061b5ea..346274f114 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -145,7 +145,7 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
 DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data);
 VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 #endif
-static int run_agent(GAState *s);
+static void run_agent(GAState *s);
 static void stop_agent(GAState *s, bool requested);
 
 static void
@@ -1521,7 +1521,7 @@ static void cleanup_agent(GAState *s)
     ga_state = NULL;
 }
 
-static int run_agent_once(GAState *s)
+static void run_agent_once(GAState *s)
 {
     if (!s->channel &&
         channel_init(s, s->config->method, s->config->channel_path,
@@ -1536,8 +1536,6 @@ static int run_agent_once(GAState *s)
         ga_channel_free(s->channel);
         s->channel = NULL;
     }
-
-    return EXIT_SUCCESS;
 }
 
 static void wait_for_channel_availability(GAState *s)
@@ -1561,21 +1559,17 @@ static void wait_for_channel_availability(GAState *s)
 #endif
 }
 
-static int run_agent(GAState *s)
+static void run_agent(GAState *s)
 {
-    int ret = EXIT_SUCCESS;
-
     s->force_exit = false;
 
     do {
-        ret = run_agent_once(s);
+        run_agent_once(s);
         if (s->config->retry_path && !s->force_exit) {
             g_warning("agent stopped unexpectedly, restarting...");
             wait_for_channel_availability(s);
         }
     } while (s->config->retry_path && !s->force_exit);
-
-    return ret;
 }
 
 static void stop_agent(GAState *s, bool requested)
@@ -1674,14 +1668,15 @@ 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);
+        run_agent(s);
     }
 #else
-    ret = run_agent(s);
+    run_agent(s);
 #endif
 
+    ret = EXIT_SUCCESS;
+
     cleanup_agent(s);
 
 end:
-- 
2.45.2

Re: [PATCH v2 4/4] qga: Make run_agent() and run_agent_once() return no value

[Konstantin Kostiuk]

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

On Thu, Dec 5, 2024 at 6:19 PM Michal Privoznik <mprivozn@redhat.com> wrote:

> After previous commits, run_agent_once() can't return anything
> else but EXIT_SUCCESS. Transitionally, run_agent() can't return
> anything else but EXIT_SUCCESS too. There's not much value in
> having these function return an integer. Make them return void.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> ---
>  qga/main.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 35f061b5ea..346274f114 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -145,7 +145,7 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD
> type, LPVOID data,
>  DWORD WINAPI handle_serial_device_events(DWORD type, LPVOID data);
>  VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
>  #endif
> -static int run_agent(GAState *s);
> +static void run_agent(GAState *s);
>  static void stop_agent(GAState *s, bool requested);
>
>  static void
> @@ -1521,7 +1521,7 @@ static void cleanup_agent(GAState *s)
>      ga_state = NULL;
>  }
>
> -static int run_agent_once(GAState *s)
> +static void run_agent_once(GAState *s)
>  {
>      if (!s->channel &&
>          channel_init(s, s->config->method, s->config->channel_path,
> @@ -1536,8 +1536,6 @@ static int run_agent_once(GAState *s)
>

run_agent_once return EXIT_FAILURE when channel_init fails, so we have
compilation issue

../qga/main.c: In function ‘run_agent_once’:
../qga/main.c:1530:16: error: ‘return’ with a value, in function returning
void [-Wreturn-mismatch]
 1530 |         return EXIT_FAILURE;
      |                ^~~~~~~~~~~~
../qga/main.c:1524:13: note: declared here
 1524 | static void run_agent_once(GAState *s)
      |             ^~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.

As on Windows, we will really reinitialize the channel, I think, we need to
get the real exit code.
If initialization fails and the service gets a stop request we will see in
Windows Events information that the service crashed.


>          ga_channel_free(s->channel);
>          s->channel = NULL;
>      }
> -
> -    return EXIT_SUCCESS;
>  }
>
>  static void wait_for_channel_availability(GAState *s)
> @@ -1561,21 +1559,17 @@ static void wait_for_channel_availability(GAState
> *s)
>  #endif
>  }
>
> -static int run_agent(GAState *s)
> +static void run_agent(GAState *s)
>  {
> -    int ret = EXIT_SUCCESS;
> -
>      s->force_exit = false;
>
>      do {
> -        ret = run_agent_once(s);
> +        run_agent_once(s);
>          if (s->config->retry_path && !s->force_exit) {
>              g_warning("agent stopped unexpectedly, restarting...");
>              wait_for_channel_availability(s);
>          }
>      } while (s->config->retry_path && !s->force_exit);
> -
> -    return ret;
>  }
>
>  static void stop_agent(GAState *s, bool requested)
> @@ -1674,14 +1668,15 @@ 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);
> +        run_agent(s);
>      }
>  #else
> -    ret = run_agent(s);
> +    run_agent(s);
>  #endif
>
> +    ret = EXIT_SUCCESS;
> +
>      cleanup_agent(s);
>
>  end:
> --
> 2.45.2
>
>

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

Re: [PATCH v2 1/4] qga: Don't access global variable in run_agent_once()

[Konstantin Kostiuk]

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

Hi Michal,

The PULL with this patch was sent. Skip it in the next version of this
series.

Best Regards,
Konstantin Kostiuk.


On Thu, Dec 5, 2024 at 6:19 PM Michal Privoznik <mprivozn@redhat.com> wrote:

> The run_agent_once() function is already given GAState via an
> argument. There's no need to access the global ga_state variable
> which points to the argument anyways (thanks to
> initialize_agent()). Worse, some parts of the function use the
> argument and the other use the global variable.  Stick with the
> function argument.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
> ---
>  qga/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 50186760bf..4a695235f0 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1519,7 +1519,7 @@ static int run_agent_once(GAState *s)
>          return EXIT_FAILURE;
>      }
>
> -    g_main_loop_run(ga_state->main_loop);
> +    g_main_loop_run(s->main_loop);
>
>      if (s->channel) {
>          ga_channel_free(s->channel);
> --
> 2.45.2
>
>

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

Re: [PATCH v2 4/4] qga: Make run_agent() and run_agent_once() return no value

[Michal Prívozník]

On 12/23/24 13:39, Konstantin Kostiuk wrote:
> Hi Michal,
> 
> Do you plan to fix this patch series?

Yes, but I got sidetracked just before leaving for Christmas. I'll post
another version soon.

Michal