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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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
        }
    }

> +
> +            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()`

> +                                .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());
        }
    }

> +
> +            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

> +                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