[PATCH 0/3] network-functions.pl : Correct comparisons and enhance testsuite

[PATCH 0/3] network-functions.pl : Correct comparisons and enhance testsuite

[Tim FitzGeorge]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

Fixes a number of minor bugs in network-functions.pl where comparisons of binary network
representations is done using string rather than integer comparision operators, and checks
for undefined values are done on the wrong variables.

Also enhances the testsuite to display information on failed test.

Fixes: 12263

Tim FitzGeorge (3):
  network-functions.pl : Enhance testsuite to show cause of error
  network-functions.pl : Compare correct variables in network_equal
  network-functions.pl : Use integer comparison rather than string

 config/cfgroot/network-functions.pl | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

-- 
2.16.4

[PATCH 1/3] network-functions.pl : Enhance testsuite to show cause of error

[Tim FitzGeorge]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
---
 config/cfgroot/network-functions.pl | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
index 8649d0502..56b4bceb7 100644
--- a/config/cfgroot/network-functions.pl
+++ b/config/cfgroot/network-functions.pl
@@ -449,14 +449,15 @@ sub get_mac_by_name($) {
 # Remove the next line to enable the testsuite
 __END__
 
-sub assert($) {
+sub assert($$) {
+	my $tst = shift;
 	my $ret = shift;
 
 	if ($ret) {
 		return;
 	}
 
-	print "ASSERTION ERROR";
+	print "ASSERTION ERROR - $tst\n";
 	exit(1);
 }
 
@@ -464,10 +465,10 @@ sub testsuite() {
 	my $result;
 
 	my $address1 = &ip2bin("8.8.8.8");
-	assert($address1 == 134744072);
+	assert('ip2bin("8.8.8.8")', $address1 == 134744072);
 
 	my $address2 = &bin2ip($address1);
-	assert($address2 eq "8.8.8.8");
+	assert("bin2ip($address1)", $address2 eq "8.8.8.8");
 
 	# Check if valid IP addresses are correctly recognised.
 	foreach my $address ("1.2.3.4", "192.168.180.1", "127.0.0.1") {
@@ -486,34 +487,34 @@ sub testsuite() {
 	}
 
 	$result = &check_ip_address_and_netmask("192.168.180.0/255.255.255.0");
-	assert($result);
+	assert('check_ip_address_and_netmask("192.168.180.0/255.255.255.0")', $result);
 
 	$result = &convert_netmask2prefix("255.255.254.0");
-	assert($result == 23);
+	assert('convert_netmask2prefix("255.255.254.0")', $result == 23);
 
 	$result = &convert_prefix2netmask(8);
-	assert($result eq "255.0.0.0");
+	assert('convert_prefix2netmask(8)', $result eq "255.0.0.0");
 
 	$result = &find_next_ip_address("1.2.3.4", 2);
-	assert($result eq "1.2.3.6");
+	assert('find_next_ip_address("1.2.3.4", 2)', $result eq "1.2.3.6");
 
 	$result = &network_equal("192.168.0.0/24", "192.168.0.0/255.255.255.0");
-	assert($result);
+	assert('network_equal("192.168.0.0/24", "192.168.0.0/255.255.255.0")', $result);
 
 	$result = &network_equal("192.168.0.0/24", "192.168.0.0/25");
-	assert(!$result);
+	assert('network_equal("192.168.0.0/24", "192.168.0.0/25")', !$result);
 
 	$result = &network_equal("192.168.0.0/24", "192.168.0.128/25");
-	assert(!$result);
+	assert('network_equal("192.168.0.0/24", "192.168.0.128/25")', !$result);
 
 	$result = &network_equal("192.168.0.1/24", "192.168.0.XXX/24");
-	assert(!$result);
+	assert('network_equal("192.168.0.1/24", "192.168.0.XXX/24")', !$result);
 
 	$result = &ip_address_in_network("10.0.1.4", "10.0.0.0/8");
-	assert($result);
+	assert('ip_address_in_network("10.0.1.4", "10.0.0.0/8"', $result);
 
 	$result = &ip_address_in_network("192.168.30.11", "192.168.30.0/255.255.255.0");
-	assert($result);
+	assert('ip_address_in_network("192.168.30.11", "192.168.30.0/255.255.255.0")', $result);
 
 	print "Testsuite completed successfully!\n";
 
-- 
2.16.4

[PATCH 2/3] network-functions.pl : Compare correct variables in network_equal

[Tim FitzGeorge]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

Check result of network2bin is correct rather than checking non-existent
variable.

Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
---
 config/cfgroot/network-functions.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
index 56b4bceb7..a3f574760 100644
--- a/config/cfgroot/network-functions.pl
+++ b/config/cfgroot/network-functions.pl
@@ -111,7 +111,7 @@ sub network_equal {
 	my @bin1 = &network2bin($network1);
 	my @bin2 = &network2bin($network2);
 
-	if (!defined $bin1 || !defined $bin2) {
+	if (@bin1 != 2 || @bin2 != 2) {
 		return undef;
 	}
 
-- 
2.16.4

[PATCH 3/3] network-functions.pl : Use integer comparison rather than string

[Tim FitzGeorge]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

Compare binary representation of networks using integer rather than string
comparison operators.

Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
---
 config/cfgroot/network-functions.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
index a3f574760..7f8bc7c52 100644
--- a/config/cfgroot/network-functions.pl
+++ b/config/cfgroot/network-functions.pl
@@ -115,7 +115,7 @@ sub network_equal {
 		return undef;
 	}
 
-	if ($bin1[0] eq $bin2[0] && $bin1[1] eq $bin2[1]) {
+	if ($bin1[0] == $bin2[0] && $bin1[1] == $bin2[1]) {
 		return 1;
 	}
 
@@ -295,7 +295,7 @@ sub ip_address_in_network($$) {
 	# Find end address
 	my $broadcast_bin = $network_bin ^ (~$netmask_bin % 2 ** 32);
 
-	return (($address_bin ge $network_bin) && ($address_bin le $broadcast_bin));
+	return (($address_bin >= $network_bin) && ($address_bin <= $broadcast_bin));
 }
 
 sub setup_upstream_proxy() {
-- 
2.16.4

Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

Hello,

I am still not sure what you are trying to solve here.

&network2bin() has two possible return values:

1) An array with two values

Or

2) Undefined

You are now checking if you have an array with any length but two, but this won’t work for undefined. That should at least print a warning that you are trying to determine the length of an undefined array.

So why is this change needed?

-Michael

> On 24 Dec 2019, at 13:56, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
> 
> Check result of network2bin is correct rather than checking non-existent
> variable.
> 
> Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
> ---
> config/cfgroot/network-functions.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
> index 56b4bceb7..a3f574760 100644
> --- a/config/cfgroot/network-functions.pl
> +++ b/config/cfgroot/network-functions.pl
> @@ -111,7 +111,7 @@ sub network_equal {
> 	my @bin1 = &network2bin($network1);
> 	my @bin2 = &network2bin($network2);
> 
> -	if (!defined $bin1 || !defined $bin2) {
> +	if (@bin1 != 2 || @bin2 != 2) {
> 		return undef;
> 	}
> 
> -- 
> 2.16.4
> 

Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal

[Tim FitzGeorge]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

Hello,

On 24/12/2019 13:04, Michael Tremer wrote:
> Hello,
> 
> I am still not sure what you are trying to solve here.
> 
> &network2bin() has two possible return values:
> 
> 1) An array with two values
> 
> Or
> 
> 2) Undefined

No. It's called in array context so the return value in the latter case
is the list ( undef ).

> 
> You are now checking if you have an array with any length but two, but this won’t work for undefined. That should at least print a warning that you are trying to determine the length of an undefined array.
> 
> So why is this change needed?

Partly because the return value is an array and partly because the
original code checked the scalar variables $bin1 and $bin2 rather than
the arrays @bin1 and @bin2 containing the return values; since these
scalars aren't used anywhere, network_equal() always returned undef,
which is actually the root cause of my original problem.

Tim

> 
> -Michael
> 
>> On 24 Dec 2019, at 13:56, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
>>
>> Check result of network2bin is correct rather than checking non-existent
>> variable.
>>
>> Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
>> ---
>> config/cfgroot/network-functions.pl | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
>> index 56b4bceb7..a3f574760 100644
>> --- a/config/cfgroot/network-functions.pl
>> +++ b/config/cfgroot/network-functions.pl
>> @@ -111,7 +111,7 @@ sub network_equal {
>> 	my @bin1 = &network2bin($network1);
>> 	my @bin2 = &network2bin($network2);
>>
>> -	if (!defined $bin1 || !defined $bin2) {
>> +	if (@bin1 != 2 || @bin2 != 2) {
>> 		return undef;
>> 	}
>>
>> -- 
>> 2.16.4
>>
> 

Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

Okay.

Did I say that I hate Perl?

I suppose if you tested this, it works. And it must have worked at some point before, because there was a test suite.

Acked-by: Michael Tremer <michael.tremer(a)ipfire.org>

-Michael

> On 28 Dec 2019, at 20:57, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
> 
> Hello,
> 
> On 24/12/2019 13:04, Michael Tremer wrote:
>> Hello,
>> 
>> I am still not sure what you are trying to solve here.
>> 
>> &network2bin() has two possible return values:
>> 
>> 1) An array with two values
>> 
>> Or
>> 
>> 2) Undefined
> 
> No. It's called in array context so the return value in the latter case
> is the list ( undef ).
> 
>> 
>> You are now checking if you have an array with any length but two, but this won’t work for undefined. That should at least print a warning that you are trying to determine the length of an undefined array.
>> 
>> So why is this change needed?
> 
> Partly because the return value is an array and partly because the
> original code checked the scalar variables $bin1 and $bin2 rather than
> the arrays @bin1 and @bin2 containing the return values; since these
> scalars aren't used anywhere, network_equal() always returned undef,
> which is actually the root cause of my original problem.
> 
> Tim
> 
>> 
>> -Michael
>> 
>>> On 24 Dec 2019, at 13:56, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
>>> 
>>> Check result of network2bin is correct rather than checking non-existent
>>> variable.
>>> 
>>> Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
>>> ---
>>> config/cfgroot/network-functions.pl | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
>>> index 56b4bceb7..a3f574760 100644
>>> --- a/config/cfgroot/network-functions.pl
>>> +++ b/config/cfgroot/network-functions.pl
>>> @@ -111,7 +111,7 @@ sub network_equal {
>>> 	my @bin1 = &network2bin($network1);
>>> 	my @bin2 = &network2bin($network2);
>>> 
>>> -	if (!defined $bin1 || !defined $bin2) {
>>> +	if (@bin1 != 2 || @bin2 != 2) {
>>> 		return undef;
>>> 	}
>>> 
>>> -- 
>>> 2.16.4
>>> 
>> 
> 

Aw: Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal

[Bernhard Bitsch]

[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]



> Gesendet: Montag, 06. Januar 2020 um 18:16 Uhr
> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
> An: "Tim FitzGeorge" <ipfr(a)tfitzgeorge.me.uk>
> Cc: development(a)lists.ipfire.org
> Betreff: Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal
>
> Okay.
> 
> Did I say that I hate Perl?
> 
> I suppose if you tested this, it works. And it must have worked at some point before, because there was a test suite.
> 

Not a testsuite, but formal verification with the language definition ( which is huge in case of Perl ;) ) proves the correctness in this special case.
SCNR

-Bernhard

> Acked-by: Michael Tremer <michael.tremer(a)ipfire.org>
> 
> -Michael
> 
> > On 28 Dec 2019, at 20:57, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
> > 
> > Hello,
> > 
> > On 24/12/2019 13:04, Michael Tremer wrote:
> >> Hello,
> >> 
> >> I am still not sure what you are trying to solve here.
> >> 
> >> &network2bin() has two possible return values:
> >> 
> >> 1) An array with two values
> >> 
> >> Or
> >> 
> >> 2) Undefined
> > 
> > No. It's called in array context so the return value in the latter case
> > is the list ( undef ).
> > 
> >> 
> >> You are now checking if you have an array with any length but two, but this won’t work for undefined. That should at least print a warning that you are trying to determine the length of an undefined array.
> >> 
> >> So why is this change needed?
> > 
> > Partly because the return value is an array and partly because the
> > original code checked the scalar variables $bin1 and $bin2 rather than
> > the arrays @bin1 and @bin2 containing the return values; since these
> > scalars aren't used anywhere, network_equal() always returned undef,
> > which is actually the root cause of my original problem.
> > 
> > Tim
> > 
> >> 
> >> -Michael
> >> 
> >>> On 24 Dec 2019, at 13:56, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
> >>> 
> >>> Check result of network2bin is correct rather than checking non-existent
> >>> variable.
> >>> 
> >>> Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
> >>> ---
> >>> config/cfgroot/network-functions.pl | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
> >>> index 56b4bceb7..a3f574760 100644
> >>> --- a/config/cfgroot/network-functions.pl
> >>> +++ b/config/cfgroot/network-functions.pl
> >>> @@ -111,7 +111,7 @@ sub network_equal {
> >>> 	my @bin1 = &network2bin($network1);
> >>> 	my @bin2 = &network2bin($network2);
> >>> 
> >>> -	if (!defined $bin1 || !defined $bin2) {
> >>> +	if (@bin1 != 2 || @bin2 != 2) {
> >>> 		return undef;
> >>> 	}
> >>> 
> >>> -- 
> >>> 2.16.4
> >>> 
> >> 
> > 
> 
>

Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

Thx.

> On 6 Jan 2020, at 17:33, Bernhard Bitsch <Bernhard.Bitsch(a)gmx.de> wrote:
> 
> 
> 
>> Gesendet: Montag, 06. Januar 2020 um 18:16 Uhr
>> Von: "Michael Tremer" <michael.tremer(a)ipfire.org>
>> An: "Tim FitzGeorge" <ipfr(a)tfitzgeorge.me.uk>
>> Cc: development(a)lists.ipfire.org
>> Betreff: Re: [PATCH 2/3] network-functions.pl : Compare correct variables in network_equal
>> 
>> Okay.
>> 
>> Did I say that I hate Perl?
>> 
>> I suppose if you tested this, it works. And it must have worked at some point before, because there was a test suite.
>> 
> 
> Not a testsuite, but formal verification with the language definition ( which is huge in case of Perl ;) ) proves the correctness in this special case.
> SCNR
> 
> -Bernhard
> 
>> Acked-by: Michael Tremer <michael.tremer(a)ipfire.org>
>> 
>> -Michael
>> 
>>> On 28 Dec 2019, at 20:57, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
>>> 
>>> Hello,
>>> 
>>> On 24/12/2019 13:04, Michael Tremer wrote:
>>>> Hello,
>>>> 
>>>> I am still not sure what you are trying to solve here.
>>>> 
>>>> &network2bin() has two possible return values:
>>>> 
>>>> 1) An array with two values
>>>> 
>>>> Or
>>>> 
>>>> 2) Undefined
>>> 
>>> No. It's called in array context so the return value in the latter case
>>> is the list ( undef ).
>>> 
>>>> 
>>>> You are now checking if you have an array with any length but two, but this won’t work for undefined. That should at least print a warning that you are trying to determine the length of an undefined array.
>>>> 
>>>> So why is this change needed?
>>> 
>>> Partly because the return value is an array and partly because the
>>> original code checked the scalar variables $bin1 and $bin2 rather than
>>> the arrays @bin1 and @bin2 containing the return values; since these
>>> scalars aren't used anywhere, network_equal() always returned undef,
>>> which is actually the root cause of my original problem.
>>> 
>>> Tim
>>> 
>>>> 
>>>> -Michael
>>>> 
>>>>> On 24 Dec 2019, at 13:56, Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk> wrote:
>>>>> 
>>>>> Check result of network2bin is correct rather than checking non-existent
>>>>> variable.
>>>>> 
>>>>> Signed-off-by: Tim FitzGeorge <ipfr(a)tfitzgeorge.me.uk>
>>>>> ---
>>>>> config/cfgroot/network-functions.pl | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-functions.pl
>>>>> index 56b4bceb7..a3f574760 100644
>>>>> --- a/config/cfgroot/network-functions.pl
>>>>> +++ b/config/cfgroot/network-functions.pl
>>>>> @@ -111,7 +111,7 @@ sub network_equal {
>>>>> 	my @bin1 = &network2bin($network1);
>>>>> 	my @bin2 = &network2bin($network2);
>>>>> 
>>>>> -	if (!defined $bin1 || !defined $bin2) {
>>>>> +	if (@bin1 != 2 || @bin2 != 2) {
>>>>> 		return undef;
>>>>> 	}
>>>>> 
>>>>> -- 
>>>>> 2.16.4
>>>>> 
>>>> 
>>> 
>> 
>>