Re: nft cache updates

[Patrick McHardy <kaber@trash.net>]

On 09.11, Pablo Neira Ayuso wrote:
> On Mon, Nov 09, 2015 at 03:30:56PM +0000, Patrick McHardy wrote:
> > Hi Pablo,
> > 
> > I'm wondering what the rational for the current cache update behaviour is.
> > The changelog states it is somehow related to the requested command, but
> > that doesn't seem to be true.
> > 
> > Even "nft describe" fails with EPERM as user since the cache appears to be
> > initialized unconditionally, which is a bit unfortunate. Also I used to
> > test things parsing, evaluation and even netlink generation without actually
> > adding those rules as user, which does not work anymore.  This might
> > be harder to get working again, but I'm not sure why we do a full
> > initialization anyways.  The only thing that appears to be needed
> > are sets, and those only in some specific circumstances.
> 
> To look up for the existing sets we need the existing tables and
> chains, they are essential part of the object hierarchy. So this is
> what we're currently dumping.

Well, its not really necessary to have all elements present, the set handle
is enough to recreate the hierarchy. Unless we need properties of the table
or chains themselves its quite useless.

> In general, we need this for incremental updates, in scenarios where
> we have objects that are defined in kernelspace but userspace refers
> to them.
> 
> As you said we can disable the cache in many cases, depending on the
> command or if the ruleset file starts by:
> 
> flush ruleset
> 
> but I have left this out as follow up work, I just wanted to make sure
> incremental updates where working, as well as the existing changes.

I understand. But with dumping the full hierarchy it seems reasonable to
do the update in the place where we are doing it right now. But we actually
only need a very small subset of that, and if we wouldn't dump all of that
information it would actually make sense to move it to the spots that refer
to (existing) sets only.

> nft describe should be easy to restore.
> 
> Regarding inconditional check for table and chain, we have to make it
> from the evaluation step in sets, so leaving other objects without
> checking this seems inconsistent to me.

But we don't actually need any of that information, do we?

> Another side effect of this is better error reporting to the user.

I can see that it is useful, but causing an error for reporting an error
doesn't seem like the correct way to do it. I'm also generally uncertain
about checking this *beforehand*, our usual approach is that solely the
kernel is responsible for determining object existance etc. We could
also interpret context to get the same error reporting.

Last point would be overhead of dumping all that information we don't
*really* need.

Even if we say this is acceptable (I think correctly putting netlink
errors in context is better since it allows to properly report many
other errors as well), I'd prefer if it wouldn't cause a hard failure
such as EPERM.