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

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

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
>