[nft PATCH 0/2] Review code regarding output_fp

[nft PATCH 0/2] Review code regarding output_fp

[Phil Sutter]

Grepping the code for 'stdout' revealed two places where
application-defined output_fp is ignored and instead direct printing to
stdout happens: One is 'nft export' output, the other is 'nft monitor'
output if JSON output was chosen. This series fixes both.

Phil Sutter (2):
  Make 'nft export' respect output_fp
  monitor: Make JSON output respect output_fp

 src/netlink.c | 38 ++++++++++++++++++--------------------
 src/rule.c    |  9 +++++++--
 2 files changed, 25 insertions(+), 22 deletions(-)

-- 
2.13.1

[nft PATCH 1/2] Make 'nft export' respect output_fp

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/rule.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 6a322167b8265..5a6c602505455 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1153,6 +1153,10 @@ static int do_command_delete(struct netlink_ctx *ctx, struct cmd *cmd)
 static int do_command_export(struct netlink_ctx *ctx, struct cmd *cmd)
 {
 	struct nftnl_ruleset *rs;
+	FILE *fp = ctx->octx->output_fp;
+
+	if (!fp)
+		return 0;
 
 	do {
 		rs = netlink_dump_ruleset(ctx, &cmd->handle, &cmd->location);
@@ -1160,8 +1164,9 @@ static int do_command_export(struct netlink_ctx *ctx, struct cmd *cmd)
 			return -1;
 	} while (rs == NULL);
 
-	nftnl_ruleset_fprintf(stdout, rs, cmd->export->format, 0);
-	fprintf(stdout, "\n");
+	nftnl_ruleset_fprintf(fp, rs, cmd->export->format, 0);
+	fprintf(fp, "\n");
+	fflush(fp);
 
 	nftnl_ruleset_free(rs);
 	return 0;
-- 
2.13.1

[nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Phil Sutter]

This introduces a rather nasty macro to call nftnl_*_fprintf() only if
output_fp is valid. On the other hand, it allows to pull the common
parts (format argument, event conversion) into a single place.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/netlink.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index 845eeeffd7387..81b92ac1e2d7c 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -40,6 +40,12 @@
 #include <iface.h>
 
 #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
+#define nftnl_mon_print(monh, type, obj, event)				\
+	if (monh->ctx->octx->output_fp) {				\
+		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
+				       obj, monh->format,		\
+				       netlink_msg2nftnl_of(event));	\
+	}
 
 const struct input_descriptor indesc_netlink = {
 	.name	= "netlink",
@@ -2018,9 +2024,8 @@ static int netlink_events_table_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
-		nftnl_table_fprintf(stdout, nlt, monh->format,
-				    netlink_msg2nftnl_of(type));
-		fprintf(stdout, "\n");
+		nftnl_mon_print(monh, table, nlt, type);
+		nft_mon_print(monh, "\n");
 		break;
 	}
 
@@ -2060,9 +2065,8 @@ static int netlink_events_chain_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
-		nftnl_chain_fprintf(stdout, nlc, monh->format,
-				    netlink_msg2nftnl_of(type));
-		fprintf(stdout, "\n");
+		nftnl_mon_print(monh, chain, nlc, type);
+		nft_mon_print(monh, "\n");
 		break;
 	}
 
@@ -2107,9 +2111,8 @@ static int netlink_events_set_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
-		nftnl_set_fprintf(stdout, nls, monh->format,
-				netlink_msg2nftnl_of(type));
-		fprintf(stdout, "\n");
+		nftnl_mon_print(monh, set, nls, type);
+		nft_mon_print(monh, "\n");
 		break;
 	}
 out:
@@ -2256,9 +2259,8 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
-		nftnl_set_fprintf(stdout, nls, monh->format,
-				  netlink_msg2nftnl_of(type));
-		fprintf(stdout, "\n");
+		nftnl_mon_print(monh, set, nls, type);
+		nft_mon_print(monh, "\n");
 		break;
 	}
 out:
@@ -2301,9 +2303,8 @@ static int netlink_events_obj_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
-		nftnl_obj_fprintf(stdout, nlo, monh->format,
-				  netlink_msg2nftnl_of(type));
-		fprintf(stdout, "\n");
+		nftnl_mon_print(monh, obj, nlo, type);
+		nft_mon_print(monh, "\n");
 		break;
 	}
 
