[pbs-devel] [PATCH proxmox-backup v3 0/6] fix #3690: wipe disk

[pbs-devel] [PATCH proxmox-backup v3 0/6] fix #3690: wipe disk

[Markus Frank]

Wipe a disk/partition like we do in PVE. (wipefs, dd, change_parttype to 0x83
with sgdisk)

v3:
* rebase on master
* added more informations to commit message

v2:
* change unwraps to matches
* renamed regex

Markus Frank (6):
  fix #3690: pbs_api_types: add regex, format & schema for partition
    names to pbs-api-types
  fix #3690: tools: add wipe_blockdev & change_parttype rust equivalent
  fix #3690: api: add function wipe_disk
  fix #3690: cli: add function wipe_disk
  fix #3690: ui: enable wipe disk in StorageAndDisks
  tools: prohibit disk wipe of EFI partition

 pbs-api-types/src/lib.rs               |  10 ++
 src/api2/node/disks/mod.rs             |  53 +++++++++-
 src/bin/proxmox_backup_manager/disk.rs |  38 ++++++-
 src/tools/disks/mod.rs                 | 138 ++++++++++++++++++++++++-
 www/Utils.js                           |   1 +
 www/panel/StorageAndDisks.js           |   1 +
 6 files changed, 232 insertions(+), 9 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 1/6] fix #3690: pbs_api_types: add regex, format & schema for partition names to pbs-api-types

[Markus Frank]

The new regex is similar to BLOCKDEVICE_NAME_REGEX but also allows
numbers at the end of the device name (also allows partitions names).
For nvme partitions it also allows the letter p and a number.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 pbs-api-types/src/lib.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index ebd5550d..795ff2a6 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -191,6 +191,7 @@ const_regex! {
     );
 
     pub BLOCKDEVICE_NAME_REGEX = r"^(?:(?:h|s|x?v)d[a-z]+)|(?:nvme\d+n\d+)$";
+    pub BLOCKDEVICE_DISK_AND_PARTITION_NAME_REGEX = r"^(?:(?:h|s|x?v)d[a-z]+\d*)|(?:nvme\d+n\d+(p\d+)?)$";
     pub SUBSCRIPTION_KEY_REGEX = concat!(r"^pbs(?:[cbsp])-[0-9a-f]{10}$");
 }
 
@@ -205,6 +206,8 @@ pub const PASSWORD_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&PASSWORD_
 pub const UUID_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&UUID_REGEX);
 pub const BLOCKDEVICE_NAME_FORMAT: ApiStringFormat =
     ApiStringFormat::Pattern(&BLOCKDEVICE_NAME_REGEX);
+pub const BLOCKDEVICE_DISK_AND_PARTITION_NAME_FORMAT: ApiStringFormat =
+    ApiStringFormat::Pattern(&BLOCKDEVICE_DISK_AND_PARTITION_NAME_REGEX);
 pub const SUBSCRIPTION_KEY_FORMAT: ApiStringFormat =
     ApiStringFormat::Pattern(&SUBSCRIPTION_KEY_REGEX);
 pub const SYSTEMD_DATETIME_FORMAT: ApiStringFormat =
@@ -285,6 +288,13 @@ pub const BLOCKDEVICE_NAME_SCHEMA: Schema =
         .max_length(64)
         .schema();
 
+pub const BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA: Schema =
+    StringSchema::new("(Partition) block device name (/sys/class/block/<name>).")
+        .format(&BLOCKDEVICE_DISK_AND_PARTITION_NAME_FORMAT)
+        .min_length(3)
+        .max_length(64)
+        .schema();
+
 pub const DISK_ARRAY_SCHEMA: Schema =
     ArraySchema::new("Disk name list.", &BLOCKDEVICE_NAME_SCHEMA).schema();
 
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 2/6] fix #3690: tools: add wipe_blockdev & change_parttype rust equivalent

[Markus Frank]

The wipe_blockdev & change_parttype functions are similar to
PVE::Diskmanage's wipe_blockdev & change_parttype functions.

