| Message ID | 5182ceb139a43aacaab2c8b833bf52b13279dcf0.1730713917.git.mprivozn@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qga: Open channel before going daemon | expand |
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
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; }
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(-)