From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3722891439 for ; Wed, 3 Apr 2024 16:53:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 172871B28B for ; Wed, 3 Apr 2024 16:53:01 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 Apr 2024 16:52:59 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9AC9E44B4E for ; Wed, 3 Apr 2024 16:52:59 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Wed, 03 Apr 2024 16:52:58 +0200 Message-Id: To: "Proxmox Backup Server development discussion" From: "Max Carrara" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240403094913.107177-1-f.schauer@proxmox.com> <20240403094913.107177-9-f.schauer@proxmox.com> In-Reply-To: <20240403094913.107177-9-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH vma-to-pbs 8/9] switch argument handling from clap to pico-args X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2024 14:53:31 -0000 On Wed Apr 3, 2024 at 11:49 AM CEST, Filip Schauer wrote: > Signed-off-by: Filip Schauer > --- > 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 =3D "2021" > [dependencies] > anyhow =3D "1.0" > bincode =3D "1.3" > -clap =3D { version =3D "4.0.32", features =3D ["cargo", "env"] } > +pico-args =3D "0.4" > md5 =3D "0.7.0" > scopeguard =3D "1.1.0" > serde =3D "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; > =20 > mod vma; > mod vma2pbs; > use vma2pbs::{backup_vma_to_pbs, BackupVmaToPbsArgs}; > =20 > -fn main() -> Result<()> { > - let matches =3D 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 =3D matches.get_one::("repository").unwra= p().to_string(); > - let vmid =3D matches.get_one::("vmid").unwrap().to_string(); > - > - let fingerprint =3D matches > - .get_one::("fingerprint") > - .expect("Fingerprint not set. Use $PBS_FINGERPRINT or --fingerpr= int") 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 =3D matches.get_one::("keyfile"); > - let master_keyfile =3D matches.get_one::("master_keyfile"); > - let compress =3D matches.get_flag("compress"); > - let encrypt =3D matches.get_flag("encrypt"); > - > - let vma_file_path =3D matches.get_one::("vma_file"); > - > - let password_file =3D matches.get_one::("password-file"); > +const CMD_HELP: &str =3D "\ > +Usage: vma-to-pbs [OPTIONS] --repository -= -vmid [vma_file] > + > +Arguments: > + [vma_file] > + > +Options: > + --repository > + Repository URL > + --vmid > + Backup ID > + --fingerprint > + Proxmox Backup Server Fingerprint [env: PBS_FINGERPRINT=3D] > + --keyfile > + Key file > + --master_keyfile > + Master key file > + -c, --compress > + Compress the Backup > + -e, --encrypt > + Encrypt the Backup > + --password_file > + Password file > + --key_password_file > + Key password file > + -h, --help > + Print help > + -V, --version > + Print version > +"; > + > +fn parse_args() -> Result { > + let mut args: Vec<_> =3D std::env::args_os().collect(); > + args.remove(0); // remove the executable path. > + > + let mut first_later_args_index =3D 0; > + let options =3D ["-h", "--help", "-c", "--compress", "-e", "--encryp= t"]; > + > + for (i, arg) in args.iter().enumerate() { > + if let Some(arg) =3D arg.to_str() { > + if arg.starts_with('-') { > + if arg =3D=3D "--" { > + args.remove(i); > + first_later_args_index =3D i; > + break; > + } > + > + first_later_args_index =3D i + 1; > + > + if !options.contains(&arg) { > + first_later_args_index +=3D 1; > + } > + } > + } > + } > + > + let forwarded_args =3D if first_later_args_index > args.len() { > + Vec::new() > + } else { > + args.split_off(first_later_args_index) > + }; > + > + let mut args =3D pico_args::Arguments::from_vec(args); > + > + if args.contains(["-h", "--help"]) { > + print!("{CMD_HELP}"); > + std::process::exit(0); > + } > + > + let pbs_repository =3D args.value_from_str("--repository")?; > + let vmid =3D args.value_from_str("--vmid")?; > + let fingerprint =3D args.opt_value_from_str("--fingerprint")?; > + let keyfile =3D args.opt_value_from_str("--keyfile")?; > + let master_keyfile =3D args.opt_value_from_str("--master_keyfile")?; > + let compress =3D args.contains(["-c", "--compress"]); > + let encrypt =3D args.contains(["-e", "--encrypt"]); > + let password_file: Option =3D args.opt_value_from_str("--p= assword-file")?; > + let key_password_file: Option =3D args.opt_value_from_str(= "--key-password-file")?; > + > + if !args.finish().is_empty() { > + bail!("unexpected extra arguments, use '-h' for usage"); > + } > + > + let fingerprint =3D match fingerprint { > + Some(v) =3D> v, > + None =3D> std::env::var("PBS_FINGERPRINT") > + .context("Fingerprint not set. Use $PBS_FINGERPRINT or --fin= gerprint")?, ... 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 =3D forwarded_args.first(); > =20 > let pbs_password =3D match password_file { > Some(password_file) =3D> { > @@ -101,46 +116,65 @@ fn main() -> Result<()> { > =20 > password > } > - None =3D> String::from_utf8(tty::read_password("Password: ")?)?, > + None =3D> { > + 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: ")?)? > + } > }; > =20 > let key_password =3D match keyfile { > - Some(_) =3D> { > - let key_password_file =3D matches.get_one::("key_pas= sword_file"); > - > - Some(match key_password_file { > - Some(key_password_file) =3D> { > - let mut key_password =3D 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(_) =3D> Some(match key_password_file { > + Some(key_password_file) =3D> { > + let mut key_password =3D std::fs::read_to_string(key_pas= sword_file) > + .context("Could not read key password file")?; > + > + if key_password.ends_with('\n') || key_password.ends_wit= h('\r') { > + key_password.pop(); > + if key_password.ends_with('\r') { > key_password.pop(); > - if key_password.ends_with('\r') { > - key_password.pop(); > - } > } > + } > =20 > - key_password > + key_password > + } > + None =3D> { > + if vma_file_path.is_none() { > + bail!( > + "Please use --key-password-file to provide the p= assword \ > + when passing the VMA file to stdin" > + ); > } > - None =3D> String::from_utf8(tty::read_password("Key Pass= word: ")?)?, > - }) > - } > + > + String::from_utf8(tty::read_password("Key Password: ")?)= ? > + } > + }), > None =3D> None, > }; > =20 > - let args =3D BackupVmaToPbsArgs { > + let options =3D 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, > }; > =20 > + Ok(options) > +} > + > +fn main() -> Result<()> { > + let args =3D parse_args()?; > backup_vma_to_pbs(args)?; > =20 > 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 =3D 65536; > =20 > pub struct BackupVmaToPbsArgs { > - pub vma_file_path: Option, > + pub vma_file_path: Option, > pub pbs_repository: String, > pub backup_id: String, > pub pbs_password: String,