diff mbox series

[nft,v4] src: enable json echo output when reading native syntax

Message ID 20200804103846.58872-1-guigom@riseup.net
State Under Review
Delegated to: Pablo Neira
Headers show
Series [nft,v4] src: enable json echo output when reading native syntax | expand

Commit Message

Jose M. Guisado Aug. 4, 2020, 10:38 a.m. UTC
This patch fixes a bug in which nft did not print any output when
specifying --echo and --json and reading nft native syntax.

This patch respects behavior when input is json, in which the output
would be the identical input plus the handles.

Adds a json_echo member inside struct nft_ctx to build and store the json object
containing the json command objects, the object is built using a mock
monitor to reuse monitor json code. This json object is only used when
we are sure we have not read json from input.

Fixes: https://bugzilla.netfilter.org/show_bug.cgi?id=1446

Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
---
v4 respects previous behavior for json echo when reading json input too

 include/nftables.h |  1 +
 src/json.c         | 13 ++++++++++---
 src/monitor.c      | 37 +++++++++++++++++++++++++++++--------
 src/parser_json.c  | 24 +++++++++++++++++-------
 4 files changed, 57 insertions(+), 18 deletions(-)

Comments

Pablo Neira Ayuso Aug. 4, 2020, 11:05 a.m. UTC | #1
On Tue, Aug 04, 2020 at 12:38:46PM +0200, Jose M. Guisado Gomez wrote:
> This patch fixes a bug in which nft did not print any output when
> specifying --echo and --json and reading nft native syntax.
> 
> This patch respects behavior when input is json, in which the output
> would be the identical input plus the handles.
> 
> Adds a json_echo member inside struct nft_ctx to build and store the json object
> containing the json command objects, the object is built using a mock
> monitor to reuse monitor json code. This json object is only used when
> we are sure we have not read json from input.
> 
> Fixes: https://bugzilla.netfilter.org/show_bug.cgi?id=1446
> 
> Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
[...]
> diff --git a/src/monitor.c b/src/monitor.c
> index 3872ebcf..868e31b5 100644
> --- a/src/monitor.c
> +++ b/src/monitor.c
> @@ -221,12 +221,14 @@ static int netlink_events_table_cb(const struct nlmsghdr *nlh, int type,
>  		if (nft_output_handle(&monh->ctx->nft->output))
>  			nft_mon_print(monh, " # handle %" PRIu64 "",
>  				      t->handle.handle.id);
> +		nft_mon_print(monh, "\n");
>  		break;
>  	case NFTNL_OUTPUT_JSON:
>  		monitor_print_table_json(monh, cmd, t);
> +		if(!nft_output_echo(&monh->ctx->nft->output))
> +			nft_mon_print(monh, "\n");
>  		break;
>  	}
> -	nft_mon_print(monh, "\n");
>  	table_free(t);
>  	nftnl_table_free(nlt);
>  	return MNL_CB_OK;
> @@ -258,12 +260,14 @@ static int netlink_events_chain_cb(const struct nlmsghdr *nlh, int type,
>  				      c->handle.chain.name);
>  			break;
>  		}
> +		nft_mon_print(monh, "\n");
>  		break;
>  	case NFTNL_OUTPUT_JSON:
>  		monitor_print_chain_json(monh, cmd, c);
> +		if(!nft_output_echo(&monh->ctx->nft->output))
> +			nft_mon_print(monh, "\n");
>  		break;
>  	}
> -	nft_mon_print(monh, "\n");
>  	chain_free(c);
>  	nftnl_chain_free(nlc);
>  	return MNL_CB_OK;
> @@ -304,12 +308,14 @@ static int netlink_events_set_cb(const struct nlmsghdr *nlh, int type,
>  				      set->handle.set.name);
>  			break;
>  		}
> +		nft_mon_print(monh, "\n");
>  		break;
>  	case NFTNL_OUTPUT_JSON:
>  		monitor_print_set_json(monh, cmd, set);
> +		if(!nft_output_echo(&monh->ctx->nft->output))
> +			nft_mon_print(monh, "\n");
>  		break;
>  	}
> -	nft_mon_print(monh, "\n");
>  	set_free(set);
>  out:
>  	nftnl_set_free(nls);
> @@ -441,6 +447,7 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
>  		nft_mon_print(monh, "%s element %s %s %s ",
>  			      cmd, family2str(family), table, setname);
>  		expr_print(dummyset->init, &monh->ctx->nft->output);
> +		nft_mon_print(monh, "\n");
>  		break;
>  	case NFTNL_OUTPUT_JSON:
>  		dummyset->handle.family = family;
> @@ -450,9 +457,10 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
>  		/* prevent set_free() from trying to free those */
>  		dummyset->handle.set.name = NULL;
>  		dummyset->handle.table.name = NULL;
> +		if(!nft_output_echo(&monh->ctx->nft->output))
                  ^
nitpick: 'if' is not a function, add space between if and parens.

> +			nft_mon_print(monh, "\n");
>  		break;
>  	}
> -	nft_mon_print(monh, "\n");
>  	set_free(dummyset);
>  out:
>  	nftnl_set_free(nls);
> @@ -492,12 +500,14 @@ static int netlink_events_obj_cb(const struct nlmsghdr *nlh, int type,
>  			       obj->handle.obj.name);
>  			break;
>  		}
> +		nft_mon_print(monh, "\n");
>  		break;
>  	case NFTNL_OUTPUT_JSON:
>  		monitor_print_obj_json(monh, cmd, obj);
> +		if(!nft_output_echo(&monh->ctx->nft->output))
                  ^
same here and everywhere else.

