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 DE869B024 for ; Wed, 6 Apr 2022 11:13:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C85E72866D for ; Wed, 6 Apr 2022 11:13:04 +0200 (CEST) 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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 5F87328664 for ; Wed, 6 Apr 2022 11:12:59 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2ED0F41FD2 for ; Wed, 6 Apr 2022 11:12:59 +0200 (CEST) Date: Wed, 6 Apr 2022 11:12:39 +0200 From: Wolfgang Bumiller To: Hannes Laimer Cc: pbs-devel@lists.proxmox.com Message-ID: <20220406091239.3sug3xq2rrw7iulr@olga.proxmox.com> References: <20220404095048.25443-1-h.laimer@proxmox.com> <20220404095048.25443-2-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220404095048.25443-2-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.347 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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 1/3] api2: disks endpoint return partitions 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: Wed, 06 Apr 2022 09:13:34 -0000 On Mon, Apr 04, 2022 at 09:50:46AM +0000, Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > src/api2/node/disks/directory.rs | 2 +- > src/api2/node/disks/mod.rs | 11 ++- > src/api2/node/disks/zfs.rs | 2 +- > src/tools/disks/mod.rs | 121 ++++++++++++++++++++++++++++++- > 4 files changed, 130 insertions(+), 6 deletions(-) > > diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs > index bf1a1be6..79fe2624 100644 > --- a/src/api2/node/disks/directory.rs > +++ b/src/api2/node/disks/directory.rs > @@ -149,7 +149,7 @@ pub fn create_datastore_disk( > > let auth_id = rpcenv.get_auth_id().unwrap(); > > - let info = get_disk_usage_info(&disk, true)?; > + let info = get_disk_usage_info(&disk, true, false)?; I already feel like 2 bool parameters in a row warrants a query builder type sane with defaults. DiskUsageQuery::new(&disk) .smart(false) // optional .partitions(true) // optional .query() but this can be added later (the implementation itself can still stay as a regular function anyway ) > > if info.used != DiskUsageType::Unused { > bail!("disk '{}' is already in use.", disk); (...) > diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs > index 94da7b3a..fb9c78a9 100644 > --- a/src/tools/disks/mod.rs > +++ b/src/tools/disks/mod.rs > @@ -602,6 +602,26 @@ fn get_file_system_devices(lsblk_info: &[LsblkInfo]) -> Result, Err > Ok(device_set) > } > > +#[api()] > +#[derive(Debug, Serialize, Deserialize, PartialEq)] +Clone And if it's just a simple list like this +Copy, however, this should probably contain the file system string in the last member, so not Copy. > +#[serde(rename_all = "lowercase")] > +pub enum PartitionUsageType { > + /// Partition is not used (as far we can tell) > + Unused, > + /// Partition is mounted > + Mounted, I don't think `Mounted` should be in the same enum here. In PVE::Diskmanage we can have a file system type with " (mounted)" attached to it as a string, as well as "just" mounted with no extra info. > + /// Partition is used by LVM > + LVM, > + /// Partition is used by ZFS > + ZFS, > + /// Partition is an EFI partition > + EFI, > + /// Partition is a BIOS partition > + BIOS, > + /// Partition contains a file system label > + FileSystem, This should contain the file system string. > +} > + > #[api()] > #[derive(Debug, Serialize, Deserialize, PartialEq)] > #[serde(rename_all = "lowercase")] (...) > @@ -837,6 +888,71 @@ pub fn get_disks( > > let wwn = disk.wwn().map(|s| s.to_string_lossy().into_owned()); > > + let partitions: Option> = if include_partitions { > + let lsblk_infos = get_lsblk_info(); Please explicitly discard the error with a `.ok()` here and match for `Some` instead of `Ok` later on. > + disk.partitions().map_or(None, |parts| { > + Some( > + parts > + .iter() > + .map(|(nr, disk)| { Too much indentation, please factorize. This looks like $determine_usage in PVE::Diskmanage. If factorized into its own function it's easy to just use `return` (and keep it in the same order as the perl code, particularly the mounted part is a bit different atm.) Makes it easier to check for equivalent API behavior (if we want that). (On a side node: Ideally I *would* like to move the perl & rust code into a direction where we can soon replace `PVE::Diskmanage` itself with this rust code via pve-rs) > + let devpath = disk > + .device_path() > + .map(|p| p.to_owned()) > + .map(|p| p.to_string_lossy().to_string()); > + > + let mut used = PartitionUsageType::Unused; > + > + if let Some(devnum) = disk.devnum().ok() { > + if lvm_devices.contains(&devnum) { > + used = PartitionUsageType::LVM; > + } > + if zfs_devices.contains(&devnum) { > + used = PartitionUsageType::ZFS; > + } > + } > + match disk.is_mounted() { > + Ok(true) => used = PartitionUsageType::Mounted, > + Ok(false) => {} > + Err(_) => {} // skip devices with undetectable mount status > + } > + > + if let Some(devpath) = devpath.as_ref() { > + if let Ok(infos) = lsblk_infos.as_ref() { Could drop a level of indentation here via a tuple match: if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_info.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 > + } PVE::Diskmanage also has a 'ZFS reserved' case, should we not add this here as well? > + _ => used, > + } > + } > + } > + } > + > + if used == PartitionUsageType::Unused > + && file_system_devices.contains(&devnum) > + { > + used = PartitionUsageType::FileSystem; > + } > + > + PartitionInfo { > + name: format!("{}{}", name, nr), > + devpath, > + used, > + size: disk.size().ok(), > + gpt: disk.has_gpt(), > + } > + }) > + .collect(), > + ) > + }) > + } else { > + None > + }; > + > if usage != DiskUsageType::Mounted { > match scan_partitions(disk_manager.clone(), &lvm_devices, &zfs_devices, &name) { > Ok(part_usage) => { > @@ -870,6 +986,7 @@ pub fn get_disks( > name: name.clone(), > vendor, > model, > + partitions, > serial, > devpath, > size, > -- > 2.30.2