Re: [pbs-devel] [PATCH vma-to-pbs 8/9] switch argument handling from clap to pico-args

["Max Carrara" <m.carrara@proxmox.com>]

On Wed Apr 3, 2024 at 11:49 AM CEST, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  Cargo.toml     |   2 +-
>  src/main.rs    | 238 ++++++++++++++++++++++++++++---------------------
>  src/vma2pbs.rs |   4 +-
>  3 files changed, 139 insertions(+), 105 deletions(-)
>
> diff --git a/Cargo.toml b/Cargo.toml
> index f56e351..804a179 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -7,7 +7,7 @@ edition = "2021"
>  [dependencies]
>  anyhow = "1.0"
>  bincode = "1.3"
> -clap = { version = "4.0.32", features = ["cargo", "env"] }
> +pico-args = "0.4"
>  md5 = "0.7.0"
>  scopeguard = "1.1.0"
>  serde = "1.0"
> diff --git a/src/main.rs b/src/main.rs
> index ff6cc4c..df9f49a 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -1,91 +1,106 @@
> -use anyhow::{Context, Result};
> -use clap::{command, Arg, ArgAction};
> +use std::ffi::OsString;
> +
> +use anyhow::{bail, Context, Result};
>  use proxmox_sys::linux::tty;
>  
>  mod vma;
>  mod vma2pbs;
>  use vma2pbs::{backup_vma_to_pbs, BackupVmaToPbsArgs};
>  
> -fn main() -> Result<()> {
> -    let matches = command!()
> -        .arg(
> -            Arg::new("repository")
> -                .long("repository")
> -                .value_name("auth_id@host:port:datastore")
> -                .help("Repository URL")
> -                .required(true),
> -        )
> -        .arg(
> -            Arg::new("vmid")
> -                .long("vmid")
> -                .value_name("VMID")
> -                .help("Backup ID")
> -                .required(true),
> -        )
> -        .arg(
> -            Arg::new("fingerprint")
> -                .long("fingerprint")
> -                .value_name("FINGERPRINT")
> -                .help("Proxmox Backup Server Fingerprint")
> -                .env("PBS_FINGERPRINT"),
> -        )
> -        .arg(
> -            Arg::new("keyfile")
> -                .long("keyfile")
> -                .value_name("KEYFILE")
> -                .help("Key file"),
> -        )
> -        .arg(
> -            Arg::new("master_keyfile")
> -                .long("master_keyfile")
> -                .value_name("MASTER_KEYFILE")
> -                .help("Master key file"),
> -        )
> -        .arg(
> -            Arg::new("compress")
> -                .long("compress")
> -                .short('c')
> -                .help("Compress the Backup")
> -                .action(ArgAction::SetTrue),
> -        )
> -        .arg(
> -            Arg::new("encrypt")
> -                .long("encrypt")
> -                .short('e')
> -                .help("Encrypt the Backup")
> -                .action(ArgAction::SetTrue),
> -        )
> -        .arg(
> -            Arg::new("password-file")
> -                .long("password-file")
> -                .value_name("PASSWORD_FILE")
> -                .help("Password file"),
> -        )
> -        .arg(
> -            Arg::new("key-password-file")
> -                .long("key-password-file")
> -                .value_name("KEY_PASSWORD_FILE")
> -                .help("Key password file"),
> -        )
> -        .arg(Arg::new("vma_file"))
> -        .get_matches();
> -
> -    let pbs_repository = matches.get_one::<String>("repository").unwrap().to_string();
> -    let vmid = matches.get_one::<String>("vmid").unwrap().to_string();
> -
> -    let fingerprint = matches
> -        .get_one::<String>("fingerprint")
> -        .expect("Fingerprint not set. Use $PBS_FINGERPRINT or --fingerprint")

As mentioned in my response to patch 05, you're removing the `expect()`
here - which is *great*, don't get me wrong ;P - ...

> -        .to_string();
> -
> -    let keyfile = matches.get_one::<String>("keyfile");
> -    let master_keyfile = matches.get_one::<String>("master_keyfile");
> -    let compress = matches.get_flag("compress");
> -    let encrypt = matches.get_flag("encrypt");
> -
> -    let vma_file_path = matches.get_one::<String>("vma_file");
> -
> -    let password_file = matches.get_one::<String>("password-file");
> +const CMD_HELP: &str = "\
> +Usage: vma-to-pbs [OPTIONS] --repository <auth_id@host:port:datastore> --vmid <VMID> [vma_file]
> +
> +Arguments:
> +  [vma_file]
> +
> +Options:
> +      --repository <auth_id@host:port:datastore>
> +          Repository URL
> +      --vmid <VMID>
> +          Backup ID
> +      --fingerprint <FINGERPRINT>
> +          Proxmox Backup Server Fingerprint [env: PBS_FINGERPRINT=]
> +      --keyfile <KEYFILE>
> +          Key file
> +      --master_keyfile <MASTER_KEYFILE>
> +          Master key file
> +  -c, --compress
> +          Compress the Backup
> +  -e, --encrypt
> +          Encrypt the Backup
> +      --password_file <PASSWORD_FILE>
> +          Password file
> +      --key_password_file <KEY_PASSWORD_FILE>
> +          Key password file
> +  -h, --help
> +          Print help
> +  -V, --version
> +          Print version
> +";
> +
> +fn parse_args() -> Result<BackupVmaToPbsArgs> {
> +    let mut args: Vec<_> = std::env::args_os().collect();
> +    args.remove(0); // remove the executable path.
> +
> +    let mut first_later_args_index = 0;
> +    let options = ["-h", "--help", "-c", "--compress", "-e", "--encrypt"];
> +
> +    for (i, arg) in args.iter().enumerate() {
> +        if let Some(arg) = arg.to_str() {
> +            if arg.starts_with('-') {
> +                if arg == "--" {
> +                    args.remove(i);
> +                    first_later_args_index = i;
> +                    break;
> +                }
> +
> +                first_later_args_index = i + 1;
> +
> +                if !options.contains(&arg) {
> +                    first_later_args_index += 1;
> +                }
> +            }
> +        }
> +    }
> +
> +    let forwarded_args = if first_later_args_index > args.len() {
> +        Vec::new()
> +    } else {
> +        args.split_off(first_later_args_index)
> +    };
> +
> +    let mut args = pico_args::Arguments::from_vec(args);
> +
> +    if args.contains(["-h", "--help"]) {
> +        print!("{CMD_HELP}");
> +        std::process::exit(0);
> +    }
> +
> +    let pbs_repository = args.value_from_str("--repository")?;
> +    let vmid = args.value_from_str("--vmid")?;
> +    let fingerprint = args.opt_value_from_str("--fingerprint")?;
> +    let keyfile = args.opt_value_from_str("--keyfile")?;
> +    let master_keyfile = args.opt_value_from_str("--master_keyfile")?;
> +    let compress = args.contains(["-c", "--compress"]);
> +    let encrypt = args.contains(["-e", "--encrypt"]);
> +    let password_file: Option<OsString> = args.opt_value_from_str("--password-file")?;
> +    let key_password_file: Option<OsString> = args.opt_value_from_str("--key-password-file")?;
> +
> +    if !args.finish().is_empty() {
> +        bail!("unexpected extra arguments, use '-h' for usage");
> +    }
> +
> +    let fingerprint = match fingerprint {
> +        Some(v) => v,
> +        None => std::env::var("PBS_FINGERPRINT")
> +            .context("Fingerprint not set. Use $PBS_FINGERPRINT or --fingerprint")?,

... and later use `.context()` here. This can just be done in one change
- think of where it would fit best.

To give a more concrete example: You could either use `.context()` in
patch 05 from the get-go where you introduced the env var fallback - or
introduce the env var fallback in this patch, which means patch 05 gets
dropped instead.

Another suggestion would be to switch to `pico-args` much earlier in the
series, granted it's not too much of a hassle to rebase or re-introduce
the remaining changes after. Then you'd be working with `pico-args` from
the start and could just add all the remaining CLI flags, error
handling, etc. in later patches as you desire. You then wouldn't have to
change it all again here.

I'll leave it up to you to decide - you'll eventually get a feeling for
what's best to do in which situation ;)

If you're not sure, holler at me.

> +    };
> +
> +    if forwarded_args.len() > 1 {
> +        bail!("too many arguments");
> +    }
> +
> +    let vma_file_path = forwarded_args.first();
>  
>      let pbs_password = match password_file {
>          Some(password_file) => {
> @@ -101,46 +116,65 @@ fn main() -> Result<()> {
>  
>              password
>          }
> -        None => String::from_utf8(tty::read_password("Password: ")?)?,
> +        None => {
> +            if vma_file_path.is_none() {
> +                bail!(
> +                    "Please use --password-file to provide the password \
> +                    when passing the VMA file to stdin"
> +                );
> +            }
> +
> +            String::from_utf8(tty::read_password("Password: ")?)?
> +        }
>      };
>  
>      let key_password = match keyfile {
> -        Some(_) => {
> -            let key_password_file = matches.get_one::<String>("key_password_file");
> -
> -            Some(match key_password_file {
> -                Some(key_password_file) => {
> -                    let mut key_password = std::fs::read_to_string(key_password_file)
> -                        .context("Could not read key password file")?;
> -
> -                    if key_password.ends_with('\n') || key_password.ends_with('\r') {
> +        Some(_) => Some(match key_password_file {
> +            Some(key_password_file) => {
> +                let mut key_password = std::fs::read_to_string(key_password_file)
> +                    .context("Could not read key password file")?;
> +
> +                if key_password.ends_with('\n') || key_password.ends_with('\r') {
> +                    key_password.pop();
> +                    if key_password.ends_with('\r') {
>                          key_password.pop();
> -                        if key_password.ends_with('\r') {
> -                            key_password.pop();
> -                        }
>                      }
> +                }
>  
> -                    key_password
> +                key_password
> +            }
> +            None => {
> +                if vma_file_path.is_none() {
> +                    bail!(
> +                        "Please use --key-password-file to provide the password \
> +                        when passing the VMA file to stdin"
> +                    );
>                  }
> -                None => String::from_utf8(tty::read_password("Key Password: ")?)?,
> -            })
> -        }
> +
> +                String::from_utf8(tty::read_password("Key Password: ")?)?
> +            }
> +        }),
>          None => None,
>      };
>  
> -    let args = BackupVmaToPbsArgs {
> +    let options = BackupVmaToPbsArgs {
>          vma_file_path: vma_file_path.cloned(),
>          pbs_repository,
>          backup_id: vmid,
>          pbs_password,
> -        keyfile: keyfile.cloned(),
> +        keyfile,
>          key_password,
> -        master_keyfile: master_keyfile.cloned(),
> +        master_keyfile,
>          fingerprint,
>          compress,
>          encrypt,
>      };
>  
> +    Ok(options)
> +}
> +
> +fn main() -> Result<()> {
> +    let args = parse_args()?;
>      backup_vma_to_pbs(args)?;
>  
>      Ok(())
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index 9483f6e..a530ddb 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -1,7 +1,7 @@
>  use std::cell::RefCell;
>  use std::collections::hash_map::Entry;
>  use std::collections::HashMap;
> -use std::ffi::{c_char, CStr, CString};
> +use std::ffi::{c_char, CStr, CString, OsString};
>  use std::fs::File;
>  use std::io::{stdin, BufRead, BufReader, Read};
>  use std::ptr;
> @@ -21,7 +21,7 @@ use crate::vma::VmaReader;
>  const VMA_CLUSTER_SIZE: usize = 65536;
>  
>  pub struct BackupVmaToPbsArgs {
> -    pub vma_file_path: Option<String>,
> +    pub vma_file_path: Option<OsString>,
>      pub pbs_repository: String,
>      pub backup_id: String,
>      pub pbs_password: String,