[nft PATCH v4 0/3] Implement --echo option

[nft PATCH v4 0/3] Implement --echo option

[Phil Sutter]

Long description of what it is and how it works in patch 3. Patch 1 is a
dependency to patch 2, Patch 3 adds a simple test suite which was
helpful during development.

Patch 1 from v3 has been applied already, so it is no longer present
here. Changelog for remaining patches is contained in them.

Phil Sutter (3):
  netlink: Pass nlmsg flags from rule.c
  Implement --echo option
  tests: Add a simple test suite for --echo option

 doc/nft.xml                   | 10 ++++++
 include/netlink.h             | 12 ++++---
 include/nftables.h            |  1 +
 src/evaluate.c                |  7 ++++
 src/main.c                    | 11 +++++-
 src/mnl.c                     | 25 +++++++++++--
 src/netlink.c                 | 84 ++++++++++++++++++++++++-------------------
 src/rule.c                    | 35 ++++++++++--------
 tests/echo/run-tests.sh       | 45 +++++++++++++++++++++++
 tests/echo/testcases/simple.t | 12 +++++++
 10 files changed, 183 insertions(+), 59 deletions(-)
 create mode 100755 tests/echo/run-tests.sh
 create mode 100644 tests/echo/testcases/simple.t

-- 
2.13.1

[nft PATCH v4 1/3] netlink: Pass nlmsg flags from rule.c

[Phil Sutter]

There is no point in checking value of excl in each called function.
Just do it in a single spot and pass resulting flags.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v3:
- Fix for 'insert rule' command.
---
 include/netlink.h | 10 ++++-----
 src/netlink.c     | 64 +++++++++++++++++++++++++------------------------------
 src/rule.c        | 26 +++++++++++-----------
 3 files changed, 48 insertions(+), 52 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index 7865186b62767..ffbc51d352fa0 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -124,7 +124,7 @@ extern int netlink_replace_rule_batch(struct netlink_ctx *ctx,
 
 extern int netlink_add_chain(struct netlink_ctx *ctx, const struct handle *h,
 			     const struct location *loc,
-			     const struct chain *chain, bool excl);
+			     const struct chain *chain, uint32_t flags);
 extern int netlink_rename_chain(struct netlink_ctx *ctx, const struct handle *h,
 				const struct location *loc, const char *name);
 extern int netlink_delete_chain(struct netlink_ctx *ctx, const struct handle *h,
@@ -140,7 +140,7 @@ extern int netlink_flush_chain(struct netlink_ctx *ctx, const struct handle *h,
 
 extern int netlink_add_table(struct netlink_ctx *ctx, const struct handle *h,
 			     const struct location *loc,
-			     const struct table *table, bool excl);
+			     const struct table *table, uint32_t flags);
 extern int netlink_delete_table(struct netlink_ctx *ctx, const struct handle *h,
 				const struct location *loc);
 extern int netlink_list_tables(struct netlink_ctx *ctx, const struct handle *h,
@@ -153,7 +153,7 @@ extern int netlink_flush_table(struct netlink_ctx *ctx, const struct handle *h,
 			       const struct location *loc);
 
 extern int netlink_add_set(struct netlink_ctx *ctx, const struct handle *h,
-			   struct set *set, bool excl);
+			   struct set *set, uint32_t flags);
 extern int netlink_delete_set(struct netlink_ctx *ctx, const struct handle *h,
 			      const struct location *loc);
 extern int netlink_list_sets(struct netlink_ctx *ctx, const struct handle *h,
@@ -165,7 +165,7 @@ extern struct stmt *netlink_parse_set_expr(const struct set *set,
 					   const struct nftnl_expr *nle);
 
 extern int netlink_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
-				const struct expr *expr, bool excl);
+				const struct expr *expr, uint32_t flags);
 extern int netlink_delete_setelems(struct netlink_ctx *ctx, const struct handle *h,
 				   const struct expr *expr);
 extern int netlink_get_setelems(struct netlink_ctx *ctx, const struct handle *h,
@@ -179,7 +179,7 @@ extern int netlink_reset_objs(struct netlink_ctx *ctx, const struct handle *h,
 			      const struct location *loc, uint32_t type,
 			      bool dump);
 extern int netlink_add_obj(struct netlink_ctx *ctx, const struct handle *h,
-			   struct obj *obj, bool excl);
+			   struct obj *obj, uint32_t flags);
 extern int netlink_delete_obj(struct netlink_ctx *ctx, const struct handle *h,
 			      struct location *loc, uint32_t type);
 