The partition_by_name & complete_partition_name functions are
modified disk_by_name & complete_disk_name functions for partitions.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 src/tools/disks/mod.rs | 125 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 124 insertions(+), 1 deletion(-)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 8b6d470b..beb8178b 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -16,10 +16,12 @@ use ::serde::{Deserialize, Serialize};
 
 use proxmox_lang::error::io_err_other;
 use proxmox_lang::{io_bail, io_format_err};
+use proxmox_rest_server::WorkerTask;
 use proxmox_schema::api;
 use proxmox_sys::linux::procfs::{mountinfo::Device, MountInfo};
+use proxmox_sys::task_log;
 
-use pbs_api_types::BLOCKDEVICE_NAME_REGEX;
+use pbs_api_types::{BLOCKDEVICE_DISK_AND_PARTITION_NAME_REGEX, BLOCKDEVICE_NAME_REGEX};
 
 mod zfs;
 pub use zfs::*;
@@ -111,6 +113,12 @@ impl DiskManage {
         self.disk_by_sys_path(syspath)
     }
 
+    /// Get a `Disk` for a name in `/sys/class/block/<name>`.
+    pub fn partition_by_name(self: Arc<Self>, name: &str) -> io::Result<Disk> {
+        let syspath = format!("/sys/class/block/{}", name);
+        self.disk_by_sys_path(syspath)
+    }
+
     /// Gather information about mounted disks:
     fn mounted_devices(&self) -> Result<&HashSet<dev_t>, Error> {
         self.mounted_devices
@@ -1056,6 +1064,105 @@ pub fn inititialize_gpt_disk(disk: &Disk, uuid: Option<&str>) -> Result<(), Erro
     Ok(())
 }
 
+/// Wipes all labels and the first 200 MiB of a disk/partition (or the whole if it is smaller).
+/// If called with a partition, also sets the partition type to 0x83 'Linux filesystem'.
+pub fn wipe_blockdev(disk: &Disk, worker: Arc<WorkerTask>) -> Result<(), Error> {
+    let disk_path = match disk.device_path() {
+        Some(path) => path,
+        None => bail!("disk {:?} has no node in /dev", disk.syspath()),
+    };
+    let disk_path_str = match disk_path.to_str() {
+        Some(path) => path,
+        None => bail!("disk {:?} could not transform into a str", disk.syspath()),
+    };
+
+    let mut is_partition = false;
+    for disk_info in get_lsblk_info()?.iter() {
+        if disk_info.path == disk_path_str && disk_info.partition_type.is_some() {
+            is_partition = true;
+        }
+    }
+
+    let mut to_wipe: Vec<PathBuf> = Vec::new();
+
+    let partitions_map = disk.partitions()?;
+    for part_disk in partitions_map.values() {
+        let part_path = match part_disk.device_path() {
+            Some(path) => path,
+            None => bail!("disk {:?} has no node in /dev", part_disk.syspath()),
+        };
+        to_wipe.push(part_path.to_path_buf());
+    }
+
+    to_wipe.push(disk_path.to_path_buf());
+
+    task_log!(worker, "Wiping block device {}", disk_path.display());
+
+    let mut wipefs_command = std::process::Command::new("wipefs");
+    wipefs_command.arg("--all").args(&to_wipe);
+
+    let wipefs_output = proxmox_sys::command::run_command(wipefs_command, None)?;
+    task_log!(worker, "wipefs output: {}", wipefs_output);
+
+    let size = disk.size().map(|size| size / 1024 / 1024)?;
+    let count = size.min(200);
+
+    let mut dd_command = std::process::Command::new("dd");
+    let mut of_path = OsString::from("of=");
+    of_path.push(disk_path);
+    let mut count_str = OsString::from("count=");
+    count_str.push(count.to_string());
+    let args = [
+        "if=/dev/zero".into(),
+        of_path,
+        "bs=1M".into(),
+        "conv=fdatasync".into(),
+        count_str.into(),
+    ];
+    dd_command.args(args);
+
+    let dd_output = proxmox_sys::command::run_command(dd_command, None)?;
+    task_log!(worker, "dd output: {}", dd_output);
+
+    if is_partition {
+        // set the partition type to 0x83 'Linux filesystem'
+        change_parttype(&disk, "8300", worker)?;
+    }
+
+    Ok(())
+}
+
+pub fn change_parttype(
+    part_disk: &Disk,
+    part_type: &str,
+    worker: Arc<WorkerTask>,
+) -> Result<(), Error> {
+    let part_path = match part_disk.device_path() {
+        Some(path) => path,
+        None => bail!("disk {:?} has no node in /dev", part_disk.syspath()),
+    };
+    if let Ok(stat) = nix::sys::stat::stat(part_path) {
+        let mut sgdisk_command = std::process::Command::new("sgdisk");
+        let major = unsafe { libc::major(stat.st_rdev) };
+        let minor = unsafe { libc::minor(stat.st_rdev) };
+        let partnum_path = &format!("/sys/dev/block/{}:{}/partition", major, minor);
+        let partnum: u32 = std::fs::read_to_string(partnum_path)?.trim_end().parse()?;
+        sgdisk_command.arg(&format!("-t{}:{}", partnum, part_type));
+        let part_disk_parent = match part_disk.parent() {
+            Some(disk) => disk,
+            None => bail!("disk {:?} has no node in /dev", part_disk.syspath()),
+        };
+        let part_disk_parent_path = match part_disk_parent.device_path() {
+            Some(path) => path,
+            None => bail!("disk {:?} has no node in /dev", part_disk.syspath()),
+        };
+        sgdisk_command.arg(part_disk_parent_path);
+        let sgdisk_output = proxmox_sys::command::run_command(sgdisk_command, None)?;
+        task_log!(worker, "sgdisk output: {}", sgdisk_output);
+    }
+    Ok(())
+}
+
 /// Create a single linux partition using the whole available space
 pub fn create_single_linux_partition(disk: &Disk) -> Result<Disk, Error> {
     let disk_path = match disk.device_path() {
@@ -1136,6 +1243,22 @@ pub fn complete_disk_name(_arg: &str, _param: &HashMap<String, String>) -> Vec<S
         .collect()
 }
 
+/// Block device partition name completion helper
+pub fn complete_partition_name(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
+    let dir = match proxmox_sys::fs::scan_subdir(
+        libc::AT_FDCWD,
+        "/sys/class/block",
+        &BLOCKDEVICE_DISK_AND_PARTITION_NAME_REGEX,
+    ) {
+        Ok(dir) => dir,
+        Err(_) => return vec![],
+    };
+
+    dir.flatten()
+        .map(|item| item.file_name().to_str().unwrap().to_string())
+        .collect()
+}
+
 /// Read the FS UUID (parse blkid output)
 ///
 /// Note: Calling blkid is more reliable than using the udev ID_FS_UUID property.
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 3/6] fix #3690: api: add function wipe_disk

