[PATCH nft] src: restore --echo with anonymous sets

[PATCH nft] src: restore --echo with anonymous sets

[Pablo Neira Ayuso]

If --echo is passed, then the cache already contains the commands that
have been sent to the kernel. However, anonymous sets are an exception
since the cache needs to be updated in this case.

Remove the old cache logic from the monitor code that has been replaced
by 01e5c6f0ed03 ("src: add cache level flags").

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/netlink.h |  1 -
 src/monitor.c     | 13 ++++++++++++-
 src/rule.c        | 19 -------------------
 3 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index 279723f33d31..e6941714d5b9 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -171,7 +171,6 @@ struct netlink_mon_handler {
 	struct netlink_ctx	*ctx;
 	const struct location	*loc;
 	unsigned int		debug_mask;
-	bool			cache_needed;
 	struct nft_cache	*cache;
 };
 
diff --git a/src/monitor.c b/src/monitor.c
index 40c381149cda..b7b00d7b1343 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -609,6 +609,12 @@ static void netlink_events_cache_addset(struct netlink_mon_handler *monh,
 		goto out;
 	}
 
+	if (nft_output_echo(&monh->ctx->nft->output) &&
+	    !(s->flags & NFT_SET_ANONYMOUS)) {
+		set_free(s);
+		goto out;
+	}
+
 	set_add_hash(s, t);
 out:
 	nftnl_set_free(nls);
@@ -636,6 +642,10 @@ static void netlink_events_cache_addsetelem(struct netlink_mon_handler *monh,
 		goto out;
 	}
 
+	if (nft_output_echo(&monh->ctx->nft->output) &&
+	    !(set->flags & NFT_SET_ANONYMOUS))
+		goto out;
+
 	nlsei = nftnl_set_elems_iter_create(nls);
 	if (nlsei == NULL)
 		memory_allocation_error();
@@ -744,7 +754,8 @@ out:
 static void netlink_events_cache_update(struct netlink_mon_handler *monh,
 					const struct nlmsghdr *nlh, int type)
 {
-	if (!monh->cache_needed)
+	if (nft_output_echo(&monh->ctx->nft->output) &&
+	    type != NFT_MSG_NEWSET && type != NFT_MSG_NEWSETELEM)
 		return;
 
 	switch (type) {
diff --git a/src/rule.c b/src/rule.c
index d4d4add1afab..54290ec1e4a7 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2502,23 +2502,6 @@ static int do_command_rename(struct netlink_ctx *ctx, struct cmd *cmd)
 	return 0;
 }
 
-static bool need_cache(const struct cmd *cmd)
-{
-	/*
-	 *  - new rules in default format
-	 *  - new elements
-	 */
-	if (((cmd->monitor->flags & (1 << NFT_MSG_NEWRULE)) &&
-	    (cmd->monitor->format == NFTNL_OUTPUT_DEFAULT)) ||
-	    (cmd->monitor->flags & (1 << NFT_MSG_NEWSETELEM)))
-		return true;
-
-	if (cmd->monitor->flags & (1 << NFT_MSG_TRACE))
-		return true;
-
-	return false;
-}
-
 static int do_command_monitor(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	struct netlink_mon_handler monhandler = {
@@ -2533,8 +2516,6 @@ static int do_command_monitor(struct netlink_ctx *ctx, struct cmd *cmd)
 	if (nft_output_json(&ctx->nft->output))
 		monhandler.format = NFTNL_OUTPUT_JSON;
 
-	monhandler.cache_needed = need_cache(cmd);
-
 	return netlink_monitor(&monhandler, ctx->nft->nf_sock);
 }
 
-- 
2.11.0

Re: [PATCH nft] src: restore --echo with anonymous sets

[Phil Sutter]

Hi Pablo,

On Thu, Oct 17, 2019 at 03:34:55PM +0200, Pablo Neira Ayuso wrote:
> If --echo is passed, then the cache already contains the commands that
> have been sent to the kernel. However, anonymous sets are an exception
> since the cache needs to be updated in this case.
> 
> Remove the old cache logic from the monitor code that has been replaced
> by 01e5c6f0ed03 ("src: add cache level flags").
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

This is a nice solution, thanks! A few nits below:

[...]
> diff --git a/src/monitor.c b/src/monitor.c
> index 40c381149cda..b7b00d7b1343 100644
> --- a/src/monitor.c
> +++ b/src/monitor.c
> @@ -609,6 +609,12 @@ static void netlink_events_cache_addset(struct netlink_mon_handler *monh,
>  		goto out;
>  	}
>  
> +	if (nft_output_echo(&monh->ctx->nft->output) &&
> +	    !(s->flags & NFT_SET_ANONYMOUS)) {

There is set_is_anonymous(), set and element printing callbacks use it
as well.

[...]
> @@ -636,6 +642,10 @@ static void netlink_events_cache_addsetelem(struct netlink_mon_handler *monh,
>  		goto out;
>  	}
>  
> +	if (nft_output_echo(&monh->ctx->nft->output) &&
> +	    !(set->flags & NFT_SET_ANONYMOUS))

Same here.

[...]
> @@ -744,7 +754,8 @@ out:
>  static void netlink_events_cache_update(struct netlink_mon_handler *monh,
>  					const struct nlmsghdr *nlh, int type)
>  {
> -	if (!monh->cache_needed)
> +	if (nft_output_echo(&monh->ctx->nft->output) &&
> +	    type != NFT_MSG_NEWSET && type != NFT_MSG_NEWSETELEM)
>  		return;

I would use switch() here, like so:

|	if (nft_output_echo(&monh->ctx->nft->output)) {
|		switch (type) {
|		case NFT_MSG_NEWSET:
|		case NFT_MSG_NEWSETELEM:
|			break;
|		default:
|			return;
|		}
|	}

it emphasises that code is filtering for certain event types. Up to you,
I'm fine with both.

Thanks, Phil