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

[Phil Sutter <phil@nwl.cc>]

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