[pve-devel] [PATCH pve-common 3/3] Inotify : add bridge ports options

[Wolfgang Bumiller]

On Mon, Jun 25, 2018 at 05:41:35AM +0200, Alexandre Derumier wrote:
> ---
>  src/PVE/INotify.pm | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index 2c1af3c..2401ec0 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -898,7 +898,9 @@ sub __read_etc_network_interfaces {
>  			} else {
>  			    $d->{$id} = 'off';
>  			}
> -		    } elsif ($id eq 'bridge_fd' || $id eq 'bridge_vids') {
> +		    } elsif ($id eq 'bridge_fd' || $id eq 'bridge_vids' || $id eq 'bridge-access' || 
> +			     $id eq 'bridge-learning' || $id eq 'bridge-arp-nd-suppress' || 
> +			     $id eq 'bridge-unicast-flood' || $id eq 'bridge-multicast-flood') {
>  			$d->{$id} = $value;

At some point we need to clean this up and just use a hash declaring
which ids are copied as-is via `$d->{$id} = $value;`
(This is mostly a reminder for me, but if you feel up to it... ;-) )

>  		    } elsif ($id eq 'bridge_vlan_aware') {
>  			$d->{$id} = 1;
> @@ -1150,7 +1152,22 @@ sub __interface_to_string {
>  	    }
>  	    $done->{'vxlan-remoteip'} = 1;
>  	}
> -
> +	if ($d->{'bridge-learning'}) {

Particularly together with the other patch this pattern is really worth
iterating over via a list now ;-)

> +	    $raw .= "\tbridge-learning $d->{'bridge-learning'}\n";
> +	    $done->{'bridge-learning'} = 1;
> +	}
> +	if ($d->{'bridge-arp-nd-suppress'}) {
> +	    $raw .= "\tbridge-arp-nd-suppress $d->{'bridge-arp-nd-suppress'}\n";
> +	    $done->{'bridge-arp-nd-suppress'} = 1;
> +	}
> +	if ($d->{'bridge-unicast-flood'}) {
> +	    $raw .= "\tbridge-unicast-flood $d->{'bridge-unicast-flood'}\n";
> +	    $done->{'bridge-unicast-flood'} = 1;
> +	}
> +	if ($d->{'bridge-multicast-flood'}) {
> +	    $raw .= "\tbridge-multicast-flood $d->{'bridge-multicast-flood'}\n";
> +	    $done->{'bridge-multicast-flood'} = 1;
> +	}
>      } elsif ($d->{type} eq 'OVSBridge') {
>  
>  	$raw .= "\tovs_type $d->{type}\n";
> @@ -1194,6 +1211,13 @@ sub __interface_to_string {
>  	$raw .= "\tovs_type $d->{type}\n";
>  	$done->{ovs_type} = 1;
>  
> +	if ($d->{type} eq 'eth' || $d->{type} eq 'bond' || $d->{type} eq 'vxlan') {

Is this specifically limited to these types of interfaces? Since below
we check whether the interface is part of a bridge I wonder if we should
just skip the type condition. Basically any port which can't be on a
bridge should be caught elsewhere already anyway.

> +	    if ($d->{'bridge-access'}) {
> +		$raw .= "\tbridge-access $d->{'bridge-access'}\n";
> +		$done->{'bridge-access'} = 1;
> +	    }
> +	}
> +
>  	if ($d->{ovs_bridge}) {
>  
>  	    if ($ifupdown2) {
> @@ -1344,6 +1368,31 @@ sub __write_etc_network_interfaces {
>  	}
>      }
>  
> +    # check bridgeport option
> +    my $bridgeports = {};
> +    my $bridges = {};
> +    foreach my $iface (keys %$ifaces) {
> +	my $d = $ifaces->{$iface};
> +	if ($d->{type} eq 'bridge') {
> +	    foreach my $p (split (/\s+/, $d->{bridge_ports})) {
> +		$bridgeports->{$p} = $iface;
> +	    }
> +	    $bridges->{$iface} = $d;
> +	}
> +    }
> +
> +    foreach my $iface (keys %$ifaces) {
> +	my $d = $ifaces->{$iface};
> +	if ($d->{'bridge-learning'} || $d->{'bridge-arp-nd-suppress'} || $d->{'bridge-unicast-flood'} || 
> +	    $d->{'bridge-multicast-flood'} || $d->{'bridge-access'}) {
> +	    die "iface $iface  : bridgeports options can be used only if interface is in a bridge" if !$bridgeports->{$iface};

double whitespace, 'bridgeports' might be misleading, this is about
bridge-related options. Perhaps we could gather them in a list and print
them with the error (would look nicer in code than the big `or` chain
anyway.
And a missing \n at the end of the error message.

> +	}
> +
> +	if ($d->{'bridge-access'} && !$bridges->{$bridgeports->{$iface}}->{bridge_vlan_aware}) {
> +	    die "iface $iface : bridge-access option can be only used if interface is in a vlan aware bridge";

missing \n at the end of the error message.

> +	}
> +    }
> +
>      my $raw = <<'NETWORKDOC';
>  # network interface settings; autogenerated
>  # Please do NOT modify this file directly, unless you know what
> -- 
> 2.11.0