> +			nft_mon_print(monh, "\n");
>  		break;
>  	}
> -	nft_mon_print(monh, "\n");
>  	obj_free(obj);
>  	nftnl_obj_free(nlo);
>  	return MNL_CB_OK;
> @@ -542,12 +552,14 @@ static int netlink_events_rule_cb(const struct nlmsghdr *nlh, int type,
>  				      r->handle.handle.id);
>  			break;
>  		}
> +		nft_mon_print(monh, "\n");
>  		break;
>  	case NFTNL_OUTPUT_JSON:
>  		monitor_print_rule_json(monh, cmd, r);
> +		if(!nft_output_echo(&monh->ctx->nft->output))
> +			nft_mon_print(monh, "\n");
>  		break;
>  	}
> -	nft_mon_print(monh, "\n");
>  	rule_free(r);
>  	nftnl_rule_free(nlr);
>  	return MNL_CB_OK;
> @@ -912,6 +924,8 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
>  {
>  	struct netlink_cb_data *nl_cb_data = data;
>  	struct netlink_ctx *ctx = nl_cb_data->nl_ctx;
> +	struct nft_ctx *nft = ctx->nft;
> +
>  	struct netlink_mon_handler echo_monh = {
>  		.format = NFTNL_OUTPUT_DEFAULT,
>  		.ctx = ctx,
> @@ -922,8 +936,15 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
>  	if (!nft_output_echo(&echo_monh.ctx->nft->output))
>  		return MNL_CB_OK;
>  
> -	if (nft_output_json(&ctx->nft->output))
> -		return json_events_cb(nlh, &echo_monh);
> +	if (nft_output_json(&nft->output)) {
> +		if (!nft->json_root) {
> +			nft->json_echo = json_array();
> +			if (!nft->json_echo)
> +				memory_allocation_error();
> +			echo_monh.format = NFTNL_OUTPUT_JSON;
> +		} else

Nitpick: Use curly brace '{' here in the else side of the branch for
consistency (even if it's only on single line).

> +			return json_events_cb(nlh, &echo_monh);
> +	}
>  
>  	return netlink_events_cb(nlh, &echo_monh);
>  }
> diff --git a/src/parser_json.c b/src/parser_json.c
> index 59347168..ef33063d 100644
> --- a/src/parser_json.c
> +++ b/src/parser_json.c
> @@ -3884,11 +3884,21 @@ int json_events_cb(const struct nlmsghdr *nlh, struct netlink_mon_handler *monh)
>  
>  void json_print_echo(struct nft_ctx *ctx)
>  {
> -	if (!ctx->json_root)
> -		return;
> -
> -	json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
> -	json_cmd_assoc_free();
> -	json_decref(ctx->json_root);
> -	ctx->json_root = NULL;
> +	if (!ctx->json_root) {
> +		if (!ctx->json_echo)
> +			return;
> +		else {
> +			ctx->json_echo = json_pack("{s:o}", "nftables", ctx->json_echo);
> +			json_dumpf(ctx->json_echo, ctx->output.output_fp, JSON_PRESERVE_ORDER);
> +			json_decref(ctx->json_echo);
> +			ctx->json_echo = NULL;
> +			fprintf(ctx->output.output_fp, "\n");
> +			fflush(ctx->output.output_fp);
> +		}
> +	} else {
> +		json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
> +		json_cmd_assoc_free();
> +		json_decref(ctx->json_root);
> +		ctx->json_root = NULL;
> +	}

I'd suggest:

void json_print_echo(struct nft_ctx *ctx)
{
	if (!ctx->json_root)
                return;

        if (ctx->json_echo) {
		ctx->json_echo = json_pack("{s:o}", "nftables", ctx->json_echo);
		json_dumpf(ctx->json_echo, ctx->output.output_fp, JSON_PRESERVE_ORDER);
		json_decref(ctx->json_echo);
		ctx->json_echo = NULL;
		fprintf(ctx->output.output_fp, "\n");
		fflush(ctx->output.output_fp);
	} else {
		json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
		json_cmd_assoc_free();
		json_decref(ctx->json_root);
		ctx->json_root = NULL;
	}
}

Thanks.
Jose M. Guisado Aug. 4, 2020, 12:13 p.m. UTC | #2
Hi Pablo, sorry about the formatting issues.

One thing about your suggestion:

On 4/8/20 13:05, Pablo Neira Ayuso wrote:
> if (!ctx->json_root)
>                  return;

Checking uniquely for the absence of json_root is not enough as 
json_echo may have been initialized. In essence, the case the patch is 
fixing is when json_root is null but json_echo is not, to denote that we 
want json echo output but have not read json from input.

In addition, v5 will contain a check for json_echo initialization inside 
monitor.c to avoid re-initializing nft->json_echo when the callback is 
built again, this happens when reading multiple times from a mnl socket 
(see mnl.c:433 inside mnl_batch_talk).

Regards.
Pablo Neira Ayuso Aug. 4, 2020, 12:15 p.m. UTC | #3
On Tue, Aug 04, 2020 at 02:13:01PM +0200, Jose M. Guisado wrote:
> Hi Pablo, sorry about the formatting issues.
> 
> One thing about your suggestion:
> 
> On 4/8/20 13:05, Pablo Neira Ayuso wrote:
> > if (!ctx->json_root)
> >                  return;
> 
> Checking uniquely for the absence of json_root is not enough as
> json_echo may have been initialized. In essence, the case the
> patch is fixing is when json_root is null but json_echo is not,
> to denote that we want json echo output but have not read json
> from input.

Ah, indeed, sorry. Then, probably:

        if (!ctx->json_root) {
               if (!ctx->json_echo)
                        return;

                ctx->json_echo = json_pack("{s:o}", "nftables", ctx->json_echo);
                json_dumpf(ctx->json_echo, ctx->output.output_fp, JSON_PRESERVE_ORDER);
                json_decref(ctx->json_echo);
                ctx->json_echo = NULL;
                fprintf(ctx->output.output_fp, "\n");
                fflush(ctx->output.output_fp);
        } else {
                json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
                json_cmd_assoc_free();
                json_decref(ctx->json_root);
                ctx->json_root = NULL;
        }
Phil Sutter Aug. 4, 2020, 12:37 p.m. UTC | #4
On Tue, Aug 04, 2020 at 12:38:46PM +0200, Jose M. Guisado Gomez wrote:
> This patch fixes a bug in which nft did not print any output when
> specifying --echo and --json and reading nft native syntax.
> 
> This patch respects behavior when input is json, in which the output
> would be the identical input plus the handles.
> 
> Adds a json_echo member inside struct nft_ctx to build and store the json object
> containing the json command objects, the object is built using a mock
> monitor to reuse monitor json code. This json object is only used when
> we are sure we have not read json from input.
> 
> Fixes: https://bugzilla.netfilter.org/show_bug.cgi?id=1446
> 
> Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
> ---
> v4 respects previous behavior for json echo when reading json input too
> 
>  include/nftables.h |  1 +
>  src/json.c         | 13 ++++++++++---
>  src/monitor.c      | 37 +++++++++++++++++++++++++++++--------
>  src/parser_json.c  | 24 +++++++++++++++++-------
>  4 files changed, 57 insertions(+), 18 deletions(-)

Why not just:

--- a/src/monitor.c
+++ b/src/monitor.c
@@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
        if (!nft_output_echo(&echo_monh.ctx->nft->output))
                return MNL_CB_OK;
 
-       if (nft_output_json(&ctx->nft->output))
-               return json_events_cb(nlh, &echo_monh);
+       if (nft_output_json(&ctx->nft->output)) {
+               if (ctx->nft->json_root)
+                       return json_events_cb(nlh, &echo_monh);
+               echo_monh.format = NFTNL_OUTPUT_JSON;
+       }
 
        return netlink_events_cb(nlh, &echo_monh);
 }

At a first glance, this seems to work just fine.

Cheers, Phil
Eric Garver Aug. 4, 2020, 12:57 p.m. UTC | #5
On Tue, Aug 04, 2020 at 12:38:46PM +0200, Jose M. Guisado Gomez wrote:
> This patch fixes a bug in which nft did not print any output when
> specifying --echo and --json and reading nft native syntax.
> 
> This patch respects behavior when input is json, in which the output
> would be the identical input plus the handles.
> 
> Adds a json_echo member inside struct nft_ctx to build and store the json object
> containing the json command objects, the object is built using a mock
> monitor to reuse monitor json code. This json object is only used when
> we are sure we have not read json from input.
> 
> Fixes: https://bugzilla.netfilter.org/show_bug.cgi?id=1446
> 
> Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
> ---
> v4 respects previous behavior for json echo when reading json input too

With this version all firewalld tests pass. Thanks!

Tested-by: Eric Garver <eric@garver.life>
Jose M. Guisado Aug. 4, 2020, 1:05 p.m. UTC | #6
On 4/8/20 14:37, Phil Sutter wrote:
> Why not just:
> 
> --- a/src/monitor.c
> +++ b/src/monitor.c
> @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
>          if (!nft_output_echo(&echo_monh.ctx->nft->output))
>                  return MNL_CB_OK;
>   
> -       if (nft_output_json(&ctx->nft->output))
> -               return json_events_cb(nlh, &echo_monh);
> +       if (nft_output_json(&ctx->nft->output)) {
> +               if (ctx->nft->json_root)
> +                       return json_events_cb(nlh, &echo_monh);
> +               echo_monh.format = NFTNL_OUTPUT_JSON;
> +       }
>   
>          return netlink_events_cb(nlh, &echo_monh);
>   }
> 
> At a first glance, this seems to work just fine.
> 
> Cheers, Phil

This does not output anything on my machine. This is because json_echo 
is not initialized before netlink_echo_callback.

The mock monitor is responsible of appending the appropriate json cmd 
object to nft->json_echo, so we need it initialized when the case is as 
we have discussed before, native input and echo+json.

In addition netlink_echo_callback is called each time we receive 
something from the mnl socket. So checking if nft->json_echo is already 
initialized is necessary too, if not checked only the last response is 
shown, and for each past response that means a lost json_t reference to 
an array of cmd objs for that given response.

Regards.
Phil Sutter Aug. 4, 2020, 1:14 p.m. UTC | #7
Hi,

On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> On 4/8/20 14:37, Phil Sutter wrote:
> > Why not just:
> > 
> > --- a/src/monitor.c
> > +++ b/src/monitor.c
> > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> >          if (!nft_output_echo(&echo_monh.ctx->nft->output))
> >                  return MNL_CB_OK;
> >   
> > -       if (nft_output_json(&ctx->nft->output))
> > -               return json_events_cb(nlh, &echo_monh);
> > +       if (nft_output_json(&ctx->nft->output)) {
> > +               if (ctx->nft->json_root)
> > +                       return json_events_cb(nlh, &echo_monh);
> > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > +       }
> >   
> >          return netlink_events_cb(nlh, &echo_monh);
> >   }
> > 
> > At a first glance, this seems to work just fine.
> > 
> > Cheers, Phil
> 
> This does not output anything on my machine. This is because json_echo 
> is not initialized before netlink_echo_callback.