@@ -2357,9 +2358,8 @@ static int netlink_events_rule_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	case NFTNL_OUTPUT_XML:
 	case NFTNL_OUTPUT_JSON:
-		nftnl_rule_fprintf(stdout, nlr, monh->format,
-				 netlink_msg2nftnl_of(type));
-		fprintf(stdout, "\n");
+		nftnl_mon_print(monh, rule, nlr, type);
+		nft_mon_print(monh, "\n");
 		break;
 	}
 
@@ -2984,8 +2984,6 @@ static int netlink_events_cb(const struct nlmsghdr *nlh, void *data)
 		ret = netlink_events_newgen_cb(nlh, type, monh);
 		break;
 	}
-	fflush(stdout);
-
 	return ret;
 }
 
-- 
2.13.1

Re: [nft PATCH 0/2] Review code regarding output_fp

[Pablo Neira Ayuso]

On Thu, Nov 16, 2017 at 09:06:27AM +0100, Phil Sutter wrote:
> Grepping the code for 'stdout' revealed two places where
> application-defined output_fp is ignored and instead direct printing to
> stdout happens: One is 'nft export' output, the other is 'nft monitor'
> output if JSON output was chosen. This series fixes both.

Series applied, thanks.

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Pablo Neira Ayuso]

On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> output_fp is valid. On the other hand, it allows to pull the common
> parts (format argument, event conversion) into a single place.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/netlink.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/src/netlink.c b/src/netlink.c
> index 845eeeffd7387..81b92ac1e2d7c 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -40,6 +40,12 @@
>  #include <iface.h>
>  
>  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> +#define nftnl_mon_print(monh, type, obj, event)				\
> +	if (monh->ctx->octx->output_fp) {				\
> +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> +				       obj, monh->format,		\
> +				       netlink_msg2nftnl_of(event));	\
> +	}

Wait.

Can't we just change nftnl_*_fprintf to do nothing if output_fp is
NULL.

That should be safe.

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Phil Sutter]

On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > output_fp is valid. On the other hand, it allows to pull the common
> > parts (format argument, event conversion) into a single place.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/netlink.c | 38 ++++++++++++++++++--------------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/netlink.c b/src/netlink.c
> > index 845eeeffd7387..81b92ac1e2d7c 100644
> > --- a/src/netlink.c
> > +++ b/src/netlink.c
> > @@ -40,6 +40,12 @@
> >  #include <iface.h>
> >  
> >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > +#define nftnl_mon_print(monh, type, obj, event)				\
> > +	if (monh->ctx->octx->output_fp) {				\
> > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > +				       obj, monh->format,		\
> > +				       netlink_msg2nftnl_of(event));	\
> > +	}
> 
> Wait.
> 
> Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> NULL.
> 
> That should be safe.

Looks like you just caught me trying to avoid changing libnftnl. :D

Yet I still consider the macro valuable since it avoids the nasty
pointer deref chain.

I'll prepare a patch to libnftnl and then provide v2 of this patch with
simplified nftnl_mon_print() macro. ACK?

Thanks, Phil

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Pablo Neira Ayuso]

On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > output_fp is valid. On the other hand, it allows to pull the common
> > parts (format argument, event conversion) into a single place.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/netlink.c | 38 ++++++++++++++++++--------------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/netlink.c b/src/netlink.c
> > index 845eeeffd7387..81b92ac1e2d7c 100644
> > --- a/src/netlink.c
> > +++ b/src/netlink.c
> > @@ -40,6 +40,12 @@
> >  #include <iface.h>
> >  
> >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > +#define nftnl_mon_print(monh, type, obj, event)				\
> > +	if (monh->ctx->octx->output_fp) {				\
> > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > +				       obj, monh->format,		\
> > +				       netlink_msg2nftnl_of(event));	\
> > +	}
> 
> Wait.
> 
> Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> NULL.
> 
> That should be safe.

Or we just save all these branches by always setting output_fp to
stdout. It should simplify things a bit.

What am I missing here?

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Pablo Neira Ayuso]

