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

Message ID 20191224125652.12232-3-ipfr@tfitzgeorge.me.uk
State Dropped
Headers
Series network-functions.pl : Correct comparisons and enhance testsuite |

Commit Message

Tim FitzGeorge Dec. 24, 2019, 12:56 p.m. UTC
  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(-)
  

Comments

Michael Tremer Dec. 24, 2019, 1:04 p.m. UTC | #1
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
>
  
Tim FitzGeorge Dec. 28, 2019, 8:57 p.m. UTC | #2
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@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
>>
>
  
Michael Tremer Jan. 6, 2020, 5:16 p.m. UTC | #3
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:
>> 
>> 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@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
>>> 
>> 
>
  
Bernhard Bitsch Jan. 6, 2020, 5:33 p.m. UTC | #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:
> >> 
> >> 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@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
> >>> 
> >> 
> > 
> 
>
  
Michael Tremer Jan. 6, 2020, 6:57 p.m. UTC | #5
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:
>>>> 
>>>> 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@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
>>>>> 
>>>> 
>>> 
>> 
>>
  

Patch

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;
 	}