Please try my diff above on upstream's master without your changes. In
the tree I did above changes, no symbol named 'json_echo' exists.

Cheers, Phil
Jose M. Guisado Aug. 4, 2020, 1:44 p.m. UTC | #8
Hi Phil.

On 4/8/20 15:14, Phil Sutter wrote:
> Hi,
> 
> On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
>> On 4/8/20 14:37, Phil Sutter wrote:
>>> Why not just:
>>>
>>> --- a/src/monitor.c
>>> +++ b/src/monitor.c
>>> @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
>>>           if (!nft_output_echo(&echo_monh.ctx->nft->output))
>>>                   return MNL_CB_OK;
>>>    
>>> -       if (nft_output_json(&ctx->nft->output))
>>> -               return json_events_cb(nlh, &echo_monh);
>>> +       if (nft_output_json(&ctx->nft->output)) {
>>> +               if (ctx->nft->json_root)
>>> +                       return json_events_cb(nlh, &echo_monh);
>>> +               echo_monh.format = NFTNL_OUTPUT_JSON;
>>> +       }
>>>    
>>>           return netlink_events_cb(nlh, &echo_monh);
>>>    }
>>>
>>> At a first glance, this seems to work just fine.
>>>
>>> Cheers, Phil
>>
>> This does not output anything on my machine. This is because json_echo
>> is not initialized before netlink_echo_callback.
> 
> Please try my diff above on upstream's master without your changes. In
> the tree I did above changes, no symbol named 'json_echo' exists.
> 
> Cheers, Phil

