cli: Rework exit status processing

We've had many bugs from internal helpers using `return EXIT_FAILURE` rather than `return FALSE`. The reason we need exit codes is to handle the `RPM_OSTREE_EXIT_UNCHANGED` case. I realized recently that we had the handy `RpmOstreeCommandInvocation` which we can use to signal back this special case. Then all of our functions otherwise are just normal `GError`. One minor wart here is the two cases of "usage error" versus "command invocation" in `main.c`, but IMO the general cleanup is well worth that. Closes: coreos#1169 Approved by: jlebon
cgwalters · Jan 6, 2018 · 855d129 · 855d129
1 parent affa50f
commit 855d129
Show file tree

Hide file tree

Showing 32 changed files with 345 additions and 291 deletions.
diff --git a/src/app/main.c b/src/app/main.c
@@ -340,15 +340,20 @@ rpmostree_handle_subcommand (int argc, char **argv,
 
       g_autofree char *help = g_option_context_get_help (context, FALSE, NULL);
       g_printerr ("%s", help);
-      return EXIT_FAILURE;
+      return FALSE;
     }
 
   g_autofree char *prgname =
     g_strdup_printf ("%s %s", g_get_prgname (), subcommand_name);
   g_set_prgname (prgname);
 
-  RpmOstreeCommandInvocation sub_invocation = { .command = subcommand };
-  return subcommand->fn (argc, argv, &sub_invocation, cancellable, error);
+  /* We need a new sub-invocation with the new command, which also carries a new
+   * exit code, but we'll proxy the latter. */
+  RpmOstreeCommandInvocation sub_invocation = { .command = subcommand, .exit_code = -1 };
+  gboolean ret = subcommand->fn (argc, argv, &sub_invocation, cancellable, error);
+  /* Proxy the exit code */
+  invocation->exit_code = sub_invocation.exit_code;
+  return ret;
 }
 
 int
@@ -357,10 +362,15 @@ main (int    argc,
 {
   GCancellable *cancellable = g_cancellable_new ();
   RpmOstreeCommand *command;
-  int exit_status = EXIT_SUCCESS;
   const char *command_name = NULL;
   g_autofree char *prgname = NULL;
   GError *local_error = NULL;
+  /* We can leave this function with an error status from both a command
+   * invocation, as well as an option processing failure. Keep an alias to the
+   * two places that hold status codes.
+   */
+  int exit_status = EXIT_SUCCESS;
+  int *exit_statusp = &exit_status;
 
   /* avoid gvfs (http://bugzilla.gnome.org/show_bug.cgi?id=526454) */
   g_setenv ("GIO_USE_VFS", "local", TRUE);
@@ -409,16 +419,29 @@ main (int    argc,
       help = g_option_context_get_help (context, FALSE, NULL);
       g_printerr ("%s", help);
       exit_status = EXIT_FAILURE;
-
       goto out;
     }
 
   prgname = g_strdup_printf ("%s %s", g_get_prgname (), command_name);
   g_set_prgname (prgname);
 
-  { RpmOstreeCommandInvocation invocation = { .command = command };
-    exit_status = command->fn (argc, argv, &invocation, cancellable, &local_error);
-  }
+  RpmOstreeCommandInvocation invocation = { .command = command,
+                                            .exit_code = -1 };
+  exit_statusp = &(invocation.exit_code);
+  if (!command->fn (argc, argv, &invocation, cancellable, &local_error))
+    {
+      if (invocation.exit_code == -1)
+        invocation.exit_code = EXIT_FAILURE;
+      g_assert (local_error);
+      goto out;
+    }
+  else
+    {
+      if (invocation.exit_code == -1)
+        invocation.exit_code = EXIT_SUCCESS;
+      else
+        g_assert (invocation.exit_code != EXIT_SUCCESS);
+    }
 
  out:
   if (local_error != NULL)
@@ -434,13 +457,9 @@ main (int    argc,
       g_dbus_error_strip_remote_error (local_error);
       g_printerr ("%serror: %s%s\n", prefix, suffix, local_error->message);
       g_error_free (local_error);
-
-      /* Print a warning if the exit status indicates success when we
-       * actually had an error, so it gets reported and fixed quickly. */
-      g_warn_if_fail (exit_status != EXIT_SUCCESS);
     }
 
   rpmostree_polkit_agent_close ();
 
-  return exit_status;
+  return *exit_statusp;
 }
diff --git a/src/app/rpmostree-builtin-cancel.c b/src/app/rpmostree-builtin-cancel.c
@@ -56,7 +56,7 @@ on_active_txn_path_changed (GObject    *object,
   g_main_context_wakeup (NULL);
 }
 
-int
+gboolean
 rpmostree_builtin_cancel (int             argc,
                           char          **argv,
                           RpmOstreeCommandInvocation *invocation,
@@ -76,7 +76,7 @@ rpmostree_builtin_cancel (int             argc,
                                        &sysroot_proxy,
                                        &peer_pid,
                                        error))
-    return EXIT_FAILURE;
+    return FALSE;
 
 
   /* Keep track of the txn path we saw first, as well as checking if it changed
@@ -86,15 +86,15 @@ rpmostree_builtin_cancel (int             argc,
   glnx_unref_object RPMOSTreeTransaction *txn_proxy = NULL;
   if (!rpmostree_transaction_connect_active (sysroot_proxy, &txn_path, &txn_proxy,
                                              cancellable, error))
-    return EXIT_FAILURE;
+    return FALSE;
   if (!txn_proxy)
     {
       /* Let's not make this an error; cancellation may race with completion.
        * Perhaps in the future we could try to see whether a txn exited
        * "recently" but eh.
        */
       g_print ("No active transaction.\n");
-      return EXIT_SUCCESS;
+      return TRUE;
     }
 
   const char *title = rpmostree_transaction_get_title (txn_proxy);
