[nft PATCH 1/3] src/rule.c: don't print trailing statement whitespace

[nft PATCH 1/3] src/rule.c: don't print trailing statement whitespace

[Arturo Borrero Gonzalez]

This trailing whitespace is annoying when working with the textual output
of nft.

Before:

table t {
	chain c {
		ct state new
			    ^
	}
}


After:

table t {
	chain c {
		ct state new
	}
}

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/rule.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 85987b9..0ed7794 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -385,14 +385,15 @@ void rule_print(const struct rule *rule)
 
 	list_for_each_entry(stmt, &rule->stmts, list) {
 		stmt->ops->print(stmt);
-		printf(" ");
+		if (!list_is_last(&stmt->list, &rule->stmts))
+			printf(" ");
 	}
 
 	if (rule->comment)
-		printf("comment \"%s\" ", rule->comment);
+		printf(" comment \"%s\"", rule->comment);
 
 	if (handle_output > 0)
-		printf("# handle %" PRIu64, rule->handle.handle);
+		printf(" # handle %" PRIu64, rule->handle.handle);
 }
 
 struct scope *scope_init(struct scope *scope, const struct scope *parent)

[nft PATCH 2/3] src/evaluate.c: improve rule management checks

[Arturo Borrero Gonzalez]

Improve checks (and error reporting) for basic rule management operations.

This includes a fix for netfilter bug #965.

Netfilter bug: http://bugzilla.netfilter.org/show_bug.cgi?id=965
Reported-by: Jesper Sander Lindgren <sander.contrib@gmail.com>
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/evaluate.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 473f014..de8302b 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2160,11 +2160,59 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	return 0;
 }
 
