Re: [iptables PATCH v2 02/10] nft: Implement nft_chain_foreach()

[Phil Sutter <phil@nwl.cc>]

Hi,

On Mon, Oct 12, 2020 at 02:01:18PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:41PM +0200, Phil Sutter wrote:
> > This is just a fancy wrapper around nftnl_chain_list_foreach() with the
> > added benefit of detecting invalid table names or uninitialized chain
> > lists. This in turn allows to drop the checks in flush_rule_cache() and
> > ignore the return code of nft_chain_foreach() as it fails only if the
> > dropped checks had failed, too.
> 
> At quick glance, this is reducing the LoC.
> 
> However, I'm not sure this is better, before this code:
> 
> 1) You fetch the list
> 2) You use it from several spots in the function
> 
> with this patch you might look up for the chain list several times in
> the same function.

Hmm. There might be exceptions, but typically we should have a function
that takes an optional chain name and the body roughly looks like this:

| if (chain) {
| 	return do_something(nft_chain_find(..., chain));
| }
| return nft_chain_foreach(..., do_something);

[...]
> I can also see calls to:
> 
> nft_chain_find(h, table, chain);
> 
> and
> 
> nft_chain_foreach(...)
> 
> from the same function.
> 
> This patch also updates paths in very different ways, there is no
> common idiom being replaced.

Which in particular are those?

The overall agenda is that looking up a chain won't be a trivial
nftnl_chain_list lookup anymore. And iterating over a table's chains
won't be a matter of iterating over a list, because I split base-chains
from user-defined ones:

* Base-chains sit in an array of size NF_INET_NUMHOOKS.
* User-defined chains sit in an (open-coded) list ordered by name.

So the old nft_chain_list_get() is not possible anymore, therefore I
replace everything by either nft_chain_find() or nft_chain_foreach().
Functions should not use both in the same code-path, so if you spot that
I should have a close look again.

Cheers, Phil