Florian Westphal <fw@xxxxxxxxx> wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > Hmm, this will get messy.
> > >
> > > I only see two alternatives:
> > >
> > > - place the bitmask in the pernet structure.
> > > - add struct nft_expr_ctx as a container structure, which has
> > > nft_ctx as first member and the bitmask as second member, to
> > > be used for NEWRULE and NEWSETELEM instead of nft_ctx.
> >
> > Can the 'level' field be moved to this nft_expr_ctx structure? This
> > field is only used from the preparation phase (not in the commit
> > phase).
> >
> > Probably we need to rename nft_ctx to nft_trans_ctx, so it contains
> > the fields that are needed from the commit phase. Then, re-add a
> > nft_ctx again which contains nft_trans_ctx at the beginning, then the
> > register bitmap and the level field. Thus, any future fields only
> > required by preparation phase only will go in nft_ctx, and fields that
> > are specifically are set up from preparation phase and consumed from
> > commit step go in nft_trans_ctx.
> >
> > It is a bit of churn, but it is probably good to tidy up this for
> > future extensions?
>
> Yes, its a lot of churn, I can have a look at how intrusive this will
> be. Problem is that we have a bunch of helpers that take
> 'struct nft_ctx *', which are fed via '&trans->ctx'.
>
> I'd like to avoid 'union nf_ctx_any *' tricks...
I would prefer not to do this.
I would have to change ->init for expressions and objects, and ->validate
too.
I would have to change ->walk() api in sets to get a
ctx-that-has-level/reg_inited fields.
But ->walk() is used in dumps too.
Compared to before this series, size increase of ctx is 48 -> 56 bytes.
This is rounded to 64 byte slab anyway, or 96 or 128 for transaction
structs that contain more data. So this change doesn't buy anything
now.
In case we get further fields that are only relevant at
->validate/->init time and size indeed becomes a problem I'd propose to
instead add
struct nft_ctx_scratch {
u8 level;
DECLARE_BITMAP(reg_inited, NFT_REG32_COUNT);
/* more temporay foo here */
;
and a 'struct nft_ctx_scratch *' member to existing nft_ctx struct.
But an even better argument to keep things as is:
If we need to adopt a lazy implicit-register-clearing in the future,
then we'd need the 'reg_inited' member in the transaction phase, so
that when the data rule blob is generated we can make sure the blob
generator can add the required 'reg := 0' store at the start of the
chain.
The only alternative that I can investigate if you like is to
add
'struct nft_trans_ctx' and then add some kind of
'nft_trans_ctx to nft_ctx' converter function, i.e. no longer
allow to do things like:
nf_tables_flowtable_notify(&trans->ctx, ..
but require sometung like
struct nft_ctx ctx;
nft_make_ctx_from_trans(&ctx, trans->saved_ctx);
nf_tables_flowtable_notify(&ctx, ..);
Downside is that we'd have silent information loss because
the 'additional late fields' would always be 0.
