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 1B0D8931C1 for ; Fri, 17 Feb 2023 15:34:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EDBEE78BF for ; Fri, 17 Feb 2023 15:34:26 +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 ; Fri, 17 Feb 2023 15:34:25 +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 5AC6247728 for ; Fri, 17 Feb 2023 15:34:25 +0100 (CET) Date: Fri, 17 Feb 2023 15:34:24 +0100 From: Wolfgang Bumiller To: Christoph Heiss Cc: pve-devel@lists.proxmox.com Message-ID: <20230217143424.6pp3dao2t3ho6uhu@fwblub> References: <20230215140245.496507-1-c.heiss@proxmox.com> <20230215140245.496507-3-c.heiss@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230215140245.496507-3-c.heiss@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.186 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 Subject: Re: [pve-devel] [PATCH v2 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: Fri, 17 Feb 2023 14:34:27 -0000 On Wed, Feb 15, 2023 at 03:02:43PM +0100, Christoph Heiss wrote: > This pattern is used in multiple places, thus extract it into a > subroutine 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 > > src/PVE/LXC.pm | 28 ++++++++++++++++------------ > src/lxcnetaddbr | 15 ++------------- > 2 files changed, 18 insertions(+), 25 deletions(-) > > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm > index 345e343..0de5ba3 100644 > --- a/src/PVE/LXC.pm > +++ b/src/PVE/LXC.pm > @@ -917,6 +917,18 @@ sub vm_stop_cleanup { > warn $@ if $@; # avoid errors - just warn > } > > +sub net_tap_plug { > + if ($have_sdn) { > + my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_; ^ please don't do this conditional on `$have_sdn`. This just makes it harder to figure out how to use the function later (ie. quicker to write this one time now at the cost of reduced maintainability in the future). I'd much rather additionally have a prototype on the sub which may even declare the $opts parameter explicitly as being optional like so: sub net_tap_plug : prototype($$$$$$;$) { my ($iface, $bridge, $tag, $firewall, $trunks, $rate, $opts) = @_; ... Apart from that, the refactoring makes sense.