| Message ID | 20171023153319.13415-3-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | libnftables preparations | expand |
On Mon, Oct 23, 2017 at 05:33:17PM +0200, Phil Sutter wrote: > This allows an application to explicitly flush caches associated with a > given nft context, as seen in cli_complete(). > > Note that this is a bit inconsistent in that it releases the global > interface cache, but nft_ctx_free() does the same so at least it's not a > regression. > > Note that there is no need for explicit cache update routine since cache > is populated during command execution depending on whether it is needed > or not. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > include/nftables/nftables.h | 1 + > src/cli.c | 3 +-- > src/libnftables.c | 9 +++++++-- > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h > index 44d3e95d399e6..1207f10cd2457 100644 > --- a/include/nftables/nftables.h > +++ b/include/nftables/nftables.h > @@ -51,6 +51,7 @@ enum nftables_exit_codes { > struct nft_ctx *nft_ctx_new(uint32_t flags); > void nft_ctx_free(struct nft_ctx *ctx); > FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); > +void nft_ctx_flush_cache(struct nft_ctx *ctx); I would prefer we rename this to nft_ctx_flush_cache(). Now, let's make an exercise of abstraction: You're not Phil Sutter, you're developer Vlamidir. Vladimir wants to make a third party application based on libnftables that is going to rock the world. He knows nothing about internals, but he looks at the API and say: "Hey, I can flush the cache, what is this cache concept?". I'm telling this story because we're leaking an internal detail implementation. So I would just rename this to nft_ctx_reset(). This magic call just cleans up the context for you. Vladimir will be just need to know that he needs to reset context if he want to make incremental updates based on current. I agree with you that we cannot allocate 'cache' from nft_ctx_alloc(), it's good you reminded me this :-). But I would say, we can just hide these details behind the curtain. Let me know, thanks P.S: I still has pushed out your patchset, it's just that I'm providing feedback so we speed up libnftables release :-). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Oct 24, 2017 at 05:52:59PM +0200, Pablo Neira Ayuso wrote: > On Mon, Oct 23, 2017 at 05:33:17PM +0200, Phil Sutter wrote: [...] > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h > > index 44d3e95d399e6..1207f10cd2457 100644 > > --- a/include/nftables/nftables.h > > +++ b/include/nftables/nftables.h > > @@ -51,6 +51,7 @@ enum nftables_exit_codes { > > struct nft_ctx *nft_ctx_new(uint32_t flags); > > void nft_ctx_free(struct nft_ctx *ctx); > > FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); > > +void nft_ctx_flush_cache(struct nft_ctx *ctx); > > I would prefer we rename this to nft_ctx_flush_cache(). > > Now, let's make an exercise of abstraction: You're not Phil Sutter, > you're developer Vlamidir. > > Vladimir wants to make a third party application based on libnftables > that is going to rock the world. > > He knows nothing about internals, but he looks at the API and say: > "Hey, I can flush the cache, what is this cache concept?". > > I'm telling this story because we're leaking an internal detail > implementation. So I would just rename this to nft_ctx_reset(). This > magic call just cleans up the context for you. Hmm. Without further information, I would guess for nft_ctx_reset() to reset all the context data which was previously modified using those setters I defined. A possible cache to reset didn't come to mind since I wasn't aware that there is a cache at all! > Vladimir will be just need to know that he needs to reset context if > he want to make incremental updates based on current. I wonder whether we need to reset the cache at all: We could make cache_update() ignore cache->initialized and instead check whether nft_genid did change after calling netlink_genid_get() - if not, cache is up to date, otherwise call cache_init(). When we discussed possible performance implications of cache updates, I suggested just that as a first counter-measure. Could this work? Or am I missing something? Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Phil, On Tue, Oct 24, 2017 at 07:40:21PM +0200, Phil Sutter wrote: [...] > I wonder whether we need to reset the cache at all: Good point. > We could make cache_update() ignore cache->initialized and instead > check whether nft_genid did change after calling netlink_genid_get() > - if not, cache is up to date, otherwise call cache_init(). When we > discussed possible performance implications of cache updates, I > suggested just that as a first counter-measure. > > Could this work? Or am I missing something? For the simple API, I think it makes sense to remove this nft_ctx_flush_cache() and just perform inconditional cache refresh on every nft_cmd_*() call by now. We can revisit later on to do incremental cache updates based on generation ID as you said. If we keep track of incremental updates, then we will need event handling. But for now I will go simple. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h index 44d3e95d399e6..1207f10cd2457 100644 --- a/include/nftables/nftables.h +++ b/include/nftables/nftables.h @@ -51,6 +51,7 @@ enum nftables_exit_codes { struct nft_ctx *nft_ctx_new(uint32_t flags); void nft_ctx_free(struct nft_ctx *ctx); FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); +void nft_ctx_flush_cache(struct nft_ctx *ctx); int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen); int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename); diff --git a/src/cli.c b/src/cli.c index cadc3af6e8034..3174cfed05a7d 100644 --- a/src/cli.c +++ b/src/cli.c @@ -140,8 +140,7 @@ static void cli_complete(char *line) nft_run(cli_nft, cli_nf_sock, scanner, state, &msgs); erec_print_list(&cli_nft->output, &msgs, cli_nft->debug_mask); xfree(line); - cache_release(&cli_nft->cache); - iface_cache_release(); + nft_ctx_flush_cache(cli_nft); } static char **cli_completion(const char *text, int start, int end) diff --git a/src/libnftables.c b/src/libnftables.c index 9bc51dd894d09..d34e52759e947 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -147,13 +147,18 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) return ctx; } +void nft_ctx_flush_cache(struct nft_ctx *ctx) +{ + iface_cache_release(); + cache_release(&ctx->cache); +} + void nft_ctx_free(struct nft_ctx *ctx) { if (ctx->nf_sock) netlink_close_sock(ctx->nf_sock); - iface_cache_release(); - cache_release(&ctx->cache); + nft_ctx_flush_cache(ctx); xfree(ctx); nft_exit(); }
This allows an application to explicitly flush caches associated with a given nft context, as seen in cli_complete(). Note that this is a bit inconsistent in that it releases the global interface cache, but nft_ctx_free() does the same so at least it's not a regression. Note that there is no need for explicit cache update routine since cache is populated during command execution depending on whether it is needed or not. Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/nftables/nftables.h | 1 + src/cli.c | 3 +-- src/libnftables.c | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-)