| Message ID | 20171116191415.19404-1-phil@nwl.cc |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,RFC] libnftables: Make output_fp default to /dev/null | expand |
Hi Phil, On Thu, Nov 16, 2017 at 08:14:15PM +0100, Phil Sutter wrote: > Ensure output_fp is never NULL which allows to drop all respective > checks. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > Hi Pablo, > > This is how I understood your suggestion to use /dev/null. While > implementing it though, I had an idea for a much simpler solution, > namely just rejecting NULL in nft_set_output() and therefore forcing the > application to deal with opening /dev/null if no output is desired. What > do you think about that? I like your idea of rejecting NULL. -- 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
On Mon, Nov 20, 2017 at 01:32:04PM +0100, Pablo Neira Ayuso wrote: > Hi Phil, > > On Thu, Nov 16, 2017 at 08:14:15PM +0100, Phil Sutter wrote: > > Ensure output_fp is never NULL which allows to drop all respective > > checks. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > Hi Pablo, > > > > This is how I understood your suggestion to use /dev/null. While > > implementing it though, I had an idea for a much simpler solution, > > namely just rejecting NULL in nft_set_output() and therefore forcing the > > application to deal with opening /dev/null if no output is desired. What > > do you think about that? > > I like your idea of rejecting NULL. BTW, why does nft_set_output() return FILE *? Is there any usecase for this? 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
On Mon, Nov 20, 2017 at 01:33:13PM +0100, Pablo Neira Ayuso wrote: > On Mon, Nov 20, 2017 at 01:32:04PM +0100, Pablo Neira Ayuso wrote: > > Hi Phil, > > > > On Thu, Nov 16, 2017 at 08:14:15PM +0100, Phil Sutter wrote: > > > Ensure output_fp is never NULL which allows to drop all respective > > > checks. > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > --- > > > Hi Pablo, > > > > > > This is how I understood your suggestion to use /dev/null. While > > > implementing it though, I had an idea for a much simpler solution, > > > namely just rejecting NULL in nft_set_output() and therefore forcing the > > > application to deal with opening /dev/null if no output is desired. What > > > do you think about that? > > > > I like your idea of rejecting NULL. OK, cool. > BTW, why does nft_set_output() return FILE *? Is there any usecase for > this? It's a quick way to change output_fp and store its old value. Current users are nft_run_cmd_from_*(). I could introduce nft_get_output() to make the return value a dedicated success/fail indicator if you prefer that, otherwise I'd just make nft_set_output() return NULL in error case. 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
On Mon, Nov 20, 2017 at 01:38:51PM +0100, Phil Sutter wrote: > On Mon, Nov 20, 2017 at 01:33:13PM +0100, Pablo Neira Ayuso wrote: > > On Mon, Nov 20, 2017 at 01:32:04PM +0100, Pablo Neira Ayuso wrote: > > > Hi Phil, > > > > > > On Thu, Nov 16, 2017 at 08:14:15PM +0100, Phil Sutter wrote: > > > > Ensure output_fp is never NULL which allows to drop all respective > > > > checks. > > > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > > > --- > > > > Hi Pablo, > > > > > > > > This is how I understood your suggestion to use /dev/null. While > > > > implementing it though, I had an idea for a much simpler solution, > > > > namely just rejecting NULL in nft_set_output() and therefore forcing the > > > > application to deal with opening /dev/null if no output is desired. What > > > > do you think about that? > > > > > > I like your idea of rejecting NULL. > > OK, cool. > > > BTW, why does nft_set_output() return FILE *? Is there any usecase for > > this? > > It's a quick way to change output_fp and store its old value. Current > users are nft_run_cmd_from_*(). Oh, I see. > I could introduce nft_get_output() to make the return value a dedicated > success/fail indicator if you prefer that, otherwise I'd just make > nft_set_output() return NULL in error case. As you prefer. For my usecase, the existing call is just fine. -- 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/src/libnftables.c b/src/libnftables.c index 9df9658930c39..64b63da7631ff 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -103,6 +103,37 @@ err1: return ret; } +static FILE *devnull_fp; + +static void devnull_fp_init(void) +{ + int fd; + + devnull_fp = fopen("/dev/null", "w"); + if (devnull_fp) + return; + + fprintf(stderr, "Warning: Opening /dev/null failed"); + + fd = fileno(stdout); + if (fd >= 0) + devnull_fp = fdopen(fd, "w"); + + if (devnull_fp) { + fprintf(stderr, ", falling back to stdout.\n"); + return; + } + + fprintf(stderr, " as well as reopening stdout. Expect problems.\n"); + devnull_fp = stdout; +} + +static void devnull_fp_exit(void) +{ + if (devnull_fp != stdout) + fclose(devnull_fp); +} + static int nft_refcnt; static pthread_mutex_t nft_refcnt_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -122,6 +153,7 @@ static void nft_init(void) #ifdef HAVE_LIBXTABLES xt_init(); #endif + devnull_fp_init(); unlock: pthread_mutex_unlock(&nft_refcnt_mutex); @@ -134,6 +166,7 @@ static void nft_exit(void) if (--nft_refcnt) goto unlock; + devnull_fp_exit(); ct_label_table_exit(); realm_table_rt_exit(); devgroup_table_exit(); @@ -187,6 +220,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) ctx->parser_max_errors = 10; init_list_head(&ctx->cache.list); ctx->flags = flags; + ctx->octx.output_fp = devnull_fp; if (flags == NFT_CTX_DEFAULT) nft_ctx_netlink_init(ctx); @@ -210,9 +244,9 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp) { FILE *old = ctx->output.output_fp; - ctx->output.output_fp = fp; + ctx->output.output_fp = fp ?: devnull_fp; - return old; + return old == devnull_fp ? NULL : old; } bool nft_ctx_get_dry_run(struct nft_ctx *ctx)
Ensure output_fp is never NULL which allows to drop all respective checks. Signed-off-by: Phil Sutter <phil@nwl.cc> --- Hi Pablo, This is how I understood your suggestion to use /dev/null. While implementing it though, I had an idea for a much simpler solution, namely just rejecting NULL in nft_set_output() and therefore forcing the application to deal with opening /dev/null if no output is desired. What do you think about that? Cheers, Phil --- src/libnftables.c | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-)