diff --git a/src/netlink.c b/src/netlink.c
index ffdadfb19a4a3..26032f956aba6 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -597,7 +597,7 @@ void netlink_dump_chain(const struct nftnl_chain *nlc)
 static int netlink_add_chain_compat(struct netlink_ctx *ctx,
 				    const struct handle *h,
 				    const struct location *loc,
-				    const struct chain *chain, bool excl)
+				    const struct chain *chain, uint32_t flags)
 {
 	struct nftnl_chain *nlc;
 	int err;
@@ -618,7 +618,7 @@ static int netlink_add_chain_compat(struct netlink_ctx *ctx,
 	}
 
 	netlink_dump_chain(nlc);
-	err = mnl_nft_chain_add(ctx->nf_sock, nlc, excl ? NLM_F_EXCL : 0);
+	err = mnl_nft_chain_add(ctx->nf_sock, nlc, flags);
 	nftnl_chain_free(nlc);
 
 	if (err < 0)
@@ -630,7 +630,7 @@ static int netlink_add_chain_compat(struct netlink_ctx *ctx,
 static int netlink_add_chain_batch(struct netlink_ctx *ctx,
 				   const struct handle *h,
 				   const struct location *loc,
-				   const struct chain *chain, bool excl)
+				   const struct chain *chain, uint32_t flags)
 {
 	struct nftnl_chain *nlc;
 	int err;
@@ -654,8 +654,7 @@ static int netlink_add_chain_batch(struct netlink_ctx *ctx,
 	}
 
 	netlink_dump_chain(nlc);
-	err = mnl_nft_chain_batch_add(nlc, ctx->batch, excl ? NLM_F_EXCL : 0,
-				      ctx->seqnum);
+	err = mnl_nft_chain_batch_add(nlc, ctx->batch, flags, ctx->seqnum);
 	nftnl_chain_free(nlc);
 
 	if (err < 0)
@@ -666,12 +665,12 @@ static int netlink_add_chain_batch(struct netlink_ctx *ctx,
 
 int netlink_add_chain(struct netlink_ctx *ctx, const struct handle *h,
 		      const struct location *loc, const struct chain *chain,
-		      bool excl)
+		      uint32_t flags)
 {
 	if (ctx->batch_supported)
-		return netlink_add_chain_batch(ctx, h, loc, chain, excl);
+		return netlink_add_chain_batch(ctx, h, loc, chain, flags);
 	else
-		return netlink_add_chain_compat(ctx, h, loc, chain, excl);
+		return netlink_add_chain_compat(ctx, h, loc, chain, flags);
 }
 
 static int netlink_rename_chain_compat(struct netlink_ctx *ctx,
@@ -901,13 +900,13 @@ int netlink_flush_chain(struct netlink_ctx *ctx, const struct handle *h,
 static int netlink_add_table_compat(struct netlink_ctx *ctx,
 				    const struct handle *h,
 				    const struct location *loc,
-				    const struct table *table, bool excl)
+				    const struct table *table, uint32_t flags)
 {
 	struct nftnl_table *nlt;
 	int err;
 
 	nlt = alloc_nftnl_table(h);
-	err = mnl_nft_table_add(ctx->nf_sock, nlt, excl ? NLM_F_EXCL : 0);
+	err = mnl_nft_table_add(ctx->nf_sock, nlt, flags);
 	nftnl_table_free(nlt);
 
 	if (err < 0)
@@ -919,7 +918,7 @@ static int netlink_add_table_compat(struct netlink_ctx *ctx,
 static int netlink_add_table_batch(struct netlink_ctx *ctx,
 				   const struct handle *h,
 				   const struct location *loc,
-				   const struct table *table, bool excl)
+				   const struct table *table, uint32_t flags)
 {
 	struct nftnl_table *nlt;
 	int err;
@@ -930,8 +929,7 @@ static int netlink_add_table_batch(struct netlink_ctx *ctx,
 	else
 		nftnl_table_set_u32(nlt, NFTNL_TABLE_FLAGS, 0);
 
-	err = mnl_nft_table_batch_add(nlt, ctx->batch, excl ? NLM_F_EXCL : 0,
-				      ctx->seqnum);
+	err = mnl_nft_table_batch_add(nlt, ctx->batch, flags, ctx->seqnum);
 	nftnl_table_free(nlt);
 
 	if (err < 0)
@@ -942,12 +940,12 @@ static int netlink_add_table_batch(struct netlink_ctx *ctx,
 
 int netlink_add_table(struct netlink_ctx *ctx, const struct handle *h,
 		      const struct location *loc,
-		      const struct table *table, bool excl)
+		      const struct table *table, uint32_t flags)
 {
 	if (ctx->batch_supported)
-		return netlink_add_table_batch(ctx, h, loc, table, excl);
+		return netlink_add_table_batch(ctx, h, loc, table, flags);
 	else
-		return netlink_add_table_compat(ctx, h, loc, table, excl);
+		return netlink_add_table_compat(ctx, h, loc, table, flags);
 }
 
 static int netlink_del_table_compat(struct netlink_ctx *ctx,
@@ -1228,9 +1226,8 @@ static struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 
 static int netlink_add_set_compat(struct netlink_ctx *ctx,
 				  const struct handle *h, struct set *set,
-				  bool excl)
+				  uint32_t flags)
 {
-	unsigned int flags = excl ? NLM_F_EXCL : 0;
 	struct nftnl_set *nls;
 	int err;
 
@@ -1261,7 +1258,7 @@ static int netlink_add_set_compat(struct netlink_ctx *ctx,
 
 static int netlink_add_set_batch(struct netlink_ctx *ctx,
 				 const struct handle *h, struct set *set,
-				 bool excl)
+				 uint32_t flags)
 {
 	struct nftnl_udata_buf *udbuf;
 	struct nftnl_set *nls;
@@ -1318,8 +1315,7 @@ static int netlink_add_set_batch(struct netlink_ctx *ctx,
 
 	netlink_dump_set(nls);
 
-	err = mnl_nft_set_batch_add(nls, ctx->batch, excl ? NLM_F_EXCL : 0,
-				    ctx->seqnum);
+	err = mnl_nft_set_batch_add(nls, ctx->batch, flags, ctx->seqnum);
 	if (err < 0)
 		netlink_io_error(ctx, &set->location, "Could not add set: %s",
 				 strerror(errno));
@@ -1329,12 +1325,12 @@ static int netlink_add_set_batch(struct netlink_ctx *ctx,
 }
 
 int netlink_add_set(struct netlink_ctx *ctx, const struct handle *h,
-		    struct set *set, bool excl)
+		    struct set *set, uint32_t flags)
 {
 	if (ctx->batch_supported)
-		return netlink_add_set_batch(ctx, h, set, excl);
+		return netlink_add_set_batch(ctx, h, set, flags);
 	else
-		return netlink_add_set_compat(ctx, h, set, excl);
+		return netlink_add_set_compat(ctx, h, set, flags);
 }
 
 static int netlink_del_set_compat(struct netlink_ctx *ctx,
@@ -1449,7 +1445,7 @@ static void alloc_setelem_cache(const struct expr *set, struct nftnl_set *nls)
 
 static int netlink_add_setelems_batch(struct netlink_ctx *ctx,
 				      const struct handle *h,
-				      const struct expr *expr, bool excl)
+				      const struct expr *expr, uint32_t flags)
 {
 	struct nftnl_set *nls;
 	int err;
@@ -1458,8 +1454,7 @@ static int netlink_add_setelems_batch(struct netlink_ctx *ctx,
 	alloc_setelem_cache(expr, nls);
 	netlink_dump_set(nls);
 
-	err = mnl_nft_setelem_batch_add(nls, ctx->batch, excl ? NLM_F_EXCL : 0,
-					ctx->seqnum);
+	err = mnl_nft_setelem_batch_add(nls, ctx->batch, flags, ctx->seqnum);
 	nftnl_set_free(nls);
 	if (err < 0)
 		netlink_io_error(ctx, &expr->location,
@@ -1470,7 +1465,7 @@ static int netlink_add_setelems_batch(struct netlink_ctx *ctx,
 
 static int netlink_add_setelems_compat(struct netlink_ctx *ctx,
 				       const struct handle *h,
-				       const struct expr *expr, bool excl)
+				       const struct expr *expr, uint32_t flags)
 {
 	struct nftnl_set *nls;
 	int err;
@@ -1479,7 +1474,7 @@ static int netlink_add_setelems_compat(struct netlink_ctx *ctx,
 	alloc_setelem_cache(expr, nls);
 	netlink_dump_set(nls);
 
-	err = mnl_nft_setelem_add(ctx->nf_sock, nls, excl ? NLM_F_EXCL : 0);
+	err = mnl_nft_setelem_add(ctx->nf_sock, nls, flags);
 	nftnl_set_free(nls);
 	if (err < 0)
 		netlink_io_error(ctx, &expr->location,
@@ -1489,12 +1484,12 @@ static int netlink_add_setelems_compat(struct netlink_ctx *ctx,
 }
 
 int netlink_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
-			 const struct expr *expr, bool excl)
+			 const struct expr *expr, uint32_t flags)
 {
 	if (ctx->batch_supported)
-		return netlink_add_setelems_batch(ctx, h, expr, excl);
+		return netlink_add_setelems_batch(ctx, h, expr, flags);
 	else
-		return netlink_add_setelems_compat(ctx, h, expr, excl);
+		return netlink_add_setelems_compat(ctx, h, expr, flags);
 }
 
 static int netlink_del_setelems_batch(struct netlink_ctx *ctx,
@@ -1770,7 +1765,7 @@ void netlink_dump_obj(struct nftnl_obj *nln)
 }
 
 int netlink_add_obj(struct netlink_ctx *ctx, const struct handle *h,
-		    struct obj *obj, bool excl)
+		    struct obj *obj, uint32_t flags)
 {
 	struct nftnl_obj *nlo;
 	int err;
@@ -1778,8 +1773,7 @@ int netlink_add_obj(struct netlink_ctx *ctx, const struct handle *h,
 	nlo = alloc_nftnl_obj(h, obj);
 	netlink_dump_obj(nlo);
 
-	err = mnl_nft_obj_batch_add(nlo, ctx->batch, excl ? NLM_F_EXCL : 0,
-				    ctx->seqnum);
+	err = mnl_nft_obj_batch_add(nlo, ctx->batch, flags, ctx->seqnum);
 	if (err < 0)
 		netlink_io_error(ctx, &obj->location, "Could not add %s: %s",
 				 obj_type_name(obj->type), strerror(errno));
diff --git a/src/rule.c b/src/rule.c
index 12714ed3ccc70..6b9dbb623b313 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -972,17 +972,17 @@ void cmd_free(struct cmd *cmd)
 #include <netlink.h>
 
 static int __do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
-			     struct set *set, struct expr *expr, bool excl)
+			     struct set *set, struct expr *expr, uint32_t flags)
 {
 	expr->set_flags |= set->flags;
-	if (netlink_add_setelems(ctx, h, expr, excl) < 0)
+	if (netlink_add_setelems(ctx, h, expr, flags) < 0)
 		return -1;
 
 	return 0;
 }
 
 static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
-			   struct expr *init, bool excl)
+			   struct expr *init, uint32_t flags)
 {
 	struct table *table;
 	struct set *set;
@@ -994,18 +994,18 @@ static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
 	    set_to_intervals(ctx->msgs, set, init, true) < 0)
 		return -1;
 
-	return __do_add_setelems(ctx, h, set, init, excl);
+	return __do_add_setelems(ctx, h, set, init, flags);
 }
 
 static int do_add_set(struct netlink_ctx *ctx, const struct handle *h,
-		      struct set *set, bool excl)
+		      struct set *set, uint32_t flags)
 {
 	if (set->init != NULL) {
 		if (set->flags & NFT_SET_INTERVAL &&
 		    set_to_intervals(ctx->msgs, set, set->init, true) < 0)
 			return -1;
 	}
-	if (netlink_add_set(ctx, h, set, excl) < 0)
+	if (netlink_add_set(ctx, h, set, flags) < 0)
 		return -1;
 	if (set->init != NULL) {
 		return __do_add_setelems(ctx, &set->handle, set, set->init,
@@ -1016,24 +1016,26 @@ static int do_add_set(struct netlink_ctx *ctx, const struct handle *h,
 
 static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
 {
+	uint32_t flags = excl ? NLM_F_EXCL : 0;
+
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
 		return netlink_add_table(ctx, &cmd->handle, &cmd->location,
-					 cmd->table, excl);
+					 cmd->table, flags);
 	case CMD_OBJ_CHAIN:
 		return netlink_add_chain(ctx, &cmd->handle, &cmd->location,
-					 cmd->chain, excl);
+					 cmd->chain, flags);
 	case CMD_OBJ_RULE:
 		return netlink_add_rule_batch(ctx, &cmd->handle,
-					      cmd->rule, NLM_F_APPEND);
+					      cmd->rule, flags | NLM_F_APPEND);
 	case CMD_OBJ_SET:
-		return do_add_set(ctx, &cmd->handle, cmd->set, excl);
+		return do_add_set(ctx, &cmd->handle, cmd->set, flags);
 	case CMD_OBJ_SETELEM:
-		return do_add_setelems(ctx, &cmd->handle, cmd->expr, excl);
+		return do_add_setelems(ctx, &cmd->handle, cmd->expr, flags);
 	case CMD_OBJ_COUNTER:
 	case CMD_OBJ_QUOTA:
 	case CMD_OBJ_CT_HELPER:
-		return netlink_add_obj(ctx, &cmd->handle, cmd->object, excl);
+		return netlink_add_obj(ctx, &cmd->handle, cmd->object, flags);
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
 	}
-- 
2.13.1

[nft PATCH v4 2/3] Implement --echo option

[Phil Sutter]

When used with add, insert or replace commands, nft tool will print
event notifications just like 'nft monitor' does for the same commands.

Apart from seeing what a given command will turn out in the rule set,
this allows to reliably retrieve a new rule's assigned handle (if used
together with --handle option).

Here are some examples of how it works:

| # nft --echo --handle add table ip t
| add table ip t
|
| # nft --echo --handle add chain ip t c \
| 	'{ type filter hook forward priority 0; }'
| add chain ip t c { type filter hook forward priority 0; policy accept; }
|
| # nft --echo --handle add rule ip t c tcp dport '{22, 80}' accept
| add rule ip t c tcp dport { ssh, http } accept # handle 2
|
| # nft --echo --handle add set ip t ipset '{ type ipv4_addr; \
| 	elements = { 192.168.0.1, 192.168.0.2 }; }'
| add set ip t ipset { type ipv4_addr; }
| add element ip t ipset { 192.168.0.1 }
| add element ip t ipset { 192.168.0.2 }

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Drop extern declaration of unused variable echo_output.
- Reworded --echo description in man page a bit.

Changes since v2:
- Get rid of NFT_MSG_META_ECHO hack, just use -1 instead.
- Fix for unknown tag <cmd> in nft.xml.

Changes since v3:
- Reuse nft monitor code completely.
- Added missing cache updates when adding a rule or named object.
- Pass flags on to __do_add_setelems() so that anonymous set elements
  are cached as well.
- Drop long description of echo option from nft.8 since it doesn't apply
  anymore.
---
 doc/nft.xml        | 10 ++++++++++
 include/netlink.h  |  2 ++
 include/nftables.h |  1 +
 src/evaluate.c     |  7 +++++++
 src/main.c         | 11 ++++++++++-
 src/mnl.c          | 25 +++++++++++++++++++++++--
 src/netlink.c      | 20 ++++++++++++++++++--
 src/rule.c         |  9 +++++++--
 8 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/doc/nft.xml b/doc/nft.xml
index 4d03a3dbc75bf..6c845013c088d 100644
--- a/doc/nft.xml
+++ b/doc/nft.xml
@@ -157,6 +157,16 @@ vi:ts=4 sw=4
 				</listitem>
 			</varlistentry>
 			<varlistentry>
+				<term><option>-e, --echo</option></term>
+				<listitem>
+					<para>
+						When inserting items into the ruleset using <command>add</command>,
+						<command>insert</command> or <command>replace</command> commands,
+						print notifications just like <command>nft monitor</command>.
+					</para>
+				</listitem>
+			</varlistentry>
+			<varlistentry>
 				<term><option>-I, --includepath <replaceable>directory</replaceable></option></term>
 				<listitem>
 					<para>
diff --git a/include/netlink.h b/include/netlink.h
index ffbc51d352fa0..47ecef38f9e9d 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -222,4 +222,6 @@ extern int netlink_monitor(struct netlink_mon_handler *monhandler,
 			    struct mnl_socket *nf_sock);
 bool netlink_batch_supported(struct mnl_socket *nf_sock);
 
+int netlink_echo_callback(const struct nlmsghdr *nlh, void *data);
+
 #endif /* NFTABLES_NETLINK_H */
diff --git a/include/nftables.h b/include/nftables.h
index 640d3c7e715d8..ca609015274a9 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -29,6 +29,7 @@ struct output_ctx {
 	unsigned int stateless;
 	unsigned int ip2name;
 	unsigned int handle;
+	unsigned int echo;
 };
 
 struct nft_ctx {
diff --git a/src/evaluate.c b/src/evaluate.c
index d24526fef2954..477fb54d51f26 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2962,6 +2962,9 @@ 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, 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:
@@ -2975,6 +2978,10 @@ 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, 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/main.c b/src/main.c
index 1535153ec815d..86862a1088e0c 100644
--- a/src/main.c
+++ b/src/main.c
@@ -49,10 +49,11 @@ enum opt_vals {
 	OPT_IP2NAME		= 'N',
 	OPT_DEBUG		= 'd',
 	OPT_HANDLE_OUTPUT	= 'a',
+	OPT_ECHO		= 'e',
 	OPT_INVALID		= '?',
 };
 
-#define OPTSTRING	"hvcf:iI:vnsNa"
+#define OPTSTRING	"hvcf:iI:vnsNae"
 
 static const struct option options[] = {
 	{
@@ -105,6 +106,10 @@ static const struct option options[] = {
 		.val		= OPT_HANDLE_OUTPUT,
 	},
 	{
+		.name		= "echo",
+		.val		= OPT_ECHO,
+	},
+	{
 		.name		= NULL
 	}
 };
@@ -128,6 +133,7 @@ static void show_help(const char *name)
 "  -s, --stateless		Omit stateful information of ruleset.\n"
 "  -N				Translate IP addresses to names.\n"
 "  -a, --handle			Output rule handle.\n"
+"  -e, --echo			Echo what has been added, inserted or replaced.\n"
 "  -I, --includepath <directory>	Add <directory> to the paths searched for include files. Default is: %s\n"
 #ifdef DEBUG
 "  --debug <level [,level...]>	Specify debugging level (scanner, parser, eval, netlink, mnl, proto-ctx, segtree, all)\n"
@@ -375,6 +381,9 @@ int main(int argc, char * const *argv)
 		case OPT_HANDLE_OUTPUT:
 			nft.output.handle++;
 			break;
+		case OPT_ECHO:
+			nft.output.echo++;
+			break;
 		case OPT_INVALID:
 			exit(NFT_EXIT_FAILURE);
 		}
diff --git a/src/mnl.c b/src/mnl.c
index 862311a740e0e..031b7f39da8f5 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -67,11 +67,32 @@ 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)
@@ -81,7 +102,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, cb, cb_data);
+	return nft_mnl_recv(nf_sock, seq, portid, &nft_mnl_talk_cb, &tcb_data);
 }
 
 /*
@@ -276,7 +297,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
 		if (ret == -1)
 			return -1;
 
-		ret = mnl_cb_run(rcv_buf, ret, 0, portid, NULL, NULL);
+		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
 		/* Continue on error, make sure we get all acknowledgments */
 		if (ret == -1)
 			mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
diff --git a/src/netlink.c b/src/netlink.c
index 26032f956aba6..b172d2cc76aca 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -464,11 +464,11 @@ int netlink_replace_rule_batch(struct netlink_ctx *ctx, const struct handle *h,
 			       const struct location *loc)
 {
 	struct nftnl_rule *nlr;
-	int err;
+	int err, flags = ctx->octx->echo ? NLM_F_ECHO : 0;
 
 	nlr = alloc_nftnl_rule(&rule->handle);
 	netlink_linearize_rule(ctx, nlr, rule);
-	err = mnl_nft_rule_batch_replace(nlr, ctx->batch, 0, ctx->seqnum);
+	err = mnl_nft_rule_batch_replace(nlr, ctx->batch, flags, ctx->seqnum);
 	nftnl_rule_free(nlr);
 
 	if (err < 0)
@@ -3069,6 +3069,22 @@ static int netlink_events_cb(const struct nlmsghdr *nlh, void *data)
 	return ret;
 }
 
+int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
+{
+	struct netlink_mon_handler echo_monh = {
+		.format = NFTNL_OUTPUT_DEFAULT,
+		.ctx = data,
+		.loc = &netlink_location,
+		.monitor_flags = 0xffffffff,
+		.cache_needed = true,
+	};
+
+	if (!echo_monh.ctx->octx->echo)
+		return MNL_CB_OK;
+
+	return netlink_events_cb(nlh, &echo_monh);
+}
+
 int netlink_monitor(struct netlink_mon_handler *monhandler,
 		     struct mnl_socket *nf_sock)
 {
diff --git a/src/rule.c b/src/rule.c
index 6b9dbb623b313..fae8352a7c3f9 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1009,7 +1009,7 @@ static int do_add_set(struct netlink_ctx *ctx, const struct handle *h,
 		return -1;
 	if (set->init != NULL) {
 		return __do_add_setelems(ctx, &set->handle, set, set->init,
-					 false);
+					 flags);
 	}
 	return 0;
 }
@@ -1018,6 +1018,9 @@ 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)
+		flags |= NLM_F_ECHO;
+
 	switch (cmd->obj) {
 	case CMD_OBJ_TABLE:
 		return netlink_add_table(ctx, &cmd->handle, &cmd->location,
@@ -1056,10 +1059,12 @@ 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;
+
 	switch (cmd->obj) {
 	case CMD_OBJ_RULE:
 		return netlink_add_rule_batch(ctx, &cmd->handle,
-					      cmd->rule, 0);
+					      cmd->rule, flags);
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
 	}
-- 
2.13.1

[nft PATCH v4 3/3] tests: Add a simple test suite for --echo option

[Phil Sutter]

The fancy thing about this is that it uses the actual echo output to
undo the changes to the rule set.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v3:
- Dropped undo logic completely, it is not feasible anymore.
- Changed testcase syntax to accommodate for rules which contain
  semi-colons.
- Call nft with '-nna' flags to avoid translating numbers into names and
  enable handle output for rules.
- Extend simple.t to test named sets as well.
---
 tests/echo/run-tests.sh       | 45 +++++++++++++++++++++++++++++++++++++++++++
 tests/echo/testcases/simple.t | 12 ++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100755 tests/echo/run-tests.sh
 create mode 100644 tests/echo/testcases/simple.t

diff --git a/tests/echo/run-tests.sh b/tests/echo/run-tests.sh
new file mode 100755
index 0000000000000..da7934d16965f
--- /dev/null
+++ b/tests/echo/run-tests.sh
@@ -0,0 +1,45 @@
+#!/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
new file mode 100644
index 0000000000000..566fd7e0f8176
--- /dev/null
+++ b/tests/echo/testcases/simple.t
@@ -0,0 +1,12 @@
+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 *
-- 
2.13.1

Re: [nft PATCH v4 0/3] Implement --echo option

[Pablo Neira Ayuso]

On Wed, Aug 09, 2017 at 01:16:40PM +0200, Phil Sutter wrote:
> Long description of what it is and how it works in patch 3. Patch 1 is a
> dependency to patch 2, Patch 3 adds a simple test suite which was
> helpful during development.

Applied, but please follow up asap to address a couple of issues:

mnl.c: In function ‘nft_mnl_talk_cb’:
mnl.c:82:5: warning: ‘rc’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
  if (rc)
     ^

        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);
        }

This code above doesn't look fine...

Apart from this, this struct nft_mnl_talk_cb_data looks... a bit
convoluted ;)

        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)
                        mnl_nlmsg_fprintf(stdout, data, len, sizeof(struct nfgenmsg));
        #endif

                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);
        }

