[nft PATCH 0/4] A bunch of fixes for echo output

[nft PATCH 0/4] A bunch of fixes for echo output

[Phil Sutter]

This series addresses the shortcomings of my echo option implementation
pointed out by Pablo. In addition to that, I figured that test suites
for monitor and echo are pretty similar so I merged both into one.

Phil Sutter (4):
  mnl: Drop --echo support for non-batch calls
  netlink: Fix segfault when using --echo flag
  echo: Fix for added delays in rule updates
  tests: Merge monitor and echo test suites

 include/netlink.h                |   5 +-
 src/evaluate.c                   |   9 ----
 src/mnl.c                        |  23 +--------
 src/netlink.c                    |  26 +++++++---
 src/rule.c                       |  26 ++++++++--
 tests/echo/run-tests.sh          |  45 ----------------
 tests/echo/testcases/simple.t    |  12 -----
 tests/monitor/run-tests.sh       | 107 +++++++++++++++++++++++++++------------
 tests/monitor/testcases/simple.t |  20 ++++++++
 9 files changed, 138 insertions(+), 135 deletions(-)
 delete mode 100755 tests/echo/run-tests.sh
 delete mode 100644 tests/echo/testcases/simple.t
 create mode 100644 tests/monitor/testcases/simple.t

-- 
2.13.1

[nft PATCH 1/4] mnl: Drop --echo support for non-batch calls

[Phil Sutter]

Echo support in nft_mnl_talk() was broken: nft_mnl_talk_cb() passed
cbdata->data as second parameter to netlink_echo_callback() which
expected it to be of type struct netlink_ctx while in fact it was
whatever callers of nft_mnl_talk() passed as callback data (in most
cases a NULL pointer).

I didn't notice this because I didn't test for kernels without support
for transactions. This has been added to nftables in kernel version 3.16
back in 2014. Since then, user space which doesn't support it can't even
add a table anymore. So adding this new feature to the old code path is
really not feasible, therefore drop this broken attempt at supporting
it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/mnl.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 031b7f39da8f5..5017b81c96e7c 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -67,32 +67,11 @@ out:
 	return ret;
 }
 
