Re: [pbs-devel] [PATCH proxmox-backup 08/10] refactor(api): simplify setting interface properties

[Lukas Wagner <l.wagner@proxmox.com>]

On 1/11/24 16:53, Stefan Lendl wrote:
> Instead of using `if ..is_some()` directly assign the Option properties of
> Interface from Option parameters of update_ and create_interface.
> Code is shorter and cleaner to read.
> 
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>   src/api2/node/network.rs | 54 +++++++++++-----------------------------
>   1 file changed, 14 insertions(+), 40 deletions(-)
> 
> diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs
> index d1393103..84a017e9 100644
> --- a/src/api2/node/network.rs
> +++ b/src/api2/node/network.rs
> @@ -301,25 +301,12 @@ pub fn create_interface(
>   
>       let mut interface = Interface::new(iface.clone());
>       interface.interface_type = interface_type;
> -
> -    if let Some(autostart) = autostart {
> -        interface.autostart = autostart;
> -    }
> -    if method.is_some() {
> -        interface.method = method;
> -    }
> -    if method6.is_some() {
> -        interface.method6 = method6;
> -    }
> -    if mtu.is_some() {
> -        interface.mtu = mtu;
> -    }
> -    if comments.is_some() {
> -        interface.comments = comments;
> -    }
> -    if comments6.is_some() {
> -        interface.comments6 = comments6;
> -    }
> +    interface.autostart = autostart.unwrap_or(false);
> +    interface.method = method;
> +    interface.method6 = method6;
> +    interface.mtu = mtu;
> +    interface.comments = comments;
> +    interface.comments6 = comments6;
>   
>       if let Some(cidr) = cidr {
>           let (_, _, is_v6) = network::parse_cidr(&cidr)?;
> @@ -697,25 +684,16 @@ pub fn update_interface(
>           }
>       }
>   
> -    if let Some(autostart) = autostart {
> -        interface.autostart = autostart;
> -    }
> -    if method.is_some() {
> -        interface.method = method;
> -    }
> -    if method6.is_some() {
> -        interface.method6 = method6;
> -    }
> -    if mtu.is_some() {
> -        interface.mtu = mtu;
> -    }
> +    interface.autostart = autostart.unwrap_or(false);
> +    interface.method = method;
> +    interface.method6 = method6;
> +    interface.mtu = mtu;
> +    interface.bridge_vlan_aware = bridge_vlan_aware;
> +

In general our PUT handlers do not require the caller to send at fields, 
but only those that have changed. Fields that should be cleared are 
represented by the 'deleted' field.

This means the old code is actually necessary: If a field is not 
submitted, it should *not* be changed. Only if a new value is sent, the 
field should be updated.

Your change happens to work from the UI, since the network dialog 
happens to always send all fields - which is not that AFAIK not that 
common for our UI.
Your change however breaks the CLI tool:
For example, set a comment for an interface and then do a

   proxmox-backup-manager network update enp6s18.10 --autostart false

You will notice that comment is cleared after that command :)


>       if let Some(ports) = bridge_ports {
>           let ports = split_interface_list(&ports)?;
>           set_bridge_ports(interface, ports)?;
>       }
> -    if bridge_vlan_aware.is_some() {
> -        interface.bridge_vlan_aware = bridge_vlan_aware;
> -    }
>       if let Some(slaves) = slaves {
>           let slaves = split_interface_list(&slaves)?;
>           set_bond_slaves(interface, slaves)?;
> @@ -768,12 +746,8 @@ pub fn update_interface(
>           interface.gateway6 = Some(gateway6);
>       }
>   
> -    if comments.is_some() {
> -        interface.comments = comments;
> -    }
> -    if comments6.is_some() {
> -        interface.comments6 = comments6;
> -    }
> +    interface.comments = comments;
> +    interface.comments6 = comments6;
>   
>       if interface.cidr.is_some() || interface.gateway.is_some() {
>           interface.method = Some(NetworkConfigMethod::Static);

-- 
- Lukas