| Message ID | 20200804103846.58872-1-guigom@riseup.net |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,v4] src: enable json echo output when reading native syntax | expand |
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.
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.
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; }
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
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>
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.
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
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!
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.
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 :-)
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
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.
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.
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
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.
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 --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; + } }
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(-)