On Thu, Nov 16, 2017 at 02:54:02PM +0100, Phil Sutter wrote:
> On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > > output_fp is valid. On the other hand, it allows to pull the common
> > > parts (format argument, event conversion) into a single place.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/netlink.c | 38 ++++++++++++++++++--------------------
> > >  1 file changed, 18 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/src/netlink.c b/src/netlink.c
> > > index 845eeeffd7387..81b92ac1e2d7c 100644
> > > --- a/src/netlink.c
> > > +++ b/src/netlink.c
> > > @@ -40,6 +40,12 @@
> > >  #include <iface.h>
> > >  
> > >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > > +#define nftnl_mon_print(monh, type, obj, event)				\
> > > +	if (monh->ctx->octx->output_fp) {				\
> > > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > > +				       obj, monh->format,		\
> > > +				       netlink_msg2nftnl_of(event));	\
> > > +	}
> > 
> > Wait.
> > 
> > Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> > NULL.
> > 
> > That should be safe.
> 
> Looks like you just caught me trying to avoid changing libnftnl. :D
> 
> Yet I still consider the macro valuable since it avoids the nasty
> pointer deref chain.
> 
> I'll prepare a patch to libnftnl and then provide v2 of this patch with
> simplified nftnl_mon_print() macro. ACK?

I wonder if we can just avoid such change by making sure that
monh->ctx->octx->output_fp is always set.

May that work too?

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Phil Sutter]

On Thu, Nov 16, 2017 at 02:54:44PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > > output_fp is valid. On the other hand, it allows to pull the common
> > > parts (format argument, event conversion) into a single place.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/netlink.c | 38 ++++++++++++++++++--------------------
> > >  1 file changed, 18 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/src/netlink.c b/src/netlink.c
> > > index 845eeeffd7387..81b92ac1e2d7c 100644
> > > --- a/src/netlink.c
> > > +++ b/src/netlink.c
> > > @@ -40,6 +40,12 @@
> > >  #include <iface.h>
> > >  
> > >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > > +#define nftnl_mon_print(monh, type, obj, event)				\
> > > +	if (monh->ctx->octx->output_fp) {				\
> > > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > > +				       obj, monh->format,		\
> > > +				       netlink_msg2nftnl_of(event));	\
> > > +	}
> > 
> > Wait.
> > 
> > Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> > NULL.
> > 
> > That should be safe.
> 
> Or we just save all these branches by always setting output_fp to
> stdout. It should simplify things a bit.
> 
> What am I missing here?

output_fp is under application control (via nft_ctx_set_output()), and I
think it's a valid use-case for applications to call
'nft_ctx_set_output(NULL)' to disable all output. On a second thought,
this is also a bit inconsistent since nft_run_cmd_from_*() set output_fp
to stderr before printing erec, but that could be exported to
applications without much effort.

Cheers, Phil

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Pablo Neira Ayuso]

On Thu, Nov 16, 2017 at 02:58:21PM +0100, Phil Sutter wrote:
> On Thu, Nov 16, 2017 at 02:54:44PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > > > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > > > output_fp is valid. On the other hand, it allows to pull the common
> > > > parts (format argument, event conversion) into a single place.
> > > > 
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > >  src/netlink.c | 38 ++++++++++++++++++--------------------
> > > >  1 file changed, 18 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/src/netlink.c b/src/netlink.c
> > > > index 845eeeffd7387..81b92ac1e2d7c 100644
> > > > --- a/src/netlink.c
> > > > +++ b/src/netlink.c
> > > > @@ -40,6 +40,12 @@
> > > >  #include <iface.h>
> > > >  
> > > >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > > > +#define nftnl_mon_print(monh, type, obj, event)				\
> > > > +	if (monh->ctx->octx->output_fp) {				\
> > > > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > > > +				       obj, monh->format,		\
> > > > +				       netlink_msg2nftnl_of(event));	\
> > > > +	}
> > > 
> > > Wait.
> > > 
> > > Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> > > NULL.
> > > 
> > > That should be safe.
> > 
> > Or we just save all these branches by always setting output_fp to
> > stdout. It should simplify things a bit.
> > 
> > What am I missing here?
> 
> output_fp is under application control (via nft_ctx_set_output()), and I
> think it's a valid use-case for applications to call
> 'nft_ctx_set_output(NULL)' to disable all output.

We can set a dummy file descriptor that point to /dev/null, right?

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Phil Sutter]

