All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4 V6 nft] Simplify parser rule_spec tree
@ 2016-08-21 21:22 Carlos Falgueras García
  2016-08-21 21:22 ` [PATCH 2/4 V6 nft] Implement deleting rule by description Carlos Falgueras García
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Carlos Falgueras García @ 2016-08-21 21:22 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo

This patch separates the rule identification from the rule localization, so
the logic moves from the evaluator to the parser. This allows to revert the
patch "evaluate: improve rule managment checks"
(4176c7d30c2ff1b3f52468fc9c08b8df83f979a8) and saves a lot of code.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 src/evaluate.c     | 68 +-----------------------------------------------------
 src/parser_bison.y | 45 +++++++++++++++++-------------------
 2 files changed, 22 insertions(+), 91 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 87f5a6d..2f94ac6 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -44,12 +44,6 @@ static const char *byteorder_names[] = {
 	__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,
@@ -2481,68 +2475,11 @@ 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, "use only `position'"
-						     " instead");
-
-		if (handle->handle.id)
-			return handle_error(ctx, "use `position' instead");
-		break;
-	case CMD_ADD:
-		if (handle->handle.id && handle->position.id)
-			return handle_position_error(ctx, "use only `position'"
-						     " instead");
-
-		if (handle->handle.id)
-			return handle_error(ctx, "use `position' instead");
-
-		break;
-	case CMD_REPLACE:
-		if (handle->handle.id && handle->position.id)
-			return handle_position_error(ctx, "use only `handle' "
-						     "instead");
-		if (handle->position.id)
-			return position_error(ctx, "use `handle' instead");
-		if (!handle->handle.id)
-			return cmd_error(ctx, "missing `handle'");
-		break;
-	case CMD_DELETE:
-		if (handle->handle.id && handle->position.id)
-			return handle_position_error(ctx, "use only `handle' "
-						     "instead");
-		if (handle->position.id)
-			return position_error(ctx, "use `handle' instead");
-		if (!handle->handle.id)
-			return cmd_error(ctx, "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));
 
@@ -2723,11 +2660,8 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 			return ret;
 
 		return setelem_evaluate(ctx, &cmd->expr);
-	case CMD_OBJ_RULE:
-		if (rule_evaluate_cmd(ctx) < 0)
-			return -1;
-		/* fall through */
 	case CMD_OBJ_SET:
+	case CMD_OBJ_RULE:
 	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_TABLE:
 		return 0;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index e16b8a3..2ca7eea 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -425,15 +425,12 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %type <cmd>			base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd list_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd
 %destructor { cmd_free($$); }	base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd list_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd
 
-%type <handle>			table_spec chain_spec chain_identifier ruleid_spec ruleset_spec
-%destructor { handle_free(&$$); } table_spec chain_spec chain_identifier ruleid_spec ruleset_spec
+%type <handle>			table_spec chain_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec
+%destructor { handle_free(&$$); } table_spec chain_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec
 %type <handle>			set_spec set_identifier
 %destructor { handle_free(&$$); } set_spec set_identifier
 %type <val>			family_spec family_spec_explicit chain_policy prio_spec
 
-%type <handle_spec>		handle_spec
-%type <position_spec>		position_spec
-
 %type <string>			dev_spec
 %destructor { xfree($$); }	dev_spec
 
@@ -720,11 +717,11 @@ add_cmd			:	TABLE		table_spec
 				close_scope(state);
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_CHAIN, &$2, &@$, $5);
 			}
-			|	RULE		ruleid_spec	rule
+			|	RULE		rule_position	rule
 			{
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$2, &@$, $3);
 			}
-			|	/* empty */	ruleid_spec	rule
+			|	/* empty */	rule_position	rule
 			{
 				$$ = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &$1, &@$, $2);
 			}
@@ -779,7 +776,7 @@ create_cmd		:	TABLE		table_spec
 			}
 			;
 
-insert_cmd		:	RULE		ruleid_spec	rule
+insert_cmd		:	RULE		rule_position	rule
 			{
 				$$ = cmd_alloc(CMD_INSERT, CMD_OBJ_RULE, &$2, &@$, $3);
 			}
@@ -1252,35 +1249,35 @@ set_identifier		:	identifier
 			}
 			;
 
-handle_spec		:	/* empty */
+handle_spec		:	HANDLE		NUM
 			{
-				memset(&$$, 0, sizeof($$));
+				$$.handle.location	= @$;
+				$$.handle.id		= $2;
 			}
