Re: [pbs-devel] [PATCH proxmox v4 3/3] network-api: add rename_interfaces method

[Stefan Hanreich <s.hanreich@proxmox.com>]

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 <s.hanreich@proxmox.com>
>> Tested-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>  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<String, String>) -> 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