Why don't you simply pass the callback that you need to nft_mnl_recv()
instead of adding this extra unnecesary abstraction...

Please, follow up with a patchset to address this.

Thanks!

Re: [nft PATCH v4 2/3] Implement --echo option

[Pablo Neira Ayuso]

On Wed, Aug 09, 2017 at 01:16:42PM +0200, Phil Sutter wrote:
> diff --git a/include/nftables.h b/include/nftables.h
> index 640d3c7e715d8..ca609015274a9 100644
> --- a/include/nftables.h
> +++ b/include/nftables.h
> @@ -29,6 +29,7 @@ struct output_ctx {
>  	unsigned int stateless;
>  	unsigned int ip2name;
>  	unsigned int handle;
> +	unsigned int echo;
>  };
>  
>  struct nft_ctx {
> diff --git a/src/evaluate.c b/src/evaluate.c
> index d24526fef2954..477fb54d51f26 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -2962,6 +2962,9 @@ 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, cmd->op, ctx->msgs);
> +		if (ret < 0)
> +			return ret;

Wow. And this is going to slow down rule updates *a lot*.

Please, revisit this...

>  		handle_merge(&cmd->rule->handle, &cmd->handle);
>  		return rule_evaluate(ctx, cmd->rule);
>  	case CMD_OBJ_CHAIN:
> @@ -2975,6 +2978,10 @@ 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, cmd->op, ctx->msgs);
> +		if (ret < 0)
> +			return ret;
> +
>  		return 0;
>  	default:
>  		BUG("invalid command object type %u\n", cmd->obj);

