From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 47A491FF0B2 for ; Thu, 20 Aug 2026 13:20:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 12E6F2157E; Thu, 20 Aug 2026 13:20:50 +0200 (CEST) Message-ID: <45e83dcf-5fd8-46f4-acdc-8978d8738760@proxmox.com> Date: Thu, 20 Aug 2026 13:20:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH datacenter-manager v2 1/9] lib: api types: add new ResourceView type and move accessors there To: Lukas Wagner , pdm-devel@lists.proxmox.com References: <20260818133000.3793412-1-d.csapak@proxmox.com> <20260818133000.3793412-2-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787224819878 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.832 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: N6RFW5UFYGZETN6AKIJ7NMCMK2LZIEW5 X-Message-ID-Hash: N6RFW5UFYGZETN6AKIJ7NMCMK2LZIEW5 X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 8/20/26 1:06 PM, Lukas Wagner wrote: > Hi Dominik, > > looking good - some of the code is missing doc-strings that could be > added, but that's just a minor nit-pick. yes of course, i'll send a v3 > > On Tue Aug 18, 2026 at 3:25 PM CEST, Dominik Csapak wrote: >> This is intended as a view type for Resource and PveResource, which will >> always share some enum type. This way, we can reuse all accessors >> cheaply for PveResource values without cloning or consuming. >> >> This makes it easier to share code for both of them, for instance in the >> UI where we want to render properties consistently for resources. >> >> Introduces also a AsResourceView trait to make converting more >> ergonomic. >> >> Signed-off-by: Dominik Csapak >> --- >> lib/pdm-api-types/src/resource.rs | 261 +++++++++++++++++++++--------- >> 1 file changed, 184 insertions(+), 77 deletions(-) >> >> diff --git a/lib/pdm-api-types/src/resource.rs b/lib/pdm-api-types/src/resource.rs >> index 303ac3cb..4447aedb 100644 >> --- a/lib/pdm-api-types/src/resource.rs >> +++ b/lib/pdm-api-types/src/resource.rs >> @@ -45,102 +45,31 @@ impl Resource { >> /// Returns the local ID, not a globally unique one, e.g. >> /// `qemu/` >> pub fn id(&self) -> String { >> - match self { >> - Resource::PveStorage(r) => format!("storage/{}/{}", r.node, r.storage), >> - Resource::PveQemu(r) => format!("qemu/{}", r.vmid), >> - Resource::PveLxc(r) => format!("lxc/{}", r.vmid), >> - Resource::PveNode(r) => format!("node/{}", r.node), >> - Resource::PveNetwork(r) => { >> - if let PveNetworkResource::Zone(z) = r { >> - if z.legacy { >> - return format!("sdn/{}/{}", r.node(), r.name()); >> - } >> - } >> - >> - format!("network/{}/{}/{}", r.node(), r.network_type(), r.name()) >> - } >> - Resource::PbsNode(r) => format!("node/{}", r.name), >> - Resource::PbsDatastore(r) => r.name.clone(), >> - } >> + self.as_resource_view().id() >> } >> >> /// Returns the PDM global ID for the resource, e.g. >> /// `remote//guest/` >> pub fn global_id(&self) -> &str { >> - match self { >> - Resource::PveStorage(r) => r.id.as_str(), >> - Resource::PveQemu(r) => r.id.as_str(), >> - Resource::PveLxc(r) => r.id.as_str(), >> - Resource::PveNode(r) => r.id.as_str(), >> - Resource::PveNetwork(r) => r.id(), >> - Resource::PbsNode(r) => r.id.as_str(), >> - Resource::PbsDatastore(r) => r.id.as_str(), >> - } >> + self.as_resource_view().global_id() >> } >> >> /// Returns the "name" of the resource, e.g. the guest name for VMs/Containers or >> /// the hostname for nodes >> pub fn name(&self) -> &str { >> - match self { >> - Resource::PveStorage(r) => r.storage.as_str(), >> - Resource::PveQemu(r) => r.name.as_str(), >> - Resource::PveLxc(r) => r.name.as_str(), >> - Resource::PveNode(r) => r.node.as_str(), >> - Resource::PveNetwork(r) => r.name(), >> - Resource::PbsNode(r) => r.name.as_str(), >> - Resource::PbsDatastore(r) => r.name.as_str(), >> - } >> + self.as_resource_view().name() >> } >> > > While at it, you could add some doc strings here. > >> pub fn resource_type(&self) -> ResourceType { >> - match self { >> - Resource::PveStorage(_) => ResourceType::PveStorage, >> - Resource::PveQemu(_) => ResourceType::PveQemu, >> - Resource::PveLxc(_) => ResourceType::PveLxc, >> - Resource::PveNetwork(_) => ResourceType::PveNetwork, >> - Resource::PveNode(_) | Resource::PbsNode(_) => ResourceType::Node, >> - Resource::PbsDatastore(_) => ResourceType::PbsDatastore, >> - } >> + self.as_resource_view().resource_type() >> } >> > While at it, you could add some doc strings here. > >> pub fn status(&self) -> &str { >> - match self { >> - Resource::PveStorage(r) => r.status.as_str(), >> - Resource::PveQemu(r) => r.status.as_str(), >> - Resource::PveLxc(r) => r.status.as_str(), >> - Resource::PveNode(r) => r.status.as_str(), >> - Resource::PveNetwork(r) => r.status(), >> - Resource::PbsNode(r) => { >> - if r.uptime > 0 { >> - "online" >> - } else { >> - "offline" >> - } >> - } >> - Resource::PbsDatastore(r) => { >> - if r.maintenance.is_none() { >> - "online" >> - } else { >> - "under-maintenance" >> - } >> - } >> - } >> + self.as_resource_view().status() >> } >> > While at it, you could add some doc strings here. > >> pub fn properties(&self) -> String { >> - let mut properties = Vec::new(); >> - if let Resource::PbsDatastore(r) = self { >> - if let Some(backend_type) = &r.backend_type { >> - properties.push(backend_type.to_string()); >> - } >> - if r.backing_device.is_some() { >> - properties.push("removable".to_string()); >> - } >> - if r.usage > PBS_DATASTORE_HIGH_USAGE_THRESHOLD { >> - properties.push("high-usage".to_string()); >> - } >> - } >> - properties.join(",") >> + self.as_resource_view().properties() >> } >> } >> >> @@ -229,6 +158,184 @@ pub enum PveResource { >> Network(PveNetworkResource), >> } >> >> +/// A borrowed view of a resource that can come from a [Resource] or a [PveResource] >> +/// This is useful for having a single implementation of field access regardless which >> +/// of the types is used. >> +#[derive(Clone, Debug, PartialEq)] >> +pub enum ResourceView<'a> { >> + /// Storage resource in PVE. >> + PveStorage(&'a PveStorageResource), >> + /// QEMU guest resource in PVE. >> + PveQemu(&'a PveQemuResource), >> + /// LXC guest resource in PVE. >> + PveLxc(&'a PveLxcResource), >> + /// A PVE node. >> + PveNode(&'a PveNodeResource), >> + /// Network in PVE. >> + PveNetwork(&'a PveNetworkResource), >> + /// A PBS node. >> + PbsNode(&'a PbsNodeResource), >> + /// Datastore on a PBS node. >> + PbsDatastore(&'a PbsDatastoreResource), >> +} >> + >> +impl<'a> ResourceView<'a> { >> + /// Returns the local ID, not a globally unique one, e.g. >> + /// `qemu/` >> + pub fn id(&self) -> String { >> + match self { >> + ResourceView::PveStorage(r) => format!("storage/{}/{}", r.node, r.storage), >> + ResourceView::PveQemu(r) => format!("qemu/{}", r.vmid), >> + ResourceView::PveLxc(r) => format!("lxc/{}", r.vmid), >> + ResourceView::PveNode(r) => format!("node/{}", r.node), >> + ResourceView::PveNetwork(r) => { >> + if let PveNetworkResource::Zone(z) = r { >> + if z.legacy { >> + return format!("sdn/{}/{}", r.node(), r.name()); >> + } >> + } >> + >> + format!("network/{}/{}/{}", r.node(), r.network_type(), r.name()) >> + } >> + ResourceView::PbsNode(r) => format!("node/{}", r.name), >> + ResourceView::PbsDatastore(r) => r.name.clone(), >> + } >> + } >> + >> + /// Returns the PDM global ID for the resource, e.g. >> + /// `remote//guest/` >> + pub fn global_id(&self) -> &'a str { >> + match self { >> + ResourceView::PveStorage(r) => r.id.as_str(), >> + ResourceView::PveQemu(r) => r.id.as_str(), >> + ResourceView::PveLxc(r) => r.id.as_str(), >> + ResourceView::PveNode(r) => r.id.as_str(), >> + ResourceView::PveNetwork(r) => r.id(), >> + ResourceView::PbsNode(r) => r.id.as_str(), >> + ResourceView::PbsDatastore(r) => r.id.as_str(), >> + } >> + } >> + >> + /// Returns the "name" of the resource, e.g. the guest name for VMs/Containers or >> + /// the hostname for nodes >> + pub fn name(&self) -> &'a str { >> + match self { >> + ResourceView::PveStorage(r) => r.storage.as_str(), >> + ResourceView::PveQemu(r) => r.name.as_str(), >> + ResourceView::PveLxc(r) => r.name.as_str(), >> + ResourceView::PveNode(r) => r.node.as_str(), >> + ResourceView::PveNetwork(r) => r.name(), >> + ResourceView::PbsNode(r) => r.name.as_str(), >> + ResourceView::PbsDatastore(r) => r.name.as_str(), >> + } >> + } >> + > > Missing doc strings here. >> + pub fn resource_type(&self) -> ResourceType { >> + match self { >> + ResourceView::PveStorage(_) => ResourceType::PveStorage, >> + ResourceView::PveQemu(_) => ResourceType::PveQemu, >> + ResourceView::PveLxc(_) => ResourceType::PveLxc, >> + ResourceView::PveNetwork(_) => ResourceType::PveNetwork, >> + ResourceView::PveNode(_) | ResourceView::PbsNode(_) => ResourceType::Node, >> + ResourceView::PbsDatastore(_) => ResourceType::PbsDatastore, >> + } >> + } >> + > > here as well >> + pub fn status(&self) -> &'a str { >> + match self { >> + ResourceView::PveStorage(r) => r.status.as_str(), >> + ResourceView::PveQemu(r) => r.status.as_str(), >> + ResourceView::PveLxc(r) => r.status.as_str(), >> + ResourceView::PveNode(r) => r.status.as_str(), >> + ResourceView::PveNetwork(r) => r.status(), >> + ResourceView::PbsNode(r) => { >> + if r.uptime > 0 { >> + "online" >> + } else { >> + "offline" >> + } >> + } >> + ResourceView::PbsDatastore(r) => { >> + if r.maintenance.is_none() { >> + "online" >> + } else { >> + "under-maintenance" >> + } >> + } >> + } >> + } >> + > > here as well >> + pub fn properties(&self) -> String { >> + let mut properties = Vec::new(); >> + if let ResourceView::PbsDatastore(r) = self { >> + if let Some(backend_type) = &r.backend_type { >> + properties.push(backend_type.to_string()); >> + } >> + if r.backing_device.is_some() { >> + properties.push("removable".to_string()); >> + } >> + if r.usage > PBS_DATASTORE_HIGH_USAGE_THRESHOLD { >> + properties.push("high-usage".to_string()); >> + } >> + } >> + properties.join(",") >> + } >> +} >> + >> +impl<'a> From<&'a Resource> for ResourceView<'a> { >> + fn from(value: &'a Resource) -> Self { >> + match value { >> + Resource::PveStorage(r) => ResourceView::PveStorage(r), >> + Resource::PveQemu(r) => ResourceView::PveQemu(r), >> + Resource::PveLxc(r) => ResourceView::PveLxc(r), >> + Resource::PveNode(r) => ResourceView::PveNode(r), >> + Resource::PveNetwork(r) => ResourceView::PveNetwork(r), >> + Resource::PbsNode(r) => ResourceView::PbsNode(r), >> + Resource::PbsDatastore(r) => ResourceView::PbsDatastore(r), >> + } >> + } >> +} >> + >> +impl<'a> From<&'a PveResource> for ResourceView<'a> { >> + fn from(value: &'a PveResource) -> Self { >> + match value { >> + PveResource::Storage(r) => ResourceView::PveStorage(r), >> + PveResource::Qemu(r) => ResourceView::PveQemu(r), >> + PveResource::Lxc(r) => ResourceView::PveLxc(r), >> + PveResource::Node(r) => ResourceView::PveNode(r), >> + PveResource::Network(r) => ResourceView::PveNetwork(r), >> + } >> + } >> +} >> + > > missing doc string here > >> +pub trait AsResourceView { >> + fn as_resource_view(&self) -> ResourceView<'_>; >> +} >> + >> +impl AsResourceView for Resource { >> + fn as_resource_view<'a>(&'a self) -> ResourceView<'a> { >> + ResourceView::from(self) >> + } >> +} >> + >> +impl AsResourceView for &Resource { >> + fn as_resource_view<'a>(&'a self) -> ResourceView<'a> { >> + ResourceView::from(*self) >> + } >> +} >> + >> +impl AsResourceView for PveResource { >> + fn as_resource_view<'a>(&'a self) -> ResourceView<'a> { >> + ResourceView::from(self) >> + } >> +} >> + >> +impl AsResourceView for &PveResource { >> + fn as_resource_view<'a>(&'a self) -> ResourceView<'a> { >> + ResourceView::from(*self) >> + } >> +} >> + >> #[api( >> properties: { >> tags: { >