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

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

[Michal Privoznik]

See 3/4 for in depth explanation.

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, 25 insertions(+), 27 deletions(-)

-- 
2.45.2

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

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

[Konstantin Kostiuk]

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

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

On Mon, Nov 4, 2024 at 11:54 AM 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>
> ---
>  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: 1547 bytes --]

[PATCH 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>
---
 qga/main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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

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

[Ján Tomko]

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

On a Monday in 2024, Michal Privoznik 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>
>---
> qga/main.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/qga/main.c b/qga/main.c
>index 4a695235f0..c003aacbe0 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;
>     }
>

Below this there's another place that misses an EXIT_SUCCESS, on _WIN32
when config->daemonize is set:

  #ifdef _WIN32
      if (config->daemonize) {
          SERVICE_TABLE_ENTRY service_table[] = {
              { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
          StartServiceCtrlDispatcher(service_table);
      } else {
          ret = run_agent(s);
      }
  #else
      ret = run_agent(s);
  #endif

But after patch 4/4 ret is set to EXIT_SUCCESS in all the cases.

Jano

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Konstantin Kostiuk]

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

Hi Michal,

Please fix the issue that Jan mentioned.
All commits should be logically correct even if it part of one series.
Applying this will cause regression.

Best Regards,
Konstantin Kostiuk.


On Wed, Nov 6, 2024 at 6:07 PM Ján Tomko <jtomko@redhat.com> wrote:

> On a Monday in 2024, Michal Privoznik 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>
> >---
> > qga/main.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >diff --git a/qga/main.c b/qga/main.c
> >index 4a695235f0..c003aacbe0 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;
> >     }
> >
>
> Below this there's another place that misses an EXIT_SUCCESS, on _WIN32
> when config->daemonize is set:
>
>   #ifdef _WIN32
>       if (config->daemonize) {
>           SERVICE_TABLE_ENTRY service_table[] = {
>               { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
>           StartServiceCtrlDispatcher(service_table);
>       } else {
>           ret = run_agent(s);
>       }
>   #else
>       ret = run_agent(s);
>   #endif
>
> But after patch 4/4 ret is set to EXIT_SUCCESS in all the cases.
>
> Jano
>

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

[PATCH 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>
---
 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;
-    }
-
     g_main_loop_run(s->main_loop);
 
     if (s->channel) {
-- 
2.45.2

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

[Konstantin Kostiuk]

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

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

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

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

[Michal Prívozník]

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

[PATCH 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>
---
 qga/main.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 6240845f39..bcc182d64d 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,15 +1521,13 @@ static void cleanup_agent(GAState *s)
     ga_state = NULL;
 }
 
-static int run_agent_once(GAState *s)
+static void run_agent_once(GAState *s)
 {
     g_main_loop_run(s->main_loop);
 
     if (s->channel) {
         ga_channel_free(s->channel);
     }
-
-    return EXIT_SUCCESS;
 }
 
 static void wait_for_channel_availability(GAState *s)
@@ -1553,21 +1551,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)
@@ -1667,12 +1661,14 @@ int main(int argc, char **argv)
             { (char *)QGA_SERVICE_NAME, service_main }, { NULL, NULL } };
         StartServiceCtrlDispatcher(service_table);
     } 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 0/4] qga: Open channel before going daemon

[Ján Tomko]

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

On a Monday in 2024, Michal Privoznik wrote:
>See 3/4 for in depth explanation.
>
>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, 25 insertions(+), 27 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Michal Prívozník]

On 11/4/24 10:54, Michal Privoznik wrote:
> See 3/4 for in depth explanation.
> 
> 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, 25 insertions(+), 27 deletions(-)
> 

Ping

Michal