Just tested it, it works great on my machine. As it outputs the same 
that would a running nft monitor.

I'm imagining this is preferred if there's no need having the json 
commands in the output be wrapped inside list of a single json object 
with its metainfo. That's the main difference with your patch.

Regards!
Pablo Neira Ayuso Aug. 4, 2020, 2:04 p.m. UTC | #9
On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> Hi Phil.
> 
> On 4/8/20 15:14, Phil Sutter wrote:
> > Hi,
> > 
> > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > Why not just:
> > > > 
> > > > --- a/src/monitor.c
> > > > +++ b/src/monitor.c
> > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > >                   return MNL_CB_OK;
> > > > -       if (nft_output_json(&ctx->nft->output))
> > > > -               return json_events_cb(nlh, &echo_monh);
> > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > +               if (ctx->nft->json_root)
> > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > +       }
> > > >           return netlink_events_cb(nlh, &echo_monh);
> > > >    }
> > > > 
> > > > At a first glance, this seems to work just fine.
> > > > 
> > > > Cheers, Phil
> > > 
> > > This does not output anything on my machine. This is because json_echo
> > > is not initialized before netlink_echo_callback.
> > 
> > Please try my diff above on upstream's master without your changes. In
> > the tree I did above changes, no symbol named 'json_echo' exists.
> > 
> > Cheers, Phil
> 
> Just tested it, it works great on my machine. As it outputs the same that
> would a running nft monitor.
> 
> I'm imagining this is preferred if there's no need having the json commands
> in the output be wrapped inside list of a single json object with its
> metainfo. That's the main difference with your patch.

If it's not wrapped by the top-level nftables root then this is
unparseable.

I think your changes for the monitor are still needed, and we'll
consolidate this code sooner or later once the JSON API is fixed.

Thanks.
Pablo Neira Ayuso Aug. 4, 2020, 2:17 p.m. UTC | #10
On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > Hi Phil.
> > 
> > On 4/8/20 15:14, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > Why not just:
> > > > > 
> > > > > --- a/src/monitor.c
> > > > > +++ b/src/monitor.c
> > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > >                   return MNL_CB_OK;
> > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > +               if (ctx->nft->json_root)
> > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > +       }
> > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > >    }
> > > > > 
> > > > > At a first glance, this seems to work just fine.
> > > > > 
> > > > > Cheers, Phil
> > > > 
> > > > This does not output anything on my machine. This is because json_echo
> > > > is not initialized before netlink_echo_callback.
> > > 
> > > Please try my diff above on upstream's master without your changes. In
> > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > 
> > > Cheers, Phil
> > 
> > Just tested it, it works great on my machine. As it outputs the same that
> > would a running nft monitor.
> > 
> > I'm imagining this is preferred if there's no need having the json commands
> > in the output be wrapped inside list of a single json object with its
> > metainfo. That's the main difference with your patch.
> 
> If it's not wrapped by the top-level nftables root then this is
> unparseable.
> 
> I think your changes for the monitor are still needed, and we'll
> consolidate this code sooner or later once the JSON API is fixed.

s/fixed/improved :-)
Phil Sutter Aug. 4, 2020, 2:20 p.m. UTC | #11
Hi,

On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > Hi Phil.
> > 
> > On 4/8/20 15:14, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > Why not just:
> > > > > 
> > > > > --- a/src/monitor.c
> > > > > +++ b/src/monitor.c
> > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > >                   return MNL_CB_OK;
> > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > +               if (ctx->nft->json_root)
> > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > +       }
> > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > >    }
> > > > > 
> > > > > At a first glance, this seems to work just fine.
> > > > > 
> > > > > Cheers, Phil
> > > > 
> > > > This does not output anything on my machine. This is because json_echo
> > > > is not initialized before netlink_echo_callback.
> > > 
> > > Please try my diff above on upstream's master without your changes. In
> > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > 
> > > Cheers, Phil
> > 
> > Just tested it, it works great on my machine. As it outputs the same that
> > would a running nft monitor.

Thanks for validating.

> > I'm imagining this is preferred if there's no need having the json commands
> > in the output be wrapped inside list of a single json object with its
> > metainfo. That's the main difference with your patch.

Yes, 'nft -j monitor' output has always been like this. Given that
monitor potentially runs for a while and picks up multiple distinct
ruleset changes, I wonder how it *should* behave.

> If it's not wrapped by the top-level nftables root then this is
> unparseable.

We could change monitor code to add the wrapping "nftables" object to
every line printed:

--- a/src/json.c
+++ b/src/json.c
@@ -1857,7 +1857,8 @@ int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
 static void monitor_print_json(struct netlink_mon_handler *monh,
                               const char *cmd, json_t *obj)
 {
-       obj = json_pack("{s:o}", cmd, obj);
+       obj = json_pack("{s:[o, {s:o}]}", "nftables",
+                       generate_json_metainfo(), cmd, obj);
        json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
        json_decref(obj);
 }

