| Message ID | 1497000633-10954-1-git-send-email-mayhs11saini@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On 9 June 2017 at 11:30, Shyam Saini <mayhs11saini@gmail.com> 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) > > 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 | 34 ++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > create mode 100755 tests/shell/testcases/sets/0023unknown_value_to_use_0 > Thanks Shyam, minor things below, almost there! Please send an v2 with the requested changes, as you could see, they are cosmetic changes > 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..011cedd > --- /dev/null > +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0 > @@ -0,0 +1,34 @@ > +#!/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) > + Better remove the indentations. > +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}" > + ) > + I don't really like this approach of using a bash array, I think it makes things harder to understand and requires to decode the rather complex bash variable expansions, but hey, it works, so OK. I left this up to you. > +$NFT add table t > +$NFT add chain t c > + > +for (( i = 0 ; i < ${#rules[@]} ; i++ )) > +do > + $1 $1 <-- what does this? I think we can safely remote it. > + `$NFT add rule t c ${rules[$i]} 2>>/dev/null` No need to run command in `a subshell`. BTW this $(syntax) is preferred. Also, no need to redirect stderr, we are actually interested in it. In fact, if you delete the redirection and you run this test: % sudo ./run-tests.sh testcases/sets/0023unknown_value_to_use_0 I: using nft binary ../../src/nft I: [OK] testcases/sets/0023unknown_value_to_use_0 <cmdline>:1:28-36: Error: you cannot use a set here, unknown value to use add rule t c tcp dport set {1, 2, 3} ~~~~~~~~~~~~~~^^^^^^^^^ <cmdline>:1:28-36: Error: you cannot use a set here, unknown value to use add rule t c udp dport set {1, 2, 3} ~~~~~~~~~~~~~~^^^^^^^^^ [...] <cmdline>:1:30-67: Error: you cannot use a set here, unknown value to use add rule t c ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02} ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ <cmdline>:1:27-50: Error: you cannot use a set here, unknown value to use add rule t c ip saddr set {192.19.1.2, 191.1.22.1} ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^ I: results: [OK] 1 [FAILED] 0 [TOTAL] 1 You get access to the actual error messages, which is useful to see that the return code of 1 maps to the error message itself. Still, if you run the complete testsuite (i.e, not a single test), you can hide/show these messages using the '-v' switch. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 9, 2017 at 3:28 PM, Arturo Borrero Gonzalez <arturo@netfilter.org> wrote: > On 9 June 2017 at 11:30, Shyam Saini <mayhs11saini@gmail.com> 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) >> >> 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 | 34 ++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> create mode 100755 tests/shell/testcases/sets/0023unknown_value_to_use_0 >> > > Thanks Shyam, > > minor things below, almost there! > Please send an v2 with the requested changes, as you could see, they > are cosmetic changes > >> 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..011cedd >> --- /dev/null >> +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0 >> @@ -0,0 +1,34 @@ >> +#!/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) >> + > > Better remove the indentations. >> +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}" >> + ) >> + > > I don't really like this approach of using a bash array, > I think it makes things harder to understand and requires to decode > the rather complex > bash variable expansions, but hey, it works, so OK. > I left this up to you. Actually it was used in some test testcases/sets/0022type_selective_flush_0 So, I thought it is correct way to use it. As it is working fine, I will keep it in V2 >> +$NFT add table t >> +$NFT add chain t c >> + >> +for (( i = 0 ; i < ${#rules[@]} ; i++ )) >> +do >> + $1 > > $1 <-- what does this? I think we can safely remote it. Done >> + `$NFT add rule t c ${rules[$i]} 2>>/dev/null` > > No need to run command in `a subshell`. BTW this $(syntax) is preferred. Thanks, I'll remove that v2. May i drop $(syntax), as former is working fine for me and but $(syntax) is throwing sytax error. $NFT add rule t c "$(rules[$i])" > Also, no need to redirect stderr, we are actually interested in it. > In fact, if you delete the redirection and you run this test: Sure, I'll remove redirection > % sudo ./run-tests.sh testcases/sets/0023unknown_value_to_use_0 > I: using nft binary ../../src/nft > > I: [OK] testcases/sets/0023unknown_value_to_use_0 > <cmdline>:1:28-36: Error: you cannot use a set here, unknown value to use > add rule t c tcp dport set {1, 2, 3} > ~~~~~~~~~~~~~~^^^^^^^^^ > <cmdline>:1:28-36: Error: you cannot use a set here, unknown value to use > add rule t c udp dport set {1, 2, 3} > ~~~~~~~~~~~~~~^^^^^^^^^ > [...] > <cmdline>:1:30-67: Error: you cannot use a set here, unknown value to use > add rule t c ether daddr set {01:00:5e:00:01:01, 01:00:5e:00:02:02} > ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > <cmdline>:1:27-50: Error: you cannot use a set here, unknown value to use > add rule t c ip saddr set {192.19.1.2, 191.1.22.1} > ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^ > > I: results: [OK] 1 [FAILED] 0 [TOTAL] 1 > > You get access to the actual error messages, which is useful to see > that the return code of 1 maps to > the error message itself. > > Still, if you run the complete testsuite (i.e, not a single test), you > can hide/show these messages > using the '-v' switch. I'll send the v2. Thanks a lot -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..011cedd --- /dev/null +++ b/tests/shell/testcases/sets/0023unknown_value_to_use_0 @@ -0,0 +1,34 @@ +#!/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 + $1 + `$NFT add rule t c ${rules[$i]} 2>>/dev/null` + ret=$? + if [ $ret -ne 1 ]; + then + echo "E: failed test: $1 returned $ret instead of 1" >&2 + exit 1 + fi +done +
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 | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100755 tests/shell/testcases/sets/0023unknown_value_to_use_0