On Tue, Feb 21, 2023 at 06:25:39PM +0100, Thomas Lamprecht wrote:
> Am 21/02/2023 um 09:05 schrieb Christoph Heiss:
> > If this network option is set, the host-side link will be forced down
> > and the interface won't be connected to the bridge.
> >
> > Signed-off-by: Christoph Heiss <c.he...@proxmox.com>
> > ---
> > Changes v1 -> v2:
> > * Split trailing whitespace fix into separate patch
> > * Rename option to kebap-case
> > * Proper option comparison using `safe_boolean_ne`
> > * Copy option to new network conf like the other options
> > * Remove the veth interface from the bridge when disconnected
> >
> > Changes v2 -> v3:
> > * Rename option to snake_case again
> > * Moved option hotplug-handling before LXC attach again
>
> while this would work I'd like to avoid duplicating the link_down check logic
> see my reply to patch 2/4
Ack, will do as said/discussed in the replies to patch 2.
>
> >
> > src/PVE/LXC.pm | 41 ++++++++++++++++++++++++++++++++++-------
> > src/PVE/LXC/Config.pm | 6 ++++++
> > src/lxcnetaddbr | 7 +++++--
> > 3 files changed, 45 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> > index d419124..2c10108 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -969,10 +970,28 @@ sub update_net {
> > }
> >
>
> from here until & including the if/else block could probably move to the
> net_tap_plug
> helper, as then we could save repeating most of it in the hotplug_net call
> site.
Ack.
>
> > my ($bridge, $mac, $firewall, $rate) = $newnet->@{'bridge',
> > 'hwaddr', 'firewall', 'rate'};
> > - PVE::LXC::net_tap_plug($veth, $bridge, $newnet->{tag},
> > $firewall, $newnet->{trunks}, $rate, { mac => $mac });
> > +
> > + if (defined($newnet->{link_down})) {
> > + # The interface must not be connected to the designated
> > + # bridge if the link was requested to be disconnected.
> > + # Otherwise it could get re-enabled by something like
> > + # `ifreload`.
> > + #
> > + # Thus only force the host-side link down here and skip
> > + # adding it to the bridge.
>
> (new) comments should expand to 100cc
Good to know! I wasn't sure about that while writing that, will change
that.
> and while highlighting that it could get
> auto "UP'd" unintentionally otherwise is def. warranted here, I'd prefer a bit
> more concise comments as above feels a bit redundant and crowds the function
>
> A one, or maybe two liner should be enough to convey the basic hint, something
> like:
>
> # don't add a disabled iface to the bridge, otherwise e.g. appyling any
> network change
> # (-> ifreload -a) could (re-)activate it unintentionally
Ack, I'll generally try to be more concise in the future with my
comments.
>
> > + PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev',
> > $veth, 'down']);
> > + } else {
> > + # Connect the interface to the bridge
>
> IMO above comments is not adding that much "why" info
I'll remove it, I agree that it does not add much value.
>
> > + PVE::LXC::net_tap_plug(
> > + $veth, $bridge, $newnet->{tag}, $firewall,
> > $newnet->{trunks}, $rate, { mac => $mac });
> > +
> > + # Force the host-side link up if it was previously down.
> > + PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev',
> > $veth, 'up'])
> > + if defined($oldnet->{link_down});
> > + }
>
> > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> > index af25a96..746df7b 100644
> > --- a/src/PVE/LXC/Config.pm
> > +++ b/src/PVE/LXC/Config.pm
> > @@ -814,6 +814,12 @@ our $netconf_desc = {
> > description => "Apply rate limiting to the interface",
> > optional => 1,
> > },
> > + # TODO: Rename to link-down for PVE 8.0
>
> maybe highlight that VMs need to change too here, e.g.:
>
> # TODO: rename this *and* the qemu-server one to [...]
Ack.
>
>
> > diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
> > index ebd6baa..0940206 100755
> > --- a/src/lxcnetaddbr
> > +++ b/src/lxcnetaddbr
>
> > - PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks,
> > $rate, { mac => $hwaddr });
> > + # Only plug the interface into the bridge if it is not set as
> > disconnected by the user.
>
> no hard feelings here but above also reads like the code tells us anyway, so
> could live
> without it, but if you think it really helps I'm fine with that comment too.
Re-reading that, it certainly does seem a bit redundant, I'll probably
remove that with the next spin. Sometimes I unfortunately get a bit
(too) inclined to comment stuff.
>
> > + PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks,
> > $rate, { mac => $hwaddr })
> > + if !defined($net->{link_down});
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