On Thu, Nov 16, 2017 at 03:12:06PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 16, 2017 at 02:58:21PM +0100, Phil Sutter wrote:
> > On Thu, Nov 16, 2017 at 02:54:44PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > > > > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > > > > output_fp is valid. On the other hand, it allows to pull the common
> > > > > parts (format argument, event conversion) into a single place.
> > > > > 
> > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > ---
> > > > >  src/netlink.c | 38 ++++++++++++++++++--------------------
> > > > >  1 file changed, 18 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/src/netlink.c b/src/netlink.c
> > > > > index 845eeeffd7387..81b92ac1e2d7c 100644
> > > > > --- a/src/netlink.c
> > > > > +++ b/src/netlink.c
> > > > > @@ -40,6 +40,12 @@
> > > > >  #include <iface.h>
> > > > >  
> > > > >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > > > > +#define nftnl_mon_print(monh, type, obj, event)				\
> > > > > +	if (monh->ctx->octx->output_fp) {				\
> > > > > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > > > > +				       obj, monh->format,		\
> > > > > +				       netlink_msg2nftnl_of(event));	\
> > > > > +	}
> > > > 
> > > > Wait.
> > > > 
> > > > Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> > > > NULL.
> > > > 
> > > > That should be safe.
> > > 
> > > Or we just save all these branches by always setting output_fp to
> > > stdout. It should simplify things a bit.
> > > 
> > > What am I missing here?
> > 
> > output_fp is under application control (via nft_ctx_set_output()), and I
> > think it's a valid use-case for applications to call
> > 'nft_ctx_set_output(NULL)' to disable all output.
> 
> We can set a dummy file descriptor that point to /dev/null, right?

Which we would have to acquire using 'open()', correct? What if that
call fails?

I could think of a static FILE *devnull in nft_ctx_set_output() which is
opened if needed, and closed if the program exits. Would that work?

Cheers, Phil

Re: [nft PATCH 2/2] monitor: Make JSON output respect output_fp

[Pablo Neira Ayuso]

On Thu, Nov 16, 2017 at 03:19:24PM +0100, Phil Sutter wrote:
> On Thu, Nov 16, 2017 at 03:12:06PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 16, 2017 at 02:58:21PM +0100, Phil Sutter wrote:
> > > On Thu, Nov 16, 2017 at 02:54:44PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Nov 16, 2017 at 02:38:24PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Thu, Nov 16, 2017 at 09:06:29AM +0100, Phil Sutter wrote:
> > > > > > This introduces a rather nasty macro to call nftnl_*_fprintf() only if
> > > > > > output_fp is valid. On the other hand, it allows to pull the common
> > > > > > parts (format argument, event conversion) into a single place.
> > > > > > 
> > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > > ---
> > > > > >  src/netlink.c | 38 ++++++++++++++++++--------------------
> > > > > >  1 file changed, 18 insertions(+), 20 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/netlink.c b/src/netlink.c
> > > > > > index 845eeeffd7387..81b92ac1e2d7c 100644
> > > > > > --- a/src/netlink.c
> > > > > > +++ b/src/netlink.c
> > > > > > @@ -40,6 +40,12 @@
> > > > > >  #include <iface.h>
> > > > > >  
> > > > > >  #define nft_mon_print(monh, ...) nft_print(monh->ctx->octx, __VA_ARGS__)
> > > > > > +#define nftnl_mon_print(monh, type, obj, event)				\
> > > > > > +	if (monh->ctx->octx->output_fp) {				\
> > > > > > +		nftnl_##type##_fprintf(monh->ctx->octx->output_fp,	\
> > > > > > +				       obj, monh->format,		\
> > > > > > +				       netlink_msg2nftnl_of(event));	\
> > > > > > +	}
> > > > > 
> > > > > Wait.
> > > > > 
> > > > > Can't we just change nftnl_*_fprintf to do nothing if output_fp is
> > > > > NULL.
> > > > > 
> > > > > That should be safe.
> > > > 
> > > > Or we just save all these branches by always setting output_fp to
> > > > stdout. It should simplify things a bit.
> > > > 
> > > > What am I missing here?
> > > 
> > > output_fp is under application control (via nft_ctx_set_output()), and I
> > > think it's a valid use-case for applications to call
> > > 'nft_ctx_set_output(NULL)' to disable all output.
> > 
> > We can set a dummy file descriptor that point to /dev/null, right?
> 
> Which we would have to acquire using 'open()', correct? What if that
> call fails?
> 
> I could think of a static FILE *devnull in nft_ctx_set_output() which is
> opened if needed, and closed if the program exits. Would that work?

Not sure how that looks like, but it removes many of the branches that
we have now in place to check if output_fp is non-null, then it will
simplify the existing code, which would be good :-).