@@ -117,5 +117,5 @@ rpmostree_builtin_cancel (int             argc,
     }
   g_print ("Cancelled.\n");
 
-  return EXIT_SUCCESS;
+  return TRUE;
 }
diff --git a/src/app/rpmostree-builtin-cleanup.c b/src/app/rpmostree-builtin-cleanup.c
@@ -45,7 +45,7 @@ static GOptionEntry option_entries[] = {
   { NULL }
 };
 
-int
+gboolean
 rpmostree_builtin_cleanup (int             argc,
                            char          **argv,
                            RpmOstreeCommandInvocation *invocation,
@@ -68,12 +68,12 @@ rpmostree_builtin_cleanup (int             argc,
                                        &sysroot_proxy,
                                        &peer_pid,
                                        error))
-    return EXIT_FAILURE;
+    return FALSE;
 
   if (argc < 1 || argc > 2)
     {
       rpmostree_usage_error (context, "Too few or too many arguments", error);
-      return EXIT_FAILURE;
+      return FALSE;
     }
 
   if (opt_base)
@@ -87,27 +87,27 @@ rpmostree_builtin_cleanup (int             argc,
   if (cleanup_types->len == 0)
     {
       glnx_throw (error, "At least one cleanup option must be specified");
-      return EXIT_FAILURE;
+      return FALSE;
     }
 
   g_ptr_array_add (cleanup_types, NULL);
 
   if (!rpmostree_load_os_proxy (sysroot_proxy, opt_osname,
                                 cancellable, &os_proxy, error))
-    return EXIT_FAILURE;
+    return FALSE;
 
   if (!rpmostree_os_call_cleanup_sync (os_proxy,
                                        (const char *const*)cleanup_types->pdata,
                                        &transaction_address,
                                        cancellable,
                                        error))
-    return EXIT_FAILURE;
+    return FALSE;
 
   if (!rpmostree_transaction_get_response_sync (sysroot_proxy,
                                                 transaction_address,
                                                 cancellable,
                                                 error))
-    return EXIT_FAILURE;
+    return FALSE;
 
-  return EXIT_SUCCESS;
+  return TRUE;
 }
diff --git a/src/app/rpmostree-builtin-compose.c b/src/app/rpmostree-builtin-compose.c
@@ -44,7 +44,7 @@ static RpmOstreeCommand compose_subcommands[] = {
   { NULL, 0, NULL, NULL }
 };
 
-int
+gboolean
 rpmostree_builtin_compose (int argc, char **argv,
                            RpmOstreeCommandInvocation *invocation,
                            GCancellable *cancellable, GError **error)

diff --git a/src/app/rpmostree-builtin-container.c b/src/app/rpmostree-builtin-container.c
@@ -34,7 +34,7 @@ static RpmOstreeCommand container_subcommands[] = {
   { NULL, 0, NULL, NULL }
 };
 
-int
+gboolean
 rpmostree_builtin_container (int argc, char **argv,
                              RpmOstreeCommandInvocation *invocation,
                              GCancellable *cancellable, GError **error)

