[pve-devel] [PATCH pve-common V2] Inotify: fix mtu check

[Alexandre DERUMIER]

> changelog: bond with ifupdown2 is working like ifupdown 

>>Please put patch changelogs after the '---' line 

Ok, no problem


>>I'm really not a fan of the 'single-line postfix-if inside regular if 
>>block' style. 
>>Also, please use helper variables when accessing the same hash element 
>>so many times. 

Ok, will do for next time. thanks.



>>If no child mtu is set we can just return, as making it default to the 
>>parent mtu has the same effect as all it does is make it pass the checks 
>>successfully. 
>>
>>Maybe the whole thing could look more like this? 
>>my $cmtu = $ifaces->{$child}->{mtu}; 
>>return if !$cmtu; 
>>
>>my $parentdata = $ifaces->{$parent}; 
>>my $pmtu = $parentdata->{mtu}; 
>>$pmtu = $cmtu if $parentdata->{type} eq 'bond' && !$pmtu; 
>>$pmtu = 1500 if !$pmtu; 

seem to be ok for me, I'll resent a v3. 
Thanks !








----- Mail original -----
De: "Wolfgang Bumiller" <w.bumiller at proxmox.com>
À: "aderumier" <aderumier at odiso.com>
Cc: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Mardi 4 Septembre 2018 14:39:44
Objet: Re: [pve-devel] [PATCH pve-common V2] Inotify: fix mtu check

On Mon, Sep 03, 2018 at 03:52:41PM +0200, Alexandre Derumier wrote: 
> changelog: bond with ifupdown2 is working like ifupdown 

Please put patch changelogs after the '---' line 

> 
> - special check for bond, set parent mtu from slaves mtu if no defined. 
> 
> - error if parent mtu is lower than child mtu (not bigger) 
> 
> - child inherit from parent mtu if not defined 
> 
> - fix vlan check (parent/child was inverted) 
> --- 
> src/PVE/INotify.pm | 12 ++++++++---- 
> 1 file changed, 8 insertions(+), 4 deletions(-) 
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm 
> index 4cf8699..5d87261 100644 
> --- a/src/PVE/INotify.pm 
> +++ b/src/PVE/INotify.pm 
> @@ -759,11 +759,15 @@ my $check_mtu = sub { 
> die "check mtu - missing parent interface\n" if !$parent; 
> die "check mtu - missing child interface\n" if !$child; 
> 
> + if($ifaces->{$parent}->{type} eq 'bond') { 
> + $ifaces->{$parent}->{mtu} = $ifaces->{$child}->{mtu} if (!$ifaces->{$parent}->{mtu} && $ifaces->{$child}->{mtu}); 
> + } 

I'm really not a fan of the 'single-line postfix-if inside regular if 
block' style. 
Also, please use helper variables when accessing the same hash element 
so many times. 

> + 
> my $pmtu = $ifaces->{$parent}->{mtu} ? $ifaces->{$parent}->{mtu} : 1500; 
> - my $cmtu = $ifaces->{$child}->{mtu} ? $ifaces->{$child}->{mtu} : 1500; 
> + my $cmtu = $ifaces->{$child}->{mtu} ? $ifaces->{$child}->{mtu} : $pmtu; 
If no child mtu is set we can just return, as making it default to the 
parent mtu has the same effect as all it does is make it pass the checks 
successfully. 

Maybe the whole thing could look more like this? 
my $cmtu = $ifaces->{$child}->{mtu}; 
return if !$cmtu; 

my $parentdata = $ifaces->{$parent}; 
my $pmtu = $parentdata->{mtu}; 
$pmtu = $cmtu if $parentdata->{type} eq 'bond' && !$pmtu; 
$pmtu = 1500 if !$pmtu; 

... error cases here 
> 
> - die "interface '$parent' - mtu $pmtu is bigger than '$child' - mtu $cmtu\n" 
> - if $pmtu > $cmtu; 
> + die "interface '$parent' - mtu $pmtu is lower than '$child' - mtu $cmtu\n" 
> + if $pmtu < $cmtu; 
> }; 
> 
> # config => { 
> @@ -1393,7 +1397,7 @@ sub __write_etc_network_interfaces { 
> die "vlan '$iface' - wrong interface type on parent '$p' " . 
> "('$n->{type}' != 'eth|bond|bridge' )\n"; 
> } 
> - &$check_mtu($ifaces, $iface, $p); 
> + &$check_mtu($ifaces, $p, $iface); 
> } 
> } 
> 
> -- 
> 2.11.0