[PATCH nft 0/4] split parse and evaluation phase to simplify cache logic

[PATCH nft 0/4] split parse and evaluation phase to simplify cache logic

[Pablo Neira Ayuso]

Hi,

There are a myriad of cache_update() calls all over the evaluation phase
that are required because the parser invokes the evaluation per command.

This complicates the cache logic, since nft may have to invalid a
partial cache to upgrade to a full cache from one command to another.
This forces nft to go back re-evaluate the existing batch that is being
processed, this overly complicates things because the number of
scenarios that trigger cache updates explode. This will get even more
complicated once we do proper cache generation and ERESTART support is
added - which is still missing.

This patchset aims to simplify cache logic by performing one single
cache_update() before the evaluation phase, the idea is to iterate over
the existing commands that come from the parser to do a cache_evaluate()
call that calculates what cache nft needs to dump from the kernel
(either partial or full, actually we have more degress of completeness,
not just these two only). Then, with the proper cache in place, the
evaluation happens.

This patchset revisits the existing design to address this problem:

1) Dynamically allocate input_descriptor object, this allows error
   reporting from the evaluation phase to access file location that
   was only available from the parser phase.

2) Evaluate the ruleset once the parser is complete. So we have two
   independent phases.

3) The mixture of parsing + evaluation was introduced to display all
   errors, either from the parsing or the evaluation phases.

4) Make a single cache_update() call based on the new cache_evaluate()
   function that calculates the kernel dump that needs to be done
   before entering the evaluation phase.

Thanks.

Pablo Neira Ayuso (4):
  src: dynamic input_descriptor allocation
  src: perform evaluation after parsing
  src: Display parser and evaluate errors in one shot
  src: single cache_update() call to build cache before evaluation

 include/nftables.h |   1 +
 include/parser.h   |   4 +-
 include/rule.h     |   1 +
 src/Makefile.am    |   1 +
 src/cache.c        | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/erec.c         |  35 +--------------
 src/evaluate.c     |  76 +--------------------------------
 src/libnftables.c  |  57 +++++++++++++++++++------
 src/mnl.c          |   8 +---
 src/parser_bison.y |  28 ++----------
 src/parser_json.c  |   9 ----
 src/rule.c         |  18 +-------
 src/scanner.l      |  63 ++++++++++++++++++---------
 13 files changed, 222 insertions(+), 202 deletions(-)
 create mode 100644 src/cache.c

-- 
2.11.0

[PATCH nft 1/4] src: dynamic input_descriptor allocation

[Pablo Neira Ayuso]

This patch introduces the input descriptor list, that stores the
existing input descriptor objects. These objects are now dynamically
allocated and release from scanner_destroy() path.

