[PATCH 1/2] tests: shell: Add test for incomplete set add set command

[PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Shyam Saini]

Before the [Test] commit if we run nft with incomplete "add set"
command it caused segmentation fault and exit with error code 139 and
further it didn't throw any error message.

  For example:
    $ sudo nft add set t s

But after the [Test] commit it throws syntax error message and exits with
return value 1.

  For example:
    $ sudo nft add set t s
    <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
    add set t s
               ^

This commit tests changes made in the [Test] commit.

Test:c6cd7c22548a ( "src: fix crash when inputting an incomplete set add
command" )
Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
---
 .../testcases/sets/0023incomplete_add_set_command_0      | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0023incomplete_add_set_command_0

diff --git a/tests/shell/testcases/sets/0023incomplete_add_set_command_0 b/tests/shell/testcases/sets/0023incomplete_add_set_command_0
new file mode 100755
index 000000000000..b7535f7059db
--- /dev/null
+++ b/tests/shell/testcases/sets/0023incomplete_add_set_command_0
@@ -0,0 +1,16 @@
+#!/bin/bash
+
+# This testscase checks bug identified and fixed in the commit Id "c6cd7c22548a"
+# Before the commit c6cd7c22548a, nft returns 139 (i.e, segmentation fault) which
+# indicates the bug but after the commit it returns 1.
+
+$NFT add table t
+$NFT add set t c
+
+ret=$?
+if [ $ret -ne 1 ] ;
+then
+	echo "E: returned $ret instead of 1" >&2
+	exit 1
+fi
+
-- 
1.9.1

[PATCH 2/2] tests: shell: Test input descriptors for included files

[Shyam Saini]

Before the [Test] commit, nft error message was pointing to wrong
file.

But after the commit it points to right file.
This commit test the changes made in the [Test] commit.

Test:b14572f72aac (" erec: Fix input descriptors for included files ")
Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
---
 .../include/0013input_descriptors_included_files_0 | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100755 tests/shell/testcases/include/0013input_descriptors_included_files_0

diff --git a/tests/shell/testcases/include/0013input_descriptors_included_files_0 b/tests/shell/testcases/include/0013input_descriptors_included_files_0
new file mode 100755
index 000000000000..26f43faf3145
--- /dev/null
+++ b/tests/shell/testcases/include/0013input_descriptors_included_files_0
@@ -0,0 +1,52 @@
+#!/bin/bash
+
+# This test the changes made in commit id "b14572f72aac".
+# When the commit was not applied, nft pointed to wrong files name.
+# As the commit only fixes the error messages and hence does not change the
+# return value so, we need to compare the "file name" in the error message
+# instead of return value of nft.
+
+
+tmpfile1=$(mktemp -p .)
+if [ ! -w $tmpfile1 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile2=$(mktemp -p .)
+if [ ! -w $tmpfile2 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile3=$(mktemp -p .)
+if [ ! -w $tmpfile3 ] ; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+tmpfile4=$(mktemp -p .)
+if [ ! -w $tmpfile4 ]; then
+        echo "Failed to create tmp file" >&2
+        exit 0
+fi
+
+trap "rm -rf $tmpfile1 $tmpfile2 $tmpfile3 $tmpfile4" EXIT # cleanup if aborted
+
+RULESET1="include \"$tmpfile2\""
+RULESET2="include \"$tmpfile3\""
+RULESET3="add rule x y anything everything"			# wrong nft syntax
+
+
+echo "$RULESET1" > $tmpfile1
+echo "$RULESET2" >> $tmpfile1
+echo "$RULESET3" > $tmpfile2
+
+$NFT -f $tmpfile1 2> $tmpfile4
+
+var=$(awk -F: '$4==" Error"{print $1;exit;}' $tmpfile4)
+
+if [ $var == "$tmpfile3" ]; then
+	echo "E: Test failed" >&2
+	exit 1
+fi
-- 
1.9.1

Re: [PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Pablo Neira Ayuso]

On Fri, Jun 23, 2017 at 05:35:55PM +0530, Shyam Saini wrote:
> Before the [Test] commit if we run nft with incomplete "add set"
> command it caused segmentation fault and exit with error code 139 and
> further it didn't throw any error message.
> 
>   For example:
>     $ sudo nft add set t s
> 
> But after the [Test] commit it throws syntax error message and exits with
> return value 1.
> 
>   For example:
>     $ sudo nft add set t s
>     <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
>     add set t s
>                ^
> 
> This commit tests changes made in the [Test] commit.

Applied, thanks.

I have reworked a bit your commit message, it looks a bit convoluted.
No worries, have a look at what I pushed out for reference.

Re: [PATCH 2/2] tests: shell: Test input descriptors for included files

[Pablo Neira Ayuso]

On Fri, Jun 23, 2017 at 05:35:56PM +0530, Shyam Saini wrote:
> Before the [Test] commit, nft error message was pointing to wrong
> file.
> 
> But after the commit it points to right file.
> This commit test the changes made in the [Test] commit.

Also applied, thanks.

Re: [PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Shyam Saini]

On Mon, Jun 26, 2017 at 10:29 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jun 23, 2017 at 05:35:55PM +0530, Shyam Saini wrote:
>> Before the [Test] commit if we run nft with incomplete "add set"
>> command it caused segmentation fault and exit with error code 139 and
>> further it didn't throw any error message.
>>
>>   For example:
>>     $ sudo nft add set t s
>>
>> But after the [Test] commit it throws syntax error message and exits with
>> return value 1.
>>
>>   For example:
>>     $ sudo nft add set t s
>>     <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
>>     add set t s
>>                ^
>>
>> This commit tests changes made in the [Test] commit.
>
> Applied, thanks.
>
> I have reworked a bit your commit message, it looks a bit convoluted.

Thanks a lot :)

> No worries, have a look at what I pushed out for reference.

Shouldn't we follow conventions mentioned in "scripts/checkpatch.pl" ?

Thanks,
shyam

Re: [PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Pablo Neira Ayuso]

On Mon, Jun 26, 2017 at 11:02:34PM +0530, Shyam Saini wrote:
> On Mon, Jun 26, 2017 at 10:29 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Jun 23, 2017 at 05:35:55PM +0530, Shyam Saini wrote:
> >> Before the [Test] commit if we run nft with incomplete "add set"
> >> command it caused segmentation fault and exit with error code 139 and
> >> further it didn't throw any error message.
> >>
> >>   For example:
> >>     $ sudo nft add set t s
> >>
> >> But after the [Test] commit it throws syntax error message and exits with
> >> return value 1.
> >>
> >>   For example:
> >>     $ sudo nft add set t s
> >>     <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
> >>     add set t s
> >>                ^
> >>
> >> This commit tests changes made in the [Test] commit.
> >
> > Applied, thanks.
> >
> > I have reworked a bit your commit message, it looks a bit convoluted.
> 
> Thanks a lot :)
> 
> > No worries, have a look at what I pushed out for reference.
> 
> Shouldn't we follow conventions mentioned in "scripts/checkpatch.pl" ?

Interesting.

So the [test] thing is something that checkpatch.pl suggests, right?

I would like to know more about that new thing, do you have
documentation about this?

I just tend to like that commit message are human-readable. I
understand this structure makes it easier for robots, more simple to
parse.

So don't take checkpatch too seriously, probably too much engineering
is going on there ;-)

