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

[Wolfgang Bumiller]

On Tue, Apr 16, 2019 at 11:05:36AM +0200, 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>
> ---
>  src/PVE/INotify.pm | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm
> index c52d992..83c592a 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}) { # e.g. netmask 255.255.255.0
> +		my $cidr = PVE::JSONSchema::get_netmask_bits($d->{netmask});

This could still yield $cidr=undef if the interfaces file is "broken",
we should probably deal with this somehow (and at least prevent the
warning from concatenating an undefined value to the string).

Maybe put the get_netmask_bits() call in the if() condition directly
then it would fall through to checking for an already existing cidr in
$d->{address} or finally drop the netmask in the else branch.

> +		$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};
> +		$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};
> -- 
> 2.11.0