From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 8E5AC9E68E for ; Mon, 27 Nov 2023 17:29:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 74A01B625 for ; Mon, 27 Nov 2023 17:29:07 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 27 Nov 2023 17:29:06 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 38FF144BF9 for ; Mon, 27 Nov 2023 17:29:06 +0100 (CET) Message-ID: <9cdef563-efe8-4afa-9fee-02dcd7d6543e@proxmox.com> Date: Mon, 27 Nov 2023 17:29:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Lukas Wagner To: Proxmox Backup Server development discussion , Markus Frank References: <20231110135010.106015-1-m.frank@proxmox.com> <20231110135010.106015-2-m.frank@proxmox.com> Content-Language: de-AT, en-US In-Reply-To: <20231110135010.106015-2-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.008 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup 2/6] feature #3690: add wipe_blockdev & change_parttype rust equivalent X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Nov 2023 16:29:37 -0000 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 > --- > 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/`. > + pub fn partition_by_name(self: Arc, name: &str) -> io::Result { > + let syspath = format!("/sys/class/block/{}", name); > + self.disk_by_sys_path(syspath) > + } > + > /// Gather information about mounted disks: > fn mounted_devices(&self) -> Result<&HashSet, 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 = 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 { > let disk_path = match disk.device_path() { > @@ -1136,6 +1208,22 @@ pub fn complete_disk_name(_arg: &str, _param: &HashMap) -> Vec .collect() > } > > +/// Block device partition name completion helper > +pub fn complete_partition_name(_arg: &str, _param: &HashMap) -> Vec { > + 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