Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > >
> > > While it is true that a cache exists, we still need to capture new sets
> > > and their elements if they are anonymous. This is because the name
> > > changes and rules will refer to them by name.
> 
> Please, tell me how I can reproduce this here with a simple snippet
> and I will have a look. Thanks!

Just run tests/monitor testsuite, echo testing simple.t will fail.
Alternatively, add a rule with anonymous set like so:
| # nft --echo add rule inet t c tcp dport '{ 22, 80 }'

> > > Given that there is no easy way to identify the anonymous set in cache
> > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > just go with cache updates. Assuming that echo option is typically used
> > > for single commands, there is not much cache updating happening anyway.
> > 
> > This was fixing a real bug, if this is breaking anything, then I think
> > we are not getting to the root cause.
> > 
> > But reverting it does not make things any better.

With all respect, this wasn't obvious. There is no test case covering
it, commit message reads like it is an optimization (apart from the
subject containing 'fix').

Also, I tried to find a real solution (assuming that I'm merely supposed
to avoid pointless cache insertion) but ended up with code updating cache
for "no cache needed" case for anonymous sets and their elements. I
didn't revert out of laziness but because I deemed anything else not
feasible, after having tried two alternatives already.

Cheers, Phil