[nft PATCH v2 0/3] monitor: Some fixes and improvements

[nft PATCH v2 0/3] monitor: Some fixes and improvements

[Phil Sutter]

This is v2 containing the patches not applied from v1. Apart from
rebasing upon current upstream master, I folded patch 4 into patch 1
since they really belong together: The code changes to make 'nft
monitor' print correct syntax (by including the mandatory semicolons)
harmed readability since it dropped whitespace from the output. Former
patch 4 resolved this, not only for the cases patch 1 broke, but also at
another spot (i.e. between 'type' attribute and the following ones).

Patch 2 of this series depends on patch 1 since the simplification
requires that 'nft monitor' output matches given input (apart from
whitespace change) which wasn't the case before patch 1.

Finally, patch 3 (along with patch 2) remains unchanged.

Phil Sutter (3):
  monitor: Fix printing of set declarations
  tests/monitor: Simplify testcases
  tests/monitor: Clear ruleset after testing

 src/rule.c                             |  8 ++++----
 tests/monitor/run-tests.sh             | 15 ++++++++++-----
 tests/monitor/testcases/set-maps.t     |  7 +++----
 tests/monitor/testcases/set-mixed.t    | 10 +++-------
 tests/monitor/testcases/set-multiple.t |  7 ++-----
 tests/monitor/testcases/set-simple.t   | 21 ++++++++-------------
 6 files changed, 30 insertions(+), 38 deletions(-)

-- 
2.13.1

[nft PATCH v2 1/3] monitor: Fix printing of set declarations

[Phil Sutter]

The optional attributes 'flags', 'gc-interval' and 'timeout' have to be
delimited by stmt_separator (either newline or semicolon), not 'nl'
which is set to whitespace by set_print_plain().

In order to restore readability, change stmt_separator to include a
single whitespace after the semicolon.

Here's monitor output for the following command:

| # nft add set ip t testset { type inet_service; \
|			timeout 60s; gc-interval 120s; }

Before this patch:

| add set ip t testset { type inet_service;timeout 1m gc-interval 2m }

With this patch applied:

| add set ip t testset { type inet_service; timeout 1m; gc-interval 2m; }

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Folded former patch 4 into this one since they are related.
---
 src/rule.c                             | 8 ++++----
 tests/monitor/testcases/set-maps.t     | 2 +-
 tests/monitor/testcases/set-mixed.t    | 2 +-
 tests/monitor/testcases/set-multiple.t | 4 ++--
 tests/monitor/testcases/set-simple.t   | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 7f83980c60818..12714ed3ccc70 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -335,18 +335,18 @@ static void set_print_declaration(const struct set *set,
 			printf("%stimeout", delim);
 			delim = ",";
 		}
-		printf("%s", opts->nl);
+		printf("%s", opts->stmt_separator);
 	}
 
 	if (set->timeout) {
 		printf("%s%stimeout ", opts->tab, opts->tab);
 		time_print(set->timeout / 1000);
-		printf("%s", opts->nl);
+		printf("%s", opts->stmt_separator);
 	}
 	if (set->gc_int) {
 		printf("%s%sgc-interval ", opts->tab, opts->tab);
 		time_print(set->gc_int / 1000);
-		printf("%s", opts->nl);
+		printf("%s", opts->stmt_separator);
 	}
 }
 
@@ -381,7 +381,7 @@ void set_print_plain(const struct set *s, struct output_ctx *octx)
 		.nl		= " ",
 		.table		= s->handle.table,
 		.family		= family2str(s->handle.family),
-		.stmt_separator	= ";",
+		.stmt_separator	= "; ",
 	};
 
 	do_set_print(s, &opts, octx);
diff --git a/tests/monitor/testcases/set-maps.t b/tests/monitor/testcases/set-maps.t
index d94016beb0767..6ea36cb9d11d6 100644
--- a/tests/monitor/testcases/set-maps.t
+++ b/tests/monitor/testcases/set-maps.t
@@ -2,7 +2,7 @@
 I add table ip t
 O add table ip t
 I add map ip t portip { type inet_service: ipv4_addr; flags interval; }
-O add map ip t portip { type inet_service : ipv4_addr;flags interval }
+O add map ip t portip { type inet_service : ipv4_addr;flags interval; }
 
 I add element ip t portip { 80-100: 10.0.0.1 }
 O add element ip t portip { 80-100 : 10.0.0.1 }
diff --git a/tests/monitor/testcases/set-mixed.t b/tests/monitor/testcases/set-mixed.t
index c4699edacec11..2eb35b5aa1efb 100644
--- a/tests/monitor/testcases/set-mixed.t
+++ b/tests/monitor/testcases/set-mixed.t
@@ -2,7 +2,7 @@
 I add table ip t
 O add table ip t
 I add set ip t portrange { type inet_service; flags interval; }