diff --git a/src/app/rpmostree-builtin-db.c b/src/app/rpmostree-builtin-db.c
@@ -90,7 +90,7 @@ rpmostree_db_option_context_parse (GOptionContext *context,
   return TRUE;
 }
 
-int
+gboolean
 rpmostree_builtin_db (int argc, char **argv,
                       RpmOstreeCommandInvocation *invocation,
                       GCancellable *cancellable, GError **error)

diff --git a/src/app/rpmostree-builtin-deploy.c b/src/app/rpmostree-builtin-deploy.c
@@ -46,7 +46,7 @@ static GOptionEntry option_entries[] = {
   { NULL }
 };
 
-int
+gboolean
 rpmostree_builtin_deploy (int            argc,
                           char         **argv,
                           RpmOstreeCommandInvocation *invocation,
@@ -75,26 +75,26 @@ rpmostree_builtin_deploy (int            argc,
                                        &sysroot_proxy,
                                        &peer_pid,
                                        error))
-    return EXIT_FAILURE;
+    return FALSE;
 
   if (argc < 2)
     {
       rpmostree_usage_error (context, "REVISION must be specified", error);
-      return EXIT_FAILURE;
+      return FALSE;
     }
 
   if (opt_preview && (install_pkgs != NULL || uninstall_pkgs != NULL))
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
                    "Cannot specify both --preview and --install/--uninstall");
-      return EXIT_FAILURE;
+      return FALSE;
     }
 
   revision = argv[1];
 
   if (!rpmostree_load_os_proxy (sysroot_proxy, opt_osname,
                                 cancellable, &os_proxy, error))
-    return EXIT_FAILURE;
+    return FALSE;
 
   g_autoptr(GVariant) previous_deployment = rpmostree_os_dup_default_deployment (os_proxy);
 
@@ -106,7 +106,7 @@ rpmostree_builtin_deploy (int            argc,
                                                             &transaction_address,
                                                             cancellable,
                                                             error))
-        return EXIT_FAILURE;
+        return FALSE;
     }
   else
     {
@@ -136,7 +136,7 @@ rpmostree_builtin_deploy (int            argc,
                                             &transaction_address,
                                             cancellable,
                                             error))
-            return EXIT_FAILURE;
+            return FALSE;
         }
       else
         {
@@ -148,15 +148,15 @@ rpmostree_builtin_deploy (int            argc,
                                               NULL,
                                               cancellable,
                                               error))
-            return EXIT_FAILURE;
+            return FALSE;
         }
     }
 
   if (!rpmostree_transaction_get_response_sync (sysroot_proxy,
                                                 transaction_address,
                                                 cancellable,
                                                 error))
-    return EXIT_FAILURE;
+    return FALSE;
 
   if (opt_preview)
     {
@@ -170,27 +170,33 @@ rpmostree_builtin_deploy (int            argc,
                                                               &details,
                                                               cancellable,
                                                               error))
-        return EXIT_FAILURE;
+        return FALSE;
 
       if (g_variant_n_children (result) == 0)
-        return RPM_OSTREE_EXIT_UNCHANGED;
+        {
+          invocation->exit_code = RPM_OSTREE_EXIT_UNCHANGED;
+          return TRUE;
+        }
 
       rpmostree_print_package_diffs (result);
     }
   else if (!opt_reboot)
     {
       if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment))
-        return RPM_OSTREE_EXIT_UNCHANGED;
+        {
+          invocation->exit_code = RPM_OSTREE_EXIT_UNCHANGED;
+          return TRUE;
+        }
 
       /* do diff without dbus: https://github.com/projectatomic/rpm-ostree/pull/116 */
       const char *sysroot_path = rpmostree_sysroot_get_path (sysroot_proxy);
       if (!rpmostree_print_treepkg_diff_from_sysroot_path (sysroot_path,
                                                            cancellable,
                                                            error))
-        return EXIT_FAILURE;
+        return FALSE;
 
       g_print ("Run \"systemctl reboot\" to start a reboot\n");
     }
 
-  return EXIT_SUCCESS;
+  return TRUE;
 }
diff --git a/src/app/rpmostree-builtin-ex.c b/src/app/rpmostree-builtin-ex.c
@@ -43,7 +43,7 @@ static GOptionEntry global_entries[] = {
 };
 */
 
-int
+gboolean
 rpmostree_builtin_ex (int argc, char **argv,
                       RpmOstreeCommandInvocation *invocation,
                       GCancellable *cancellable, GError **error)