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(-)
Signed-off-by: Tim FitzGeorge ipfr@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";
Check result of network2bin is correct rather than checking non-existent variable.
Signed-off-by: Tim FitzGeorge ipfr@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; }
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@tfitzgeorge.me.uk wrote:
Check result of network2bin is correct rather than checking non-existent variable.
Signed-off-by: Tim FitzGeorge ipfr@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
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:
- An array with two values
Or
- 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@tfitzgeorge.me.uk wrote:
Check result of network2bin is correct rather than checking non-existent variable.
Signed-off-by: Tim FitzGeorge ipfr@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
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@ipfire.org
-Michael
On 28 Dec 2019, at 20:57, Tim FitzGeorge ipfr@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:
- An array with two values
Or
- 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@tfitzgeorge.me.uk wrote:
Check result of network2bin is correct rather than checking non-existent variable.
Signed-off-by: Tim FitzGeorge ipfr@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
Gesendet: Montag, 06. Januar 2020 um 18:16 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Tim FitzGeorge" ipfr@tfitzgeorge.me.uk Cc: development@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@ipfire.org
-Michael
On 28 Dec 2019, at 20:57, Tim FitzGeorge ipfr@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:
- An array with two values
Or
- 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@tfitzgeorge.me.uk wrote:
Check result of network2bin is correct rather than checking non-existent variable.
Signed-off-by: Tim FitzGeorge ipfr@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
Thx.
On 6 Jan 2020, at 17:33, Bernhard Bitsch Bernhard.Bitsch@gmx.de wrote:
Gesendet: Montag, 06. Januar 2020 um 18:16 Uhr Von: "Michael Tremer" michael.tremer@ipfire.org An: "Tim FitzGeorge" ipfr@tfitzgeorge.me.uk Cc: development@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@ipfire.org
-Michael
On 28 Dec 2019, at 20:57, Tim FitzGeorge ipfr@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:
- An array with two values
Or
- 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@tfitzgeorge.me.uk wrote:
Check result of network2bin is correct rather than checking non-existent variable.
Signed-off-by: Tim FitzGeorge ipfr@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
Compare binary representation of networks using integer rather than string comparison operators.
Signed-off-by: Tim FitzGeorge ipfr@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() {
-
Bernhard Bitsch -
Michael Tremer -
Tim FitzGeorge