-O add set ip t portrange { type inet_service;flags interval }
+O add set ip t portrange { type inet_service;flags interval; }
 I add set ip t ports { type inet_service; }
 O add set ip t ports { type inet_service;}
 
diff --git a/tests/monitor/testcases/set-multiple.t b/tests/monitor/testcases/set-multiple.t
index d94f941b39693..ce919125a593f 100644
--- a/tests/monitor/testcases/set-multiple.t
+++ b/tests/monitor/testcases/set-multiple.t
@@ -2,9 +2,9 @@
 I add table ip t
 O add table ip t
 I add set ip t portrange { type inet_service; flags interval; }
-O add set ip t portrange { type inet_service;flags interval }
+O add set ip t portrange { type inet_service;flags interval; }
 I add set ip t portrange2 { type inet_service; flags interval; }
-O add set ip t portrange2 { type inet_service;flags interval }
+O add set ip t portrange2 { type inet_service;flags interval; }
 
 # make sure concurrent adds work
 I add element ip t portrange { 1024-65535 }
diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
index 22f648dbfa232..e44cce08f575c 100644
--- a/tests/monitor/testcases/set-simple.t
+++ b/tests/monitor/testcases/set-simple.t
@@ -2,7 +2,7 @@
 I add table ip t
 O add table ip t
 I add set ip t portrange { type inet_service; flags interval; }
-O add set ip t portrange { type inet_service;flags interval }
+O add set ip t portrange { type inet_service;flags interval; }
 
 # adding some ranges
 I add element ip t portrange { 1-10 }
-- 
2.13.1

[nft PATCH v2 2/3] tests/monitor: Simplify testcases

[Phil Sutter]

By introducing 'O -' indicating that output should be identical as
input, testcases can be simplified quite a bit.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/monitor/run-tests.sh             |  4 ++++
 tests/monitor/testcases/set-maps.t     |  7 +++----
 tests/monitor/testcases/set-mixed.t    | 10 +++-------
 tests/monitor/testcases/set-multiple.t |  7 ++-----
 tests/monitor/testcases/set-simple.t   | 21 ++++++++-------------
 5 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/tests/monitor/run-tests.sh b/tests/monitor/run-tests.sh