-			|	HANDLE		NUM
+			;
+
+position_spec		:	POSITION	NUM
 			{
-				memset(&$$, 0, sizeof($$));
-				$$.location	= @$;
-				$$.id		= $2;
+				$$.position.location	= @$;
+				$$.position.id		= $2;
 			}
 			;
 
-position_spec		:	/* empty */
+rule_position		:	chain_spec
 			{
-				memset(&$$, 0, sizeof($$));
+				$$ = $1;
 			}
-			|	POSITION	NUM
+			|	chain_spec position_spec
 			{
-				memset(&$$, 0, sizeof($$));
-				$$.location	= @$;
-				$$.id		= $2;
+				$$ = $1;
+				handle_merge(&$$, &$2);
 			}
 			;
 
-ruleid_spec		:	chain_spec	handle_spec	position_spec
+ruleid_spec		:	chain_spec handle_spec
 			{
-				$$		= $1;
-				$$.handle	= $2;
-				$$.position	= $3;
+				$$ = $1;
+				handle_merge(&$$, &$2);
 			}
 			;
 
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4 V6 nft] Implement deleting rule by description
  2016-08-21 21:22 [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Carlos Falgueras García
@ 2016-08-21 21:22 ` Carlos Falgueras García
  2016-08-22 16:13   ` Pablo Neira Ayuso
  2016-08-21 21:22 ` [PATCH 3/4 V6 nft] test: shell: Add tests for " Carlos Falgueras García
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Carlos Falgueras García @ 2016-08-21 21:22 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo

This patch introduces deletion in a similar fashion as in iptables, thus,
we can delete the first rule that matches our description, for example:

	$ nft list -a ruleset
	table ip t {
		chain c {
			ip saddr 1.1.1.1 counter packets 0 bytes 0 # handle 1
			ip saddr 1.1.1.2 counter packets 0 bytes 0 # handle 2
			ip saddr 1.1.1.2 counter packets 0 bytes 0 # handle 3
			ip saddr 1.1.1.4 counter packets 0 bytes 0 # handle 4
		}
	}
	$ nft delete rule table chain ip saddr 1.1.1.2 counter
	$ nft list -a ruleset
	table ip t {
		chain c {
			ip saddr 1.1.1.1 counter packets 0 bytes 0 # handle 1
			ip saddr 1.1.1.2 counter packets 0 bytes 0 # handle 3
			ip saddr 1.1.1.4 counter packets 0 bytes 0 # handle 4
		}
	}

The parser rule 'ruleid_spec' is now of the type 'struct rule' in order to
hold a rule description. When rule is identified with its handle a dummy
'struct rule' is allocated to hold the specified handle.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 src/evaluate.c     |  6 ++++++
 src/parser_bison.y | 24 ++++++++++++++++--------
 src/rule.c         | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 2f94ac6..f7b349b 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2661,7 +2661,13 @@ static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 
 		return setelem_evaluate(ctx, &cmd->expr);
 	case CMD_OBJ_SET:
+		return 0;
 	case CMD_OBJ_RULE:
+		/* CMD_LIST force caching all ruleset */
+		ret = cache_update(CMD_LIST, ctx->msgs);
+		if (ret < 0)
+			return ret;
+		return rule_evaluate(ctx, cmd->rule);
 	case CMD_OBJ_CHAIN:
 	case CMD_OBJ_TABLE:
 		return 0;
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 2ca7eea..beea38b 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -425,8 +425,8 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %type <cmd>			base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd list_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd
 %destructor { cmd_free($$); }	base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd list_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd
 
-%type <handle>			table_spec chain_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec
-%destructor { handle_free(&$$); } table_spec chain_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec
+%type <handle>			table_spec chain_spec chain_identifier handle_spec position_spec rule_position ruleset_spec
+%destructor { handle_free(&$$); } table_spec chain_spec chain_identifier handle_spec position_spec rule_position ruleset_spec
 %type <handle>			set_spec set_identifier
 %destructor { handle_free(&$$); } set_spec set_identifier
 %type <val>			family_spec family_spec_explicit chain_policy prio_spec
@@ -438,7 +438,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { close_scope(state); table_free($$); }	table_block_alloc
 %type <chain>			chain_block_alloc chain_block
 %destructor { close_scope(state); chain_free($$); }	chain_block_alloc
-%type <rule>			rule rule_alloc
+%type <rule>			rule ruleid_spec rule_alloc
 %destructor { rule_free($$); }	rule
 
 %type <val>			set_flag_list	set_flag
@@ -745,9 +745,10 @@ add_cmd			:	TABLE		table_spec
 			}
 			;
 
-replace_cmd		:	RULE		ruleid_spec	rule
+replace_cmd		:	RULE		chain_spec	handle_spec	rule
 			{
-				$$ = cmd_alloc(CMD_REPLACE, CMD_OBJ_RULE, &$2, &@$, $3);
+				handle_merge(&$2, &$3);
+				$$ = cmd_alloc(CMD_REPLACE, CMD_OBJ_RULE, &$2, &@$, $4);
 			}
 			;
 
@@ -792,7 +793,7 @@ delete_cmd		:	TABLE		table_spec
 			}
 			|	RULE		ruleid_spec
 			{
-				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_RULE, &$2, &@$, NULL);
+				$$ = cmd_alloc(CMD_DELETE, CMD_OBJ_RULE, &$2->handle, &@$, $2);
 			}
 			|	SET		set_spec
 			{
@@ -1276,8 +1277,15 @@ rule_position		:	chain_spec
 
 ruleid_spec		:	chain_spec handle_spec
 			{
-				$$ = $1;
-				handle_merge(&$$, &$2);
+				$$ = rule_alloc(&@$, NULL);
+				$$->handle = $1;
+				handle_merge(&$$->handle, &$2);
+			}
+			|
+				chain_spec rule
+			{
+				$$ = $2;
+				handle_merge(&$$->handle, &$1);
 			}
 			;
 
diff --git a/src/rule.c b/src/rule.c
index 14e57f2..e9672a7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -20,6 +20,7 @@
 #include <rule.h>
 #include <utils.h>
 #include <netlink.h>
+#include <erec.h>
 
 #include <libnftnl/common.h>
 #include <libnftnl/ruleset.h>
@@ -402,6 +403,32 @@ void rule_print(const struct rule *rule)
 		printf(" # handle %" PRIu64, rule->handle.handle.id);
 }
 
+static struct rule *rule_find_first(const struct rule *rule)
+{
+	struct nftnl_rule *nlr1, *nlr2;
+	struct rule *rule_idx;
+	struct table *table;
+	struct chain *chain;
+
+	table = table_lookup(&rule->handle);
+	if (!table)
+		return NULL;
+	chain = chain_lookup(table, &rule->handle);
+	if (!chain)
+		return NULL;
+
+	nlr1 = alloc_nftnl_rule(&rule->handle);
+	netlink_linearize_rule(NULL, nlr1, rule);
+
+	list_for_each_entry(rule_idx, &chain->rules, list) {
+		nlr2 = alloc_nftnl_rule(&rule_idx->handle);
+		netlink_linearize_rule(NULL, nlr2, rule_idx);
+		if (nftnl_rule_cmp(nlr1, nlr2))
+			return rule_idx;
+	}
+	return NULL;
+}
+
 struct rule *rule_lookup(const struct chain *chain, uint64_t handle)
 {
 	struct rule *rule;
@@ -1010,6 +1037,26 @@ static int do_delete_setelems(struct netlink_ctx *ctx, const struct handle *h,
 	return 0;
 }
 
+static int do_delete_rule(struct netlink_ctx *ctx, const struct cmd *cmd)
+{
+	struct rule *rule;
+
+	/* Delete by handle */
+	if (cmd->handle.handle.id)
+		return netlink_del_rule_batch(ctx, &cmd->handle, &cmd->location);
+
+	/* Delete by description */
+	rule = rule_find_first(cmd->rule);
+	if (!rule) {
+		struct error_record *e = erec_create(EREC_ERROR, &cmd->location,
+			"Could not delete rule to batch: Rule not found");
+		erec_queue(e, ctx->msgs);
+		return -1;
+	}
+	return netlink_del_rule_batch(ctx, &rule->handle,
+				      &rule->handle.position.location);
+}
+
 static int do_command_delete(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	switch (cmd->obj) {
@@ -1018,8 +1065,7 @@ static int do_command_delete(struct netlink_ctx *ctx, struct cmd *cmd)
 	case CMD_OBJ_CHAIN:
 		return netlink_delete_chain(ctx, &cmd->handle, &cmd->location);
 	case CMD_OBJ_RULE:
-		return netlink_del_rule_batch(ctx, &cmd->handle,
-					      &cmd->location);
+		return do_delete_rule(ctx, cmd);
 	case CMD_OBJ_SET:
 		return netlink_delete_set(ctx, &cmd->handle, &cmd->location);
 	case CMD_OBJ_SETELEM:
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4 V6 nft] test: shell: Add tests for deleting rule by description
  2016-08-21 21:22 [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Carlos Falgueras García
  2016-08-21 21:22 ` [PATCH 2/4 V6 nft] Implement deleting rule by description Carlos Falgueras García
@ 2016-08-21 21:22 ` Carlos Falgueras García
  2016-08-22 16:13   ` Pablo Neira Ayuso
       [not found]   ` <20160822162050.GA9973@salvia>
  2016-08-21 21:22 ` [PATCH 4/4 V6 nft] parser: Improve syntax errors Carlos Falgueras García
  2016-08-22 16:13 ` [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Pablo Neira Ayuso
  3 siblings, 2 replies; 9+ messages in thread
From: Carlos Falgueras García @ 2016-08-21 21:22 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo

They checks if commands like "nft delete rule <table> <chain> <rule desc>"
works as is expected.

First one checks if command deletes only one of the matched rules.
Second one checks if command fails when rule did not found.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 .../testcases/rule_management/0010delete-by-desc_0 | 39 ++++++++++++++++++++++
 .../testcases/rule_management/0011delete-by-desc_1 | 20 +++++++++++
 2 files changed, 59 insertions(+)
 create mode 100755 tests/shell/testcases/rule_management/0010delete-by-desc_0
 create mode 100755 tests/shell/testcases/rule_management/0011delete-by-desc_1

diff --git a/tests/shell/testcases/rule_management/0010delete-by-desc_0 b/tests/shell/testcases/rule_management/0010delete-by-desc_0
new file mode 100755
index 0000000..6afdec1
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0010delete-by-desc_0
@@ -0,0 +1,39 @@
+#!/bin/bash
+
+# positive tests for rule deletion by description:
+#	$ nft delete rule <table> <chain> <rule description>
+
+RULE2DEL="ip saddr 1.1.1.1 counter"
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT add rule t c ip saddr 1.1.1.1
+$NFT add rule t c $RULE2DEL
+$NFT add rule t c ip saddr 1.1.1.1 accept
+$NFT add rule t c $RULE2DEL
+
+$NFT delete rule t c $RULE2DEL
+if [ $? -ne 0 ]; then
+	echo "E: unable to delete rule \"$RULE2DEL\"" >&2
+	exit 1
+fi
+
+set +e; # Next commands can return 0
+REMAINS_RULE2DEL=$($NFT list -a ruleset | grep -c "$RULE2DEL")
+REMAINS_RULES=$(( $($NFT list -a ruleset | wc -l) - 4 ))
+set -e
+
+if   [ $REMAINS_RULE2DEL -eq 2 ]; then
+	echo "E: First rule \"$RULE2DEL\" should have been deleted" >&2
+	exit 1
+elif [ $REMAINS_RULE2DEL -eq 0 ]; then
+	echo "E: Second rule \"$RULE2DEL\" should not have been deleted" >&2
+	exit 1
+fi
+
+if [ $REMAINS_RULES -ne 3 ]; then
+	echo "E: Rest of rules should not have been deleted" >&2
+	$NFT list -a ruleset
+	exit 1
+fi
diff --git a/tests/shell/testcases/rule_management/0011delete-by-desc_1 b/tests/shell/testcases/rule_management/0011delete-by-desc_1
new file mode 100755
index 0000000..3ddb5ef
--- /dev/null
+++ b/tests/shell/testcases/rule_management/0011delete-by-desc_1
@@ -0,0 +1,20 @@
+#!/bin/bash
+
+# negative tests for rule deletion by description:
+#	$ nft delete rule <table> <chain> <rule description>
+
+set -e
+$NFT add table t
+$NFT add chain t c
+$NFT add rule t c ip saddr 1.1.1.1
+$NFT add rule t c ip saddr 1.1.1.1 accept
+
+set +e; # Next command must fail
+$NFT delete rule t c ip saddr 2.2.2.2
+RET=$?
+if [ $RET -ne 1 ]; then
+	echo "E: Try to delete a nonexistent rule should throw an error" >&2
+	exit $RET
+fi
+
+exit $RET
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4 V6 nft] parser: Improve syntax errors
  2016-08-21 21:22 [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Carlos Falgueras García
  2016-08-21 21:22 ` [PATCH 2/4 V6 nft] Implement deleting rule by description Carlos Falgueras García
  2016-08-21 21:22 ` [PATCH 3/4 V6 nft] test: shell: Add tests for " Carlos Falgueras García
@ 2016-08-21 21:22 ` Carlos Falgueras García
  2016-08-22 16:13 ` [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Pablo Neira Ayuso
  3 siblings, 0 replies; 9+ messages in thread
From: Carlos Falgueras García @ 2016-08-21 21:22 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo

Shows a more informative message when user commits a syntax error:

	$ nft add rule t c handle 3 ...
	<cmdline>:1:14-19: Error: Did you mean `position'?
	add rule t c handle 3 ...
	             ^^^^^^
	$ nft delete rule t c position 3 ...
	<cmdline>:1:17-24: Error: Did you mean `handle' or insert a rule description?
	delete rule t c position 3 ...
	                ^^^^^^^^

Adds function 'erec_del_last' that deletes last error from the error queue.
This is needed to do not show two error messages.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 include/erec.h     |  5 +++++
 src/parser_bison.y | 17 +++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/erec.h b/include/erec.h
index 36e0efa..95ed24a 100644
--- a/include/erec.h
+++ b/include/erec.h
@@ -58,6 +58,11 @@ static inline void erec_queue(struct error_record *erec,
 	list_add_tail(&erec->list, queue);
 }
 
+static inline void erec_del_last(struct list_head *queue)
+{
+	list_del(queue->prev);
+}
+
 extern void erec_print(FILE *f, const struct error_record *erec);
 extern void erec_print_list(FILE *f, struct list_head *list);
 
diff --git a/src/parser_bison.y b/src/parser_bison.y
index beea38b..9739cb1 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1273,6 +1273,14 @@ rule_position		:	chain_spec
 				$$ = $1;
 				handle_merge(&$$, &$2);
 			}
+			|	chain_spec HANDLE error
+			{
+				erec_del_last(state->msgs);
+				erec_queue(error(&@2, "Did you mean `position'?"),
+					   state->msgs);
+				$$ = $1;
+				YYERROR;
+			}
 			;
 
 ruleid_spec		:	chain_spec handle_spec
