On Fri, Jun 9, 2017 at 3:28 PM, Arturo Borrero Gonzalez
<arturo@xxxxxxxxxxxxx> wrote:
> On 9 June 2017 at 11:30, Shyam Saini <mayhs11saini@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
