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

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

[Markus Frank]

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 4306eca3..af8ad9cc 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_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_PARTITION_NAME_FORMAT: ApiStringFormat =
+    ApiStringFormat::Pattern(&BLOCKDEVICE_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_PARTITION_NAME_SCHEMA: Schema =
+    StringSchema::new("(Partition) block device name (/sys/class/block/<name>).")
+        .format(&BLOCKDEVICE_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 2/6] feature #3690: 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 | 90 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 72e374ca..75d84696 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -19,7 +19,7 @@ use proxmox_lang::{io_bail, io_format_err};
 use proxmox_schema::api;
 use proxmox_sys::linux::procfs::{mountinfo::Device, MountInfo};
 
-use pbs_api_types::BLOCKDEVICE_NAME_REGEX;
+use pbs_api_types::{BLOCKDEVICE_NAME_REGEX, BLOCKDEVICE_PARTITION_NAME_REGEX};
 
 mod zfs;
 pub use zfs::*;
@@ -111,6 +111,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 +1062,72 @@ 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) -> Result<(), Error> {
+    let disk_path = disk.device_path().unwrap();
+
+    let mut is_partition = false;
+    for disk_i in get_lsblk_info()?.iter() {
+        if disk_i.path == disk_path.to_str().unwrap() && disk_i.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.iter() {
+        let part_path = part_disk.device_path().unwrap();
+        to_wipe.push(part_path.to_path_buf());
+    }
+
+    to_wipe.push(disk_path.to_path_buf());
+
+    println!("Wiping block device {}", disk_path.display());
+
+    let mut wipefs_command = std::process::Command::new("wipefs");
+    wipefs_command.arg("--all").args(&to_wipe);
+
+    proxmox_sys::command::run_command(wipefs_command, None)?;
+
+    let size = disk.size().map(|size| size / 1024 / 1024)?;
+    let count = size.min(200);
+
+    let mut dd_command = std::process::Command::new("dd");
+    let args = [
+        "if=/dev/zero",
+        &format!("of={}", disk_path.display()),
+        "bs=1M",
+        "conv=fdatasync",
+        &format!("count={}", count),
+    ];
+    dd_command.args(args);
+
+    proxmox_sys::command::run_command(dd_command, None)?;
+
+    if is_partition {
+        change_parttype(&disk, "8300")?;
+    }
+
+    Ok(())
+}
+
+pub fn change_parttype(part_disk: &Disk, part_type: &str) -> Result<(), Error> {
+    let part_path = part_disk.device_path().unwrap();
+    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));
+        sgdisk_command.arg(&part_disk.parent().unwrap().device_path().unwrap());
+        proxmox_sys::command::run_command(sgdisk_command, None)?;
+    }
+    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 +1208,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_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 3/6] feature #3690: api: add function wipe_disk

[Markus Frank]

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..55da0b54 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_NAME_SCHEMA, BLOCKDEVICE_PARTITION_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_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)?;
+
+            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 4/6] feature #3690: cli: add function wipe_disk

[Markus Frank]

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

diff --git a/src/bin/proxmox_backup_manager/disk.rs b/src/bin/proxmox_backup_manager/disk.rs
index 73cb95e6..7505a42a 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_NAME_SCHEMA, BLOCKDEVICE_PARTITION_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_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: {
@@ -350,10 +376,10 @@ pub fn filesystem_commands() -> CommandLineInterface {
             CliCommand::new(&API_METHOD_CREATE_DATASTORE_DISK)
                 .arg_param(&["name"])
                 .completion_cb("disk", complete_disk_name),
-        ).insert(
+        )
+        .insert(
             "delete",
-            CliCommand::new(&API_METHOD_DELETE_DATASTORE_DISK)
-                .arg_param(&["name"]),
+            CliCommand::new(&API_METHOD_DELETE_DATASTORE_DISK).arg_param(&["name"]),
         );
 
     cmd_def.into()
@@ -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 5/6] feature #3690: ui: enable wipe disk in StorageAndDisks

[Markus Frank]

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

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 6/6] prohibit disk wipe of EFI partition