@@ -1287,6 +1295,15 @@ ruleid_spec		:	chain_spec handle_spec
 				$$ = $2;
 				handle_merge(&$$->handle, &$1);
 			}
+			|	chain_spec POSITION error
+			{
+				erec_del_last(state->msgs);
+				erec_queue(error(&@2, "Did you mean `handle' or insert a rule description?"),
+					   state->msgs);
+				$$ = rule_alloc(&@$, NULL);
+				handle_merge(&$$->handle, &$1);
+				YYERROR;
+			}
 			;
 
 comment_spec		:	COMMENT		string
-- 
2.8.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4 V6 nft] Simplify parser rule_spec tree
  2016-08-21 21:22 [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Carlos Falgueras García
                   ` (2 preceding siblings ...)
  2016-08-21 21:22 ` [PATCH 4/4 V6 nft] parser: Improve syntax errors Carlos Falgueras García
@ 2016-08-22 16:13 ` Pablo Neira Ayuso
  3 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-22 16:13 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel

On Sun, Aug 21, 2016 at 11:22:07PM +0200, Carlos Falgueras García wrote:
> This patch separates the rule identification from the rule localization, so
> the logic moves from the evaluator to the parser. This allows to revert the
> patch "evaluate: improve rule managment checks"
> (4176c7d30c2ff1b3f52468fc9c08b8df83f979a8) and saves a lot of code.

Applied, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4 V6 nft] Implement deleting rule by description
  2016-08-21 21:22 ` [PATCH 2/4 V6 nft] Implement deleting rule by description Carlos Falgueras García
@ 2016-08-22 16:13   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-22 16:13 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel

On Sun, Aug 21, 2016 at 11:22:08PM +0200, Carlos Falgueras García wrote:
> This patch introduces deletion in a similar fashion as in iptables, thus,
> we can delete the first rule that matches our description, for example:
> 
> 	$ nft list -a ruleset
> 	table ip t {
> 		chain c {
> 			ip saddr 1.1.1.1 counter packets 0 bytes 0 # handle 1
> 			ip saddr 1.1.1.2 counter packets 0 bytes 0 # handle 2
> 			ip saddr 1.1.1.2 counter packets 0 bytes 0 # handle 3
> 			ip saddr 1.1.1.4 counter packets 0 bytes 0 # handle 4
> 		}
> 	}
> 	$ nft delete rule table chain ip saddr 1.1.1.2 counter
> 	$ nft list -a ruleset
> 	table ip t {
> 		chain c {
> 			ip saddr 1.1.1.1 counter packets 0 bytes 0 # handle 1
> 			ip saddr 1.1.1.2 counter packets 0 bytes 0 # handle 3
> 			ip saddr 1.1.1.4 counter packets 0 bytes 0 # handle 4
> 		}
> 	}
> 
> The parser rule 'ruleid_spec' is now of the type 'struct rule' in order to
> hold a rule description. When rule is identified with its handle a dummy
> 'struct rule' is allocated to hold the specified handle.

Applied, thanks Carlos.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4 V6 nft] test: shell: Add tests for deleting rule by description
  2016-08-21 21:22 ` [PATCH 3/4 V6 nft] test: shell: Add tests for " Carlos Falgueras García
@ 2016-08-22 16:13   ` Pablo Neira Ayuso
       [not found]   ` <20160822162050.GA9973@salvia>
  1 sibling, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-22 16:13 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel

On Sun, Aug 21, 2016 at 11:22:09PM +0200, Carlos Falgueras García wrote:
> They checks if commands like "nft delete rule <table> <chain> <rule desc>"
> works as is expected.
> 
> First one checks if command deletes only one of the matched rules.
> Second one checks if command fails when rule did not found.

Also applied, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4 V6 nft] test: shell: Add tests for deleting rule by description
       [not found]   ` <20160822162050.GA9973@salvia>
