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?
> > - 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. 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?
Cheers, Phil
