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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <c.heiss@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
> 
>  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.