@ 2016-08-22 16:21     ` Pablo Neira Ayuso
  2016-08-22 16:27       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-22 16:21 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel

Hi Carlos,

One of this test fails... so please send me a follow up to fix it.

W: [FAILED]     ./testcases/rule_management/0010delete-by-desc_0

This chunk also looks a bit strange to me.

set +e; # Next commands can return 0
REMAINS_RULE2DEL=$($NFT list -a ruleset | grep -c "$RULE2DEL")
REMAINS_RULES=$(( $($NFT list -a ruleset | wc -l) - 4 ))
set -e

Please, send me follow up patchset to fix this.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4 V6 nft] test: shell: Add tests for deleting rule by description
  2016-08-22 16:21     ` Pablo Neira Ayuso
@ 2016-08-22 16:27       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-22 16:27 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel

On Mon, Aug 22, 2016 at 06:21:46PM +0200, Pablo Neira Ayuso wrote:
> Hi Carlos,
> 
> One of this test fails... so please send me a follow up to fix it.
> 
> W: [FAILED]     ./testcases/rule_management/0010delete-by-desc_0
> 
> This chunk also looks a bit strange to me.
> 
> set +e; # Next commands can return 0
> REMAINS_RULE2DEL=$($NFT list -a ruleset | grep -c "$RULE2DEL")
> REMAINS_RULES=$(( $($NFT list -a ruleset | wc -l) - 4 ))
> set -e
> 
> Please, send me follow up patchset to fix this.

Also hitting this.

rule.c: In function ‘rule_find_first’:
rule.c:427:3: warning: implicit declaration of function
‘nftnl_rule_cmp’ [-Wimplicit-function-declaration]
   if (nftnl_rule_cmp(nlr1, nlr2))
   ^

Sorry, I'm tossing this patchset, send a v7.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-08-22 16:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-21 21:22 [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Carlos Falgueras García
2016-08-21 21:22 ` [PATCH 2/4 V6 nft] Implement deleting rule by description Carlos Falgueras García
2016-08-22 16:13   ` Pablo Neira Ayuso
2016-08-21 21:22 ` [PATCH 3/4 V6 nft] test: shell: Add tests for " Carlos Falgueras García
2016-08-22 16:13   ` Pablo Neira Ayuso
     [not found]   ` <20160822162050.GA9973@salvia>
2016-08-22 16:21     ` Pablo Neira Ayuso
2016-08-22 16:27       ` Pablo Neira Ayuso
2016-08-21 21:22 ` [PATCH 4/4 V6 nft] parser: Improve syntax errors Carlos Falgueras García
2016-08-22 16:13 ` [PATCH 1/4 V6 nft] Simplify parser rule_spec tree Pablo Neira Ayuso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.