+static int rule_evaluate_cmd(struct eval_ctx *ctx)
+{
+	struct handle *handle = &ctx->cmd->handle;
+
+	/* allowed:
+	 * - insert [position] (no handle)
+	 * - add [position] (no handle)
+	 * - replace <handle> (no position)
+	 * - delete <handle> (no position)
+	 */
+
+	switch (ctx->cmd->op) {
+	case CMD_INSERT:
+		if (handle->handle != 0)
+			return cmd_error(ctx, "Could not insert rule: handle "
+					 "not allowed.");
+		break;
+	case CMD_ADD:
+		if (handle->handle != 0)
+			return cmd_error(ctx, "Could not add rule: handle not "
+					 "allowed.");
+		break;
+	case CMD_REPLACE:
+		if (handle->position != 0)
+			return cmd_error(ctx, "Could not replace rule: "
+					 "position not allowed.");
+		if (handle->handle == 0)
+			return cmd_error(ctx, "Could not replace rule: missing"
+					 " handle.");
+		break;
+	case CMD_DELETE:
+		if (handle->position != 0)
+			return cmd_error(ctx, "Could not delete rule: position"
+					 " not allowed.");
+		if (handle->handle == 0)
+			return cmd_error(ctx, "Could not delete rule: missing "
+					 "handle.");
+		break;
+	default:
+		BUG("unkown command type %u\n", ctx->cmd->op);
+	}
+
+	return 0;
+}
+
 static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
 {
 	struct stmt *stmt, *tstmt = NULL;
 	struct error_record *erec;
 
+	if (rule_evaluate_cmd(ctx) < 0)
+		return -1;
+
 	proto_ctx_init(&ctx->pctx, rule->handle.family);
 	memset(&ctx->ectx, 0, sizeof(ctx->ectx));
 
@@ -2345,8 +2393,11 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 			return ret;
 
 		return setelem_evaluate(ctx, &cmd->expr);
-	case CMD_OBJ_SET:
 	case CMD_OBJ_RULE:
+		if (rule_evaluate_cmd(ctx) < 0)
+			return -1;
+		/* fall through */
+	case CMD_OBJ_SET:
 	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_TABLE:
 		return 0;

[nft PATCH 3/3] tests/shell: add testcases for Netfilter bug #965

[Arturo Borrero Gonzalez]

Testscases for Netfilter bug #965:
 * add rule at position
 * insert rule at position
 * replace rule with given handle
 * delete rule with given handle
 * don't allow to delete rules with position keyword

Netfilter Bugzilla: http://bugzilla.netfilter.org/show_bug.cgi?id=965
Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 .../testcases/rule_management/0001addposition_0    |   27 ++++++++++++++++++++
 .../testcases/rule_management/0002insertposition_0 |   27 ++++++++++++++++++++
 tests/shell/testcases/rule_management/0003insert_0 |   27 ++++++++++++++++++++
 .../shell/testcases/rule_management/0004replace_0  |   24 ++++++++++++++++++
 .../shell/testcases/rule_management/0005replace_1  |   11 ++++++++
 .../shell/testcases/rule_management/0006replace_1  |   11 ++++++++
 tests/shell/testcases/rule_management/0007delete_0 |   25 +++++++++++++++++++
 tests/shell/testcases/rule_management/0008delete_1 |   11 ++++++++
 tests/shell/testcases/rule_management/0009delete_1 |   11 ++++++++
 9 files changed, 174 insertions(+)
 create mode 100755 tests/shell/testcases/rule_management/0001addposition_0
 create mode 100755 tests/shell/testcases/rule_management/0002insertposition_0
 create mode 100755 tests/shell/testcases/rule_management/0003insert_0
 create mode 100755 tests/shell/testcases/rule_management/0004replace_0
 create mode 100755 tests/shell/testcases/rule_management/0005replace_1
 create mode 100755 tests/shell/testcases/rule_management/0006replace_1
 create mode 100755 tests/shell/testcases/rule_management/0007delete_0
 create mode 100755 tests/shell/testcases/rule_management/0008delete_1
 create mode 100755 tests/shell/testcases/rule_management/0009delete_1

diff --git a/tests/shell/testcases/rule_management/0001addposition_0 b/tests/shell/testcases/rule_management/0001addposition_0
new file mode 100755
index 0000000..e66bfff
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0001addposition_0
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT add rule t c accept	# should have handle 2
+$NFT add rule t c accept	# should have handle 3
+$NFT add rule t c position 2 drop
+
+EXPECTED="table ip t {
+	chain c {
+		accept
+		drop
+		accept
+	}
+}"
+
+GET="$($NFT list ruleset)"
+
+if [ "$EXPECTED" != "$GET" ] ; then
+	DIFF="$(which diff)"
+	[ -x $DIFF ] && $DIFF -u <(echo "$EXPECTED") <(echo "$GET")
+	exit 1
+fi
diff --git a/tests/shell/testcases/rule_management/0002insertposition_0 b/tests/shell/testcases/rule_management/0002insertposition_0
new file mode 100755
index 0000000..cf8a568
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0002insertposition_0
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT add rule t c accept	# should have handle 2
+$NFT add rule t c accept	# should have handle 3
+$NFT insert rule t c position 2 drop
+
+EXPECTED="table ip t {
+	chain c {
+		drop
+		accept
+		accept
+	}
+}"
+
+GET="$($NFT list ruleset)"
+
+if [ "$EXPECTED" != "$GET" ] ; then
+	DIFF="$(which diff)"
+	[ -x $DIFF ] && $DIFF -u <(echo "$EXPECTED") <(echo "$GET")
+	exit 1
+fi
diff --git a/tests/shell/testcases/rule_management/0003insert_0 b/tests/shell/testcases/rule_management/0003insert_0
new file mode 100755
index 0000000..6691c16
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0003insert_0
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT insert rule t c accept
+$NFT insert rule t c drop
+$NFT insert rule t c masquerade
+
+EXPECTED="table ip t {
+	chain c {
+		masquerade
+		drop
+		accept
+	}
+}"
+
+GET="$($NFT list ruleset)"
+
+if [ "$EXPECTED" != "$GET" ] ; then
+	DIFF="$(which diff)"
+	[ -x $DIFF ] && $DIFF -u <(echo "$EXPECTED") <(echo "$GET")
+	exit 1
+fi
diff --git a/tests/shell/testcases/rule_management/0004replace_0 b/tests/shell/testcases/rule_management/0004replace_0
new file mode 100755
index 0000000..6a4b949
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0004replace_0
@@ -0,0 +1,24 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT add rule t c accept	# should have handle 2
+$NFT replace rule t c handle 2 drop
+
+EXPECTED="table ip t {
+	chain c {
+		drop
+	}
+}"
+
+GET="$($NFT list ruleset)"
+
+if [ "$EXPECTED" != "$GET" ] ; then
+	DIFF="$(which diff)"
+	[ -x $DIFF ] && $DIFF -u <(echo "$EXPECTED") <(echo "$GET")
+	exit 1
+fi
diff --git a/tests/shell/testcases/rule_management/0005replace_1 b/tests/shell/testcases/rule_management/0005replace_1
new file mode 100755
index 0000000..e82995a
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0005replace_1
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+# kernel should return ENOENT
+$NFT replace rule t c handle 2 drop 2>/dev/null
+echo "E: missing kernel ENOENT" >&2
diff --git a/tests/shell/testcases/rule_management/0006replace_1 b/tests/shell/testcases/rule_management/0006replace_1
new file mode 100755
index 0000000..5dfcba0
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0006replace_1
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+# position keyword with replace action is not allowed, this should fail
+$NFT replace rule t c position 2 drop 2>/dev/null
+echo "E: allowed replace with position specification" >&2
diff --git a/tests/shell/testcases/rule_management/0007delete_0 b/tests/shell/testcases/rule_management/0007delete_0
new file mode 100755
index 0000000..126fe5d
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0007delete_0
@@ -0,0 +1,25 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT add rule t c accept	# should have handle 2
+$NFT add rule t c drop		# should have handle 3
+$NFT delete rule t c handle 2
+
+EXPECTED="table ip t {
+	chain c {
+		drop
+	}
+}"
+
+GET="$($NFT list ruleset)"
+
+if [ "$EXPECTED" != "$GET" ] ; then
+	DIFF="$(which diff)"
+	[ -x $DIFF ] && $DIFF -u <(echo "$EXPECTED") <(echo "$GET")
+	exit 1
+fi
diff --git a/tests/shell/testcases/rule_management/0008delete_1 b/tests/shell/testcases/rule_management/0008delete_1
new file mode 100755
index 0000000..3dce219
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0008delete_1
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+# this should fail, we don't allow delete with position
+$NFT delete rule t c position 2 drop 2>/dev/null
+echo "E: allowed position spec with delete action" >&2
diff --git a/tests/shell/testcases/rule_management/0009delete_1 b/tests/shell/testcases/rule_management/0009delete_1
new file mode 100755
index 0000000..87fec60
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0009delete_1
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+# tests for Netfilter bug #965 and the related fix
+# (regarding rule management with a given position/handle spec)
+
+set -e
+$NFT add table t
+$NFT add chain t c
+# kernel ENOENT
+$NFT delete rule t c handle 3333 2>/dev/null
+echo "E: missing kernel ENOENT" >&2