Cheers, Phil
Jose M. Guisado Aug. 4, 2020, 3:47 p.m. UTC | #12
On 4/8/20 16:20, Phil Sutter wrote:
> Yes, 'nft -j monitor' output has always been like this. Given that
> monitor potentially runs for a while and picks up multiple distinct
> ruleset changes, I wonder how it *should* behave.
> 
>> If it's not wrapped by the top-level nftables root then this is
>> unparseable.
> > We could change monitor code to add the wrapping "nftables" object to
> every line printed:
> 
> --- a/src/json.c
> +++ b/src/json.c
> @@ -1857,7 +1857,8 @@ int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
>   static void monitor_print_json(struct netlink_mon_handler *monh,
>                                 const char *cmd, json_t *obj)
>   {
> -       obj = json_pack("{s:o}", cmd, obj);
> +       obj = json_pack("{s:[o, {s:o}]}", "nftables",
> +                       generate_json_metainfo(), cmd, obj);
>          json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
>          json_decref(obj);
>   }
> 
> Cheers, Phil
>
This would work on a line by line basis.

After giving another read to COMMAND OBJECTS section of 
libnftables-json(5) the only thing that comes to mind is that a line by 
line basis of JSON command objects would not take advantage of batching. 
If I'm not mistaken, each list of cmds is encapsulated inside the 
{nftables : ...} json object and it is then tried to be sent to netlink 
to be batched.

In addition, the output as a whole could not be parseable , only a 
single "nftables" object is expected when nft input is json.

My previous comments assume whole output of echo is expected to be 
admissible as input in nft for reproducibility, but I don't know if that 
is the case.


Regards.
Pablo Neira Ayuso Aug. 4, 2020, 7:10 p.m. UTC | #13
On Tue, Aug 04, 2020 at 04:20:27PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > > Hi Phil.
> > > 
> > > On 4/8/20 15:14, Phil Sutter wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > > Why not just:
> > > > > > 
> > > > > > --- a/src/monitor.c
> > > > > > +++ b/src/monitor.c
> > > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > > >                   return MNL_CB_OK;
> > > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > > +               if (ctx->nft->json_root)
> > > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > > +       }
> > > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > > >    }
> > > > > > 
> > > > > > At a first glance, this seems to work just fine.
> > > > > > 
> > > > > > Cheers, Phil
> > > > > 
> > > > > This does not output anything on my machine. This is because json_echo
> > > > > is not initialized before netlink_echo_callback.
> > > > 
> > > > Please try my diff above on upstream's master without your changes. In
> > > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > > 
> > > > Cheers, Phil
> > > 
> > > Just tested it, it works great on my machine. As it outputs the same that
> > > would a running nft monitor.
> 
> Thanks for validating.
> 
> > > I'm imagining this is preferred if there's no need having the json commands
> > > in the output be wrapped inside list of a single json object with its
> > > metainfo. That's the main difference with your patch.
> 
> Yes, 'nft -j monitor' output has always been like this. Given that
> monitor potentially runs for a while and picks up multiple distinct
> ruleset changes, I wonder how it *should* behave.
> 
> > If it's not wrapped by the top-level nftables root then this is
> > unparseable.
> 
> We could change monitor code to add the wrapping "nftables" object to
> every line printed:
> 
> --- a/src/json.c
> +++ b/src/json.c
> @@ -1857,7 +1857,8 @@ int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
>  static void monitor_print_json(struct netlink_mon_handler *monh,
>                                const char *cmd, json_t *obj)
>  {
> -       obj = json_pack("{s:o}", cmd, obj);
> +       obj = json_pack("{s:[o, {s:o}]}", "nftables",
> +                       generate_json_metainfo(), cmd, obj);
>         json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
>         json_decref(obj);
>  }

This is probably fine for the monitor + json.

However, nft --echo --json should provide a consistent output whether
the input comes from a json file or not.
Phil Sutter Aug. 5, 2020, 9:31 a.m. UTC | #14
Hi,

On Tue, Aug 04, 2020 at 09:10:57PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 04, 2020 at 04:20:27PM +0200, Phil Sutter wrote:
> > On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > > > On 4/8/20 15:14, Phil Sutter wrote:
> > > > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > > > Why not just:
> > > > > > > 
> > > > > > > --- a/src/monitor.c
> > > > > > > +++ b/src/monitor.c
> > > > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > > > >                   return MNL_CB_OK;
> > > > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > > > +               if (ctx->nft->json_root)
> > > > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > > > +       }
> > > > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > > > >    }
> > > > > > > 
> > > > > > > At a first glance, this seems to work just fine.
> > > > > > > 
> > > > > > > Cheers, Phil
> > > > > > 
> > > > > > This does not output anything on my machine. This is because json_echo
> > > > > > is not initialized before netlink_echo_callback.
> > > > > 
> > > > > Please try my diff above on upstream's master without your changes. In
> > > > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > > > 
> > > > > Cheers, Phil
> > > > 
> > > > Just tested it, it works great on my machine. As it outputs the same that
> > > > would a running nft monitor.
> > 
> > Thanks for validating.
> > 
> > > > I'm imagining this is preferred if there's no need having the json commands
> > > > in the output be wrapped inside list of a single json object with its
> > > > metainfo. That's the main difference with your patch.
> > 
> > Yes, 'nft -j monitor' output has always been like this. Given that
> > monitor potentially runs for a while and picks up multiple distinct
> > ruleset changes, I wonder how it *should* behave.
> > 
> > > If it's not wrapped by the top-level nftables root then this is
> > > unparseable.
> > 
> > We could change monitor code to add the wrapping "nftables" object to
> > every line printed:
> > 
> > --- a/src/json.c
> > +++ b/src/json.c
> > @@ -1857,7 +1857,8 @@ int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
> >  static void monitor_print_json(struct netlink_mon_handler *monh,
> >                                const char *cmd, json_t *obj)
> >  {
> > -       obj = json_pack("{s:o}", cmd, obj);
> > +       obj = json_pack("{s:[o, {s:o}]}", "nftables",
> > +                       generate_json_metainfo(), cmd, obj);
> >         json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
> >         json_decref(obj);
> >  }
> 
> This is probably fine for the monitor + json.
> 
> However, nft --echo --json should provide a consistent output whether
> the input comes from a json file or not.

