[PATCHv2] tests: shell: Add test for ambguity while setting the value

[PATCHv2] tests: shell: Add test for ambguity while setting the value

[Shyam Saini]

This test checks bug identified and fixed in the commit mentioned below
In a statement if there are  multiple src data then it  would be
totally ambiguous to decide which value to set.

We don't add this test in python testsuite, because there we only have
"ok"  and "fail"  as return code. So, we can't detect 134 != 1 there.
(both 1 and 134 stats failure)

Test: 986dea8 ("evaluate: avoid reference to multiple src data in
statements which set values")
Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
---
 .../testcases/sets/0023unknown_value_to_use_0      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0023unknown_value_to_use_0

diff --git a/tests/shell/testcases/sets/0023unknown_value_to_use_0 b/tests/shell/testcases/sets/0023unknown_value_to_use_0
new file mode 100755
index 0000000..ee9be68
--- /dev/null
+++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+	# This test checks bug identified and fixed in the commit Id "986dea8".
+	# i.e, If in a statement there are  multiple src data then it would be totally ambiguous to decide which value to set.
+        
+	# Before this commit 986dea8, nft returns 134 which indicates the bug but after this commit it returns 1.
+	# We don't add this test in python testsuite, because there we can't detect 134 != 1 (returns code stating failure) 	
+
+declare -a rules=(
+"tcp dport set {1, 2, 3}" "udp dport set {1, 2, 3}"
+"meta pkttype set {unicast, multicast, broadcast}"
+"meta mark set {0xffff, 0xcc}"
+"ct mark set {0x11333, 0x11}" "ct zone set {123, 127}"
+"ct label set {123, 127}" 
+"ct event set {new, related, destroy, label}"
+"ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02}"
+"ip saddr set {192.19.1.2, 191.1.22.1}"
+)
+
+$NFT add table t
+$NFT add chain t c
+
+for (( i = 0 ; i < ${#rules[@]} ; i++ ))
+do
+	$NFT add rule t c ${rules[$i]}
+	ret=$?
+	if [ $ret -ne 1 ];
+	then
+		echo "E: failed test: returned $ret instead of 1" >&2
+		exit 1
+	fi
+done
+
-- 
1.9.1

Re: [PATCHv2] tests: shell: Add test for ambguity while setting the value

[Pablo Neira Ayuso]

On Fri, Jun 09, 2017 at 09:31:00PM +0530, Shyam Saini wrote:
> diff --git a/tests/shell/testcases/sets/0023unknown_value_to_use_0 b/tests/shell/testcases/sets/0023unknown_value_to_use_0
> new file mode 100755
> index 0000000..ee9be68
> --- /dev/null
> +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0
> @@ -0,0 +1,33 @@
> +#!/bin/bash
> +
> +	# This test checks bug identified and fixed in the commit Id "986dea8".
> +	# i.e, If in a statement there are  multiple src data then it would be totally ambiguous to decide which value to set.
> +        
> +	# Before this commit 986dea8, nft returns 134 which indicates the bug but after this commit it returns 1.
> +	# We don't add this test in python testsuite, because there we can't detect 134 != 1 (returns code stating failure) 	
> +
> +declare -a rules=(
> +"tcp dport set {1, 2, 3}" "udp dport set {1, 2, 3}"
> +"meta pkttype set {unicast, multicast, broadcast}"
> +"meta mark set {0xffff, 0xcc}"
> +"ct mark set {0x11333, 0x11}" "ct zone set {123, 127}"
> +"ct label set {123, 127}" 
> +"ct event set {new, related, destroy, label}"
> +"ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02}"
> +"ip saddr set {192.19.1.2, 191.1.22.1}"
> +)
> +
> +$NFT add table t
> +$NFT add chain t c
> +
> +for (( i = 0 ; i < ${#rules[@]} ; i++ ))
> +do
> +	$NFT add rule t c ${rules[$i]}
> +	ret=$?
> +	if [ $ret -ne 1 ];
> +	then
> +		echo "E: failed test: returned $ret instead of 1" >&2
> +		exit 1
> +	fi
> +done

Better use tests/py/ for this?

Re: [PATCHv2] tests: shell: Add test for ambguity while setting the value

[Shyam Saini]

On Mon, Jun 12, 2017 at 2:52 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jun 09, 2017 at 09:31:00PM +0530, Shyam Saini wrote:
>> diff --git a/tests/shell/testcases/sets/0023unknown_value_to_use_0 b/tests/shell/testcases/sets/0023unknown_value_to_use_0
>> new file mode 100755
>> index 0000000..ee9be68
>> --- /dev/null
>> +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0
>> @@ -0,0 +1,33 @@
>> +#!/bin/bash
>> +
>> +     # This test checks bug identified and fixed in the commit Id "986dea8".
>> +     # i.e, If in a statement there are  multiple src data then it would be totally ambiguous to decide which value to set.
>> +
>> +     # Before this commit 986dea8, nft returns 134 which indicates the bug but after this commit it returns 1.
>> +     # We don't add this test in python testsuite, because there we can't detect 134 != 1 (returns code stating failure)
>> +
>> +declare -a rules=(
>> +"tcp dport set {1, 2, 3}" "udp dport set {1, 2, 3}"
>> +"meta pkttype set {unicast, multicast, broadcast}"
>> +"meta mark set {0xffff, 0xcc}"
>> +"ct mark set {0x11333, 0x11}" "ct zone set {123, 127}"
>> +"ct label set {123, 127}"
>> +"ct event set {new, related, destroy, label}"
>> +"ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02}"
>> +"ip saddr set {192.19.1.2, 191.1.22.1}"
>> +)
>> +
>> +$NFT add table t
>> +$NFT add chain t c
>> +
>> +for (( i = 0 ; i < ${#rules[@]} ; i++ ))
>> +do
>> +     $NFT add rule t c ${rules[$i]}
>> +     ret=$?
>> +     if [ $ret -ne 1 ];
>> +     then
>> +             echo "E: failed test: returned $ret instead of 1" >&2
>> +             exit 1
>> +     fi
>> +done
>
> Better use tests/py/ for this?

We only have "ok" and "fail" as return codes in python test suites and
we can't can't detect 134 != 1 there.
So it was added in the tests/shell/.

further we would need "*.payload" file for that respective test.

$ nft --debug=netlink add rule t c udp dport set {1, 3}

But in our case it is not generated at all.

Is it right ?

Please correct me

Thanks,
shyam

Re: [PATCHv2] tests: shell: Add test for ambguity while setting the value

[Pablo Neira Ayuso]

On Mon, Jun 12, 2017 at 04:16:16PM +0530, Shyam Saini wrote:
> On Mon, Jun 12, 2017 at 2:52 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Jun 09, 2017 at 09:31:00PM +0530, Shyam Saini wrote:
> >> diff --git a/tests/shell/testcases/sets/0023unknown_value_to_use_0 b/tests/shell/testcases/sets/0023unknown_value_to_use_0
> >> new file mode 100755
> >> index 0000000..ee9be68
> >> --- /dev/null
> >> +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0
> >> @@ -0,0 +1,33 @@
> >> +#!/bin/bash
> >> +
> >> +     # This test checks bug identified and fixed in the commit Id "986dea8".
> >> +     # i.e, If in a statement there are  multiple src data then it would be totally ambiguous to decide which value to set.
> >> +
> >> +     # Before this commit 986dea8, nft returns 134 which indicates the bug but after this commit it returns 1.
> >> +     # We don't add this test in python testsuite, because there we can't detect 134 != 1 (returns code stating failure)
> >> +
> >> +declare -a rules=(
> >> +"tcp dport set {1, 2, 3}" "udp dport set {1, 2, 3}"
> >> +"meta pkttype set {unicast, multicast, broadcast}"
> >> +"meta mark set {0xffff, 0xcc}"
> >> +"ct mark set {0x11333, 0x11}" "ct zone set {123, 127}"
> >> +"ct label set {123, 127}"
> >> +"ct event set {new, related, destroy, label}"
> >> +"ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02}"
> >> +"ip saddr set {192.19.1.2, 191.1.22.1}"
> >> +)
> >> +
> >> +$NFT add table t
> >> +$NFT add chain t c
> >> +
> >> +for (( i = 0 ; i < ${#rules[@]} ; i++ ))
> >> +do
> >> +     $NFT add rule t c ${rules[$i]}
> >> +     ret=$?
> >> +     if [ $ret -ne 1 ];
> >> +     then
> >> +             echo "E: failed test: returned $ret instead of 1" >&2
> >> +             exit 1
> >> +     fi
> >> +done
> >
> > Better use tests/py/ for this?
> 
> We only have "ok" and "fail" as return codes in python test suites and
> we can't can't detect 134 != 1 there.

Not sure what you mean "we can't can't detect 134 != 1 there."

You probably refer to the fact that we cannot distinguish between
assertion failure and command failure?

> So it was added in the tests/shell/.
> 
> further we would need "*.payload" file for that respective test.
> 
> $ nft --debug=netlink add rule t c udp dport set {1, 3}
> 
> But in our case it is not generated at all.

We have no *.payload for tests that fail anyway, right?

> Is it right ?

Probably I missing anything, I'm asking because I just wondering if py
tests would be a better fit for this.

Re: [PATCHv2] tests: shell: Add test for ambguity while setting the value

[Shyam Saini]

On Mon, Jun 12, 2017 at 4:19 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jun 12, 2017 at 04:16:16PM +0530, Shyam Saini wrote:
>> On Mon, Jun 12, 2017 at 2:52 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Fri, Jun 09, 2017 at 09:31:00PM +0530, Shyam Saini wrote:
>> >> diff --git a/tests/shell/testcases/sets/0023unknown_value_to_use_0 b/tests/shell/testcases/sets/0023unknown_value_to_use_0
>> >> new file mode 100755
>> >> index 0000000..ee9be68
>> >> --- /dev/null
>> >> +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0
>> >> @@ -0,0 +1,33 @@
>> >> +#!/bin/bash
>> >> +
>> >> +     # This test checks bug identified and fixed in the commit Id "986dea8".
>> >> +     # i.e, If in a statement there are  multiple src data then it would be totally ambiguous to decide which value to set.
>> >> +
>> >> +     # Before this commit 986dea8, nft returns 134 which indicates the bug but after this commit it returns 1.
>> >> +     # We don't add this test in python testsuite, because there we can't detect 134 != 1 (returns code stating failure)
>> >> +
>> >> +declare -a rules=(
>> >> +"tcp dport set {1, 2, 3}" "udp dport set {1, 2, 3}"
>> >> +"meta pkttype set {unicast, multicast, broadcast}"
>> >> +"meta mark set {0xffff, 0xcc}"
>> >> +"ct mark set {0x11333, 0x11}" "ct zone set {123, 127}"
>> >> +"ct label set {123, 127}"
>> >> +"ct event set {new, related, destroy, label}"
>> >> +"ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02}"
>> >> +"ip saddr set {192.19.1.2, 191.1.22.1}"
>> >> +)
>> >> +
>> >> +$NFT add table t
>> >> +$NFT add chain t c
>> >> +
>> >> +for (( i = 0 ; i < ${#rules[@]} ; i++ ))
>> >> +do
>> >> +     $NFT add rule t c ${rules[$i]}
>> >> +     ret=$?
>> >> +     if [ $ret -ne 1 ];
>> >> +     then
>> >> +             echo "E: failed test: returned $ret instead of 1" >&2
>> >> +             exit 1
>> >> +     fi
>> >> +done
>> >
>> > Better use tests/py/ for this?
>>
>> We only have "ok" and "fail" as return codes in python test suites and
>> we can't can't detect 134 != 1 there.
>
> Not sure what you mean "we can't can't detect 134 != 1 there."
> You probably refer to the fact that we cannot distinguish between
> assertion failure and command failure?
>

Yes,
Sorry my bad. I was missing the  context.

I'm adding snippet from Arturo where he explained why we don't add
this in python test suite

                          "They are good candidates for the python testsuite,
                           but there we have no way to detect return
codes (apart of fail/ok), so we can't
                           detect 134 != 1 (which both indicates failure)"


>> So it was added in the tests/shell/.
>>
>> further we would need "*.payload" file for that respective test.
>>
>> $ nft --debug=netlink add rule t c udp dport set {1, 3}
>>
>> But in our case it is not generated at all.
>
> We have no *.payload for tests that fail anyway, right?

say we add some test.t file in the tests suite then don't we need
corresponding test.t.payload file?
and in our case we if add some test.t file as our test then wouldn't
we need corresponding test.t.payload file?


>> Is it right ?
>
> Probably I missing anything, I'm asking because I just wondering if py
> tests would be a better fit for this.

Do we still need this in python test suite?

Thanks,
Shyam

Re: [PATCHv2] tests: shell: Add test for ambguity while setting the value

[Pablo Neira Ayuso]

On Fri, Jun 09, 2017 at 09:31:00PM +0530, Shyam Saini wrote:
> This test checks bug identified and fixed in the commit mentioned below
> In a statement if there are  multiple src data then it  would be
> totally ambiguous to decide which value to set.
> 
> We don't add this test in python testsuite, because there we only have
> "ok"  and "fail"  as return code. So, we can't detect 134 != 1 there.
> (both 1 and 134 stats failure)

OK I see, so 134 and 1 refers to the exit code we return to the shell
(or whoever runs nft).

Could you have a look at tests/py/nft-test.py

If the problem is just the exit code, then I suggest you add these
tests to the python infrastructure, then make sure that the python
script checks for 1 for 'fail'.

Could you have a look, please?

Re: [PATCHv2] tests: shell: Add test for ambguity while setting the value

[Shyam Saini]

On Thu, Jun 15, 2017 at 3:21 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jun 09, 2017 at 09:31:00PM +0530, Shyam Saini wrote:
>> This test checks bug identified and fixed in the commit mentioned below
>> In a statement if there are  multiple src data then it  would be
>> totally ambiguous to decide which value to set.
>>
>> We don't add this test in python testsuite, because there we only have
>> "ok"  and "fail"  as return code. So, we can't detect 134 != 1 there.
>> (both 1 and 134 stats failure)
>
> OK I see, so 134 and 1 refers to the exit code we return to the shell
> (or whoever runs nft).
>
> Could you have a look at tests/py/nft-test.py
>
Sure

> If the problem is just the exit code, then I suggest you add these
> tests to the python infrastructure, then make sure that the python
> script checks for 1 for 'fail'.
>
> Could you have a look, please?

I'll send the python version of this test.

Thanks,
shyam