Re: [pbs-devel] [PATCH proxmox-backup] cli: add interactive confirmation for block device wipe

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 29/11/2023 um 13:14 schrieb Markus Frank:
> If stdin is a TTY, an interactive prompt is added to confirm the deletion
> of a block device, ensuring user verification before proceeding.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/bin/proxmox_backup_manager/disk.rs | 27 ++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs
> index 7a292098..4baa2a25 100644
> --- a/src/bin/proxmox_backup_manager/disk.rs
> +++ b/src/bin/proxmox_backup_manager/disk.rs
> @@ -3,6 +3,8 @@ use serde_json::Value;
>  
>  use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
>  use proxmox_schema::api;
> +use proxmox_sys::linux::tty::stdin_isatty;

as said off-list, just use the since rust 1.70 available std function:

std::io::stdin().is_terminal()

> +use std::io::Write;
>  
>  use pbs_api_types::{
>      ZfsCompressionType, ZfsRaidLevel, BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA,
> @@ -152,6 +154,31 @@ async fn initialize_disk(
>  async fn wipe_disk(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
>      param["node"] = "localhost".into();
>  
> +    // If we're on a TTY, query the user
> +    if stdin_isatty() {
> +        loop {
> +            eprintln!("You are about to wipe block device {}.", param["disk"]);
> +            eprintln!("Please make sure you have selected the correct device!");

would drop that line, it's not really adding anything

> +            eprint!("Are you sure you want to continue? (y/n): ");

would make this: (y/N)
i.e., N default, see below

> +            let _ = std::io::stdout().flush();
> +            use std::io::{BufRead, BufReader};
> +            let mut line = String::new();
> +            match BufReader::new(std::io::stdin()).read_line(&mut line) {
> +                Ok(_) => {
> +                    let trimmed = line.trim();
> +                    if trimmed == "y" || trimmed == "Y" {
> +                        break;
> +                    } else if trimmed == "n" || trimmed == "N" {
> +                        bail!("Aborting.");
> +                    } else {
> +                        continue;

would not loop, but rather always abort in the else:

if trimmed == "y" || trimmed == "Y" {
    break;
} else {
    bail!("Aborting.");
}


Or shorter by using a match:

match line.trim() {
    "y" | "Y" => break,
    _ => bail!("Aborting."),
}

> +                    }
> +                }
> +                Err(err) => bail!("Failed to read line - {}.", err),

can move err inline here: bail!("Failed to read line - {err}."),

> +            }
> +        }
> +    }
> +
>      let info = &api2::node::disks::API_METHOD_WIPE_DISK;
>      let result = match info.handler {
>          ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,