-struct nft_mnl_talk_cb_data {
-	int (*cb)(const struct nlmsghdr *nlh, void *data);
-	void *data;
-};
-
-static int nft_mnl_talk_cb(const struct nlmsghdr *nlh, void *data)
-{
-	struct nft_mnl_talk_cb_data *cbdata = data;
-	int rc;
-
-	if (cbdata->cb)
-		rc = cbdata->cb(nlh, cbdata->data);
-	if (rc)
-		return rc;
-	return netlink_echo_callback(nlh, cbdata->data);
-}
-
 static int
 nft_mnl_talk(struct mnl_socket *nf_sock, const void *data, unsigned int len,
 	     int (*cb)(const struct nlmsghdr *nlh, void *data), void *cb_data)
 {
 	uint32_t portid = mnl_socket_get_portid(nf_sock);
-	struct nft_mnl_talk_cb_data tcb_data = {
-		.cb = cb,
-		.data = cb_data,
-	};
 
 #ifdef DEBUG
 	if (debug_level & DEBUG_MNL)
@@ -102,7 +81,7 @@ nft_mnl_talk(struct mnl_socket *nf_sock, const void *data, unsigned int len,
 	if (mnl_socket_sendto(nf_sock, data, len) < 0)
 		return -1;
 
-	return nft_mnl_recv(nf_sock, seq, portid, &nft_mnl_talk_cb, &tcb_data);
+	return nft_mnl_recv(nf_sock, seq, portid, cb, cb_data);
 }
 
 /*
-- 
2.13.1

[nft PATCH 2/4] netlink: Fix segfault when using --echo flag

[Phil Sutter]

Commit 07b45939972eb ("src: introduce struct nft_cache") added cache
pointer to struct netlink_mon_handler and the code assumes it is never
NULL. Therefore initialize it in the dummy version of
netlink_mon_handler in netlink_echo_callback().

Fixes: b99c4d072d996 ("Implement --echo option")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/netlink.c b/src/netlink.c
index 8aef8d9ab4070..f631c26b2b9ca 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -3075,12 +3075,14 @@ static int netlink_events_cb(const struct nlmsghdr *nlh, void *data)
 
 int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
 {
+	struct netlink_ctx *ctx = data;
 	struct netlink_mon_handler echo_monh = {
 		.format = NFTNL_OUTPUT_DEFAULT,
-		.ctx = data,
+		.ctx = ctx,
 		.loc = &netlink_location,
 		.monitor_flags = 0xffffffff,
 		.cache_needed = true,
+		.cache = ctx->cache,
 	};
 
 	if (!echo_monh.ctx->octx->echo)
-- 
2.13.1

[nft PATCH 3/4] echo: Fix for added delays in rule updates

[Phil Sutter]

The added cache update upon every command dealing with rules was a
bummer. Instead, perform the needed cache update only if echo option was
set.

Initially, I tried to perform the cache update from within
netlink_echo_callback(), but that turned into a mess since the shared
socket between cache_init() and mnl_batch_talk() would receive
unexpected new input. So instead update the cache from do_command_add(),
netlink_replace_rule_batch() and do_comand_insert() so it completes
before mnl_batch_talk() starts listening.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/netlink.h |  5 +----
 src/evaluate.c    |  9 ---------
 src/netlink.c     | 22 +++++++++++++++-------
 src/rule.c        | 26 ++++++++++++++++++++++----
 4 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index 3726171424c33..e7e4bbcfc0f51 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -119,10 +119,7 @@ extern int netlink_add_rule_batch(struct netlink_ctx *ctx,
 extern int netlink_del_rule_batch(struct netlink_ctx *ctx,
 				  const struct handle *h,
 				  const struct location *loc);
-extern int netlink_replace_rule_batch(struct netlink_ctx *ctx,
-				      const struct handle *h,
-				      const struct rule *rule,
-				      const struct location *loc);
+extern int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd);
 
 extern int netlink_add_chain(struct netlink_ctx *ctx, const struct handle *h,
 			     const struct location *loc,
diff --git a/src/evaluate.c b/src/evaluate.c
index 64e14b8ba8d98..0fad0913488d0 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2965,10 +2965,6 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 		handle_merge(&cmd->set->handle, &cmd->handle);
 		return set_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
-		ret = cache_update(ctx->nf_sock, ctx->cache, cmd->op,
-				   ctx->msgs);
-		if (ret < 0)
-			return ret;
 		handle_merge(&cmd->rule->handle, &cmd->handle);
 		return rule_evaluate(ctx, cmd->rule);
 	case CMD_OBJ_CHAIN:
@@ -2983,11 +2979,6 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 	case CMD_OBJ_COUNTER:
 	case CMD_OBJ_QUOTA:
 	case CMD_OBJ_CT_HELPER:
-		ret = cache_update(ctx->nf_sock, ctx->cache, cmd->op,
-				   ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		return 0;
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
diff --git a/src/netlink.c b/src/netlink.c
index f631c26b2b9ca..e7fe62b21da6a 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -459,20 +459,28 @@ int netlink_add_rule_batch(struct netlink_ctx *ctx,
 	return err;
 }
 
-int netlink_replace_rule_batch(struct netlink_ctx *ctx, const struct handle *h,
-			       const struct rule *rule,
-			       const struct location *loc)
+int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	struct nftnl_rule *nlr;
-	int err, flags = ctx->octx->echo ? NLM_F_ECHO : 0;
+	int err, flags = 0;
 
-	nlr = alloc_nftnl_rule(&rule->handle);
-	netlink_linearize_rule(ctx, nlr, rule);
+	if (ctx->octx->echo) {
+		err = cache_update(ctx->nf_sock, ctx->cache,
+				   cmd->obj, ctx->msgs);
+		if (err < 0)
+			return err;
+
+		flags |= NLM_F_ECHO;
+	}
+
+	nlr = alloc_nftnl_rule(&cmd->rule->handle);
+	netlink_linearize_rule(ctx, nlr, cmd->rule);
 	err = mnl_nft_rule_batch_replace(nlr, ctx->batch, flags, ctx->seqnum);
 	nftnl_rule_free(nlr);
 
 	if (err < 0)
-		netlink_io_error(ctx, loc, "Could not replace rule to batch: %s",
+		netlink_io_error(ctx, &cmd->location,
+				 "Could not replace rule to batch: %s",
 				 strerror(errno));
 	return err;
 }
diff --git a/src/rule.c b/src/rule.c
index 1bd5c80158b7b..ab19525757fff 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1017,8 +1017,16 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
 {
 	uint32_t flags = excl ? NLM_F_EXCL : 0;
 
-	if (ctx->octx->echo)
+	if (ctx->octx->echo) {
+		int rc;
+
+		rc = cache_update(ctx->nf_sock, ctx->cache,
+				  cmd->obj, ctx->msgs);
+		if (rc < 0)
+			return rc;
+
 		flags |= NLM_F_ECHO;
+	}
 
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
@@ -1048,8 +1056,7 @@ static int do_command_replace(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	switch (cmd->obj) {
 	case CMD_OBJ_RULE:
-		return netlink_replace_rule_batch(ctx, &cmd->handle, cmd->rule,
-						  &cmd->location);
+		return netlink_replace_rule_batch(ctx, cmd);
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
 	}
@@ -1058,7 +1065,18 @@ static int do_command_replace(struct netlink_ctx *ctx, struct cmd *cmd)
 
 static int do_command_insert(struct netlink_ctx *ctx, struct cmd *cmd)
 {
-	uint32_t flags = ctx->octx->echo ? NLM_F_ECHO : 0;
+	uint32_t flags = 0;
+
+	if (ctx->octx->echo) {
+		int rc;
+
+		rc = cache_update(ctx->nf_sock, ctx->cache,
+				  cmd->obj, ctx->msgs);
+		if (rc < 0)
+			return rc;
+
+		flags |= NLM_F_ECHO;
+	}
 
 	switch (cmd->obj) {
 	case CMD_OBJ_RULE:
-- 
2.13.1

[nft PATCH 4/4] tests: Merge monitor and echo test suites

[Phil Sutter]

The two test suites were pretty similar already, and since echo output
is supposed to be identical to monitor output apart from delete
commands, they can be merged together with litte effort.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/echo/run-tests.sh          |  45 ----------------
 tests/echo/testcases/simple.t    |  12 -----
 tests/monitor/run-tests.sh       | 107 +++++++++++++++++++++++++++------------
 tests/monitor/testcases/simple.t |  20 ++++++++
 4 files changed, 96 insertions(+), 88 deletions(-)
 delete mode 100755 tests/echo/run-tests.sh
 delete mode 100644 tests/echo/testcases/simple.t
 create mode 100644 tests/monitor/testcases/simple.t

diff --git a/tests/echo/run-tests.sh b/tests/echo/run-tests.sh
deleted file mode 100755
index da7934d16965f..0000000000000
--- a/tests/echo/run-tests.sh
+++ /dev/null
@@ -1,45 +0,0 @@
-#!/bin/bash
-
-cd $(dirname $0)
-nft=../../src/nft
-nft_opts="-nn -a --echo"
-debug=false
-
-debug_echo() {
-	$debug || return
-
-	echo "$@"
-}
-
-trap "$nft flush ruleset" EXIT
-
-for testcase in testcases/*.t; do
-	echo "running tests from file $(basename $testcase)"
-	# files are like this:
-	#
-	# <input command>[;;<output regexp>]
-
-	$nft flush ruleset
-
-	while read line; do
-		[[ -z "$line" || "$line" == "#"* ]] && continue
-
-		# XXX: this only works if there is no semicolon in output
-		input="${line%;;*}"
-		output="${line##*;;}"
-
-		[[ -z $output ]] && output="$input"
-
-		debug_echo "calling '$nft $nft_opts $input'"
-		cmd_out=$($nft $nft_opts $input)
-		# strip trailing whitespace (happens when adding a named set)
-		cmd_out="${cmd_out% }"
-		debug_echo "got output '$cmd_out'"
-		[[ $cmd_out == $output ]] || {
-			echo "Warning: Output differs:"
-			echo "# nft $nft_opts $input"
-			echo "- $output"
-			echo "+ $cmd_out"
-		}
-	done <$testcase
-done
diff --git a/tests/echo/testcases/simple.t b/tests/echo/testcases/simple.t
deleted file mode 100644
index 566fd7e0f8176..0000000000000
--- a/tests/echo/testcases/simple.t
+++ /dev/null
@@ -1,12 +0,0 @@
-add table ip t
-add chain ip t c
-
-# note the added handle output
-add rule ip t c accept;;add rule ip t c accept # handle *
-add rule ip t c tcp dport { 22, 80, 443 } accept;;add rule ip t c tcp dport { 22, 80, 443 } accept # handle *
-
-add set ip t ipset { type ipv4_addr; }
-add element ip t ipset { 192.168.0.1 }
-
-# counter output comes with statistics
-add counter ip t cnt;;add counter ip t cnt *
diff --git a/tests/monitor/run-tests.sh b/tests/monitor/run-tests.sh
index 9fd0e504d08c0..23d4e21288e27 100755
--- a/tests/monitor/run-tests.sh
+++ b/tests/monitor/run-tests.sh
@@ -1,8 +1,9 @@
 #!/bin/bash
 
 cd $(dirname $0)
-
 nft=../../src/nft
+debug=false
+
 mydiff() {
 	diff -w -I '^# ' "$@"
 }
@@ -20,20 +21,38 @@ output_file=$(mktemp -p $testdir)
 cmd_append() {
 	echo "$*" >>$command_file
 }
-output_append() {
+monitor_output_append() {
 	[[ "$*" == '-' ]] && {
 		cat $command_file >>$output_file
 		return
 	}
 	echo "$*" >>$output_file
 }
-run_test() {
+echo_output_append() {
+	# this is a bit tricky: for replace commands, nft prints a delete
+	# command - so in case there is a replace command in $command_file,
+	# just assume any other commands in the same file are sane
+	grep -q '^replace' $command_file >/dev/null 2>&1 && {
+		monitor_output_append "$*"
+		return
+	}
+	[[ "$*" == '-' ]] && {
+		grep '^\(add\|replace\|insert\)' $command_file >>$output_file
+		return
+	}
+	[[ "$*" =~ ^add|replace|insert ]] && echo "$*" >>$output_file
+}
+monitor_run_test() {
 	monitor_output=$(mktemp -p $testdir)
-	$nft monitor >$monitor_output &
+	$nft -nn monitor >$monitor_output &
 	monitor_pid=$!
 
 	sleep 0.5
 
+	$debug && {
+		echo "command file:"
+		cat $command_file
+	}
 	$nft -f $command_file || {
 		echo "nft command failed!"
 		kill $monitor_pid
@@ -54,33 +73,59 @@ run_test() {
 	touch $output_file
 }
 
-for testcase in testcases/*.t; do
-	echo "running tests from file $(basename $testcase)"
-	# files are like this:
-	#
-	# I add table ip t
-	# O add table ip t
-	# I add chain ip t c
-	# O add chain ip t c
+echo_run_test() {
+	echo_output=$(mktemp -p $testdir)
+	$debug && {
+		echo "command file:"
+		cat $command_file
+	}
+	$nft -nn -e -f $command_file >$echo_output || {
+		echo "nft command failed!"
+		exit 1
+	}
+	if ! mydiff -q $echo_output $output_file >/dev/null 2>&1; then
+		echo "echo output differs!"
+		mydiff -u $output_file $echo_output
+		exit 1
+	fi
+	rm $command_file
+	rm $output_file
+	touch $command_file
+	touch $output_file
+}
+
+for variant in monitor echo; do
+	run_test=${variant}_run_test
+	output_append=${variant}_output_append
+
+	for testcase in testcases/*.t; do
+		echo "$variant: running tests from file $(basename $testcase)"
+		# files are like this:
+		#
+		# I add table ip t
+		# O add table ip t
+		# I add chain ip t c
+		# O add chain ip t c
 
-	$nft flush ruleset
+		$nft flush ruleset
 
-	input_complete=false
-	while read dir line; do
-		case $dir in
-		I)
-			$input_complete && run_test
-			input_complete=false
-			cmd_append "$line"
-			;;
-		O)
-			input_complete=true
-			output_append "$line"
-			;;
-		'#'|'')
-			# ignore comments and empty lines
-			;;
-		esac
-	done <$testcase
-	$input_complete && run_test
+		input_complete=false
+		while read dir line; do
+			case $dir in
+			I)
+				$input_complete && $run_test
+				input_complete=false
+				cmd_append "$line"
+				;;
+			O)
+				input_complete=true
+				$output_append "$line"
+				;;
+			'#'|'')
+				# ignore comments and empty lines
+				;;
+			esac
+		done <$testcase
+		$input_complete && $run_test
+	done
 done
diff --git a/tests/monitor/testcases/simple.t b/tests/monitor/testcases/simple.t
new file mode 100644
index 0000000000000..e4dc073e14b65
--- /dev/null
+++ b/tests/monitor/testcases/simple.t
@@ -0,0 +1,20 @@
+# first the setup
+I add table ip t
+I add chain ip t c
+O -
+
+I add rule ip t c accept
+O -
+
+I add rule ip t c tcp dport { 22, 80, 443 } accept
+O -
+
+I insert rule ip t c counter accept
+O add rule ip t c counter packets 0 bytes 0 accept
+
+I replace rule ip t c handle 2 accept comment "foo bar"
+O delete rule ip t c handle 2
+O add rule ip t c accept comment "foo bar"
+
+I add counter ip t cnt
+O add counter ip t cnt { packets 0 bytes 0 }
-- 
2.13.1

Re: [nft PATCH 1/4] mnl: Drop --echo support for non-batch calls

[Pablo Neira Ayuso]

On Tue, Aug 15, 2017 at 01:43:02AM +0200, Phil Sutter wrote:
> Echo support in nft_mnl_talk() was broken: nft_mnl_talk_cb() passed
> cbdata->data as second parameter to netlink_echo_callback() which
> expected it to be of type struct netlink_ctx while in fact it was
> whatever callers of nft_mnl_talk() passed as callback data (in most
> cases a NULL pointer).

Applied, thanks.

> I didn't notice this because I didn't test for kernels without support
> for transactions. This has been added to nftables in kernel version 3.16
> back in 2014. Since then, user space which doesn't support it can't even
> add a table anymore. So adding this new feature to the old code path is
> really not feasible, therefore drop this broken attempt at supporting
> it.

We fixed this problem with nft and 3.16 IIRC. So at least the very
basic featureset still available there works fine.

Re: [nft PATCH 2/4] netlink: Fix segfault when using --echo flag

[Pablo Neira Ayuso]

On Tue, Aug 15, 2017 at 01:43:03AM +0200, Phil Sutter wrote:
> Commit 07b45939972eb ("src: introduce struct nft_cache") added cache
> pointer to struct netlink_mon_handler and the code assumes it is never
> NULL. Therefore initialize it in the dummy version of
> netlink_mon_handler in netlink_echo_callback().

Also applied, thanks.

Re: [nft PATCH 3/4] echo: Fix for added delays in rule updates

[Pablo Neira Ayuso]

On Tue, Aug 15, 2017 at 01:43:04AM +0200, Phil Sutter wrote:
> The added cache update upon every command dealing with rules was a
> bummer. Instead, perform the needed cache update only if echo option was
> set.
> 
> Initially, I tried to perform the cache update from within
> netlink_echo_callback(), but that turned into a mess since the shared
> socket between cache_init() and mnl_batch_talk() would receive
> unexpected new input. So instead update the cache from do_command_add(),
> netlink_replace_rule_batch() and do_comand_insert() so it completes
> before mnl_batch_talk() starts listening.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/netlink.h |  5 +----
>  src/evaluate.c    |  9 ---------
>  src/netlink.c     | 22 +++++++++++++++-------
>  src/rule.c        | 26 ++++++++++++++++++++++----
>  4 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/include/netlink.h b/include/netlink.h
> index 3726171424c33..e7e4bbcfc0f51 100644
> --- a/include/netlink.h
> +++ b/include/netlink.h
> @@ -119,10 +119,7 @@ extern int netlink_add_rule_batch(struct netlink_ctx *ctx,
>  extern int netlink_del_rule_batch(struct netlink_ctx *ctx,
>  				  const struct handle *h,
>  				  const struct location *loc);
> -extern int netlink_replace_rule_batch(struct netlink_ctx *ctx,
> -				      const struct handle *h,
> -				      const struct rule *rule,
> -				      const struct location *loc);
> +extern int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd);

This patch comes with an interesting cleanup, that is that you just
pass struct cmd as function parameter.

Probably we can do this everywhere in the netlink.c code? I wonder if
it's better just to fix this without changing the function footprint.
Then, work a cleanup patch to update all netlink_* functions to pass
struct cmd as parameter.

So we leave everything looking consistent.

>  extern int netlink_add_chain(struct netlink_ctx *ctx, const struct handle *h,
>  			     const struct location *loc,
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 64e14b8ba8d98..0fad0913488d0 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -2965,10 +2965,6 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
>  		handle_merge(&cmd->set->handle, &cmd->handle);
>  		return set_evaluate(ctx, cmd->set);
>  	case CMD_OBJ_RULE:
> -		ret = cache_update(ctx->nf_sock, ctx->cache, cmd->op,
> -				   ctx->msgs);
> -		if (ret < 0)
> -			return ret;
>  		handle_merge(&cmd->rule->handle, &cmd->handle);
>  		return rule_evaluate(ctx, cmd->rule);
>  	case CMD_OBJ_CHAIN:
> @@ -2983,11 +2979,6 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
>  	case CMD_OBJ_COUNTER:
>  	case CMD_OBJ_QUOTA:
>  	case CMD_OBJ_CT_HELPER:
> -		ret = cache_update(ctx->nf_sock, ctx->cache, cmd->op,
> -				   ctx->msgs);
> -		if (ret < 0)
> -			return ret;
> -
>  		return 0;
>  	default:
>  		BUG("invalid command object type %u\n", cmd->obj);
> diff --git a/src/netlink.c b/src/netlink.c
> index f631c26b2b9ca..e7fe62b21da6a 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -459,20 +459,28 @@ int netlink_add_rule_batch(struct netlink_ctx *ctx,
>  	return err;
>  }
>  
> -int netlink_replace_rule_batch(struct netlink_ctx *ctx, const struct handle *h,
> -			       const struct rule *rule,
> -			       const struct location *loc)
> +int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd)
>  {
>  	struct nftnl_rule *nlr;
> -	int err, flags = ctx->octx->echo ? NLM_F_ECHO : 0;
> +	int err, flags = 0;
>  
> -	nlr = alloc_nftnl_rule(&rule->handle);
> -	netlink_linearize_rule(ctx, nlr, rule);
> +	if (ctx->octx->echo) {
> +		err = cache_update(ctx->nf_sock, ctx->cache,
> +				   cmd->obj, ctx->msgs);
> +		if (err < 0)
> +			return err;
> +
> +		flags |= NLM_F_ECHO;
> +	}
> +
> +	nlr = alloc_nftnl_rule(&cmd->rule->handle);
> +	netlink_linearize_rule(ctx, nlr, cmd->rule);
>  	err = mnl_nft_rule_batch_replace(nlr, ctx->batch, flags, ctx->seqnum);
>  	nftnl_rule_free(nlr);
>  
>  	if (err < 0)
> -		netlink_io_error(ctx, loc, "Could not replace rule to batch: %s",
> +		netlink_io_error(ctx, &cmd->location,
> +				 "Could not replace rule to batch: %s",
>  				 strerror(errno));
>  	return err;
>  }
> diff --git a/src/rule.c b/src/rule.c
> index 1bd5c80158b7b..ab19525757fff 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1017,8 +1017,16 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
>  {
>  	uint32_t flags = excl ? NLM_F_EXCL : 0;
>  
> -	if (ctx->octx->echo)
> +	if (ctx->octx->echo) {
> +		int rc;

Another nitpick: We seem to use 'int ret' everywhere in the code. So
probably for consistency, use this name here too.

> +
> +		rc = cache_update(ctx->nf_sock, ctx->cache,
> +				  cmd->obj, ctx->msgs);
> +		if (rc < 0)
> +			return rc;
> +
>  		flags |= NLM_F_ECHO;
> +	}
>  
>  	switch (cmd->obj) {
>  	case CMD_OBJ_TABLE:
> @@ -1048,8 +1056,7 @@ static int do_command_replace(struct netlink_ctx *ctx, struct cmd *cmd)
>  {
>  	switch (cmd->obj) {
>  	case CMD_OBJ_RULE:
> -		return netlink_replace_rule_batch(ctx, &cmd->handle, cmd->rule,
> -						  &cmd->location);
> +		return netlink_replace_rule_batch(ctx, cmd);
>  	default:
>  		BUG("invalid command object type %u\n", cmd->obj);
>  	}
> @@ -1058,7 +1065,18 @@ static int do_command_replace(struct netlink_ctx *ctx, struct cmd *cmd)
>  
>  static int do_command_insert(struct netlink_ctx *ctx, struct cmd *cmd)
>  {
> -	uint32_t flags = ctx->octx->echo ? NLM_F_ECHO : 0;
> +	uint32_t flags = 0;
> +
> +	if (ctx->octx->echo) {
> +		int rc;
> +
> +		rc = cache_update(ctx->nf_sock, ctx->cache,
> +				  cmd->obj, ctx->msgs);
> +		if (rc < 0)
> +			return rc;
> +
> +		flags |= NLM_F_ECHO;
> +	}
>  
>  	switch (cmd->obj) {
>  	case CMD_OBJ_RULE:
> -- 
> 2.13.1
> 

Re: [nft PATCH 4/4] tests: Merge monitor and echo test suites

[Pablo Neira Ayuso]

On Tue, Aug 15, 2017 at 01:43:05AM +0200, Phil Sutter wrote:
> The two test suites were pretty similar already, and since echo output
> is supposed to be identical to monitor output apart from delete
> commands, they can be merged together with litte effort.

Applied, thanks Phil.

Re: [nft PATCH 1/4] mnl: Drop --echo support for non-batch calls

[Phil Sutter]

Hi,

On Tue, Aug 15, 2017 at 12:25:00PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 15, 2017 at 01:43:02AM +0200, Phil Sutter wrote:
[...]
> > I didn't notice this because I didn't test for kernels without support
> > for transactions. This has been added to nftables in kernel version 3.16
> > back in 2014. Since then, user space which doesn't support it can't even
> > add a table anymore. So adding this new feature to the old code path is
> > really not feasible, therefore drop this broken attempt at supporting
> > it.
> 
> We fixed this problem with nft and 3.16 IIRC. So at least the very
> basic featureset still available there works fine.

I was speaking of the other way around, namely old user space with
kernel >= 3.16 (that's what I simulated by forcing batch_supported to
false).

Given that kernel user API isn't completely compatible, do you see a
chance to drop the non-batch code from user space at some point?

Cheers, Phil

Re: [nft PATCH 3/4] echo: Fix for added delays in rule updates

[Phil Sutter]

Hi,

On Tue, Aug 15, 2017 at 12:35:30PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 15, 2017 at 01:43:04AM +0200, Phil Sutter wrote:
[...]
> > diff --git a/include/netlink.h b/include/netlink.h
> > index 3726171424c33..e7e4bbcfc0f51 100644
> > --- a/include/netlink.h
> > +++ b/include/netlink.h
> > @@ -119,10 +119,7 @@ extern int netlink_add_rule_batch(struct netlink_ctx *ctx,
> >  extern int netlink_del_rule_batch(struct netlink_ctx *ctx,
> >  				  const struct handle *h,
> >  				  const struct location *loc);
> > -extern int netlink_replace_rule_batch(struct netlink_ctx *ctx,
> > -				      const struct handle *h,
> > -				      const struct rule *rule,
> > -				      const struct location *loc);
> > +extern int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd);
> 
> This patch comes with an interesting cleanup, that is that you just
> pass struct cmd as function parameter.
> 
> Probably we can do this everywhere in the netlink.c code? I wonder if
> it's better just to fix this without changing the function footprint.
> Then, work a cleanup patch to update all netlink_* functions to pass
> struct cmd as parameter.
> 
> So we leave everything looking consistent.

This change was necessary in order to pass the required parameters to
cache_update(). Doing without, I would have to pass nf_sock, cache, obj
and msgs fields additionally, and the number of parameters was already
quite big.

I would instead suggest to follow-up with a patch applying the change to
all other functions as well, though I'm not sure whether Eric might
make a voodoo doll which looks like me if I submit that now.

[...]
> > diff --git a/src/rule.c b/src/rule.c
> > index 1bd5c80158b7b..ab19525757fff 100644
> > --- a/src/rule.c
> > +++ b/src/rule.c
> > @@ -1017,8 +1017,16 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
> >  {
> >  	uint32_t flags = excl ? NLM_F_EXCL : 0;
> >  
> > -	if (ctx->octx->echo)
> > +	if (ctx->octx->echo) {
> > +		int rc;
> 
> Another nitpick: We seem to use 'int ret' everywhere in the code. So
> probably for consistency, use this name here too.

You mean if 'int err' is not used? But OK, in src/rule.c we have at
least three cases of 'return ret' and only two of 'return rc' (from my
patch) so I'll change that.

Cheers, Phil

Re: [nft PATCH 3/4] echo: Fix for added delays in rule updates

[Phil Sutter]

On Tue, Aug 15, 2017 at 01:27:56PM +0200, Phil Sutter wrote:
> On Tue, Aug 15, 2017 at 12:35:30PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 15, 2017 at 01:43:04AM +0200, Phil Sutter wrote:
> [...]
> > > diff --git a/include/netlink.h b/include/netlink.h
> > > index 3726171424c33..e7e4bbcfc0f51 100644
> > > --- a/include/netlink.h
> > > +++ b/include/netlink.h
> > > @@ -119,10 +119,7 @@ extern int netlink_add_rule_batch(struct netlink_ctx *ctx,
> > >  extern int netlink_del_rule_batch(struct netlink_ctx *ctx,
> > >  				  const struct handle *h,
> > >  				  const struct location *loc);
> > > -extern int netlink_replace_rule_batch(struct netlink_ctx *ctx,
> > > -				      const struct handle *h,
> > > -				      const struct rule *rule,
> > > -				      const struct location *loc);
> > > +extern int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd);
> > 
> > This patch comes with an interesting cleanup, that is that you just
> > pass struct cmd as function parameter.
> > 
> > Probably we can do this everywhere in the netlink.c code? I wonder if
> > it's better just to fix this without changing the function footprint.
> > Then, work a cleanup patch to update all netlink_* functions to pass
> > struct cmd as parameter.
> > 
> > So we leave everything looking consistent.
> 
> This change was necessary in order to pass the required parameters to
> cache_update(). Doing without, I would have to pass nf_sock, cache, obj
> and msgs fields additionally, and the number of parameters was already
> quite big.

ENOCOFFEE: Actually I only use obj field of struct cmd, and that should
be optional since I can also just use CMD_INVALID instead - so I'll drop
the signature changes in v2.

Thanks, Phil

Re: [nft PATCH 1/4] mnl: Drop --echo support for non-batch calls

[Pablo Neira Ayuso]

On Tue, Aug 15, 2017 at 01:05:04PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Tue, Aug 15, 2017 at 12:25:00PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 15, 2017 at 01:43:02AM +0200, Phil Sutter wrote:
> [...]
> > > I didn't notice this because I didn't test for kernels without support
> > > for transactions. This has been added to nftables in kernel version 3.16
> > > back in 2014. Since then, user space which doesn't support it can't even
> > > add a table anymore. So adding this new feature to the old code path is
> > > really not feasible, therefore drop this broken attempt at supporting
> > > it.
> > 
> > We fixed this problem with nft and 3.16 IIRC. So at least the very
> > basic featureset still available there works fine.
> 
> I was speaking of the other way around, namely old user space with
> kernel >= 3.16 (that's what I simulated by forcing batch_supported to
> false).
> 
> Given that kernel user API isn't completely compatible, do you see a
> chance to drop the non-batch code from user space at some point?

Yes, as soon as 3.16 becomes unsupported we can let that code sink I
would suggest.

Re: [nft PATCH 3/4] echo: Fix for added delays in rule updates

[Pablo Neira Ayuso]

On Tue, Aug 15, 2017 at 01:34:25PM +0200, Phil Sutter wrote:
> On Tue, Aug 15, 2017 at 01:27:56PM +0200, Phil Sutter wrote:
> > On Tue, Aug 15, 2017 at 12:35:30PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Aug 15, 2017 at 01:43:04AM +0200, Phil Sutter wrote:
> > [...]
> > > > diff --git a/include/netlink.h b/include/netlink.h
> > > > index 3726171424c33..e7e4bbcfc0f51 100644
> > > > --- a/include/netlink.h
> > > > +++ b/include/netlink.h
> > > > @@ -119,10 +119,7 @@ extern int netlink_add_rule_batch(struct netlink_ctx *ctx,
> > > >  extern int netlink_del_rule_batch(struct netlink_ctx *ctx,
> > > >  				  const struct handle *h,
> > > >  				  const struct location *loc);
> > > > -extern int netlink_replace_rule_batch(struct netlink_ctx *ctx,
> > > > -				      const struct handle *h,
> > > > -				      const struct rule *rule,
> > > > -				      const struct location *loc);
> > > > +extern int netlink_replace_rule_batch(struct netlink_ctx *ctx, struct cmd *cmd);
> > > 
> > > This patch comes with an interesting cleanup, that is that you just
> > > pass struct cmd as function parameter.
> > > 
> > > Probably we can do this everywhere in the netlink.c code? I wonder if
> > > it's better just to fix this without changing the function footprint.
> > > Then, work a cleanup patch to update all netlink_* functions to pass
> > > struct cmd as parameter.
> > > 
> > > So we leave everything looking consistent.
> > 
> > This change was necessary in order to pass the required parameters to
> > cache_update(). Doing without, I would have to pass nf_sock, cache, obj
> > and msgs fields additionally, and the number of parameters was already
> > quite big.
> 
> ENOCOFFEE: Actually I only use obj field of struct cmd, and that should
> be optional since I can also just use CMD_INVALID instead - so I'll drop
> the signature changes in v2.

That's good, Eric won't do any voodoo toll with you then.