Follow up patches that decouple the parsing and the evaluation phases
require this for error reporting as described by b14572f72aac ("erec:
Fix input descriptors for included files"), this patch partially reverts
such partial.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/nftables.h |  1 +
 include/parser.h   |  3 ++-
 src/erec.c         | 35 +-----------------------------
 src/parser_bison.y |  1 +
 src/scanner.l      | 63 ++++++++++++++++++++++++++++++++++++------------------
 5 files changed, 47 insertions(+), 56 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index bb9bb2091716..af2c1ea16cfb 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -165,6 +165,7 @@ enum input_descriptor_types {
  * @line_offset:	offset of the current line to the beginning
  */
 struct input_descriptor {
+	struct list_head		list;
 	struct location			location;
 	enum input_descriptor_types	type;
 	const char			*name;
diff --git a/include/parser.h b/include/parser.h
index 8e57899eb1a3..a5ae802b288a 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -15,8 +15,9 @@
 
 struct parser_state {
 	struct input_descriptor		*indesc;
-	struct input_descriptor		indescs[MAX_INCLUDE_DEPTH];
+	struct input_descriptor		*indescs[MAX_INCLUDE_DEPTH];
 	unsigned int			indesc_idx;
+	struct list_head		indesc_list;
 
 	struct list_head		*msgs;
 	unsigned int			nerrs;
diff --git a/src/erec.c b/src/erec.c
index cf543a980bc0..c550a596b38c 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -34,50 +34,17 @@ static const char * const error_record_names[] = {
 	[EREC_ERROR]		= "Error"
 };
 
-static void input_descriptor_destroy(const struct input_descriptor *indesc)
-{
-	if (indesc->location.indesc &&
-	    indesc->location.indesc->type != INDESC_INTERNAL) {
-		input_descriptor_destroy(indesc->location.indesc);
-	}
-	if (indesc->name)
-		xfree(indesc->name);
-	xfree(indesc);
-}
-
-static struct input_descriptor *input_descriptor_dup(const struct input_descriptor *indesc)
-{
-	struct input_descriptor *dup_indesc;
-
-	dup_indesc = xmalloc(sizeof(struct input_descriptor));
-	*dup_indesc = *indesc;
-
-	if (indesc->location.indesc &&
-	    indesc->location.indesc->type != INDESC_INTERNAL)
-		dup_indesc->location.indesc = input_descriptor_dup(indesc->location.indesc);
-
-	if (indesc->name)
-		dup_indesc->name = xstrdup(indesc->name);
-
-	return dup_indesc;
-}
-
 void erec_add_location(struct error_record *erec, const struct location *loc)
 {
 	assert(erec->num_locations < EREC_LOCATIONS_MAX);
 	erec->locations[erec->num_locations] = *loc;
-	erec->locations[erec->num_locations].indesc = input_descriptor_dup(loc->indesc);
+	erec->locations[erec->num_locations].indesc = loc->indesc;
 	erec->num_locations++;
 }
 
 void erec_destroy(struct error_record *erec)
 {
-	unsigned int i;
-
 	xfree(erec->msg);
-	for (i = 0; i < erec->num_locations; i++) {
-		input_descriptor_destroy(erec->locations[i].indesc);
-	}
 	xfree(erec);
 }
 
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 62e76fe617c8..2a39db3148ef 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -50,6 +50,7 @@ void parser_init(struct nft_ctx *nft, struct parser_state *state,
 	state->scopes[0] = scope_init(&state->top_scope, NULL);
 	state->ectx.nft = nft;
 	state->ectx.msgs = msgs;
+	init_list_head(&state->indesc_list);
 }
 
 static void yyerror(struct location *loc, struct nft_ctx *nft, void *scanner,
diff --git a/src/scanner.l b/src/scanner.l
index 558bf9209853..d1f6e8799834 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -59,12 +59,12 @@
 static void scanner_pop_buffer(yyscan_t scanner);
 
 
-static void init_pos(struct parser_state *state)
+static void init_pos(struct input_descriptor *indesc)
 {
-	state->indesc->lineno		= 1;
-	state->indesc->column		= 1;
-	state->indesc->token_offset	= 0;
-	state->indesc->line_offset 	= 0;
+	indesc->lineno		= 1;
+	indesc->column		= 1;
+	indesc->token_offset	= 0;
+	indesc->line_offset 	= 0;
 }
 
 static void update_pos(struct parser_state *state, struct location *loc,
@@ -656,13 +656,14 @@ static void scanner_pop_buffer(yyscan_t scanner)
 	struct parser_state *state = yyget_extra(scanner);
 
 	yypop_buffer_state(scanner);
-	state->indesc = &state->indescs[--state->indesc_idx - 1];
+	state->indesc = state->indescs[--state->indesc_idx];
 }
 
 static struct error_record *scanner_push_file(struct nft_ctx *nft, void *scanner,
 					      const char *filename, const struct location *loc)
 {
 	struct parser_state *state = yyget_extra(scanner);
+	struct input_descriptor *indesc;
 	YY_BUFFER_STATE b;
 
 	if (state->indesc_idx == MAX_INCLUDE_DEPTH)
@@ -672,12 +673,18 @@ static struct error_record *scanner_push_file(struct nft_ctx *nft, void *scanner
 	b = yy_create_buffer(nft->f[state->indesc_idx], YY_BUF_SIZE, scanner);
 	yypush_buffer_state(b, scanner);
 
-	state->indesc = &state->indescs[state->indesc_idx++];
+	indesc = xzalloc(sizeof(struct input_descriptor));
+
 	if (loc != NULL)
-		state->indesc->location = *loc;
-	state->indesc->type	= INDESC_FILE;
-	state->indesc->name	= xstrdup(filename);
-	init_pos(state);
+		indesc->location = *loc;
+	indesc->type	= INDESC_FILE;
+	indesc->name	= xstrdup(filename);
+	init_pos(indesc);
+
+	state->indescs[state->indesc_idx] = indesc;
+	state->indesc = state->indescs[state->indesc_idx++];
+	list_add_tail(&indesc->list, &state->indesc_list);
+
 	return NULL;
 }
 
@@ -874,39 +881,52 @@ void scanner_push_buffer(void *scanner, const struct input_descriptor *indesc,
 	struct parser_state *state = yyget_extra(scanner);
 	YY_BUFFER_STATE b;
 
-	state->indesc = &state->indescs[state->indesc_idx++];
+	state->indesc = xzalloc(sizeof(struct input_descriptor));
+	state->indescs[state->indesc_idx] = state->indesc;
+	state->indesc_idx++;
+
 	memcpy(state->indesc, indesc, sizeof(*state->indesc));
 	state->indesc->data = buffer;
 	state->indesc->name = NULL;
+	list_add_tail(&state->indesc->list, &state->indesc_list);
 
 	b = yy_scan_string(buffer, scanner);
 	assert(b != NULL);
-	init_pos(state);
+	init_pos(state->indesc);
 }
 
 void *scanner_init(struct parser_state *state)
 {
 	yyscan_t scanner;
 
-	state->indesc = state->indescs;
-
 	yylex_init_extra(state, &scanner);
 	yyset_out(NULL, scanner);
 
 	return scanner;
 }
 
+static void input_descriptor_destroy(const struct input_descriptor *indesc)
+{
+	if (indesc->name)
+		xfree(indesc->name);
+	xfree(indesc);
+}
+
+static void input_descriptor_list_destroy(struct parser_state *state)
+{
+	struct input_descriptor *indesc, *next;
+
+	list_for_each_entry_safe(indesc, next, &state->indesc_list, list) {
+		list_del(&indesc->list);
+		input_descriptor_destroy(indesc);
+	}
+}
+
 void scanner_destroy(struct nft_ctx *nft)
 {
 	struct parser_state *state = yyget_extra(nft->scanner);
 
 	do {
-		struct input_descriptor *inpdesc =
-			&state->indescs[state->indesc_idx];
-		if (inpdesc && inpdesc->name) {
-			xfree(inpdesc->name);
-			inpdesc->name = NULL;
-		}
 		yypop_buffer_state(nft->scanner);
 
 		if (nft->f[state->indesc_idx]) {
@@ -915,5 +935,6 @@ void scanner_destroy(struct nft_ctx *nft)
 		}
 	} while (state->indesc_idx--);
 
+	input_descriptor_list_destroy(state);
 	yylex_destroy(nft->scanner);
 }
-- 
2.11.0

[PATCH nft 2/4] src: perform evaluation after parsing

[Pablo Neira Ayuso]

Since 61236968b7a1 ("parser: evaluate commands immediately after
parsing"), evaluation is invoked from the parsing phase in order to
improve error reporting.

However, this approach is problematic from the cache perspective since
we don't know if a full or partial netlink dump from the kernel is
needed. If the number of objects in the kernel is significant, the
netlink dump operation to build the cache may significantly slow down
commands.

This patch moves the evaluation phase after the parsing phase as a
preparation update to allow for a better strategy to build the cache.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/parser.h   |  1 -
 src/libnftables.c  | 30 +++++++++++++++++++++++++-----
 src/parser_bison.y | 27 ++-------------------------
 3 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index a5ae802b288a..39a752121a6b 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -27,7 +27,6 @@ struct parser_state {
 	unsigned int			scope;
 
 	struct list_head		*cmds;
-	struct eval_ctx			ectx;
 };
 
 struct mnl_socket;
diff --git a/src/libnftables.c b/src/libnftables.c
index d8de89ca509c..8720fe2bebaf 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -348,7 +348,6 @@ static const struct input_descriptor indesc_cmdline = {
 static int nft_parse_bison_buffer(struct nft_ctx *nft, const char *buf,
 				  struct list_head *msgs, struct list_head *cmds)
 {
-	struct cmd *cmd;
 	int ret;
 
 	parser_init(nft, nft->state, msgs, cmds);
@@ -359,16 +358,12 @@ static int nft_parse_bison_buffer(struct nft_ctx *nft, const char *buf,
 	if (ret != 0 || nft->state->nerrs > 0)
 		return -1;
 
-	list_for_each_entry(cmd, cmds, list)
-		nft_cmd_expand(cmd);
-
 	return 0;
 }
 
 static int nft_parse_bison_filename(struct nft_ctx *nft, const char *filename,
 				    struct list_head *msgs, struct list_head *cmds)
 {
-	struct cmd *cmd;
 	int ret;
 
 	parser_init(nft, nft->state, msgs, cmds);
@@ -380,6 +375,23 @@ static int nft_parse_bison_filename(struct nft_ctx *nft, const char *filename,
 	if (ret != 0 || nft->state->nerrs > 0)
 		return -1;
 
+	return 0;
+}
+
+static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
+			struct list_head *cmds)
+{
+	struct cmd *cmd;
+
+	list_for_each_entry(cmd, cmds, list) {
+		struct eval_ctx ectx = {
+			.nft	= nft,
+			.msgs	= msgs,
+		};
+		if (cmd_evaluate(&ectx, cmd) < 0)
+			return -1;
+	}
+
 	list_for_each_entry(cmd, cmds, list)
 		nft_cmd_expand(cmd);
 
@@ -404,6 +416,10 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 	if (rc)
 		goto err;
 
+	rc = nft_evaluate(nft, &msgs, &cmds);
+	if (rc < 0)
+		goto err;
+
 	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
@@ -448,6 +464,10 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 	if (rc)
 		goto err;
 
+	rc = nft_evaluate(nft, &msgs, &cmds);
+	if (rc < 0)
+		goto err;
+
 	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 2a39db3148ef..8026708ed859 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -48,8 +48,6 @@ void parser_init(struct nft_ctx *nft, struct parser_state *state,
 	state->msgs = msgs;
 	state->cmds = cmds;
 	state->scopes[0] = scope_init(&state->top_scope, NULL);
-	state->ectx.nft = nft;
-	state->ectx.msgs = msgs;
 	init_list_head(&state->indesc_list);
 }
 
@@ -792,17 +790,8 @@ input			:	/* empty */
 			|	input		line
 			{
 				if ($2 != NULL) {
-					LIST_HEAD(list);
-
 					$2->location = @2;
-
-					list_add_tail(&$2->list, &list);
-					if (cmd_evaluate(&state->ectx, $2) < 0) {
-						cmd_free($2);
-						if (++state->nerrs == nft->parser_max_errors)
-							YYABORT;
-					} else
-						list_splice_tail(&list, state->cmds);
+					list_add_tail(&$2->list, state->cmds);
 				}
 			}
 			;
@@ -878,22 +867,10 @@ line			:	common_block			{ $$ = NULL; }
 				 * work.
 				 */
 				if ($1 != NULL) {
-					LIST_HEAD(list);
-
 					$1->location = @1;
-
-					list_add_tail(&$1->list, &list);
-					if (cmd_evaluate(&state->ectx, $1) < 0) {
-						cmd_free($1);
-						if (++state->nerrs == nft->parser_max_errors)
-							YYABORT;
-					} else
-						list_splice_tail(&list, state->cmds);
+					list_add_tail(&$1->list, state->cmds);
 				}
-				if (state->nerrs)
-					YYABORT;
 				$$ = NULL;
-
 				YYACCEPT;
 			}
 			;
-- 
2.11.0

[PATCH nft 3/4] src: Display parser and evaluate errors in one shot

[Pablo Neira Ayuso]

This patch restores 61236968b7a1 ("parser: evaluate commands immediately
after parsing") following a different approach.

In this patch, the evaluation phase is done if the parsing phase fails,
hence the user gets parsing and evaluation errors in one shot, which is
the purpose of 61236968b7a1.

Note that evaluation errors are now shown after parser errors, the example
available in 61236968b7a1 displays with this patch the following error:

 # nft -f /tmp/bad.nft
 /tmp/bad.nft:3:32-32: Error: syntax error, unexpected newline
 add rule filter input tcp dport
                                ^
 /tmp/bad.nft:5:37-41: Error: syntax error, unexpected dport, expecting end of file or newline or semicolon
 add rule filter input tcp dport tcp dport
                                     ^^^^^
 /tmp/bad.nft:4:33-35: Error: datatype mismatch, expected internet network service, expression has type Internet protocol
 add rule filter input tcp dport tcp
                       ~~~~~~~~~ ^^^

So evaluation pointing to line 4 happens after line error reporting
generated by the parser that points to line 3, while 61236968b7a1 was
showing errors per line in order. As a future work, we can sort the
error reporting list to restore exactly the same behaviour.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/libnftables.c | 22 ++++++++++++++++------
 src/parser_json.c |  9 ---------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 8720fe2bebaf..f459ecd50e45 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -400,11 +400,11 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 {
+	int rc = -EINVAL, parser_rc;
 	struct cmd *cmd, *next;
 	LIST_HEAD(msgs);
 	LIST_HEAD(cmds);
 	char *nlbuf;
-	int rc = -EINVAL;
 
 	nlbuf = xzalloc(strlen(buf) + 2);
 	sprintf(nlbuf, "%s\n", buf);
@@ -413,13 +413,18 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 		rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
 	if (rc == -EINVAL)
 		rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds);
-	if (rc)
-		goto err;
+
+	parser_rc = rc;
 
 	rc = nft_evaluate(nft, &msgs, &cmds);
 	if (rc < 0)
 		goto err;
 
+	if (parser_rc) {
+		rc = parser_rc;
+		goto err;
+	}
+
 	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
@@ -445,9 +450,9 @@ err:
 int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 {
 	struct cmd *cmd, *next;
+	int rc, parser_rc;
 	LIST_HEAD(msgs);
 	LIST_HEAD(cmds);
-	int rc;
 
 	rc = cache_update(nft, CMD_INVALID, &msgs);
 	if (rc < 0)
@@ -461,13 +466,18 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 		rc = nft_parse_json_filename(nft, filename, &msgs, &cmds);
 	if (rc == -EINVAL)
 		rc = nft_parse_bison_filename(nft, filename, &msgs, &cmds);
-	if (rc)
-		goto err;
+
+	parser_rc = rc;
 
 	rc = nft_evaluate(nft, &msgs, &cmds);
 	if (rc < 0)
 		goto err;
 
+	if (parser_rc) {
+		rc = parser_rc;
+		goto err;
+	}
+
 	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
 		rc = -1;
 err:
diff --git a/src/parser_json.c b/src/parser_json.c
index 5532ead36c6a..081cf5da7f39 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3390,10 +3390,6 @@ static json_t *seqnum_to_json(const uint32_t seqnum)
 
 static int __json_parse(struct json_ctx *ctx)
 {
-	struct eval_ctx ectx = {
-		.nft = ctx->nft,
-		.msgs = ctx->msgs,
-	};
 	json_t *tmp, *value;
 	size_t index;
 
@@ -3435,11 +3431,6 @@ static int __json_parse(struct json_ctx *ctx)
 
 		list_add_tail(&cmd->list, &list);
 
-		if (cmd_evaluate(&ectx, cmd) < 0) {
-			cmd_free(cmd);
-			json_error(ctx, "Evaluating command at index %zd failed.", index);
-			return -1;
-		}
 		list_splice_tail(&list, ctx->cmds);
 
 		if (nft_output_echo(&ctx->nft->output))
-- 
2.11.0

[PATCH nft 4/4] src: single cache_update() call to build cache before evaluation

[Pablo Neira Ayuso]

This patch allows us to make one single cache_update() call. Thus, there
is not need to rebuild an incomplete cache from the middle of the batch
processing.

Note that nft_run_cmd_from_filename() does not need a full netlink dump
to build the cache anymore, this should speed nft -f with incremental
updates and very large rulesets.

cache_evaluate() calculates the netlink dump to populate the cache that
this batch needs.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/rule.h    |   1 +
 src/Makefile.am   |   1 +
 src/cache.c       | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/evaluate.c    |  76 +--------------------------------
 src/libnftables.c |   9 ++--
 src/mnl.c         |   8 +---
 src/rule.c        |  18 +-------
 7 files changed, 134 insertions(+), 102 deletions(-)
 create mode 100644 src/cache.c

diff --git a/include/rule.h b/include/rule.h
index 8e70c129fcce..bf3f39636efb 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -631,6 +631,7 @@ extern struct error_record *rule_postprocess(struct rule *rule);
 struct netlink_ctx;
 extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
 
+extern int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds);
 extern int cache_update(struct nft_ctx *ctx, enum cmd_ops cmd,
 			struct list_head *msgs);
 extern void cache_flush(struct nft_ctx *ctx, enum cmd_ops cmd,
diff --git a/src/Makefile.am b/src/Makefile.am
index 8e1a4d8795dc..fd641755b7a4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -31,6 +31,7 @@ lib_LTLIBRARIES = libnftables.la
 libnftables_la_SOURCES =			\
 		rule.c				\
 		statement.c			\
+		cache.c				\
 		datatype.c			\
 		expression.c			\
 		evaluate.c			\
diff --git a/src/cache.c b/src/cache.c
new file mode 100644
index 000000000000..89a884012a90
--- /dev/null
+++ b/src/cache.c
@@ -0,0 +1,123 @@
+/*
+ * Copyright (c) 2019 Pablo Neira Ayuso <pablo@netfilter.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <expression.h>
+#include <statement.h>
+#include <rule.h>
+#include <erec.h>
+#include <utils.h>
+
+static void evaluate_cache_add(struct cmd *cmd, unsigned int *completeness)
+{
+	switch (cmd->obj) {
+	case CMD_OBJ_SETELEM:
+	case CMD_OBJ_SET:
+	case CMD_OBJ_CHAIN:
+	case CMD_OBJ_FLOWTABLE:
+		if (*completeness < cmd->op)
+			*completeness = cmd->op;
+		break;
+	case CMD_OBJ_RULE:
+		/* XXX index is set to zero unless this handle_merge() call is
+		 * invoked, this handle_merge() call is done from the
+		 * evaluation, which is too late.
+		 */
+		handle_merge(&cmd->rule->handle, &cmd->handle);
+
+		if (cmd->rule->handle.index.id &&
+		    *completeness < CMD_LIST)
+			*completeness = CMD_LIST;
+		break;
+	default:
+		break;
+	}
+}
+
+static void evaluate_cache_del(struct cmd *cmd, unsigned int *completeness)
+{
+	switch (cmd->obj) {
+	case CMD_OBJ_SETELEM:
+		if (*completeness < cmd->op)
+			*completeness = cmd->op;
+		break;
+	default:
+		break;
+	}
+}
+
+static void evaluate_cache_flush(struct cmd *cmd, unsigned int *completeness)
+{
+	switch (cmd->obj) {
+	case CMD_OBJ_SET:
+	case CMD_OBJ_MAP:
+	case CMD_OBJ_METER:
+		if (*completeness < cmd->op)
+			*completeness = cmd->op;
+		break;
+	default:
+		break;
+	}
+}
+
+static void evaluate_cache_rename(struct cmd *cmd, unsigned int *completeness)
+{
+	switch (cmd->obj) {
+	case CMD_OBJ_CHAIN:
+		if (*completeness < cmd->op)
+			*completeness = cmd->op;
+		break;
+	default:
+		break;
+	}
+}
+
+int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
+{
+	unsigned int completeness = CMD_INVALID;
+	struct cmd *cmd;
+
+	list_for_each_entry(cmd, cmds, list) {
+		switch (cmd->op) {
+		case CMD_ADD:
+		case CMD_INSERT:
+		case CMD_REPLACE:
+			if (nft_output_echo(&nft->output) &&
+			    completeness < cmd->op)
+				return cmd->op;
+
+			/* Fall through */
+		case CMD_CREATE:
+			evaluate_cache_add(cmd, &completeness);
+			break;
+		case CMD_DELETE:
+			evaluate_cache_del(cmd, &completeness);
+			break;
+		case CMD_GET:
+		case CMD_LIST:
+		case CMD_RESET:
+		case CMD_EXPORT:
+		case CMD_MONITOR:
+			if (completeness < cmd->op)
+				completeness = cmd->op;
+			break;
+		case CMD_FLUSH:
+			evaluate_cache_flush(cmd, &completeness);
+			break;
+		case CMD_RENAME:
+			evaluate_cache_rename(cmd, &completeness);
+			break;
+		case CMD_DESCRIBE:
+		case CMD_IMPORT:
+			break;
+		default:
+			break;
+		}
+	}
+
+	return completeness;
+}
diff --git a/src/evaluate.c b/src/evaluate.c
index 55fb3b6131e0..63be2dde8fa2 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -226,7 +226,6 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 	struct table *table;
 	struct set *set;
 	struct expr *new;
-	int ret;
 
 	switch ((*expr)->symtype) {
 	case SYMBOL_VALUE:
@@ -238,10 +237,6 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 		}
 		break;
 	case SYMBOL_SET:
-		ret = cache_update(ctx->nft, ctx->cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		table = table_lookup_global(ctx);
 		if (table == NULL)
 			return table_not_found(ctx);
@@ -3191,12 +3186,6 @@ static int rule_translate_index(struct eval_ctx *ctx, struct rule *rule)
 	struct chain *chain;
 	uint64_t index = 0;
 	struct rule *r;
-	int ret;
-
-	/* update cache with CMD_LIST so that rules are fetched, too */
-	ret = cache_update(ctx->nft, CMD_LIST, ctx->msgs);
-	if (ret < 0)
-		return ret;
 
 	table = table_lookup(&rule->handle, &ctx->nft->cache);
 	if (!table)
@@ -3412,38 +3401,20 @@ static int table_evaluate(struct eval_ctx *ctx, struct table *table)
 
 static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 {
-	int ret;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		return setelem_evaluate(ctx, &cmd->expr);
 	case CMD_OBJ_SET:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		handle_merge(&cmd->set->handle, &cmd->handle);
 		return set_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
 		handle_merge(&cmd->rule->handle, &cmd->handle);
 		return rule_evaluate(ctx, cmd->rule);
 	case CMD_OBJ_CHAIN:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		return chain_evaluate(ctx, cmd->chain);
 	case CMD_OBJ_TABLE:
 		return table_evaluate(ctx, cmd->table);
 	case CMD_OBJ_FLOWTABLE:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		handle_merge(&cmd->flowtable->handle, &cmd->handle);
 		return flowtable_evaluate(ctx, cmd->flowtable);
 	case CMD_OBJ_COUNTER:
@@ -3460,14 +3431,8 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 
 static int cmd_evaluate_delete(struct eval_ctx *ctx, struct cmd *cmd)
 {
-	int ret;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		return setelem_evaluate(ctx, &cmd->expr);
 	case CMD_OBJ_SET:
 	case CMD_OBJ_RULE:
@@ -3490,11 +3455,6 @@ static int cmd_evaluate_get(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	struct table *table;
 	struct set *set;
-	int ret;
-
-	ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-	if (ret < 0)
-		return ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_SETELEM:
@@ -3553,11 +3513,6 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	struct table *table;
 	struct set *set;
-	int ret;
-
-	ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-	if (ret < 0)
-		return ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
@@ -3648,12 +3603,6 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 
 static int cmd_evaluate_reset(struct eval_ctx *ctx, struct cmd *cmd)
 {
-	int ret;
-
-	ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-	if (ret < 0)
-		return ret;
-
 	switch (cmd->obj) {
 	case CMD_OBJ_COUNTER:
 	case CMD_OBJ_QUOTA:
@@ -3674,7 +3623,6 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	struct table *table;
 	struct set *set;
-	int ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_RULESET:
@@ -3688,10 +3636,6 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 		/* Chains don't hold sets */
 		break;
 	case CMD_OBJ_SET:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		table = table_lookup(&cmd->handle, &ctx->nft->cache);
 		if (table == NULL)
 			return table_not_found(ctx);
@@ -3703,10 +3647,6 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 
 		return 0;
 	case CMD_OBJ_MAP:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		table = table_lookup(&cmd->handle, &ctx->nft->cache);
 		if (table == NULL)
 			return table_not_found(ctx);
@@ -3718,10 +3658,6 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 
 		return 0;
 	case CMD_OBJ_METER:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		table = table_lookup(&cmd->handle, &ctx->nft->cache);
 		if (table == NULL)
 			return table_not_found(ctx);
@@ -3741,14 +3677,9 @@ static int cmd_evaluate_flush(struct eval_ctx *ctx, struct cmd *cmd)
 static int cmd_evaluate_rename(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	struct table *table;
-	int ret;
 
 	switch (cmd->obj) {
 	case CMD_OBJ_CHAIN:
-		ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
 		table = table_lookup(&ctx->cmd->handle, &ctx->nft->cache);
 		if (table == NULL)
 			return table_not_found(ctx);
@@ -3840,11 +3771,6 @@ static uint32_t monitor_flags[CMD_MONITOR_EVENT_MAX][CMD_MONITOR_OBJ_MAX] = {
 static int cmd_evaluate_monitor(struct eval_ctx *ctx, struct cmd *cmd)
 {
 	uint32_t event;
-	int ret;
-
-	ret = cache_update(ctx->nft, cmd->op, ctx->msgs);
-	if (ret < 0)
-		return ret;
 
 	if (cmd->monitor->event == NULL)
 		event = CMD_MONITOR_EVENT_ANY;
@@ -3870,7 +3796,7 @@ static int cmd_evaluate_export(struct eval_ctx *ctx, struct cmd *cmd)
 		return cmd_error(ctx, &cmd->location,
 				 "JSON export is no longer supported, use 'nft -j list ruleset' instead");
 
-	return cache_update(ctx->nft, cmd->op, ctx->msgs);
+	return 0;
 }
 
 static int cmd_evaluate_import(struct eval_ctx *ctx, struct cmd *cmd)
diff --git a/src/libnftables.c b/src/libnftables.c
index f459ecd50e45..4bb770c07819 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -381,8 +381,13 @@ static int nft_parse_bison_filename(struct nft_ctx *nft, const char *filename,
 static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 			struct list_head *cmds)
 {
+	unsigned int completeness;
 	struct cmd *cmd;
 
+	completeness = cache_evaluate(nft, cmds);
+	if (cache_update(nft, completeness, msgs) < 0)
+		return -1;
+
 	list_for_each_entry(cmd, cmds, list) {
 		struct eval_ctx ectx = {
 			.nft	= nft,
@@ -454,10 +459,6 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 	LIST_HEAD(msgs);
 	LIST_HEAD(cmds);
 
-	rc = cache_update(nft, CMD_INVALID, &msgs);
-	if (rc < 0)
-		return -1;
-
 	if (!strcmp(filename, "-"))
 		filename = "/dev/stdin";
 
diff --git a/src/mnl.c b/src/mnl.c
index 579210e4736a..c0df2c941d88 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -394,15 +394,9 @@ int mnl_nft_rule_replace(struct netlink_ctx *ctx, const struct cmd *cmd)
 	unsigned int flags = 0;
 	struct nftnl_rule *nlr;
 	struct nlmsghdr *nlh;
-	int err;
-
-	if (nft_output_echo(&ctx->nft->output)) {
-		err = cache_update(ctx->nft, CMD_INVALID, ctx->msgs);
-		if (err < 0)
-			return err;
 
+	if (nft_output_echo(&ctx->nft->output))
 		flags |= NLM_F_ECHO;
-	}
 
 	nlr = nftnl_rule_alloc();
 	if (!nlr)
diff --git a/src/rule.c b/src/rule.c
index 326edb5dd459..c87375d51ccc 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1501,15 +1501,8 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
 {
 	uint32_t flags = excl ? NLM_F_EXCL : 0;
 
-	if (nft_output_echo(&ctx->nft->output)) {
-		int ret;
-
-		ret = cache_update(ctx->nft, cmd->obj, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
+	if (nft_output_echo(&ctx->nft->output))
 		flags |= NLM_F_ECHO;
-	}
 
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
@@ -1552,15 +1545,8 @@ static int do_command_insert(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	uint32_t flags = 0;
 
-	if (nft_output_echo(&ctx->nft->output)) {
-		int ret;
-
-		ret = cache_update(ctx->nft, cmd->obj, ctx->msgs);
-		if (ret < 0)
-			return ret;
-
+	if (nft_output_echo(&ctx->nft->output))
 		flags |= NLM_F_ECHO;
-	}
 
 	switch (cmd->obj) {
 	case CMD_OBJ_RULE:
-- 
2.11.0

Re: [PATCH nft 4/4] src: single cache_update() call to build cache before evaluation

[Phil Sutter]

Hi,

On Wed, Jun 05, 2019 at 06:46:52PM +0200, Pablo Neira Ayuso wrote:
[...]
> diff --git a/src/cache.c b/src/cache.c
> new file mode 100644
> index 000000000000..89a884012a90
> --- /dev/null
> +++ b/src/cache.c
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright (c) 2019 Pablo Neira Ayuso <pablo@netfilter.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <expression.h>
> +#include <statement.h>
> +#include <rule.h>
> +#include <erec.h>
> +#include <utils.h>
> +
> +static void evaluate_cache_add(struct cmd *cmd, unsigned int *completeness)
> +{
> +	switch (cmd->obj) {
> +	case CMD_OBJ_SETELEM:
> +	case CMD_OBJ_SET:
> +	case CMD_OBJ_CHAIN:
> +	case CMD_OBJ_FLOWTABLE:
> +		if (*completeness < cmd->op)
> +			*completeness = cmd->op;
> +		break;
> +	case CMD_OBJ_RULE:
> +		/* XXX index is set to zero unless this handle_merge() call is
> +		 * invoked, this handle_merge() call is done from the
> +		 * evaluation, which is too late.
> +		 */

Using the right handle was somehow tricky. IIRC, getting it right was
part of the work on v5 of my series. Maybe you were working around a bug
in my code?

> +		handle_merge(&cmd->rule->handle, &cmd->handle);
> +
> +		if (cmd->rule->handle.index.id &&
> +		    *completeness < CMD_LIST)
> +			*completeness = CMD_LIST;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void evaluate_cache_del(struct cmd *cmd, unsigned int *completeness)
> +{
> +	switch (cmd->obj) {
> +	case CMD_OBJ_SETELEM:
> +		if (*completeness < cmd->op)
> +			*completeness = cmd->op;

This manual max() thing seems to be a common pattern. Maybe make these
functions return cmd->op (or the desired completeness) and handle the
max() check in caller?

[...]
> +int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> +{
> +	unsigned int completeness = CMD_INVALID;
> +	struct cmd *cmd;
> +
> +	list_for_each_entry(cmd, cmds, list) {
> +		switch (cmd->op) {
> +		case CMD_ADD:
> +		case CMD_INSERT:
> +		case CMD_REPLACE:
> +			if (nft_output_echo(&nft->output) &&
> +			    completeness < cmd->op)
> +				return cmd->op;

Is 'return' correct here? That would abort cmd list processing.

> +
> +			/* Fall through */
> +		case CMD_CREATE:
> +			evaluate_cache_add(cmd, &completeness);
> +			break;
> +		case CMD_DELETE:
> +			evaluate_cache_del(cmd, &completeness);
> +			break;
> +		case CMD_GET:
> +		case CMD_LIST:
> +		case CMD_RESET:
> +		case CMD_EXPORT:
> +		case CMD_MONITOR:
> +			if (completeness < cmd->op)
> +				completeness = cmd->op;
> +			break;
> +		case CMD_FLUSH:
> +			evaluate_cache_flush(cmd, &completeness);
> +			break;
> +		case CMD_RENAME:
> +			evaluate_cache_rename(cmd, &completeness);

As suggested above, I would do (also in the other cases):

			tmp = evaluate_cache_rename(cmd);

> +			break;
> +		case CMD_DESCRIBE:
> +		case CMD_IMPORT:
> +			break;
> +		default:
> +			break;
> +		}

And here:
		completeness = max(tmp, completeness);

> +	}
> +
> +	return completeness;
> +}

Cheers, Phil

Re: [PATCH nft 0/4] split parse and evaluation phase to simplify cache logic

[Pablo Neira Ayuso]

Phil,

I have pushed out this to master so you can rebase your patchset to
revisit index logic on top of what is in git HEAD.

If there's any fallout from this, just let me know, I'll be swift
about addressing it.

Thanks!