Re: [nft PATCH v4 0/3] Implement --echo option

[Phil Sutter]

On Mon, Aug 14, 2017 at 11:26:51AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 09, 2017 at 01:16:40PM +0200, Phil Sutter wrote:
> > Long description of what it is and how it works in patch 3. Patch 1 is a
> > dependency to patch 2, Patch 3 adds a simple test suite which was
> > helpful during development.
> 
> Applied, but please follow up asap to address a couple of issues:
> 
> mnl.c: In function ‘nft_mnl_talk_cb’:
> mnl.c:82:5: warning: ‘rc’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>   if (rc)
>      ^

Hmm, that's weird - the warning is correct, but gcc on my system doesn't
complain. Even after explicitly setting -Wmaybe-uninitialized.

[...]
> Apart from this, this struct nft_mnl_talk_cb_data looks... a bit
> convoluted ;)
> 
>         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)
>                         mnl_nlmsg_fprintf(stdout, data, len, sizeof(struct nfgenmsg));
>         #endif
> 
>                 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);
>         }
> 
> Why don't you simply pass the callback that you need to nft_mnl_recv()
> instead of adding this extra unnecesary abstraction...

It is not unnecessary: There are several callers passing a callback to
nft_mnl_talk(). I didn't want to mess with all of them but still insert
netlink_echo_callback(). Hence I introduced nft_mnl_talk_cb() which
takes care of the callback passed by callers and ultimately calls the
echo callback.

