Hi Phil, On Fri, Jun 14, 2019 at 03:41:24PM +0200, Phil Sutter wrote: [...] > On Fri, Jun 14, 2019 at 03:04:38PM +0200, Pablo Neira Ayuso wrote: > > On Fri, Jun 14, 2019 at 02:59:10PM +0200, Pablo Neira Ayuso wrote: > > > On Fri, Jun 14, 2019 at 02:54:32PM +0200, Phil Sutter wrote: > > > > Hi Pablo, > > > > > > > > On Fri, Jun 14, 2019 at 02:36:30PM +0200, Pablo Neira Ayuso wrote: > > > > > __CMD_FLUSH_RULESET is a dummy definition that used to skip the netlink > > > > > dump to populate the cache. This patch is a workaround until we have a > > > > > better infrastructure to track the state of the cache objects. > > > > > > > > I assumed the problem wouldn't exist anymore since we're populating the > > > > cache just once. Can you maybe elaborate a bit on the problem you're > > > > trying to solve with that workaround? > > > > > > If nft segfaults to dump the cache, 'nft flush ruleset' will not work > > > since it always fetches the cache, it will segfault too. > > > > > > The flush ruleset command was still dumping the cache before this > > > patch. > > > > In general, we still need to improve the cache logic, to make it finer > > grain. Now that we have a single point to populate the cache, things > > will get more simple. We need to replace the cache command level to > > cache flags or our own cache level definitions. The existing approach > > that uses of commands to define the cache level completeness has its > > own limitations. We can discuss this during the NFWS :-). > > Yes, I had the same thought already. It is quite unintuitive how we link > cache completeness to certain commands. :) Just sent a follow up patch to introduce cache level flags [1]. The existing approach is rather conservative in what we fetch from the kernel (sometimes more than needed) but it should be possible to review this logic incrementally. > Regarding your problem, maybe cache_update() should exit immediately if > passed cmd is CMD_INVALID? Unless I miss something, if cache_evaluate() > returns that value, we don't need a cache at all. Problem is that CMD_INVALID does not mean empty cache. With the new cache level infrastructure, now there is a NFT_CACHE_EMPTY that provides the semantics this workaround was providing in a clearer way I think. [1] https://patchwork.ozlabs.org/patch/1116973/
