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

[Max Carrara <m.carrara@proxmox.com>]

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(-)
>