[PATCH installer] fix #7833: common: cli: ensure process exits with 1 if subcommand fails

[PATCH installer] fix #7833: common: cli: ensure process exits with 1 if subcommand fails

[Christoph Heiss]

Fixes #7833 [0].

Currently, errors from the `AppInfo::on_command` handler is propagated
and printed, but does not cause cli::run() to return a failure exit
code.

A small, custom error enum is introduced to differentiate between an
unknown/missing subcommand - which should also trigger the global help
menu - and errors from the command that was run.

Based on that, the appropriate `ExitCode` variant is set and propagated
back to the caller.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7833

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-auto-install-assistant/src/main.rs |  4 +-
 proxmox-chroot/src/main.rs                 |  4 +-
 proxmox-installer-common/src/cli.rs        | 91 ++++++++++++++++++----
 3 files changed, 78 insertions(+), 21 deletions(-)

diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index 730d7d8..728d8ee 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -560,8 +560,8 @@ GLOBAL OPTIONS:
             Some("device-match") => cli::handle_command::<CommandDeviceMatchArgs>(args),
             Some("device-info") => cli::handle_command::<CommandDeviceInfoArgs>(args),
             Some("system-info") => cli::handle_command::<CommandSystemInfoArgs>(args),
-            Some(s) => bail!("unknown subcommand '{s}'"),
-            None => bail!("subcommand required"),
+            Some(s) => Err(cli::Error::UnknownSubcommand(s.to_owned())),
+            None => Err(cli::Error::SubcommandRequired),
         },
     })
 }
diff --git a/proxmox-chroot/src/main.rs b/proxmox-chroot/src/main.rs
index 5f087bb..6544bf1 100644
--- a/proxmox-chroot/src/main.rs
+++ b/proxmox-chroot/src/main.rs
@@ -125,8 +125,8 @@ GLOBAL OPTIONS:
         on_command: |s, args| match s {
             Some("prepare") => cli::handle_command::<CommandPrepareArgs>(args),
             Some("cleanup") => cli::handle_command::<CommandCleanupArgs>(args),
-            Some(s) => bail!("unknown subcommand '{s}'"),
-            None => bail!("subcommand required"),
+            Some(s) => Err(cli::Error::UnknownSubcommand(s.to_owned())),
+            None => Err(cli::Error::SubcommandRequired),
         },
     })
 }
diff --git a/proxmox-installer-common/src/cli.rs b/proxmox-installer-common/src/cli.rs
index e2b4d81..1c9ef29 100644
--- a/proxmox-installer-common/src/cli.rs
+++ b/proxmox-installer-common/src/cli.rs
@@ -1,7 +1,7 @@
 //! Provides a simple command line parsing interface, with special support for
 //! (one-level deep) subcommands.
 
-use std::process;
+use std::{fmt, process::ExitCode};
 
 use anyhow::Result;
 
@@ -22,41 +22,98 @@ pub trait Subcommand {
     fn run(&self) -> Result<()>;
 }
 
+/// Describes a CLI app.
+///
+/// ### Example usage
+/// ```
+/// fn main() -> process::ExitCode {
+///     cli::run(cli::AppInfo {
+///         global_help: "Some CLI app",
+///         on_command: |s, args| match s {
+///             Some("foo") => cli::handle_command::<FooCommand>(args),
+///             Some(s) => Err(cli::Error::UnknownSubcommand(s.to_owned())),
+///             None => Err(cli::Error::SubcommandRequired),
+///         }
+///     })
+/// }
+/// ```
 pub struct AppInfo<'a> {
+    /// Global help menu to display if `-h`/`--help` was given without any subcommand or an invalid
+    /// subcommand was specified.
     pub global_help: &'a str,
-    pub on_command: fn(Option<&str>, &mut Arguments) -> Result<()>,
+    /// Callback to invoke with the subcommand name (if given) and the argument list.
+    ///
+    /// Receives the subcommand name (if one was given on the command line) and the rest of the
+    /// argument list.
+    pub on_command: fn(Option<&str>, &mut Arguments) -> Result<(), Error>,
 }
 
-pub fn run(info: AppInfo) -> process::ExitCode {
-    if let Err(err) = parse_args(&info) {
-        eprintln!("Error: {err:#}\n\n{}", info.global_help);
-        process::ExitCode::FAILURE
-    } else {
-        process::ExitCode::SUCCESS
+#[derive(Debug)]
+pub enum Error {
+    UnknownSubcommand(String),
+    SubcommandRequired,
+    Other(anyhow::Error),
+}
+
+impl std::error::Error for Error {}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Error::UnknownSubcommand(s) => write!(f, "unknown subcommand '{s}'"),
+            Error::SubcommandRequired => write!(f, "subcommand required"),
+            Error::Other(s) => write!(f, "{s}"),
+        }
     }
 }
 
-fn parse_args(info: &AppInfo) -> Result<()> {
+/// Retrieves the process command line, checks for a subcommand name and for the default global
+/// flags `-h`/`--help` and `-V`/`--version`.
+///
+/// If no global flag was given, runs the command handler given in [`AppInfo::on_command`].
+pub fn run(info: AppInfo) -> ExitCode {
     let mut args = pico_args::Arguments::from_env();
-    let subcommand = args.subcommand()?;
+
+    let subcommand = match args.subcommand() {
+        Ok(s) => s,
+        Err(err) => {
+            eprintln!("Error: {err:#}\n\n{}", info.global_help);
+            return ExitCode::FAILURE;
+        }
+    };
 
     if subcommand.is_none() && args.contains(["-h", "--help"]) {
         eprintln!("{}", info.global_help);
-        Ok(())
+        ExitCode::SUCCESS
     } else if args.contains(["-V", "--version"]) {
         eprintln!("{} v{}", env!("CARGO_PKG_NAME"), env!("CARGO_PKG_VERSION"));
-        Ok(())
+        ExitCode::SUCCESS
     } else {
-        (info.on_command)(subcommand.as_deref(), &mut args)
+        match (info.on_command)(subcommand.as_deref(), &mut args) {
+            Ok(()) => ExitCode::SUCCESS,
+            Err(Error::Other(err)) => {
+                eprintln!("Error: {err:#}");
+                ExitCode::FAILURE
+            }
+            // For non-command errors (i.e. invalid subcommand), also print the help text, in
+            // addition to the error itself.
+            Err(err) => {
+                eprintln!("Error: {err:#}\n\n{}", info.global_help);
+                ExitCode::FAILURE
+            }
+        }
     }
 }
 
-pub fn handle_command<T: Subcommand>(args: &mut pico_args::Arguments) -> Result<()> {
+/// Runs a specific subcommand. If the arguments contain -h/--help, the help text of the subcommand
+/// is printed and success is returned.
+pub fn handle_command<T: Subcommand>(args: &mut pico_args::Arguments) -> Result<(), Error> {
     if args.contains(["-h", "--help"]) {
         T::print_usage();
-    } else if let Err(err) = T::parse(args).and_then(|cmd| cmd.run()) {
-        eprintln!("Error: {err:#}");
+        return Ok(());
     }
 
-    Ok(())
+    T::parse(args)
+        .and_then(|cmd| cmd.run())
+        .map_err(Error::Other)
 }
-- 
2.55.0