[nft PATCH RFC] libnftables: Make output_fp default to /dev/null

[Phil Sutter]

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(-)

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)
-- 
2.13.1

Re: [nft PATCH RFC] libnftables: Make output_fp default to /dev/null

[Pablo Neira Ayuso]

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.

Re: [nft PATCH RFC] libnftables: Make output_fp default to /dev/null

[Pablo Neira Ayuso]

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!

Re: [nft PATCH RFC] libnftables: Make output_fp default to /dev/null

[Phil Sutter]

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

Re: [nft PATCH RFC] libnftables: Make output_fp default to /dev/null

[Pablo Neira Ayuso]

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.

[nft PATCH] libnftables: Ensure output_fp is never NULL

[Phil Sutter]

Initialize output_fp to 'stdout' upon context creation and check output
stream validity in nft_ctx_set_output(). This allows to drop checks in
nft_{gmp_,}print() and do_command_export(). While doing so for the
latter, simplify it a bit by using nft_print() which takes care of
flushing the output stream.

If applications desire to drop all output, they are supposed to open
/dev/null and assign that.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/libnftables.c | 10 ++++------
 src/main.c        |  1 -
 src/rule.c        |  6 +-----
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index e8fa6742f7d17..c86d89477e778 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -167,6 +167,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->output.output_fp = stdout;
 
 	if (flags == NFT_CTX_DEFAULT)
 		nft_ctx_netlink_init(ctx);
@@ -190,6 +191,9 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
 {
 	FILE *old = ctx->output.output_fp;
 
+	if (!fp || ferror(fp))
+		return NULL;
+
 	ctx->output.output_fp = fp;
 
 	return old;
@@ -333,9 +337,6 @@ int nft_print(struct output_ctx *octx, const char *fmt, ...)
 	int ret;
 	va_list arg;
 
-	if (!octx->output_fp)
-		return -1;
-
 	va_start(arg, fmt);
 	ret = vfprintf(octx->output_fp, fmt, arg);
 	va_end(arg);
@@ -349,9 +350,6 @@ int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
 	int ret;
 	va_list arg;
 
-	if (!octx->output_fp)
-		return -1;
-
 	va_start(arg, fmt);
 	ret = gmp_vfprintf(octx->output_fp, fmt, arg);
 	va_end(arg);
diff --git a/src/main.c b/src/main.c
index ff7878c94ccb3..353b87bc66631 100644
--- a/src/main.c
+++ b/src/main.c
@@ -173,7 +173,6 @@ int main(int argc, char * const *argv)
 	int i, val, rc;
 
 	nft = nft_ctx_new(NFT_CTX_DEFAULT);
-	nft_ctx_set_output(nft, stdout);
 
 	while (1) {
 		val = getopt_long(argc, argv, OPTSTRING, options, NULL);
diff --git a/src/rule.c b/src/rule.c
index eb91be4636e21..37d99c2200471 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1153,9 +1153,6 @@ static int do_command_export(struct netlink_ctx *ctx, struct cmd *cmd)
 	struct nftnl_ruleset *rs;
 	FILE *fp = ctx->octx->output_fp;
 
-	if (!fp)
-		return 0;
-
 	do {
 		rs = netlink_dump_ruleset(ctx, &cmd->handle, &cmd->location);
 		if (rs == NULL && errno != EINTR)
@@ -1163,8 +1160,7 @@ static int do_command_export(struct netlink_ctx *ctx, struct cmd *cmd)
 	} while (rs == NULL);
 
 	nftnl_ruleset_fprintf(fp, rs, cmd->export->format, 0);
-	fprintf(fp, "\n");
-	fflush(fp);
+	nft_print(ctx->octx, "\n");
 
 	nftnl_ruleset_free(rs);
 	return 0;
-- 
2.13.1

Re: [nft PATCH] libnftables: Ensure output_fp is never NULL

[Pablo Neira Ayuso]

On Mon, Nov 20, 2017 at 04:54:04PM +0100, Phil Sutter wrote:
> Initialize output_fp to 'stdout' upon context creation and check output
> stream validity in nft_ctx_set_output(). This allows to drop checks in
> nft_{gmp_,}print() and do_command_export(). While doing so for the
> latter, simplify it a bit by using nft_print() which takes care of
> flushing the output stream.
> 
> If applications desire to drop all output, they are supposed to open
> /dev/null and assign that.

Applied, thanks Phil.