[Markus Frank]

An api function similar to PVE wipedisk function that takes
a disk/partition dev name as argument to wipe it in a new WorkerTask thread.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 src/api2/node/disks/mod.rs | 53 +++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/src/api2/node/disks/mod.rs b/src/api2/node/disks/mod.rs
index 5ee959cd..711dae7b 100644
--- a/src/api2/node/disks/mod.rs
+++ b/src/api2/node/disks/mod.rs
@@ -9,12 +9,13 @@ use proxmox_sortable_macro::sortable;
 use proxmox_sys::task_log;
 
 use pbs_api_types::{
-    BLOCKDEVICE_NAME_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
+    BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA, BLOCKDEVICE_NAME_SCHEMA, NODE_SCHEMA,
+    PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
 };
 
 use crate::tools::disks::{
-    get_smart_data, inititialize_gpt_disk, DiskManage, DiskUsageInfo, DiskUsageQuery,
-    DiskUsageType, SmartData,
+    get_smart_data, inititialize_gpt_disk, wipe_blockdev, DiskManage, DiskUsageInfo,
+    DiskUsageQuery, DiskUsageType, SmartData,
 };
 use proxmox_rest_server::WorkerTask;
 
@@ -178,6 +179,51 @@ pub fn initialize_disk(
     Ok(json!(upid_str))
 }
 
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            node: {
+                schema: NODE_SCHEMA,
+            },
+            disk: {
+                schema: BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA,
+            },
+        },
+    },
+    returns: {
+        schema: UPID_SCHEMA,
+    },
+    access: {
+        permission: &Permission::Privilege(&["system", "disks"], PRIV_SYS_MODIFY, false),
+    },
+)]
+/// wipe disk
+pub fn wipe_disk(disk: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+    let auth_id = rpcenv.get_auth_id().unwrap();
+
+    let upid_str = WorkerTask::new_thread(
+        "wipedisk",
+        Some(disk.clone()),
+        auth_id,
+        to_stdout,
+        move |worker| {
+            task_log!(worker, "wipe disk {}", disk);
+
+            let disk_manager = DiskManage::new();
+            let disk_info = disk_manager.partition_by_name(&disk)?;
+
+            wipe_blockdev(&disk_info, worker)?;
+
+            Ok(())
+        },
+    )?;
+
+    Ok(json!(upid_str))
+}
+
 #[sortable]
 const SUBDIRS: SubdirMap = &sorted!([
     //    ("lvm", &lvm::ROUTER),
@@ -186,6 +232,7 @@ const SUBDIRS: SubdirMap = &sorted!([
     ("initgpt", &Router::new().post(&API_METHOD_INITIALIZE_DISK)),
     ("list", &Router::new().get(&API_METHOD_LIST_DISKS)),
     ("smart", &Router::new().get(&API_METHOD_SMART_STATUS)),
+    ("wipedisk", &Router::new().put(&API_METHOD_WIPE_DISK)),
 ]);
 
 pub const ROUTER: Router = Router::new()
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 4/6] fix #3690: cli: add function wipe_disk

[Markus Frank]

A new cli subcommand which calls the api wipe_disk function
to wipe a disk/partition with a specified dev name.

Examples:
proxmox-backup-manager disk wipe sda2
proxmox-backup-manager disk wipe sda
proxmox-backup-manager disk wipe nvme0n1p1

The complete_partition_name from tools/disks/mod.rs is used for
command completion.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 src/bin/proxmox_backup_manager/disk.rs | 38 ++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs
index fae8829f..7a292098 100644
--- a/src/bin/proxmox_backup_manager/disk.rs
+++ b/src/bin/proxmox_backup_manager/disk.rs
@@ -5,10 +5,12 @@ use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
 use proxmox_schema::api;
 
 use pbs_api_types::{
-    ZfsCompressionType, ZfsRaidLevel, BLOCKDEVICE_NAME_SCHEMA, DATASTORE_SCHEMA, DISK_LIST_SCHEMA,
-    ZFS_ASHIFT_SCHEMA,
+    ZfsCompressionType, ZfsRaidLevel, BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA,
+    BLOCKDEVICE_NAME_SCHEMA, DATASTORE_SCHEMA, DISK_LIST_SCHEMA, ZFS_ASHIFT_SCHEMA,
+};
+use proxmox_backup::tools::disks::{
+    complete_disk_name, complete_partition_name, FileSystemType, SmartAttribute,
 };
-use proxmox_backup::tools::disks::{complete_disk_name, FileSystemType, SmartAttribute};
 
 use proxmox_backup::api2;
 
@@ -137,6 +139,30 @@ async fn initialize_disk(
     Ok(Value::Null)
 }
 
+#[api(
+   input: {
+        properties: {
+            disk: {
+                schema: BLOCKDEVICE_DISK_AND_PARTITION_NAME_SCHEMA,
+            },
+        },
+   },
+)]
+/// wipe disk
+async fn wipe_disk(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    param["node"] = "localhost".into();
+
+    let info = &api2::node::disks::API_METHOD_WIPE_DISK;
+    let result = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+
+    Ok(Value::Null)
+}
+
 #[api(
    input: {
         properties: {
@@ -375,6 +401,12 @@ pub fn disk_commands() -> CommandLineInterface {
             CliCommand::new(&API_METHOD_INITIALIZE_DISK)
                 .arg_param(&["disk"])
                 .completion_cb("disk", complete_disk_name),
+        )
+        .insert(
+            "wipe",
+            CliCommand::new(&API_METHOD_WIPE_DISK)
+                .arg_param(&["disk"])
+                .completion_cb("disk", complete_partition_name),
         );
 
     cmd_def.into()
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 5/6] fix #3690: ui: enable wipe disk in StorageAndDisks

[Markus Frank]

supportsWipeDisk: true -> enables the wipe button in the WebUI (same as
in pve)

The entry for override_task_descriptions is copied from pve-manager.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 www/Utils.js                 | 1 +
 www/panel/StorageAndDisks.js | 1 +
 2 files changed, 2 insertions(+)

diff --git a/www/Utils.js b/www/Utils.js
index 8fbc6b23..7592d1bd 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -425,6 +425,7 @@ Ext.define('PBS.Utils', {
 	    verify: ['Datastore', gettext('Verification')],
 	    verify_group: ['Group', gettext('Verification')],
 	    verify_snapshot: ['Snapshot', gettext('Verification')],
+	    wipedisk: ['Device', gettext('Wipe Disk')],
 	    zfscreate: [gettext('ZFS Storage'), gettext('Create')],
 	});
 
diff --git a/www/panel/StorageAndDisks.js b/www/panel/StorageAndDisks.js
index 7bd7042c..e9d7ed19 100644
--- a/www/panel/StorageAndDisks.js
+++ b/www/panel/StorageAndDisks.js
@@ -17,6 +17,7 @@ Ext.define('PBS.StorageAndDiskPanel', {
 	    xtype: 'pmxDiskList',
 	    title: gettext('Disks'),
 	    includePartitions: true,
+	    supportsWipeDisk: true,
 	    itemId: 'disks',
 	    iconCls: 'fa fa-hdd-o',
 	},
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v3 6/6] tools: prohibit disk wipe of EFI partition

[Markus Frank]

If the GUID is c12a7328-f81f-11d2-ba4b-00a0c93ec93b the partition is an
EFI Partition and should not be wiped.

Since this GUID is used multiple times, a constant for the EFI & BIOS
GUID is useful.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 src/tools/disks/mod.rs | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index beb8178b..0fe57d23 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -39,6 +39,9 @@ lazy_static::lazy_static! {
         regex::Regex::new(r"host[^/]*/session[^/]*").unwrap();
 }
 
+const EFI_PARTITION_TYPE: &str = "c12a7328-f81f-11d2-ba4b-00a0c93ec93b";
+const BIOS_PARTITION_TYPE: &str = "21686148-6449-6e6f-744e-656564454649";
+
 /// Disk management context.
 ///
 /// This provides access to disk information with some caching for faster querying of multiple
@@ -844,8 +847,8 @@ fn get_partitions_info(
             if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_infos.as_ref()) {
                 for info in infos.iter().filter(|i| i.path.eq(devpath)) {
                     used = match info.partition_type.as_deref() {
-                        Some("21686148-6449-6e6f-744e-656564454649") => PartitionUsageType::BIOS,
-                        Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => PartitionUsageType::EFI,
+                        Some(BIOS_PARTITION_TYPE) => PartitionUsageType::BIOS,
+                        Some(EFI_PARTITION_TYPE) => PartitionUsageType::EFI,
                         Some("6a945a3b-1dd2-11b2-99a6-080020736631") => {
                             PartitionUsageType::ZfsReserved
                         }
@@ -1080,6 +1083,12 @@ pub fn wipe_blockdev(disk: &Disk, worker: Arc<WorkerTask>) -> Result<(), Error>
     for disk_info in get_lsblk_info()?.iter() {
         if disk_info.path == disk_path_str && disk_info.partition_type.is_some() {
             is_partition = true;
+            if matches!(
+                disk_info.partition_type.as_deref(),
+                Some(EFI_PARTITION_TYPE)
+            ) {
+                bail!("You will not be able to boot if you wipe the EFI partition.");
+            }
         }
     }
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup v3 0/6] fix #3690: wipe disk

[Lukas Wagner]

On 11/28/23 14:23, Markus Frank wrote:
> Wipe a disk/partition like we do in PVE. (wipefs, dd, change_parttype to 0x83
> with sgdisk)
> 

Looks good to me now, my comments from the first review
have been addressed and everything seems to work as expected.
Tested wiping individual partitions as well as whole disks.
EFI disks are protected (maybe this should be done with BIOS Boot 
partitions as well?) from accidental wipes.

The task log could be improved (e.g. printing the actual commands that 
were executed, 'wipe disk /sdx' is printed twice), but this can happen 
in a followup IMO.

Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup v3 0/6] fix #3690: wipe disk

[Max Carrara]

On 11/28/23 14:23, Markus Frank wrote:
> Wipe a disk/partition like we do in PVE. (wipefs, dd, change_parttype to 0x83
> with sgdisk)
> 
> v3:
> * rebase on master
> * added more informations to commit message
> 
> v2:
> * change unwraps to matches
> * renamed regex

Looks pretty good overall to me; however, I do have a couple things to note:

* It's possible to also wipe individual partitions using this (as the code
  suggests) so maybe a it would be beneficial or more user-friendly to change
  the text of e.g. the button to "Wipe Partition" if the user clicks on a
  partition instead of a disk? Or maybe just change it to "Wipe" to cover
  both cases?

* An error will be thrown if one attempts do wipe a disk / partition that's
  currently mounted:

# proxmox-backup-manager disk wipe sdd
wipe disk sdd
Wiping block device /dev/sdd
TASK ERROR: command "wipefs" "--all" "/dev/sdd1" "/dev/sdd" failed - status code: 1 - wipefs: error: /dev/sdd1: probing initialization failed: Device or resource busy

  Maybe this case could be handled in a more user-friendly manner as well?
  E.g. checking which disks / partitions are mounted first and then showing an
  error with a more expressive message.

* There are a couple things clippy seems to complain about, but nothing that's
  an immediate code smell.

Overall it looks pretty good to me though; the things I mentioned above are
somewhat bikesheddy can also be addressed in a follow-up.


Tested-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@proxmox.com>


> 
> Markus Frank (6):
>   fix #3690: pbs_api_types: add regex, format & schema for partition
>     names to pbs-api-types
>   fix #3690: tools: add wipe_blockdev & change_parttype rust equivalent
>   fix #3690: api: add function wipe_disk
>   fix #3690: cli: add function wipe_disk
>   fix #3690: ui: enable wipe disk in StorageAndDisks
>   tools: prohibit disk wipe of EFI partition
> 
>  pbs-api-types/src/lib.rs               |  10 ++
>  src/api2/node/disks/mod.rs             |  53 +++++++++-
>  src/bin/proxmox_backup_manager/disk.rs |  38 ++++++-
>  src/tools/disks/mod.rs                 | 138 ++++++++++++++++++++++++-
>  www/Utils.js                           |   1 +
>  www/panel/StorageAndDisks.js           |   1 +
>  6 files changed, 232 insertions(+), 9 deletions(-)
> 

[pbs-devel] applied: [PATCH proxmox-backup v3 1/6] fix #3690: pbs_api_types: add regex, format & schema for partition names to pbs-api-types

[Thomas Lamprecht]

Am 28/11/2023 um 14:23 schrieb Markus Frank:
> The new regex is similar to BLOCKDEVICE_NAME_REGEX but also allows
> numbers at the end of the device name (also allows partitions names).
> For nvme partitions it also allows the letter p and a number.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  pbs-api-types/src/lib.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
>

applied, but reworded subject, it had pbs api types in there in two different
spelling variant and was all to readable in general, thanks!

[pbs-devel] applied: [PATCH proxmox-backup v3 2/6] fix #3690: tools: add wipe_blockdev & change_parttype rust equivalent

[Thomas Lamprecht]

Am 28/11/2023 um 14:23 schrieb Markus Frank:
> The wipe_blockdev & change_parttype functions are similar to
> PVE::Diskmanage's wipe_blockdev & change_parttype functions.
> 
> The partition_by_name & complete_partition_name functions are
> modified disk_by_name & complete_disk_name functions for partitions.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/tools/disks/mod.rs | 125 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 124 insertions(+), 1 deletion(-)
> 
>

applied, with slight subscet reword of subject thanks!

[pbs-devel] applied: [PATCH proxmox-backup v3 3/6] fix #3690: api: add function wipe_disk

[Thomas Lamprecht]

Am 28/11/2023 um 14:23 schrieb Markus Frank:
> An api function similar to PVE wipedisk function that takes
> a disk/partition dev name as argument to wipe it in a new WorkerTask thread.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/api2/node/disks/mod.rs | 53 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 50 insertions(+), 3 deletions(-)
> 
>

applied, with slight subject reword, thanks!

[pbs-devel] applied: [PATCH proxmox-backup v3 4/6] fix #3690: cli: add function wipe_disk

[Thomas Lamprecht]

Am 28/11/2023 um 14:23 schrieb Markus Frank:
> A new cli subcommand which calls the api wipe_disk function
> to wipe a disk/partition with a specified dev name.
> 
> Examples:
> proxmox-backup-manager disk wipe sda2
> proxmox-backup-manager disk wipe sda
> proxmox-backup-manager disk wipe nvme0n1p1
> 
> The complete_partition_name from tools/disks/mod.rs is used for
> command completion.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/bin/proxmox_backup_manager/disk.rs | 38 ++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
>

applied, with slight subject reword, thanks!

As this has no confirmation and is just taking any partition name without a
/dev/ (e.g., disk by-id) path, can we at least add a query if stdin is a tty?

[pbs-devel] applied: [PATCH proxmox-backup v3 5/6] fix #3690: ui: enable wipe disk in StorageAndDisks

[Thomas Lamprecht]

Am 28/11/2023 um 14:23 schrieb Markus Frank:
> supportsWipeDisk: true -> enables the wipe button in the WebUI (same as
> in pve)
> 
> The entry for override_task_descriptions is copied from pve-manager.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  www/Utils.js                 | 1 +
>  www/panel/StorageAndDisks.js | 1 +
>  2 files changed, 2 insertions(+)
> 
>

applied, with a commit message reword (please try to avoid copying random
function, variable, .. names, those aren't really useful there) thanks!

Re: [pbs-devel] [PATCH proxmox-backup v3 6/6] tools: prohibit disk wipe of EFI partition

[Thomas Lamprecht]

Am 28/11/2023 um 14:23 schrieb Markus Frank:
> If the GUID is c12a7328-f81f-11d2-ba4b-00a0c93ec93b the partition is an
> EFI Partition and should not be wiped.
> 
> Since this GUID is used multiple times, a constant for the EFI & BIOS
> GUID is useful.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/tools/disks/mod.rs | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index beb8178b..0fe57d23 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -39,6 +39,9 @@ lazy_static::lazy_static! {
>          regex::Regex::new(r"host[^/]*/session[^/]*").unwrap();
>  }
>  
> +const EFI_PARTITION_TYPE: &str = "c12a7328-f81f-11d2-ba4b-00a0c93ec93b";
> +const BIOS_PARTITION_TYPE: &str = "21686148-6449-6e6f-744e-656564454649";
> +
>  /// Disk management context.
>  ///
>  /// This provides access to disk information with some caching for faster querying of multiple
> @@ -844,8 +847,8 @@ fn get_partitions_info(
>              if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_infos.as_ref()) {
>                  for info in infos.iter().filter(|i| i.path.eq(devpath)) {
>                      used = match info.partition_type.as_deref() {
> -                        Some("21686148-6449-6e6f-744e-656564454649") => PartitionUsageType::BIOS,
> -                        Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => PartitionUsageType::EFI,
> +                        Some(BIOS_PARTITION_TYPE) => PartitionUsageType::BIOS,
> +                        Some(EFI_PARTITION_TYPE) => PartitionUsageType::EFI,
>                          Some("6a945a3b-1dd2-11b2-99a6-080020736631") => {
>                              PartitionUsageType::ZfsReserved
>                          }
> @@ -1080,6 +1083,12 @@ pub fn wipe_blockdev(disk: &Disk, worker: Arc<WorkerTask>) -> Result<(), Error>
>      for disk_info in get_lsblk_info()?.iter() {
>          if disk_info.path == disk_path_str && disk_info.partition_type.is_some() {
>              is_partition = true;
> +            if matches!(
> +                disk_info.partition_type.as_deref(),
> +                Some(EFI_PARTITION_TYPE)
> +            ) {
> +                bail!("You will not be able to boot if you wipe the EFI partition.");
> +            }
>          }
>      }
>  

I skipped this one for now, see no real sense in singling this out, and why should
one be forbidden to wipe the paritions of a hard-disk that one moved over from another
system, where it was previously used for booting, or if one has to switch EFI partition
and wants to wipe the old one afterwards?

I'd rather see:
- such special partitions get an extra, specific warning in the UI about the
  potential impact
- a force flag added, which then also passes that to wipedisk to be able to
  actually clear a disk that was in use sometimes
- the same then implemented for Proxmox VE

Re: [pbs-devel] [PATCH proxmox-backup v3 6/6] tools: prohibit disk wipe of EFI partition

[Roland]

>This patch is based on a suggestion by Dominik.
>I am not so sure if we should prohibit wiping EFI partitions.
>Any opinions on this?

i'm a systems admin and following pbs/pve dev a little bit and i just
stumbled across this, raising an eyebrow

i'd second what thomas writes - why shoud a disk with efi partitions be
protected from wiping? it could be ANY important disk with ANY important
data on it, no matter of there is a efi partition or not.

i we add a disk we want to use and want to wipe - we just want to wipe,
no matter what's on it.

>- such special partitions get an extra, specific warning in the UI about the
>  potential impact

yes, that would be probably good.

when wiping a disk, you always should think twice and double check,
anyway...

roland

Am 28.11.23 um 18:48 schrieb Thomas Lamprecht:

> Am 28/11/2023 um 14:23 schrieb Markus Frank:
>> If the GUID is c12a7328-f81f-11d2-ba4b-00a0c93ec93b the partition is an
>> EFI Partition and should not be wiped.
>>
>> Since this GUID is used multiple times, a constant for the EFI & BIOS
>> GUID is useful.
>>
>> Signed-off-by: Markus Frank <m.frank@proxmox.com>
>> ---
>>   src/tools/disks/mod.rs | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
>> index beb8178b..0fe57d23 100644
>> --- a/src/tools/disks/mod.rs
>> +++ b/src/tools/disks/mod.rs
>> @@ -39,6 +39,9 @@ lazy_static::lazy_static! {
>>           regex::Regex::new(r"host[^/]*/session[^/]*").unwrap();
>>   }
>>
>> +const EFI_PARTITION_TYPE: &str = "c12a7328-f81f-11d2-ba4b-00a0c93ec93b";
>> +const BIOS_PARTITION_TYPE: &str = "21686148-6449-6e6f-744e-656564454649";
>> +
>>   /// Disk management context.
>>   ///
>>   /// This provides access to disk information with some caching for faster querying of multiple
>> @@ -844,8 +847,8 @@ fn get_partitions_info(
>>               if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_infos.as_ref()) {
>>                   for info in infos.iter().filter(|i| i.path.eq(devpath)) {
>>                       used = match info.partition_type.as_deref() {
>> -                        Some("21686148-6449-6e6f-744e-656564454649") => PartitionUsageType::BIOS,
>> -                        Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => PartitionUsageType::EFI,
>> +                        Some(BIOS_PARTITION_TYPE) => PartitionUsageType::BIOS,
>> +                        Some(EFI_PARTITION_TYPE) => PartitionUsageType::EFI,
>>                           Some("6a945a3b-1dd2-11b2-99a6-080020736631") => {
>>                               PartitionUsageType::ZfsReserved
>>                           }
>> @@ -1080,6 +1083,12 @@ pub fn wipe_blockdev(disk: &Disk, worker: Arc<WorkerTask>) -> Result<(), Error>
>>       for disk_info in get_lsblk_info()?.iter() {
>>           if disk_info.path == disk_path_str && disk_info.partition_type.is_some() {
>>               is_partition = true;
>> +            if matches!(
>> +                disk_info.partition_type.as_deref(),
>> +                Some(EFI_PARTITION_TYPE)
>> +            ) {
>> +                bail!("You will not be able to boot if you wipe the EFI partition.");
>> +            }
>>           }
>>       }
>>
> I skipped this one for now, see no real sense in singling this out, and why should
> one be forbidden to wipe the paritions of a hard-disk that one moved over from another
> system, where it was previously used for booting, or if one has to switch EFI partition
> and wants to wipe the old one afterwards?
>
> I'd rather see:
> - such special partitions get an extra, specific warning in the UI about the
>    potential impact
> - a force flag added, which then also passes that to wipedisk to be able to
>    actually clear a disk that was in use sometimes
> - the same then implemented for Proxmox VE
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>