> Please, follow up with a patchset to address this.

Will do, thanks for the feedback!

Cheers, Phil

Re: [nft PATCH v4 0/3] Implement --echo option

[Pablo Neira Ayuso]

On Mon, Aug 14, 2017 at 01:36:44PM +0200, Phil Sutter wrote:
> On Mon, Aug 14, 2017 at 11:26:51AM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Aug 09, 2017 at 01:16:40PM +0200, Phil Sutter wrote:
> > > Long description of what it is and how it works in patch 3. Patch 1 is a
> > > dependency to patch 2, Patch 3 adds a simple test suite which was
> > > helpful during development.
> > 
> > Applied, but please follow up asap to address a couple of issues:
> > 
> > mnl.c: In function ‘nft_mnl_talk_cb’:
> > mnl.c:82:5: warning: ‘rc’ may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> >   if (rc)
> >      ^
> 
> Hmm, that's weird - the warning is correct, but gcc on my system doesn't
> complain. Even after explicitly setting -Wmaybe-uninitialized.

May be your gcc version.

Anyway, the warning is clearly right.

> [...]
> > Apart from this, this struct nft_mnl_talk_cb_data looks... a bit
> > convoluted ;)
> > 
> >         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)
> >                         mnl_nlmsg_fprintf(stdout, data, len, sizeof(struct nfgenmsg));
> >         #endif
> > 
> >                 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);
> >         }
> > 
> > Why don't you simply pass the callback that you need to nft_mnl_recv()
> > instead of adding this extra unnecesary abstraction...
> 
> It is not unnecessary: There are several callers passing a callback to
> nft_mnl_talk().

