Re: [nft PATCH 1/4] xt: Delay libxtables access until translation

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Dec 09, 2022 at 01:14:50AM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> 
> On Thu, Dec 08, 2022 at 10:21:02PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 24, 2022 at 05:56:38PM +0100, Phil Sutter wrote:
> > > There is no point in spending efforts setting up the xt match/target
> > > when it is not printed afterwards. So just store the statement data from
> > > libnftnl in struct xt_stmt and perform the extension lookup from
> > > xt_stmt_xlate() instead.
> > 
> > There is nft -i and nft monitor which keep a ruleset cache. Both are
> > sort of incomplete: nft -i resorts to cleaning up the cache based on
> > the generation number and nft monitor still needs to be updated to
> > keep track of incremental ruleset updates via netlink events. Sooner
> > or later these two will get better support for incremental ruleset
> > updates.
> > 
> > I mean, in those two cases, every call to print the translation will
> > trigger the allocation of the xt structures, fill them and then call
> > .xlate. I agree it is a bit more work, I guess this won't case any
> > noticeable penalty, but it might be work that needs to be done over
> > and over again when ruleset uses xt match / target.
> 
> So you're saying the overhead when printing a rule might be more
> significant than when fetching it.

I'm saying that there might be scenarios where, once the xt
match/target is set up, we might call .xlate to print it not just once
(considering nft -i use case), but see below.

> I doubt this simply because the same rule is usually printed at most
> once and there are multiple other commands requiring a rule cache.
>
> IMO we may also just leave the code as-is and wait for someone to
> complain about bad performance with rulesets containing many compat
> expressions.
>
> Depending on the actual report, we may also follow a hybrid approach
> and do the match/target lookup only when needed and cache it for
> later use.
>
> My patch made most sense with an nft in mind which does not need xtables
> support for saving/restoring compat expressions. Users depending on this
> for whatever reason will execute the xlate code path in any case now.

OK, you are refering to commands that do not need ruleset listing, in
that case setting up the xt match/target structure makes no sense.

Your original patch description says:

> Also no need to clone the looked up extension, it is needed only to call
> the functions it provides.

I agree, removing this extra clone is good.

I think this patch is fine, the caching of the xt match/target setup
should be easy to do, struct stmt is const on that path, but it could
be overrided in this case.