I get your point, but honestly think this is not a straightforward
question to answer: You qualify consistent output based on JSON input,
which simply doesn't exist if input is standard syntax. Saying the JSON
output you get from echo mode is inconsistent because an equivalent JSON
input would look differently is rather a matter of definition.

Look at non-JSON echo behaviour:

# nft -e 'add table t2; add chain t2 c'
add table ip t2
add chain ip t2 c

# nft -e -f - <<EOF
heredoc> table t3 {
heredoc>   chain c {
heredoc>   }
heredoc> }
heredoc> EOF
add table ip t3
add chain ip t3 c

I'd say this rather resembles how my simplistic patch makes JSON-echo
behave when reacting to non-JSON input than what Jose's patch is trying
to achieve.

Jose, what's your use-case anyway? Do you depend on being able to insert
standard syntax and get JSON back for some reason?

Cheers, Phil
Pablo Neira Ayuso Aug. 5, 2020, 9:45 a.m. UTC | #15
On Wed, Aug 05, 2020 at 11:31:50AM +0200, Phil Sutter wrote:
> Hi,
> 
> On Tue, Aug 04, 2020 at 09:10:57PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 04, 2020 at 04:20:27PM +0200, Phil Sutter wrote:
> > > On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > > > > On 4/8/20 15:14, Phil Sutter wrote:
> > > > > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > > > > Why not just:
> > > > > > > > 
> > > > > > > > --- a/src/monitor.c
> > > > > > > > +++ b/src/monitor.c
> > > > > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > > > > >                   return MNL_CB_OK;
> > > > > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > > > > +               if (ctx->nft->json_root)
> > > > > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > > > > +       }
> > > > > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > > > > >    }
> > > > > > > > 
> > > > > > > > At a first glance, this seems to work just fine.
> > > > > > > > 
> > > > > > > > Cheers, Phil
> > > > > > > 
> > > > > > > This does not output anything on my machine. This is because json_echo
> > > > > > > is not initialized before netlink_echo_callback.
> > > > > > 
> > > > > > Please try my diff above on upstream's master without your changes. In
> > > > > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > > > > 
> > > > > > Cheers, Phil
> > > > > 
> > > > > Just tested it, it works great on my machine. As it outputs the same that
> > > > > would a running nft monitor.
> > > 
> > > Thanks for validating.
> > > 
> > > > > I'm imagining this is preferred if there's no need having the json commands
> > > > > in the output be wrapped inside list of a single json object with its
> > > > > metainfo. That's the main difference with your patch.
> > > 
> > > Yes, 'nft -j monitor' output has always been like this. Given that
> > > monitor potentially runs for a while and picks up multiple distinct
> > > ruleset changes, I wonder how it *should* behave.
> > > 
> > > > If it's not wrapped by the top-level nftables root then this is
> > > > unparseable.
> > > 
> > > We could change monitor code to add the wrapping "nftables" object to
> > > every line printed:
> > > 
> > > --- a/src/json.c
> > > +++ b/src/json.c
> > > @@ -1857,7 +1857,8 @@ int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
> > >  static void monitor_print_json(struct netlink_mon_handler *monh,
> > >                                const char *cmd, json_t *obj)
> > >  {
> > > -       obj = json_pack("{s:o}", cmd, obj);
> > > +       obj = json_pack("{s:[o, {s:o}]}", "nftables",
> > > +                       generate_json_metainfo(), cmd, obj);
> > >         json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
> > >         json_decref(obj);
> > >  }
> > 
> > This is probably fine for the monitor + json.
> > 
> > However, nft --echo --json should provide a consistent output whether
> > the input comes from a json file or not.
> 
> I get your point, but honestly think this is not a straightforward
> question to answer: You qualify consistent output based on JSON input,
> which simply doesn't exist if input is standard syntax. Saying the JSON
> output you get from echo mode is inconsistent because an equivalent JSON
> input would look differently is rather a matter of definition.

You get an input json file, then the output looks like this:

{"nftables": [{"metainfo": {"json_schema_version": 1}}, {"add":
{"table": {"family": "inet", "name": "firewalld"}}}, {"add": {"table":
{"family": "ip", "name": "firewalld"}}}, {"add": {"table": {"family":
"ip6", "name": "firewalld"}}}}

but if your input is not a json file, then this will look like this:

{"nftables": [{"metainfo": {"json_schema_version": 1}}, {"add":
{"table": {"family": "inet", "name": "firewalld"}}}, {"add": {"table":
{"family": "ip", "name": "firewalld"}}}}
{"nftables": [{"metainfo": {"json_schema_version": 1}}, {"add":
{"table": {"family": "inet", "name": "firewalld"}}}, {"add": {"table": {"family":
"ip6", "name": "firewalld"}}}}

I'm also assuming all what is wrapped by the top-level "nftables" root
in JSON is a transaction?

> Look at non-JSON echo behaviour:
> 
> # nft -e 'add table t2; add chain t2 c'
> add table ip t2
> add chain ip t2 c
> 
> # nft -e -f - <<EOF
> heredoc> table t3 {
> heredoc>   chain c {
> heredoc>   }
> heredoc> }
> heredoc> EOF
> add table ip t3
> add chain ip t3 c
> 
> I'd say this rather resembles how my simplistic patch makes JSON-echo
> behave when reacting to non-JSON input than what Jose's patch is trying
> to achieve.

Probably the --echo code can be made smarter to display the output
using the flat or nested syntax depending on the input.
Phil Sutter Aug. 6, 2020, 7:28 a.m. UTC | #16
Hi,

On Wed, Aug 05, 2020 at 11:45:21AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 05, 2020 at 11:31:50AM +0200, Phil Sutter wrote:
> > On Tue, Aug 04, 2020 at 09:10:57PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Aug 04, 2020 at 04:20:27PM +0200, Phil Sutter wrote:
> > > > On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > > > > > On 4/8/20 15:14, Phil Sutter wrote:
> > > > > > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > > > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > > > > > Why not just:
> > > > > > > > > 
> > > > > > > > > --- a/src/monitor.c
> > > > > > > > > +++ b/src/monitor.c
> > > > > > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > > > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > > > > > >                   return MNL_CB_OK;
> > > > > > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > > > > > +               if (ctx->nft->json_root)
> > > > > > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > > > > > +       }
> > > > > > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > > > > > >    }
> > > > > > > > > 
> > > > > > > > > At a first glance, this seems to work just fine.
> > > > > > > > > 
> > > > > > > > > Cheers, Phil
> > > > > > > > 
> > > > > > > > This does not output anything on my machine. This is because json_echo
> > > > > > > > is not initialized before netlink_echo_callback.
> > > > > > > 
> > > > > > > Please try my diff above on upstream's master without your changes. In
> > > > > > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > > > > > 
> > > > > > > Cheers, Phil
> > > > > > 
> > > > > > Just tested it, it works great on my machine. As it outputs the same that
> > > > > > would a running nft monitor.
> > > > 
> > > > Thanks for validating.
> > > > 
> > > > > > I'm imagining this is preferred if there's no need having the json commands
> > > > > > in the output be wrapped inside list of a single json object with its
> > > > > > metainfo. That's the main difference with your patch.
> > > > 
> > > > Yes, 'nft -j monitor' output has always been like this. Given that
> > > > monitor potentially runs for a while and picks up multiple distinct
> > > > ruleset changes, I wonder how it *should* behave.
> > > > 
> > > > > If it's not wrapped by the top-level nftables root then this is
> > > > > unparseable.
> > > > 
> > > > We could change monitor code to add the wrapping "nftables" object to
> > > > every line printed:
> > > > 
> > > > --- a/src/json.c
> > > > +++ b/src/json.c
> > > > @@ -1857,7 +1857,8 @@ int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
> > > >  static void monitor_print_json(struct netlink_mon_handler *monh,
> > > >                                const char *cmd, json_t *obj)
> > > >  {
> > > > -       obj = json_pack("{s:o}", cmd, obj);
> > > > +       obj = json_pack("{s:[o, {s:o}]}", "nftables",
> > > > +                       generate_json_metainfo(), cmd, obj);
> > > >         json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
> > > >         json_decref(obj);
> > > >  }
> > > 
> > > This is probably fine for the monitor + json.
> > > 
> > > However, nft --echo --json should provide a consistent output whether
> > > the input comes from a json file or not.
> > 
> > I get your point, but honestly think this is not a straightforward
> > question to answer: You qualify consistent output based on JSON input,
> > which simply doesn't exist if input is standard syntax. Saying the JSON
> > output you get from echo mode is inconsistent because an equivalent JSON
> > input would look differently is rather a matter of definition.
> 
> You get an input json file, then the output looks like this:
> 
> {"nftables": [{"metainfo": {"json_schema_version": 1}}, {"add":
> {"table": {"family": "inet", "name": "firewalld"}}}, {"add": {"table":
> {"family": "ip", "name": "firewalld"}}}, {"add": {"table": {"family":
> "ip6", "name": "firewalld"}}}}
> 
> but if your input is not a json file, then this will look like this:
> 
> {"nftables": [{"metainfo": {"json_schema_version": 1}}, {"add":
> {"table": {"family": "inet", "name": "firewalld"}}}, {"add": {"table":
> {"family": "ip", "name": "firewalld"}}}}
> {"nftables": [{"metainfo": {"json_schema_version": 1}}, {"add":
> {"table": {"family": "inet", "name": "firewalld"}}}, {"add": {"table": {"family":
> "ip6", "name": "firewalld"}}}}

It is possible to make the JSON parser accept multiple nftables objects
in series and combine them into a single JSON tree. So the above would
parse correctly and turn into a single transaction. This does not break
compatibility, right now the parser will just reject input which
consists of more than a single object on top-level.

> I'm also assuming all what is wrapped by the top-level "nftables" root
> in JSON is a transaction?

Each call to nft_run_cmd_from_{buffer,filename}() results in a single
transaction.

> > Look at non-JSON echo behaviour:
> > 
> > # nft -e 'add table t2; add chain t2 c'
> > add table ip t2
> > add chain ip t2 c
> > 
> > # nft -e -f - <<EOF
> > heredoc> table t3 {
> > heredoc>   chain c {
> > heredoc>   }
> > heredoc> }
> > heredoc> EOF
> > add table ip t3
> > add chain ip t3 c
> > 
> > I'd say this rather resembles how my simplistic patch makes JSON-echo
> > behave when reacting to non-JSON input than what Jose's patch is trying
> > to achieve.
> 
> Probably the --echo code can be made smarter to display the output
> using the flat or nested syntax depending on the input.

What for? It is valid syntax.

Cheers, Phil
diff mbox series

Patch

diff --git a/include/nftables.h b/include/nftables.h
index 3556728d..9095ff3d 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -122,6 +122,7 @@  struct nft_ctx {
 	void			*scanner;
 	struct scope		*top_scope;
 	void			*json_root;
+	json_t			*json_echo;
 };
 
 enum nftables_exit_codes {
diff --git a/src/json.c b/src/json.c
index 888cb371..44b3b042 100644
--- a/src/json.c
+++ b/src/json.c
@@ -1857,9 +1857,16 @@  int do_command_list_json(struct netlink_ctx *ctx, struct cmd *cmd)
 static void monitor_print_json(struct netlink_mon_handler *monh,
 			       const char *cmd, json_t *obj)
 {
-	obj = json_pack("{s:o}", cmd, obj);
-	json_dumpf(obj, monh->ctx->nft->output.output_fp, 0);
-	json_decref(obj);
+	struct nft_ctx *nft = monh->ctx->nft;
+
+	if (nft_output_echo(&nft->output) && !nft->json_root) {
+		obj = json_pack("{s:o}", cmd, obj);
+		json_array_append_new(nft->json_echo, obj);
+	} else {
+		obj = json_pack("{s:o}", cmd, obj);
+		json_dumpf(obj, nft->output.output_fp, 0);
+		json_decref(obj);
+	}
 }
 
 void monitor_print_table_json(struct netlink_mon_handler *monh,
diff --git a/src/monitor.c b/src/monitor.c
index 3872ebcf..868e31b5 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -221,12 +221,14 @@  static int netlink_events_table_cb(const struct nlmsghdr *nlh, int type,
 		if (nft_output_handle(&monh->ctx->nft->output))
 			nft_mon_print(monh, " # handle %" PRIu64 "",
 				      t->handle.handle.id);
+		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
 		monitor_print_table_json(monh, cmd, t);
+		if(!nft_output_echo(&monh->ctx->nft->output))
+			nft_mon_print(monh, "\n");
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	table_free(t);
 	nftnl_table_free(nlt);
 	return MNL_CB_OK;
@@ -258,12 +260,14 @@  static int netlink_events_chain_cb(const struct nlmsghdr *nlh, int type,
 				      c->handle.chain.name);
 			break;
 		}
+		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
 		monitor_print_chain_json(monh, cmd, c);
+		if(!nft_output_echo(&monh->ctx->nft->output))
+			nft_mon_print(monh, "\n");
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	chain_free(c);
 	nftnl_chain_free(nlc);
 	return MNL_CB_OK;
@@ -304,12 +308,14 @@  static int netlink_events_set_cb(const struct nlmsghdr *nlh, int type,
 				      set->handle.set.name);
 			break;
 		}
+		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
 		monitor_print_set_json(monh, cmd, set);
+		if(!nft_output_echo(&monh->ctx->nft->output))
+			nft_mon_print(monh, "\n");
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	set_free(set);
 out:
 	nftnl_set_free(nls);
@@ -441,6 +447,7 @@  static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 		nft_mon_print(monh, "%s element %s %s %s ",
 			      cmd, family2str(family), table, setname);
 		expr_print(dummyset->init, &monh->ctx->nft->output);
+		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
 		dummyset->handle.family = family;
@@ -450,9 +457,10 @@  static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 		/* prevent set_free() from trying to free those */
 		dummyset->handle.set.name = NULL;
 		dummyset->handle.table.name = NULL;
+		if(!nft_output_echo(&monh->ctx->nft->output))
+			nft_mon_print(monh, "\n");
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	set_free(dummyset);
 out:
 	nftnl_set_free(nls);
@@ -492,12 +500,14 @@  static int netlink_events_obj_cb(const struct nlmsghdr *nlh, int type,
 			       obj->handle.obj.name);
 			break;
 		}
+		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
 		monitor_print_obj_json(monh, cmd, obj);
+		if(!nft_output_echo(&monh->ctx->nft->output))
+			nft_mon_print(monh, "\n");
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	obj_free(obj);
 	nftnl_obj_free(nlo);
 	return MNL_CB_OK;
@@ -542,12 +552,14 @@  static int netlink_events_rule_cb(const struct nlmsghdr *nlh, int type,
 				      r->handle.handle.id);
 			break;
 		}
+		nft_mon_print(monh, "\n");
 		break;
 	case NFTNL_OUTPUT_JSON:
 		monitor_print_rule_json(monh, cmd, r);
+		if(!nft_output_echo(&monh->ctx->nft->output))
+			nft_mon_print(monh, "\n");
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	rule_free(r);
 	nftnl_rule_free(nlr);
 	return MNL_CB_OK;
@@ -912,6 +924,8 @@  int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
 {
 	struct netlink_cb_data *nl_cb_data = data;
 	struct netlink_ctx *ctx = nl_cb_data->nl_ctx;
+	struct nft_ctx *nft = ctx->nft;
+
 	struct netlink_mon_handler echo_monh = {
 		.format = NFTNL_OUTPUT_DEFAULT,
 		.ctx = ctx,
@@ -922,8 +936,15 @@  int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
 	if (!nft_output_echo(&echo_monh.ctx->nft->output))
 		return MNL_CB_OK;
 
-	if (nft_output_json(&ctx->nft->output))
-		return json_events_cb(nlh, &echo_monh);
+	if (nft_output_json(&nft->output)) {
+		if (!nft->json_root) {
+			nft->json_echo = json_array();
+			if (!nft->json_echo)
+				memory_allocation_error();
+			echo_monh.format = NFTNL_OUTPUT_JSON;
+		} else
+			return json_events_cb(nlh, &echo_monh);
+	}
 
 	return netlink_events_cb(nlh, &echo_monh);
 }
diff --git a/src/parser_json.c b/src/parser_json.c
index 59347168..ef33063d 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3884,11 +3884,21 @@  int json_events_cb(const struct nlmsghdr *nlh, struct netlink_mon_handler *monh)
 
 void json_print_echo(struct nft_ctx *ctx)
 {
-	if (!ctx->json_root)
-		return;
-
-	json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
-	json_cmd_assoc_free();
-	json_decref(ctx->json_root);
-	ctx->json_root = NULL;
+	if (!ctx->json_root) {
+		if (!ctx->json_echo)
+			return;
+		else {
+			ctx->json_echo = json_pack("{s:o}", "nftables", ctx->json_echo);
+			json_dumpf(ctx->json_echo, ctx->output.output_fp, JSON_PRESERVE_ORDER);
+			json_decref(ctx->json_echo);
+			ctx->json_echo = NULL;
+			fprintf(ctx->output.output_fp, "\n");
+			fflush(ctx->output.output_fp);
+		}
+	} else {
+		json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
+		json_cmd_assoc_free();
+		json_decref(ctx->json_root);
+		ctx->json_root = NULL;
+	}
 }