Re: [nft PATCH 2/3] src/evaluate.c: improve rule management checks

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Wed, Mar 23, 2016 at 01:51:38PM +0100, Arturo Borrero Gonzalez wrote:
> Improve checks (and error reporting) for basic rule management operations.
> 
> This includes a fix for netfilter bug #965.

Thanks for working on this.

With a bit more work we can achieve better error reporting:

# nft insert rule x y handle 10 position 10 ip saddr 1.1.1.1
<cmdline>:1:17-25: Error: Wrong combination, use `position' instead
insert rule x y handle 10 position 10 ip saddr 1.1.1.1
                ^^^^^^^^^ ~~~~~~~~~~~

# nft insert rule x y handle 10 ip saddr 1.1.1.1
<cmdline>:1:17-25: Error: Cannot use this, use `position' instead
insert rule x y handle 10 ip saddr 1.1.1.1
                ^^^^^^^^^

You will need to rework this patch applying this patch in first place:

http://patchwork.ozlabs.org/patch/601270/

I'm also attaching a quick patch for the two examples, but remaining
spots are left unchanges. Please feel free to follow up on this.

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2935 bytes --]

diff --git a/src/evaluate.c b/src/evaluate.c
index 473f014..db89a0f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -65,6 +65,12 @@ static int __fmtstring(4, 5) __stmt_binary_error(struct eval_ctx *ctx,
 	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
 #define cmd_error(ctx, fmt, args...) \
 	__stmt_binary_error(ctx, &(ctx->cmd)->location, NULL, fmt, ## args)
+#define handle_error(ctx, fmt, args...) \
+	__stmt_binary_error(ctx, &ctx->cmd->handle.handle.location, NULL, fmt, ## args)
+#define position_error(ctx, fmt, args...) \
+	__stmt_binary_error(ctx, &ctx->cmd->handle.position.location, NULL, fmt, ## args)
+#define handle_position_error(ctx, fmt, args...) \
+	__stmt_binary_error(ctx, &ctx->cmd->handle.handle.location, &ctx->cmd->handle.position.location, fmt, ## args)
 
 static int __fmtstring(3, 4) set_error(struct eval_ctx *ctx,
 				       const struct set *set,
@@ -2160,11 +2166,61 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	return 0;
 }
 
+static int rule_evaluate_cmd(struct eval_ctx *ctx)
+{
+	struct handle *handle = &ctx->cmd->handle;
+
+	/* allowed:
+	 * - insert [position] (no handle)
+	 * - add [position] (no handle)
+	 * - replace <handle> (no position)
+	 * - delete <handle> (no position)
+	 */
+
+	switch (ctx->cmd->op) {
+	case CMD_INSERT:
+		if (handle->handle.id && handle->position.id)
+			return handle_position_error(ctx, "Wrong combination, use `position' instead");
+
+		if (handle->handle.id != 0)
+			return handle_error(ctx, "Cannot use this, use `position' instead");
+		break;
+	case CMD_ADD:
+		if (handle->handle.id != 0)
+			return cmd_error(ctx, "Could not add rule: handle not "
+					 "allowed.");
+		break;
+	case CMD_REPLACE:
+		if (handle->position.id != 0)
+			return cmd_error(ctx, "Could not replace rule: "
+					 "position not allowed.");
+		if (handle->handle.id == 0)
+			return cmd_error(ctx, "Could not replace rule: missing"
+					 " handle.");
+		break;
+	case CMD_DELETE:
+		if (handle->position.id != 0)
+			return cmd_error(ctx, "Could not delete rule: position"
+					 " not allowed.");
+		if (handle->handle.id == 0)
+			return cmd_error(ctx, "Could not delete rule: missing "
+					 "handle.");
+		break;
+	default:
+		BUG("unkown command type %u\n", ctx->cmd->op);
+	}
+
+	return 0;
+}
+
 static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
 {
 	struct stmt *stmt, *tstmt = NULL;
 	struct error_record *erec;
 
+	if (rule_evaluate_cmd(ctx) < 0)
+		return -1;
+
 	proto_ctx_init(&ctx->pctx, rule->handle.family);
 	memset(&ctx->ectx, 0, sizeof(ctx->ectx));
 
@@ -2345,8 +2401,11 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 			return ret;
 
 		return setelem_evaluate(ctx, &cmd->expr);
-	case CMD_OBJ_SET:
 	case CMD_OBJ_RULE:
+		if (rule_evaluate_cmd(ctx) < 0)
+			return -1;
+		/* fall through */
+	case CMD_OBJ_SET:
 	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_TABLE:
 		return 0;

Re: [nft PATCH 2/3] src/evaluate.c: improve rule management checks

[Arturo Borrero Gonzalez]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On 23 March 2016 at 17:08, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Mar 23, 2016 at 01:51:38PM +0100, Arturo Borrero Gonzalez wrote:
>> Improve checks (and error reporting) for basic rule management operations.
>>
>> This includes a fix for netfilter bug #965.
>
> Thanks for working on this.
>
> With a bit more work we can achieve better error reporting:
>
> # nft insert rule x y handle 10 position 10 ip saddr 1.1.1.1
> <cmdline>:1:17-25: Error: Wrong combination, use `position' instead
> insert rule x y handle 10 position 10 ip saddr 1.1.1.1
>                 ^^^^^^^^^ ~~~~~~~~~~~
>
> # nft insert rule x y handle 10 ip saddr 1.1.1.1
> <cmdline>:1:17-25: Error: Cannot use this, use `position' instead
> insert rule x y handle 10 ip saddr 1.1.1.1
>                 ^^^^^^^^^
>
> You will need to rework this patch applying this patch in first place:
>
> http://patchwork.ozlabs.org/patch/601270/
>
> I'm also attaching a quick patch for the two examples, but remaining
> spots are left unchanges. Please feel free to follow up on this.
>
> Thanks.

Hi,

I completed the patch, amended a bit the error messages.

Find it attached.

The applies in in this order:
 * http://patchwork.ozlabs.org/patch/601270/ (src: store parser
location for handle and position IDs)
 * http://patchwork.ozlabs.org/patch/601216/ (src/rule.c: don't print
trailing statement whitespace)
 * the attached patch
 * http://patchwork.ozlabs.org/patch/601218/ (tests/shell: add
testcases for Netfilter bug #965)


best regards.

-- 
Arturo Borrero González

[-- Attachment #2: nft-2-3-src-evaluate.c-improve-rule-management-checks.patch --]
[-- Type: text/x-patch, Size: 3973 bytes --]



From: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>


---
 src/evaluate.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 473f014..cf157f7 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -65,6 +65,12 @@ static int __fmtstring(4, 5) __stmt_binary_error(struct eval_ctx *ctx,
 	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
 #define cmd_error(ctx, fmt, args...) \
 	__stmt_binary_error(ctx, &(ctx->cmd)->location, NULL, fmt, ## args)
+#define handle_error(ctx, fmt, args...) \
+	__stmt_binary_error(ctx, &ctx->cmd->handle.handle.location, NULL, fmt, ## args)
+#define position_error(ctx, fmt, args...) \
+	__stmt_binary_error(ctx, &ctx->cmd->handle.position.location, NULL, fmt, ## args)
+#define handle_position_error(ctx, fmt, args...) \
+	__stmt_binary_error(ctx, &ctx->cmd->handle.handle.location, &ctx->cmd->handle.position.location, fmt, ## args)
 
 static int __fmtstring(3, 4) set_error(struct eval_ctx *ctx,
 				       const struct set *set,
@@ -2160,11 +2166,86 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	return 0;
 }
 
+static int rule_evaluate_cmd(struct eval_ctx *ctx)
+{
+	struct handle *handle = &ctx->cmd->handle;
+
+	/* allowed:
+	 * - insert [position] (no handle)
+	 * - add [position] (no handle)
+	 * - replace <handle> (no position)
+	 * - delete <handle> (no position)
+	 */
+
+	switch (ctx->cmd->op) {
+	case CMD_INSERT:
+		if (handle->handle.id && handle->position.id)
+			return handle_position_error(ctx, "Could not insert "
+						     "rule: wrong combination"
+						     ", use only `position' "
+						     "instead");
+
+		if (handle->handle.id)
+			return handle_error(ctx, "Could not insert rule: "
+					    "cannot use this, use "
+					    "`position' instead");
+		break;
+	case CMD_ADD:
+		if (handle->handle.id && handle->position.id)
+			return handle_position_error(ctx, "Could not add "
+						     "rule: wrong combination"
+						     ", use only `position' "
+						     "instead");
+
+		if (handle->handle.id)
+			return handle_error(ctx, "Could not add rule: "
+					    "cannot use this, use "
+					    "`position' instead");
+
+		break;
+	case CMD_REPLACE:
+		if (handle->handle.id && handle->position.id)
+			return handle_position_error(ctx, "Could not replace "
+						     "rule: wrong combination"
+						     ", use only `handle' "
+						     "instead");
+		if (handle->position.id)
+			return position_error(ctx, "Could not replace rule: "
+					      "cannot use this, use `handle' "
+					      "instead");
+		if (!handle->handle.id)
+			return cmd_error(ctx, "Could not replace rule: missing"
+					 " `handle'.");
+		break;
+	case CMD_DELETE:
+		if (handle->handle.id && handle->position.id)
+			return handle_position_error(ctx, "Could not replace "
+						     "rule: wrong combination"
+						     ", use only `handle' "
+						     "instead");
+		if (handle->position.id)
+			return position_error(ctx, "Could not replace rule: "
+					      "cannot use this, use `handle' "
+					      "instead");
+		if (!handle->handle.id)
+			return cmd_error(ctx, "Could not replace rule: missing"
+					 " `handle'.");
+		break;
+	default:
+		BUG("unkown command type %u\n", ctx->cmd->op);
+	}
+
+	return 0;
+}
+
 static int rule_evaluate(struct eval_ctx *ctx, struct rule *rule)
 {
 	struct stmt *stmt, *tstmt = NULL;
 	struct error_record *erec;
 
+	if (rule_evaluate_cmd(ctx) < 0)
+		return -1;
+
 	proto_ctx_init(&ctx->pctx, rule->handle.family);
 	memset(&ctx->ectx, 0, sizeof(ctx->ectx));
 
@@ -2345,8 +2426,11 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 			return ret;
 
 		return setelem_evaluate(ctx, &cmd->expr);
-	case CMD_OBJ_SET:
 	case CMD_OBJ_RULE:
+		if (rule_evaluate_cmd(ctx) < 0)
+			return -1;
+		/* fall through */
+	case CMD_OBJ_SET:
 	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_TABLE:
 		return 0;

Re: [nft PATCH 1/3] src/rule.c: don't print trailing statement whitespace

[Pablo Neira Ayuso]

On Wed, Mar 23, 2016 at 01:51:33PM +0100, Arturo Borrero Gonzalez wrote:
> This trailing whitespace is annoying when working with the textual output
> of nft.
> 
> Before:
> 
> table t {
> 	chain c {
> 		ct state new
> 			    ^
> 	}
> }
> 
> 
> After:
> 
> table t {
> 	chain c {
> 		ct state new
> 	}
> }

Applied, thanks.

Re: [nft PATCH 2/3] src/evaluate.c: improve rule management checks

[Pablo Neira Ayuso]

On Mon, Mar 28, 2016 at 01:32:41PM +0200, Arturo Borrero Gonzalez wrote:
> +static int rule_evaluate_cmd(struct eval_ctx *ctx)
> +{
> +	struct handle *handle = &ctx->cmd->handle;
> +
> +	/* allowed:
> +	 * - insert [position] (no handle)
> +	 * - add [position] (no handle)
> +	 * - replace <handle> (no position)
> +	 * - delete <handle> (no position)
> +	 */
> +
> +	switch (ctx->cmd->op) {
> +	case CMD_INSERT:
> +		if (handle->handle.id && handle->position.id)
> +			return handle_position_error(ctx, "Could not insert "
> +						     "rule: wrong combination"
> +						     ", use only `position' "
> +						     "instead");

I know we have this "Could not insert rule:" thing in other four spots
in the evaluation.c, but those are my fault and I'll get rid of them
soon.

Given that we now have good error reporting through location, I
suggest you use the shortened version.

	return handle_position_error(ctx, "you cannot combine this");

The error reporting we have visualizes the problem to the user.

Thanks.

Re: [nft PATCH 3/3] tests/shell: add testcases for Netfilter bug #965

[Pablo Neira Ayuso]

On Wed, Mar 23, 2016 at 01:51:43PM +0100, Arturo Borrero Gonzalez wrote:
> Testscases for Netfilter bug #965:
>  * add rule at position
>  * insert rule at position
>  * replace rule with given handle
>  * delete rule with given handle
>  * don't allow to delete rules with position keyword

Please, resubmit this now that we got the fix into master.

Thanks Arturo.