index e21ed6880bc13..de19462438d64 100755
--- a/tests/monitor/run-tests.sh
+++ b/tests/monitor/run-tests.sh
@@ -20,6 +20,10 @@ cmd_append() {
 	echo "$*" >>$command_file
 }
 output_append() {
+	[[ "$*" == '-' ]] && {
+		cat $command_file >>$output_file
+		return
+	}
 	echo "$*" >>$output_file
 }
 run_test() {
diff --git a/tests/monitor/testcases/set-maps.t b/tests/monitor/testcases/set-maps.t
index 6ea36cb9d11d6..3d86720ec8136 100644
--- a/tests/monitor/testcases/set-maps.t
+++ b/tests/monitor/testcases/set-maps.t
@@ -1,11 +1,10 @@
 # first the setup
 I add table ip t
-O add table ip t
 I add map ip t portip { type inet_service: ipv4_addr; flags interval; }
-O add map ip t portip { type inet_service : ipv4_addr;flags interval; }
+O -
 
 I add element ip t portip { 80-100: 10.0.0.1 }
-O add element ip t portip { 80-100 : 10.0.0.1 }
+O -
 
 I add element ip t portip { 1024-65535: 10.0.0.1 }
-O add element ip t portip { 1024-65535 : 10.0.0.1 }
+O -
diff --git a/tests/monitor/testcases/set-mixed.t b/tests/monitor/testcases/set-mixed.t
index 2eb35b5aa1efb..9c1c5323f2e4e 100644
--- a/tests/monitor/testcases/set-mixed.t
+++ b/tests/monitor/testcases/set-mixed.t
@@ -1,19 +1,15 @@
 # first the setup
 I add table ip t
-O add table ip t
 I add set ip t portrange { type inet_service; flags interval; }
-O add set ip t portrange { type inet_service;flags interval; }
 I add set ip t ports { type inet_service; }
-O add set ip t ports { type inet_service;}
+O -
 
 # make sure concurrent adds work
 I add element ip t portrange { 1024-65535 }
 I add element ip t ports { 10 }
-O add element ip t portrange { 1024-65535 }
-O add element ip t ports { 10 }
+O -
 
 # delete items again
 I delete element ip t portrange { 1024-65535 }
 I delete element ip t ports { 10 }
-O delete element ip t portrange { 1024-65535 }
-O delete element ip t ports { 10 }
+O -
diff --git a/tests/monitor/testcases/set-multiple.t b/tests/monitor/testcases/set-multiple.t
index ce919125a593f..ad91fac047fe8 100644
--- a/tests/monitor/testcases/set-multiple.t
+++ b/tests/monitor/testcases/set-multiple.t
@@ -1,13 +1,10 @@
 # first the setup
 I add table ip t
-O add table ip t
 I add set ip t portrange { type inet_service; flags interval; }
-O add set ip t portrange { type inet_service;flags interval; }
 I add set ip t portrange2 { type inet_service; flags interval; }
-O add set ip t portrange2 { type inet_service;flags interval; }
+O -
 
 # make sure concurrent adds work
 I add element ip t portrange { 1024-65535 }
 I add element ip t portrange2 { 10-20 }
-O add element ip t portrange { 1024-65535 }
-O add element ip t portrange2 { 10-20 }
+O -
diff --git a/tests/monitor/testcases/set-simple.t b/tests/monitor/testcases/set-simple.t
index e44cce08f575c..ebff7cbda0c8b 100644
--- a/tests/monitor/testcases/set-simple.t
+++ b/tests/monitor/testcases/set-simple.t
@@ -1,14 +1,13 @@
 # first the setup
 I add table ip t
-O add table ip t
 I add set ip t portrange { type inet_service; flags interval; }
-O add set ip t portrange { type inet_service;flags interval; }
+O -
 
 # adding some ranges
 I add element ip t portrange { 1-10 }
-O add element ip t portrange { 1-10 }
+O -
 I add element ip t portrange { 1024-65535 }
-O add element ip t portrange { 1024-65535 }
+O -
 I add element ip t portrange { 20-30, 40-50 }
 O add element ip t portrange { 20-30 }
 O add element ip t portrange { 40-50 }
@@ -22,26 +21,22 @@ O delete element ip t portrange { 1-10 }
 
 # make sure lower scope boundary works
 I add element ip t portrange { 0-10 }
-O add element ip t portrange { 0-10 }
+O -
 
 # make sure half open before other element works
 I add element ip t portrange { 1024-65535 }
 I add element ip t portrange { 100-200 }
-O add element ip t portrange { 1024-65535 }
-O add element ip t portrange { 100-200 }
+O -
 
 # make sure deletion of elements works
 I delete element ip t portrange { 0-10 }
-O delete element ip t portrange { 0-10 }
+O -
 I delete element ip t portrange { 100-200 }
 I delete element ip t portrange { 1024-65535 }
-O delete element ip t portrange { 100-200 }
-O delete element ip t portrange { 1024-65535 }
+O -
 
 # make sure mixed add/delete works
 I add element ip t portrange { 10-20 }
 I add element ip t portrange { 1024-65535 }
 I delete element ip t portrange { 10-20 }
-O add element ip t portrange { 10-20 }
-O add element ip t portrange { 1024-65535 }
-O delete element ip t portrange { 10-20 }
+O -
-- 
2.13.1

[nft PATCH v2 3/3] tests/monitor: Clear ruleset after testing

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/monitor/run-tests.sh | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tests/monitor/run-tests.sh b/tests/monitor/run-tests.sh
index de19462438d64..9fd0e504d08c0 100755
--- a/tests/monitor/run-tests.sh
+++ b/tests/monitor/run-tests.sh
@@ -2,17 +2,18 @@
 
 cd $(dirname $0)
 
+nft=../../src/nft
+mydiff() {
+	diff -w -I '^# ' "$@"
+}
+
 testdir=$(mktemp -d)
 if [ ! -d $testdir ]; then
 	echo "Failed to create test directory" >&2
 	exit 0
 fi
-trap "rm -rf $testdir" EXIT
+trap "rm -rf $testdir; $nft flush ruleset" EXIT
 
-nft=../../src/nft
-mydiff() {
-	diff -w -I '^# ' "$@"
-}
 command_file=$(mktemp -p $testdir)
 output_file=$(mktemp -p $testdir)
 
-- 
2.13.1

Re: [nft PATCH v2 0/3] monitor: Some fixes and improvements

[Pablo Neira Ayuso]

On Tue, Jul 25, 2017 at 08:39:41PM +0200, Phil Sutter wrote:
> This is v2 containing the patches not applied from v1. Apart from
> rebasing upon current upstream master, I folded patch 4 into patch 1
> since they really belong together: The code changes to make 'nft
> monitor' print correct syntax (by including the mandatory semicolons)
> harmed readability since it dropped whitespace from the output. Former
> patch 4 resolved this, not only for the cases patch 1 broke, but also at
> another spot (i.e. between 'type' attribute and the following ones).
> 
> Patch 2 of this series depends on patch 1 since the simplification
> requires that 'nft monitor' output matches given input (apart from
> whitespace change) which wasn't the case before patch 1.
> 
> Finally, patch 3 (along with patch 2) remains unchanged.

Series applied, thanks a lot for addressing all my feedback!