Then, you pass the callback that you need to nft_mnl_talk() as parameter.

> I didn't want to mess with all of them but still insert
> netlink_echo_callback(). Hence I introduced nft_mnl_talk_cb() which
> takes care of the callback passed by callers and ultimately calls the
> echo callback.

Why don't you just add context as cb_data so you know you have to do
the netlink_echo_callback() handling?

There must be a better way to do this...

> > Please, follow up with a patchset to address this.
> 
> Will do, thanks for the feedback!

Thanks!

Re: [nft PATCH v4 0/3] Implement --echo option

[Phil Sutter]

On Mon, Aug 14, 2017 at 01:43:22PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 14, 2017 at 01:36:44PM +0200, Phil Sutter wrote:
> > On Mon, Aug 14, 2017 at 11:26:51AM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, Aug 09, 2017 at 01:16:40PM +0200, Phil Sutter wrote:
> > > > Long description of what it is and how it works in patch 3. Patch 1 is a
> > > > dependency to patch 2, Patch 3 adds a simple test suite which was
> > > > helpful during development.
> > > 
> > > Applied, but please follow up asap to address a couple of issues:
> > > 
> > > mnl.c: In function ‘nft_mnl_talk_cb’:
> > > mnl.c:82:5: warning: ‘rc’ may be used uninitialized in this function
> > > [-Wmaybe-uninitialized]
> > >   if (rc)
> > >      ^
> > 
> > Hmm, that's weird - the warning is correct, but gcc on my system doesn't
> > complain. Even after explicitly setting -Wmaybe-uninitialized.
> 
> May be your gcc version.