[Markus Frank]

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
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?

 src/tools/disks/mod.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 75d84696..8a28159e 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -1071,6 +1071,12 @@ pub fn wipe_blockdev(disk: &Disk) -> Result<(), Error> {
     for disk_i in get_lsblk_info()?.iter() {
         if disk_i.path == disk_path.to_str().unwrap() && disk_i.partition_type.is_some() {
             is_partition = true;
+            if matches!(
+                disk_i.partition_type.as_deref(),
+                Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b")
+            ) {
+                bail!("You will not be able to boot if you wipe the EFI partition.");
+            }
         }
     }
 
-- 
2.39.2

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

[Lukas Wagner]

Hello, first some general remarks about this series:

  - the prefix for the commits should be 'fix #num:' or 'close #num:' [1]
  - patches 1, 2 and 6 are missing subsystem tags (e.g. for 1 this could 
be 'api-types')
  - A cover letter with some brief summary of the changes would be nice, 
so that I don't have to pull up the bugzilla entry first to see what 
this series is actually about.


The features seems to work as intended, I tested it on the latest 
master. A few things should be changed though before this goes in,
see the inline comments for the individual patches.

[1] 
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages


On 11/10/23 14:50, Markus Frank wrote:
> 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 4306eca3..af8ad9cc 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_PARTITION_NAME_REGEX = r"^(?:(?:h|s|x?v)d[a-z]+\d*)|(?:nvme\d+n\d+(p\d+)?)$";

It appears that this regex also matches disks, not only partitions (e.g 
sda AND sda1 both match). From the rest of the code this seems intended, 
since you are also able to wipe disks AND partitions, but maybe the name 
for this constant should reflect that.

>       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_PARTITION_NAME_FORMAT: ApiStringFormat =
> +    ApiStringFormat::Pattern(&BLOCKDEVICE_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_PARTITION_NAME_SCHEMA: Schema =
> +    StringSchema::new("(Partition) block device name (/sys/class/block/<name>).")
> +        .format(&BLOCKDEVICE_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();
>   

-- 
- Lukas

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

[Lukas Wagner]

Hi Markus,

some comments inline.

On 11/10/23 14:50, Markus Frank wrote:
> 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 | 90 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index 72e374ca..75d84696 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -19,7 +19,7 @@ use proxmox_lang::{io_bail, io_format_err};
>   use proxmox_schema::api;
>   use proxmox_sys::linux::procfs::{mountinfo::Device, MountInfo};
>   
> -use pbs_api_types::BLOCKDEVICE_NAME_REGEX;
> +use pbs_api_types::{BLOCKDEVICE_NAME_REGEX, BLOCKDEVICE_PARTITION_NAME_REGEX};
>   
>   mod zfs;
>   pub use zfs::*;
> @@ -111,6 +111,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 +1062,72 @@ 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) -> Result<(), Error> {
> +    let disk_path = disk.device_path().unwrap();

Really don't like the .unwrap() here. If you can prove that this option
will never be `None` there should be a comment that explains it,
otherwise I'd just convert the None to an appropriate Error and bubble
it up to the caller.

> +
> +    let mut is_partition = false;
> +    for disk_i in get_lsblk_info()?.iter() {
Maybe call it `disk_info` instead?

> +        if disk_i.path == disk_path.to_str().unwrap() && disk_i.partition_type.is_some() {
                                                 ^
`to_str` can return None (e.g. in case the path is not valid unicode) - 
better be defensive here and return an error.


> +            is_partition = true;
> +        }
> +    }
> +
> +    let mut to_wipe: Vec<PathBuf> = Vec::new();
> +
> +    let partitions_map = disk.partitions()?;
> +    for (_, part_disk) in partitions_map.iter() {
Since you discard the key, you should use partions_map.values():

    for part_disk in partitions_map.values() {
       ...
    }


> +        let part_path = part_disk.device_path().unwrap();
                                                             ^
Unwrap again

> +        to_wipe.push(part_path.to_path_buf());
> +    }
> +
> +    to_wipe.push(disk_path.to_path_buf());
> +
> +    println!("Wiping block device {}", disk_path.display());

This is never printed to the task log. Instead of printing to
stdout, you should pass the worker task to the function and then use
`task_log!`

Note: Once Gabriel's tracing-patches have landed, this should probably 
be a tracing::info!, the reference to the worker task is not needed any 
more after that.

> +
> +    let mut wipefs_command = std::process::Command::new("wipefs");
> +    wipefs_command.arg("--all").args(&to_wipe);
> +
> +    proxmox_sys::command::run_command(wipefs_command, None)?;

This returns a String, which contains the stdout/sterr output from 
wipefs. Maybe log the output (I don't know if wipefs actually logs 
anything, but even if not it would not hurt)?

> +
> +    let size = disk.size().map(|size| size / 1024 / 1024)?;
> +    let count = size.min(200);
> +
> +    let mut dd_command = std::process::Command::new("dd");
> +    let args = [
> +        "if=/dev/zero",
> +        &format!("of={}", disk_path.display()),

.display() can be lossy (skipping anything that is not valid unicode)

But since .args wants AsRef<&OsStr> anyway, you could:

let mut of_path = OsString::from("of=");
of_path.push(disk_path);

and then use that in `args`? I guess then
the other params need to be the same type of course.
(e.g. "if=/dev/zero".into())


> +        "bs=1M",
> +        "conv=fdatasync",
> +        &format!("count={}", count),
> +    ];
> +    dd_command.args(args);
> +
> +    proxmox_sys::command::run_command(dd_command, None)?;
Same here, I think the output should be logged.

> +
> +    if is_partition {
> +        change_parttype(&disk, "8300")?;
Maybe extract this into a constant e.g. PARTITION_TYPE_LINUX ?
Or, alternatively, add a comment that explains what this magic
string actually is.

> +    }
> +
> +    Ok(())
> +}
> +
> +pub fn change_parttype(part_disk: &Disk, part_type: &str) -> Result<(), Error> {
> +    let part_path = part_disk.device_path().unwrap();
.unwrap again

> +    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));
> +        sgdisk_command.arg(&part_disk.parent().unwrap().device_path().unwrap());
.unwraps again

> +        proxmox_sys::command::run_command(sgdisk_command, None)?;
Same as above, maybe log the output? (not sure again if sgdisk actually 
prints anything. Maybe there is a --verbose that could be used then?

Maybe it would also be good to print the command that was executed to 
the logs - so that in case anything goes wrong, the admin sees what 
steps were performed. (applies to the other commands as well)

> +    }
> +    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 +1208,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_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.

-- 
- Lukas

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

[Lukas Wagner]

General note about the UI patch:

You should also add an entry in in www/Utils.js -> 
override_task_descriptions, so that tasks of type 'wipedisk' are 
rendered properly in the task history. I guess you can just copy the 
entry from PVE.

On 11/10/23 14:50, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   www/panel/StorageAndDisks.js | 1 +
>   1 file changed, 1 insertion(+)
> 
> 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',
>   	},

-- 
- Lukas

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

[Lukas Wagner]

On 11/10/23 14:50, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> 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?
> 
>   src/tools/disks/mod.rs | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index 75d84696..8a28159e 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -1071,6 +1071,12 @@ pub fn wipe_blockdev(disk: &Disk) -> Result<(), Error> {
>       for disk_i in get_lsblk_info()?.iter() {
>           if disk_i.path == disk_path.to_str().unwrap() && disk_i.partition_type.is_some() {
>               is_partition = true;
> +            if matches!(
> +                disk_i.partition_type.as_deref(),
> +                Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b")
Maybe add some comment on what this magic string means, e.g. with
a link to some spec that defines that?
Could also extract the string into a local constant with a good name 
(e.g. EFI_PARTITION_TYPE, or something alike),
that would maybe be a bit nicer to read.

> +            ) {
> +                bail!("You will not be able to boot if you wipe the EFI partition.");
> +            }
>           }
>       }
>   

-- 
- Lukas