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

["Ján Tomko" <jtomko@redhat.com>]

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