It's 6.3.0, but checking for uninitialized access is far from new so no
idea what is going on here.

> Anyway, the warning is clearly right.
> 
> > [...]
> > > Apart from this, this struct nft_mnl_talk_cb_data looks... a bit
> > > convoluted ;)
> > > 
> > >         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)
> > >                         mnl_nlmsg_fprintf(stdout, data, len, sizeof(struct nfgenmsg));
> > >         #endif
> > > 
> > >                 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);
> > >         }
> > > 
> > > Why don't you simply pass the callback that you need to nft_mnl_recv()
> > > instead of adding this extra unnecesary abstraction...
> > 
> > It is not unnecessary: There are several callers passing a callback to
> > nft_mnl_talk().
> 
> Then, you pass the callback that you need to nft_mnl_talk() as parameter.
> 
> > I didn't want to mess with all of them but still insert
> > netlink_echo_callback(). Hence I introduced nft_mnl_talk_cb() which
> > takes care of the callback passed by callers and ultimately calls the
> > echo callback.
> 
> Why don't you just add context as cb_data so you know you have to do
> the netlink_echo_callback() handling?
> 
> There must be a better way to do this...

Please disregard - I just noticed that in the only cases where echo
callback is required no custom callback is passed yet, so I can just get
by without any wrappers. Also, that code even doesn't work like this -
netlink_echo_callback() expects netlink_ctx as second parameter, which
is not the case. I'll get this sorted as well and then make sure it's
actually tested by manually disabling batch support in code.