Re: [PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Shyam Saini]

On Mon, Jun 26, 2017 at 11:07 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jun 26, 2017 at 11:02:34PM +0530, Shyam Saini wrote:
>> On Mon, Jun 26, 2017 at 10:29 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Fri, Jun 23, 2017 at 05:35:55PM +0530, Shyam Saini wrote:
>> >> Before the [Test] commit if we run nft with incomplete "add set"
>> >> command it caused segmentation fault and exit with error code 139 and
>> >> further it didn't throw any error message.
>> >>
>> >>   For example:
>> >>     $ sudo nft add set t s
>> >>
>> >> But after the [Test] commit it throws syntax error message and exits with
>> >> return value 1.
>> >>
>> >>   For example:
>> >>     $ sudo nft add set t s
>> >>     <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
>> >>     add set t s
>> >>                ^
>> >>
>> >> This commit tests changes made in the [Test] commit.
>> >
>> > Applied, thanks.
>> >
>> > I have reworked a bit your commit message, it looks a bit convoluted.
>>
>> Thanks a lot :)
>>
>> > No worries, have a look at what I pushed out for reference.
>>
>> Shouldn't we follow conventions mentioned in "scripts/checkpatch.pl" ?
>
> Interesting.
>
> So the [test] thing is something that checkpatch.pl suggests, right?
yes something like that.

> I would like to know more about that new thing, do you have
> documentation about this?

No documentation but yeah it throws following error when convention is
not followed.

"ERROR: Please use git commit description style 'commit <12+ chars of
sha1> ("<title line>")' - ie: 'commit c6cd7c22548a ("src: fix crash
when inputting an incomplete set add command")'"

> I just tend to like that commit message are human-readable. I
> understand this structure makes it easier for robots, more simple to
> parse.

> So don't take checkpatch too seriously, probably too much engineering
> is going on there ;-)

Sure,

Thanks for the correction

Re: [PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Pablo Neira Ayuso]

