From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D4689C1A38 for ; Wed, 17 Jan 2024 10:50:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BDE443FEBC for ; Wed, 17 Jan 2024 10:50:35 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 17 Jan 2024 10:50:35 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0D9C449194 for ; Wed, 17 Jan 2024 10:50:35 +0100 (CET) Message-ID: Date: Wed, 17 Jan 2024 10:50:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Lukas Wagner To: Proxmox Backup Server development discussion , Stefan Lendl References: <20240111155303.1072675-1-s.lendl@proxmox.com> <20240111155303.1072675-17-s.lendl@proxmox.com> Content-Language: de-AT, en-US In-Reply-To: <20240111155303.1072675-17-s.lendl@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.004 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup 08/10] refactor(api): simplify setting interface properties 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: , X-List-Received-Date: Wed, 17 Jan 2024 09:50:35 -0000 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 > --- > 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