[pve-devel] [PATCH common v2 2/2] INotify: map address/netmask to cidr while parsing interfaces

[Thomas Lamprecht]

On 5/3/19 9:28 AM, Dominik Csapak wrote:
> this allows us to always show the 'address' the 'netmask' and the 'cidr'
> both for ipv4 and ipv6
> 
> there is a small api change involved in one scenario:
> if one manually changed the address to cidr format like
>     '10.0.0.4/24'
> 
> we now get from the api the parsed values
>     addr => 10.0.0.4
>     netmask => 24
>     cidr => 10.0.0.4/24
> 
> instead of
>     addr => 10.0.0.4/24
>     netmask =>
> 
> but i think that circumventing our api when writing the file, but still
> relying on the api for reading is not a valid use case, i would argue
> that we can change this, especially since we have a new field that
> contains that information again (cidr)
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * moved the cidr parsing into the if condition
> 
>  src/PVE/INotify.pm | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index c52d992..11b6728 100644
> --- a/src/PVE/INotify.pm
> +++ b/src/PVE/INotify.pm
> @@ -16,6 +16,7 @@ use PVE::Exception qw(raise_param_exc);
>  use PVE::Network;
>  use PVE::Tools;
>  use PVE::ProcFSTools;
> +use PVE::JSONSchema;
>  use Clone qw(clone);
>  use Linux::Inotify2;
>  use base 'Exporter';
> @@ -1103,6 +1104,34 @@ sub __read_etc_network_interfaces {
>  	    }
>  	}
>  
> +	# map address and netmask to cidr
> +	if ($d->{address}) {
> +	    if ($d->{netmask} =~ m/^\d+$/) { # e.g. netmask 20
> +		$d->{cidr} = $d->{address} . "/" . $d->{netmask};
> +	    } elsif ($d->{netmask} &&
> +		     (my $cidr = PVE::JSONSchema::get_netmask_bits($d->{netmask}))) { # e.g. netmask 255.255.255.0
> +		$d->{cidr} = $d->{address} . "/" . $cidr;
> +	    } elsif ($d->{address} =~ m!^(.*)/(\d+)$!) {
> +		$d->{cidr} = $d->{address};
> +		$d->{address} = $1;
> +		$d->{netmask} = $2;
> +	    } else {
> +		$d->{cidr} = $d->{address};
> +	    }
> +	}
> +
> +	# map address6 and netmask6 to cidr6
> +	if ($d->{address6}) {
> +	    $d->{cidr6} = $d->{address6};
> +	    if ($d->{netmask6}) {
> +		$d->{cidr6} .= "/" . $d->{netmask6} if $d->{netmask6};
> +	    } elsif ($d->{address6} =~ m!^(.*)/(\d+)$!) {
> +		$d->{cidr6} = $d->{address6};

and above statement is also redundant, or did you include it for "cleanliness"
sake?

> +		$d->{address6} = $1;
> +		$d->{netmask6} = $2;
> +	    }
> +	}
> +
>  	$d->{method} = 'manual' if !$d->{method};
>  	$d->{method6} = 'manual' if !$d->{method6};
>  
> @@ -1317,6 +1346,9 @@ sub __write_etc_network_interfaces {
>      foreach my $iface (keys %$ifaces) {
>  	my $d = $ifaces->{$iface};
>  
> +	delete $d->{cidr};
> +	delete $d->{cidr6};
> +
>  	my $ports = '';
>  	foreach my $k (qw(bridge_ports ovs_ports slaves ovs_bonds)) {
>  	    $ports .= " $d->{$k}" if $d->{$k};
>