From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <m.carrara@proxmox.com>
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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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 <pbs-devel@lists.proxmox.com>; 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: <D0AKN1UPTVPB.1FGYGNU9BQND9@proxmox.com>
To: "Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>
From: "Max Carrara" <m.carrara@proxmox.com>
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
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=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 <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 =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::<String>("repository").unwra=
p().to_string();
> - let vmid =3D matches.get_one::<String>("vmid").unwrap().to_string();
> -
> - let fingerprint =3D matches
> - .get_one::<String>("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::<String>("keyfile");
> - let master_keyfile =3D matches.get_one::<String>("master_keyfile");
> - let compress =3D matches.get_flag("compress");
> - let encrypt =3D matches.get_flag("encrypt");
> -
> - let vma_file_path =3D matches.get_one::<String>("vma_file");
> -
> - let password_file =3D matches.get_one::<String>("password-file");
> +const CMD_HELP: &str =3D "\
> +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=3D]
> + --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<_> =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<OsString> =3D args.opt_value_from_str("--p=
assword-file")?;
> + let key_password_file: Option<OsString> =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::<String>("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<String>,
> + pub vma_file_path: Option<OsString>,
> pub pbs_repository: String,
> pub backup_id: String,
> pub pbs_password: String,