Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Jul 31, 2020 at 11:22:12AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jul 31, 2020 at 02:00:22AM +0200, Jose M. Guisado Gomez wrote:
> > > diff --git a/src/parser_json.c b/src/parser_json.c
> > > index 59347168..237b6f3e 100644
> > > --- a/src/parser_json.c
> > > +++ b/src/parser_json.c
> > > @@ -3884,11 +3884,15 @@ 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)
> > > +	if (!ctx->json_echo)
> > >		return;
> 
> Why not reuse json_root?

Now that json_root is released from nft_parse_json_buffer() that is
possible, yes.

However, it is only possible to reuse ctx->json_root if the
ctx->json_root is released right after the parsing.

Otherwise the semantics of ctx->json_root starts getting confusing.

> > > -	json_dumpf(ctx->json_root, ctx->output.output_fp, JSON_PRESERVE_ORDER);
> > > +	ctx->json_echo = json_pack("{s:o}", "nftables", ctx->json_echo);
> > > +	json_dumpf(ctx->json_echo, ctx->output.output_fp, JSON_PRESERVE_ORDER);
> > > +	printf("\n");
> > >	json_cmd_assoc_free();
> > > -	json_decref(ctx->json_root);
> > > -	ctx->json_root = NULL;
> > > +	if (ctx->json_echo) {
> > > +		json_decref(ctx->json_echo);
> > > +		ctx->json_echo = NULL;
> > > +	}
> > 
> > I think json_print_echo() should look like this - note I replaced the
> > printf("\n"); by fprintf. Also remove the if (ctx->json_echo) branch.
> > 
> > void json_print_echo(struct nft_ctx *ctx)
> > {
> > 	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);
> > }
> > 
> > Please, include this update. I'm also attaching a patch that you can
> > squash to your v3 patch.
> > 
> > @Phil, I think the entire assoc code can just go away? Maybe you can also
> > run firewalld tests to make sure v3 works fine?  IIRC that is a heavy user
> > of --echo and --json.
> 
> Keeping JSON input in place and merely updating it with handles
> retrieved from kernel was a deliberate choice to make sure scripts can
> rely upon echo output to not differ from input unexpectedly.

Hm, this is not trusting what the kernel is sending to us via echo.
And this approach differs from what it is done for --echo with native
nft syntax.

> Given that output often deviates from input due to rule optimizing
> or loss of information, I'd say this code change will break that
> promise. Can't we enable JSON echo with non-JSON input while
> upholding it?

I would prefer to remove this code. What is your concern?