From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 720D31FF185 for ; Mon, 4 Aug 2025 16:22:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3EBD933C7E; Mon, 4 Aug 2025 16:24:17 +0200 (CEST) Message-ID: <04be1dbf-a44f-4058-a61b-8fbaa8c05946@proxmox.com> Date: Mon, 4 Aug 2025 16:24:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Wolfgang Bumiller References: <20250731140855.573717-1-s.hanreich@proxmox.com> <20250731140855.573717-5-s.hanreich@proxmox.com> <4bs5sbsu3jnjtxtz3jiryeepw6od4nd2dd6anzdcc4n66lthbv@eqqjtpokkw2m> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <4bs5sbsu3jnjtxtz3jiryeepw6od4nd2dd6anzdcc4n66lthbv@eqqjtpokkw2m> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.705 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 Subject: Re: [pbs-devel] [PATCH proxmox v4 3/3] network-api: add rename_interfaces method 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: , Reply-To: Proxmox Backup Server development discussion Cc: pbs-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" thanks for the review! On 8/4/25 2:55 PM, Wolfgang Bumiller wrote: > On Thu, Jul 31, 2025 at 04:08:47PM +0200, Stefan Hanreich wrote: >> Used for batch renaming interfaces in the /e/n/i configuration file by >> the proxmox-network-interface-pinning tool. >> >> Signed-off-by: Stefan Hanreich >> Tested-by: Christian Ebner >> --- >> proxmox-network-api/src/config/mod.rs | 68 +++++++++++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> >> diff --git a/proxmox-network-api/src/config/mod.rs b/proxmox-network-api/src/config/mod.rs >> index e8cb81d1..3b2ffdc2 100644 >> --- a/proxmox-network-api/src/config/mod.rs >> +++ b/proxmox-network-api/src/config/mod.rs >> @@ -267,6 +267,74 @@ impl NetworkConfig { >> Ok(interface) >> } >> >> + pub fn rename_interfaces(&mut self, mapping: &HashMap) -> Result<(), Error> { >> + for (old_name, new_name) in mapping.iter() { >> + self.interfaces >> + .remove(old_name) >> + .map(|interface| self.interfaces.insert(new_name.to_string(), interface)); > > A freestanding `.map` with the `map` doing a thing like that is bad > style. > And shouldn't we also update `.name` right away? > Also consider that we should probably issue a warning if the new name > already existed? > > if let Some(mut interface) == self.interfaces.remove(old_name) { > interface.name = new_name.clone(); > if self.interfaces.insert(new_name.to_string(), interface).is_some() { > warn about this > } > } done >> + >> + if let Some(idx) = self >> + .order >> + .iter() >> + .position(|elem| matches!(elem, NetworkOrderEntry::Iface(name) if name == new_name)) > > Shouldn't this check for `old_name`? > > Also, could use `iter_mut().find()` and then assign to resulting mut ref > > if let Some(elem) = self.order.iter_mut().find(...) { > *elem = ... > } > >> + { >> + self.order[idx] = NetworkOrderEntry::Iface(new_name.to_string()); >> + } >> + } >> + >> + for interface in self.interfaces.values_mut() { >> + if let Some(new_name) = mapping.get(&interface.name) { >> + interface.name = new_name.to_string(); >> + } >> + >> + if let Some(bridge_ports) = interface.bridge_ports.take() { >> + interface.bridge_ports = Some( >> + bridge_ports >> + .into_iter() >> + .map(|interface| { >> + mapping >> + .get(&interface) >> + .map(String::from) > > could just use `.cloned()` done >> + .unwrap_or(interface) >> + }) >> + .collect(), >> + ) >> + } >> + >> + if let Some(vlan_raw_device) = interface.vlan_raw_device.take() { >> + if let Some(new_name) = mapping.get(&vlan_raw_device) { >> + interface.vlan_raw_device = Some(new_name.to_string()); >> + } else { >> + interface.vlan_raw_device = Some(vlan_raw_device); >> + } >> + } > > could skip the else if we modify in-place with `.as_mut()`: > > if let Some(vlan_raw_device) = interface.vlan_raw_device.as_mut() { > if let Some(new_name) = mapping.get(&*vlan_raw_device) { // note the extra '*' deref > *vlan_raw_device = Some(new_name.clone()); > } > } done >> + >> + if let Some(slaves) = interface.slaves.take() { >> + interface.slaves = Some( >> + slaves >> + .into_iter() >> + .map(|interface| { >> + mapping >> + .get(&interface) >> + .map(String::from) >> + .unwrap_or(interface) >> + }) >> + .collect(), >> + ) >> + } >> + >> + if let Some(bond_primary) = interface.bond_primary.take() { > > same as the vlan_raw_device case done >> + if let Some(new_name) = mapping.get(&bond_primary) { >> + interface.bond_primary = Some(new_name.to_string()); >> + } else { >> + interface.bond_primary = Some(bond_primary); >> + } >> + } >> + } >> + >> + Ok(()) >> + } >> + >> /// Check that there is no other gateway. >> /// >> /// The gateway property is only allowed on passed 'iface'. This should be >> -- >> 2.47.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel