From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH v2] Fix network-functions net membership check Date: Tue, 24 Dec 2019 11:21:13 +0100 Message-ID: In-Reply-To: <20191224101503.7483-1-ipfr@tfitzgeorge.me.uk> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7041778516014225817==" List-Id: --===============7041778516014225817== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Tim, This is a large patch with many independent changes which would have been eas= ier to review in multiple smaller patches=E2=80=A6 > On 24 Dec 2019, at 11:15, Tim FitzGeorge wrote: >=20 > Replace textual comparison of two number with arithmetic in > ip_address_in_Network(). >=20 > Changes since V1: > - Correctly copied change from production system > - Fixed test of non-existent variables in network2bin > - Enhanced testsuite to report which test failed >=20 > Fixes: 12263 > Signed-off-by: Tim FitzGeorge > --- > config/cfgroot/network-functions.pl | 38 ++++++++++++++++++++--------------= --- > 1 file changed, 21 insertions(+), 17 deletions(-) >=20 > diff --git a/config/cfgroot/network-functions.pl b/config/cfgroot/network-f= unctions.pl > index 8649d0502..b911e214f 100644 > --- a/config/cfgroot/network-functions.pl > +++ b/config/cfgroot/network-functions.pl > @@ -111,11 +111,11 @@ sub network_equal { > my @bin1 =3D &network2bin($network1); > my @bin2 =3D &network2bin($network2); >=20 > - if (!defined $bin1 || !defined $bin2) { > + if (!defined $bin1[0] || !defined $bin2[0]) { > return undef; > } What is this part supposed to do? &network2bin() might return =E2=80=9Cundef=E2=80=9D and therefore you cannot = check for $bin1[0] straight away. This has been introduced in: https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommitdiff;h=3D3f3974b711391= b6bbc05e5435398c1b3ee26c6e8 > - if ($bin1[0] eq $bin2[0] && $bin1[1] eq $bin2[1]) { > + if ($bin1[0] =3D=3D $bin2[0] && $bin1[1] =3D=3D $bin2[1]) { > return 1; > } I agree with this one. >=20 > @@ -295,7 +295,7 @@ sub ip_address_in_network($$) { > # Find end address > my $broadcast_bin =3D $network_bin ^ (~$netmask_bin % 2 ** 32); >=20 > - return (($address_bin ge $network_bin) && ($address_bin le $broadcast_bin= )); > + return (($address_bin >=3D $network_bin) && ($address_bin <=3D $broadcast= _bin)); Likewise. > } >=20 > sub setup_upstream_proxy() { > @@ -449,14 +449,15 @@ sub get_mac_by_name($) { > # Remove the next line to enable the testsuite > __END__ >=20 > -sub assert($) { > +sub assert($$) { > + my $tst =3D shift; > my $ret =3D shift; Indentation is off here. Not a major thing, but a thing nevertheless. > if ($ret) { > return; > } >=20 > - print "ASSERTION ERROR"; > + print "ASSERTION ERROR - $tst\n"; > exit(1); > } >=20 > @@ -464,10 +465,10 @@ sub testsuite() { > my $result; >=20 > my $address1 =3D &ip2bin("8.8.8.8"); > - assert($address1 =3D=3D 134744072); > + assert('ip2bin("8.8.8.8")', $address1 =3D=3D 134744072); >=20 > my $address2 =3D &bin2ip($address1); > - assert($address2 eq "8.8.8.8"); > + assert("bin2ip($address1)", $address2 eq "8.8.8.8"); >=20 > # 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,37 @@ sub testsuite() { > } >=20 > $result =3D &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")', $re= sult); >=20 > $result =3D &convert_netmask2prefix("255.255.254.0"); > - assert($result =3D=3D 23); > + assert('convert_netmask2prefix("255.255.254.0")', $result =3D=3D 23); >=20 > $result =3D &convert_prefix2netmask(8); > - assert($result eq "255.0.0.0"); > + assert('convert_prefix2netmask(8)', $result eq "255.0.0.0"); >=20 > $result =3D &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"); >=20 > $result =3D &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")', $r= esult); >=20 > $result =3D &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); >=20 > $result =3D &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); >=20 > $result =3D &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); >=20 > $result =3D &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); >=20 > $result =3D &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); > + > + $result =3D &ip_address_in_network("192.168.30.11", "0.0.0.0/8"); > + assert('ip_address_in_network("192.168.30.11", "0.0.0.0/8")', !$result); >=20 > print "Testsuite completed successfully!\n"; >=20 > --=20 > 2.16.4 >=20 --===============7041778516014225817==--