From: Lukas Wagner <l.wagner@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Markus Frank <m.frank@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup 2/6] feature #3690: add wipe_blockdev & change_parttype rust equivalent
Date: Mon, 27 Nov 2023 17:29:04 +0100 [thread overview]
Message-ID: <9cdef563-efe8-4afa-9fee-02dcd7d6543e@proxmox.com> (raw)
In-Reply-To: <20231110135010.106015-2-m.frank@proxmox.com>
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
next prev parent reply other threads:[~2023-11-27 16:29 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-10 13:50 [pbs-devel] [PATCH proxmox-backup 1/6] feature #3690: add regex, format & schema for partition names to pbs-api-types Markus Frank
2023-11-10 13:50 ` [pbs-devel] [PATCH proxmox-backup 2/6] feature #3690: add wipe_blockdev & change_parttype rust equivalent Markus Frank
2023-11-27 16:29 ` Lukas Wagner [this message]
2023-11-10 13:50 ` [pbs-devel] [PATCH proxmox-backup 3/6] feature #3690: api: add function wipe_disk Markus Frank
2023-11-10 13:50 ` [pbs-devel] [PATCH proxmox-backup 4/6] feature #3690: cli: " Markus Frank
2023-11-10 13:50 ` [pbs-devel] [PATCH proxmox-backup 5/6] feature #3690: ui: enable wipe disk in StorageAndDisks Markus Frank
2023-11-27 16:29 ` Lukas Wagner
2023-11-10 13:50 ` [pbs-devel] [PATCH proxmox-backup 6/6] prohibit disk wipe of EFI partition Markus Frank
2023-11-27 16:29 ` Lukas Wagner
2023-11-27 16:29 ` [pbs-devel] [PATCH proxmox-backup 1/6] feature #3690: add regex, format & schema for partition names to pbs-api-types Lukas Wagner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9cdef563-efe8-4afa-9fee-02dcd7d6543e@proxmox.com \
--to=l.wagner@proxmox.com \
--cc=m.frank@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal