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

[Wolfgang Bumiller]

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