[pve-devel] applied: [PATCH v3 container 2/4] lxc: Avoid open-coding normal vs SDN-specific tap_plug()

[Thomas Lamprecht]

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 <c.heiss at proxmox.com>
> ---
> 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?