Thanks, Phil

Re: [nft PATCH v4 0/3] Implement --echo option

[Pablo Neira Ayuso]

On Mon, Aug 14, 2017 at 01:54:39PM +0200, Phil Sutter wrote:
> On Mon, Aug 14, 2017 at 01:43:22PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Aug 14, 2017 at 01:36:44PM +0200, Phil Sutter wrote:
> > > On Mon, Aug 14, 2017 at 11:26:51AM +0200, Pablo Neira Ayuso wrote:
> > > > On Wed, Aug 09, 2017 at 01:16:40PM +0200, Phil Sutter wrote:
> > > > > Long description of what it is and how it works in patch 3. Patch 1 is a
> > > > > dependency to patch 2, Patch 3 adds a simple test suite which was
> > > > > helpful during development.
> > > > 
> > > > Applied, but please follow up asap to address a couple of issues:
> > > > 
> > > > mnl.c: In function ‘nft_mnl_talk_cb’:
> > > > mnl.c:82:5: warning: ‘rc’ may be used uninitialized in this function
> > > > [-Wmaybe-uninitialized]
> > > >   if (rc)
> > > >      ^
> > > 
> > > Hmm, that's weird - the warning is correct, but gcc on my system doesn't
> > > complain. Even after explicitly setting -Wmaybe-uninitialized.
> > 
> > May be your gcc version.
> 
> It's 6.3.0, but checking for uninitialized access is far from new so no
> idea what is going on here.

Ok, no problem, let's just fix this.

[...]
> > > > Why don't you simply pass the callback that you need to nft_mnl_recv()
> > > > instead of adding this extra unnecesary abstraction...
> > > 
> > > It is not unnecessary: There are several callers passing a callback to
> > > nft_mnl_talk().
> > 
> > Then, you pass the callback that you need to nft_mnl_talk() as parameter.
> > 
> > > I didn't want to mess with all of them but still insert
> > > netlink_echo_callback(). Hence I introduced nft_mnl_talk_cb() which
> > > takes care of the callback passed by callers and ultimately calls the
> > > echo callback.
> > 
> > Why don't you just add context as cb_data so you know you have to do
> > the netlink_echo_callback() handling?
> > 
> > There must be a better way to do this...
> 
> Please disregard - I just noticed that in the only cases where echo
> callback is required no custom callback is passed yet, so I can just get
> by without any wrappers. Also, that code even doesn't work like this -
> netlink_echo_callback() expects netlink_ctx as second parameter, which
> is not the case. I'll get this sorted as well and then make sure it's
> actually tested by manually disabling batch support in code.

Great, wait for your patches, thanks Phil!