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

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

[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;
+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!");
+            eprint!("Are you sure you want to continue? (y/n): ");
+            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;
+                    }
+                }
+                Err(err) => 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)?,
-- 
2.39.2

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

[Thomas Lamprecht]

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)?,

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

[Gabriel Goller]

We had a discussion in another thread [0] about localizing the choices 
"y" and "n", since
there are some locales where "y" and "n" doesn't exist...
Was there some decision made off-list?

Either way, we should move this to a common crate as it will be used 
multiple times.

[0]: 
https://lists.proxmox.com/pipermail/pbs-devel/2023-September/006563.html

On 11/29/23 13:14, Markus Frank wrote:
> [..]

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

[Thomas Lamprecht]

Am 29/11/2023 um 14:00 schrieb Gabriel Goller:
> We had a discussion in another thread [0] about localizing the choices 

thanks for referencing this!

> "y" and "n", since
> there are some locales where "y" and "n" doesn't exist...
> Was there some decision made off-list?

not really, at least not that I remember, for now let's just ignore that
(but would be great if we (you) try to remember this when cleaning that up)


> 
> Either way, we should move this to a common crate as it will be used 
> multiple times.

definitively, can be done later though.

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

[Gabriel Goller]

On 11/29/23 14:04, Thomas Lamprecht wrote:
>> "y" and "n", since
>> there are some locales where "y" and "n" doesn't exist...
>> Was there some decision made off-list?
> not really, at least not that I remember, for now let's just ignore that
> (but would be great if we (you) try to remember this when cleaning that up)
yep, will do!