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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id CAF2393A8C for ; Tue, 21 Feb 2023 18:07:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AEAD45AB8 for ; Tue, 21 Feb 2023 18:07:24 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 21 Feb 2023 18:07:21 +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 2329247F20 for ; Tue, 21 Feb 2023 18:07:21 +0100 (CET) Message-ID: <57862005-fc5b-e180-c228-165289067254@proxmox.com> Date: Tue, 21 Feb 2023 18:07:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:110.0) Gecko/20100101 Thunderbird/110.0 Content-Language: de-AT, en-GB To: Proxmox VE development discussion , Christoph Heiss References: <20230221080550.43336-1-c.heiss@proxmox.com> <20230221080550.43336-3-c.heiss@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20230221080550.43336-3-c.heiss@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.051 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lxc.pm] Subject: [pve-devel] applied: [PATCH v3 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug() X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Feb 2023 17:07:54 -0000 Am 21/02/2023 um 09:05 schrieb Christoph Heiss: > This pattern is used in multiple places, thus just extract it into a sub > on its own. > > No functional changes. > > Signed-off-by: Christoph Heiss > --- > Might not be the best place for net_tap_plug(), putting this logic > inside PVE::Network would probably make more sense. But that would > entail a (bigger) refactoring, since it then also must be done for all > other tap_*() and veth_*() subroutines (and maybe some other things?) > for consistency.. > In any case, that definitely would be too much for this series. I can do > that, but I'd do it as a follow-up series. > > Changes v1 -> v2: > * New patch > > Changes v2 -> v3: > * Add prototype to net_tap_plug() > > src/PVE/LXC.pm | 28 ++++++++++++++++------------ > src/lxcnetaddbr | 15 ++------------- > 2 files changed, 18 insertions(+), 25 deletions(-) > applied, thanks! But I got some feedback/question inline affecting patch 3/4 > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm > index cbbb82d..d419124 100644 > --- a/src/PVE/LXC.pm > +++ b/src/PVE/LXC.pm > @@ -918,6 +918,18 @@ sub vm_stop_cleanup { > warn $@ if $@; # avoid errors - just warn > } > > +sub net_tap_plug : prototype($$$$$$;$) { > + my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_; IMO having more than ~5 parameter is most of the time a code smell, and sure while we ain't in rust where we can ensure some sane API and existence of struct/trait members or methods it's not really that better to expand everything, as scalar on it's own is way to broad anyway to guarantee anything relevant on calling. So, maybe we could change this to take sub net_tap_plug : prototype($$;$) { my ($iface, $net, $old_net) = @_; as then we might even pull in the whole link_down logic separation in here and avoid duplicating that then again (after just cleaning something similar like that up here). What do you think?