On Mon, Jun 26, 2017 at 11:24:23PM +0530, Shyam Saini wrote:
> On Mon, Jun 26, 2017 at 11:07 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Jun 26, 2017 at 11:02:34PM +0530, Shyam Saini wrote:
> >> On Mon, Jun 26, 2017 at 10:29 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Fri, Jun 23, 2017 at 05:35:55PM +0530, Shyam Saini wrote:
> >> >> Before the [Test] commit if we run nft with incomplete "add set"
> >> >> command it caused segmentation fault and exit with error code 139 and
> >> >> further it didn't throw any error message.
> >> >>
> >> >>   For example:
> >> >>     $ sudo nft add set t s
> >> >>
> >> >> But after the [Test] commit it throws syntax error message and exits with
> >> >> return value 1.
> >> >>
> >> >>   For example:
> >> >>     $ sudo nft add set t s
> >> >>     <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
> >> >>     add set t s
> >> >>                ^
> >> >>
> >> >> This commit tests changes made in the [Test] commit.
> >> >
> >> > Applied, thanks.
> >> >
> >> > I have reworked a bit your commit message, it looks a bit convoluted.
> >>
> >> Thanks a lot :)
> >>
> >> > No worries, have a look at what I pushed out for reference.
> >>
> >> Shouldn't we follow conventions mentioned in "scripts/checkpatch.pl" ?
> >
> > Interesting.
> >
> > So the [test] thing is something that checkpatch.pl suggests, right?
> yes something like that.
> 
> > I would like to know more about that new thing, do you have
> > documentation about this?
> 
> No documentation but yeah it throws following error when convention is
> not followed.
> 
> "ERROR: Please use git commit description style 'commit <12+ chars of
> sha1> ("<title line>")' - ie: 'commit c6cd7c22548a ("src: fix crash
> when inputting an incomplete set add command")'"

Ah, that's different thing.

so no need for the [Test] tag you are adding.

This means that:

commit c6cd7c22548a ("src: fix crash when inputting an incomplete set add command")

is the right way to refer to commits. That's a valid thing indeed, we
should stick to that.

Let's stick to this in follow up patches, OK?

Re: [PATCH 1/2] tests: shell: Add test for incomplete set add set command

[Shyam Saini]

On Mon, Jun 26, 2017 at 11:38 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jun 26, 2017 at 11:24:23PM +0530, Shyam Saini wrote:
>> On Mon, Jun 26, 2017 at 11:07 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Mon, Jun 26, 2017 at 11:02:34PM +0530, Shyam Saini wrote:
>> >> On Mon, Jun 26, 2017 at 10:29 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >> > On Fri, Jun 23, 2017 at 05:35:55PM +0530, Shyam Saini wrote:
>> >> >> Before the [Test] commit if we run nft with incomplete "add set"
>> >> >> command it caused segmentation fault and exit with error code 139 and
>> >> >> further it didn't throw any error message.
>> >> >>
>> >> >>   For example:
>> >> >>     $ sudo nft add set t s
>> >> >>
>> >> >> But after the [Test] commit it throws syntax error message and exits with
>> >> >> return value 1.
>> >> >>
>> >> >>   For example:
>> >> >>     $ sudo nft add set t s
>> >> >>     <cmdline>:1:12-12: Error: syntax error, unexpected newline, expecting '{'
>> >> >>     add set t s
>> >> >>                ^
>> >> >>
>> >> >> This commit tests changes made in the [Test] commit.
>> >> >
>> >> > Applied, thanks.
>> >> >
>> >> > I have reworked a bit your commit message, it looks a bit convoluted.
>> >>
>> >> Thanks a lot :)
>> >>
>> >> > No worries, have a look at what I pushed out for reference.
>> >>
>> >> Shouldn't we follow conventions mentioned in "scripts/checkpatch.pl" ?
>> >
>> > Interesting.
>> >
>> > So the [test] thing is something that checkpatch.pl suggests, right?
>> yes something like that.
>>
>> > I would like to know more about that new thing, do you have
>> > documentation about this?
>>
>> No documentation but yeah it throws following error when convention is
>> not followed.
>>
>> "ERROR: Please use git commit description style 'commit <12+ chars of
>> sha1> ("<title line>")' - ie: 'commit c6cd7c22548a ("src: fix crash
>> when inputting an incomplete set add command")'"
>
> Ah, that's different thing.
>
> so no need for the [Test] tag you are adding.

Sure
> This means that:
>
> commit c6cd7c22548a ("src: fix crash when inputting an incomplete set add command")
>
> is the right way to refer to commits. That's a valid thing indeed, we
> should stick to that.

After adding  "commit" keyword in the same commit message the error gone .
Sorry, my bad.

> Let's stick to this in follow up patches, OK?
Sure

Thanks a lot