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

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

[Jose M. Guisado Gomez]

This patch fixes a bug in which nft did not print any output when
specifying --echo and --json and reading nftables syntax.
This was because struct nft_ctx member json_root is not inizialized when
reading a json formatted file or buffer.

Create a json_echo member inside struct nft_ctx to build and store the json object
containing the command objects when --json and --echo are passed to nft.

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

Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
---
 include/nftables.h |  1 +
 src/json.c         | 13 ++++++++++---
 src/monitor.c      | 24 ++++++++++++++++--------
 src/parser_json.c  | 12 ++++++++----
 4 files changed, 35 insertions(+), 15 deletions(-)

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..ffe0e57d 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)) {
+		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, monh->ctx->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..27406906 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -221,12 +221,12 @@ 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);
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	table_free(t);
 	nftnl_table_free(nlt);
 	return MNL_CB_OK;
@@ -258,12 +258,12 @@ 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);
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	chain_free(c);
 	nftnl_chain_free(nlc);
 	return MNL_CB_OK;
@@ -304,12 +304,12 @@ 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);
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	set_free(set);
 out:
 	nftnl_set_free(nls);
@@ -441,6 +441,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;
@@ -452,7 +453,6 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 		dummyset->handle.table.name = NULL;
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	set_free(dummyset);
 out:
 	nftnl_set_free(nls);
@@ -492,12 +492,12 @@ 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);
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	obj_free(obj);
 	nftnl_obj_free(nlo);
 	return MNL_CB_OK;
@@ -542,12 +542,12 @@ 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);
 		break;
 	}
-	nft_mon_print(monh, "\n");
 	rule_free(r);
 	nftnl_rule_free(nlr);
 	return MNL_CB_OK;
@@ -912,6 +912,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 +924,14 @@ 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_echo) {
+			nft->json_echo = json_array();
+			if (!nft->json_echo)
+				memory_allocation_error();
+		}
+		echo_monh.format = NFTNL_OUTPUT_JSON;
+	}
 
 	return netlink_events_cb(nlh, &echo_monh);
 }
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;
 
-	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;
+	}
 }
-- 
2.27.0

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

[Jose M. Guisado Gomez]

Version 2 of the patch passes tests/monitor by fixing newline formatting
issues that were introduced in v1. Newline output inside monitor.c
netlink callbacks functions are now checked per type, nftables or json. 

nftables always outputs a newline when finishing a netlink_event
callback inside monitor.c

json on the other hand only outputs a newline when we no echo output
flag has been provided (this is, nft running in monitor mode, not using
a mock monitor like the --echo and --json case)

Jose M. Guisado Gomez (1):
  src: enable output with "nft --echo --json" and nftables syntax

 include/nftables.h |  1 +
 src/json.c         | 13 ++++++++++---
 src/monitor.c      | 36 ++++++++++++++++++++++++++++--------
 src/parser_json.c  | 12 ++++++++----
 4 files changed, 47 insertions(+), 15 deletions(-)

-- 
2.28.0

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

[Jose M. Guisado Gomez]

This patch fixes a bug in which nft did not print any output when
specifying --echo and --json and reading nftables syntax.
This was because struct nft_ctx member json_root was only inizialized when
reading a json formatted file or buffer.

Create a json_echo member inside struct nft_ctx to build and store the json object
containing the json command objects when --json and --echo are passed to nft.

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

Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
---
 include/nftables.h |  1 +
 src/json.c         | 13 ++++++++++---
 src/monitor.c      | 36 ++++++++++++++++++++++++++++--------
 src/parser_json.c  | 12 ++++++++----
 4 files changed, 47 insertions(+), 15 deletions(-)

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..ffe0e57d 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)) {
+		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, monh->ctx->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..ea901c53 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,14 @@ 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_echo) {
+			nft->json_echo = json_array();
+			if (!nft->json_echo)
+				memory_allocation_error();
+		}
+		echo_monh.format = NFTNL_OUTPUT_JSON;
+	}
 
 	return netlink_events_cb(nlh, &echo_monh);
 }
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;
 
-	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;
+	}
 }
-- 
2.28.0

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

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

Hi,

Cc'ing Phil.

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;
>
> -	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.

Thanks.

[-- Attachment #2: remove-json-assoc-code.patch --]
[-- Type: text/x-diff, Size: 4444 bytes --]

diff --git a/src/parser_json.c b/src/parser_json.c
index 237b6f3e1732..66539ef5c13b 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3645,46 +3645,6 @@ static int json_verify_metainfo(struct json_ctx *ctx, json_t *root)
 	return 0;
 }
 
-struct json_cmd_assoc {
-	struct json_cmd_assoc *next;
-	const struct cmd *cmd;
-	json_t *json;
-};
-
-static struct json_cmd_assoc *json_cmd_list = NULL;
-
-static void json_cmd_assoc_free(void)
-{
-	struct json_cmd_assoc *cur;
-
-	while (json_cmd_list) {
-		cur = json_cmd_list;
-		json_cmd_list = cur->next;
-		free(cur);
-	}
-}
-
-static void json_cmd_assoc_add(json_t *json, const struct cmd *cmd)
-{
-	struct json_cmd_assoc *new = xzalloc(sizeof *new);
-
-	new->next	= json_cmd_list;
-	new->json	= json;
-	new->cmd	= cmd;
-	json_cmd_list	= new;
-}
-
-static json_t *seqnum_to_json(const uint32_t seqnum)
-{
-	const struct json_cmd_assoc *cur;
-
-	for (cur = json_cmd_list; cur; cur = cur->next) {
-		if (cur->cmd->seqnum == seqnum)
-			return cur->json;
-	}
-	return NULL;
-}
-
 static int __json_parse(struct json_ctx *ctx)
 {
 	json_t *tmp, *value;
@@ -3729,9 +3689,6 @@ static int __json_parse(struct json_ctx *ctx)
 		list_add_tail(&cmd->list, &list);
 
 		list_splice_tail(&list, ctx->cmds);
-
-		if (nft_output_echo(&ctx->nft->output))
-			json_cmd_assoc_add(value, cmd);
 	}
 
 	return 0;
@@ -3757,10 +3714,9 @@ int nft_parse_json_buffer(struct nft_ctx *nft, const char *buf,
 
 	ret = __json_parse(&ctx);
 
-	if (!nft_output_echo(&nft->output)) {
-		json_decref(nft->json_root);
-		nft->json_root = NULL;
-	}
+	json_decref(nft->json_root);
+	nft->json_root = NULL;
+
 	return ret;
 }
 
@@ -3792,96 +3748,6 @@ int nft_parse_json_filename(struct nft_ctx *nft, const char *filename,
 	return ret;
 }
 
-static int json_echo_error(struct netlink_mon_handler *monh,
-			   const char *fmt, ...)
-{
-	struct error_record *erec;
-	va_list ap;
-
-	va_start(ap, fmt);
-	erec = erec_vcreate(EREC_ERROR, int_loc, fmt, ap);
-	va_end(ap);
-	erec_queue(erec, monh->ctx->msgs);
-
-	return MNL_CB_ERROR;
-}
-
-static uint64_t handle_from_nlmsg(const struct nlmsghdr *nlh)
-{
-	struct nftnl_table *nlt;
-	struct nftnl_chain *nlc;
-	struct nftnl_rule *nlr;
-	struct nftnl_set *nls;
-	struct nftnl_obj *nlo;
-	uint64_t handle = 0;
-	uint32_t flags;
-
-	switch (NFNL_MSG_TYPE(nlh->nlmsg_type)) {
-	case NFT_MSG_NEWTABLE:
-		nlt = netlink_table_alloc(nlh);
-		handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
-		nftnl_table_free(nlt);
-		break;
-	case NFT_MSG_NEWCHAIN:
-		nlc = netlink_chain_alloc(nlh);
-		handle = nftnl_chain_get_u64(nlc, NFTNL_CHAIN_HANDLE);
-		nftnl_chain_free(nlc);
-		break;
-	case NFT_MSG_NEWRULE:
-		nlr = netlink_rule_alloc(nlh);
-		handle = nftnl_rule_get_u64(nlr, NFTNL_RULE_HANDLE);
-		nftnl_rule_free(nlr);
-		break;
-	case NFT_MSG_NEWSET:
-		nls = netlink_set_alloc(nlh);
-		flags = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
-		if (!set_is_anonymous(flags))
-			handle = nftnl_set_get_u64(nls, NFTNL_SET_HANDLE);
-		nftnl_set_free(nls);
-		break;
-	case NFT_MSG_NEWOBJ:
-		nlo = netlink_obj_alloc(nlh);
-		handle = nftnl_obj_get_u64(nlo, NFTNL_OBJ_HANDLE);
-		nftnl_obj_free(nlo);
-		break;
-	}
-	return handle;
-}
-int json_events_cb(const struct nlmsghdr *nlh, struct netlink_mon_handler *monh)
-{
-	uint64_t handle = handle_from_nlmsg(nlh);
-	json_t *tmp, *json;
-	void *iter;
-
-	if (!handle)
-		return MNL_CB_OK;
-
-	json = seqnum_to_json(nlh->nlmsg_seq);
-	if (!json)
-		return MNL_CB_OK;
-
-	tmp = json_object_get(json, "add");
-	if (!tmp)
-		tmp = json_object_get(json, "insert");
-	if (!tmp)
-		/* assume loading JSON dump */
-		tmp = json;
-
-	iter = json_object_iter(tmp);
-	if (!iter) {
-		json_echo_error(monh, "Empty JSON object in cmd list\n");
-		return MNL_CB_OK;
-	}
-	json = json_object_iter_value(iter);
-	if (!json_is_object(json) || json_object_iter_next(tmp, iter)) {
-		json_echo_error(monh, "Malformed JSON object in cmd list\n");
-		return MNL_CB_OK;
-	}
-
-	json_object_set_new(json, "handle", json_integer(handle));
-	return MNL_CB_OK;
-}
-
 void json_print_echo(struct nft_ctx *ctx)
 {
 	if (!ctx->json_echo)
@@ -3890,7 +3756,6 @@ void json_print_echo(struct nft_ctx *ctx)
 	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();
 	if (ctx->json_echo) {
 		json_decref(ctx->json_echo);
 		ctx->json_echo = NULL;

[PATCH nft v3] src: enable output with "nft --echo --json" and nftables syntax

[Jose M. Guisado Gomez]

This patch fixes a bug in which nft did not print any output when
specifying --echo and --json and reading nftables syntax.
This was because struct nft_ctx member json_root was only inizialized when
reading a json formatted file or buffer.

Create a json_echo member inside struct nft_ctx to build and store the json object
containing the json command objects when --json and --echo are passed to nft.

Removes cmd json assoc code which is no longer used at this point.

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

Signed-off-by: Jose M. Guisado Gomez <guigom@riseup.net>
---
Adds proposed changes on top of v2

 include/nftables.h |   1 +
 src/json.c         |  13 +++-
 src/monitor.c      |  36 ++++++++---
 src/parser_json.c  | 152 +++------------------------------------------
 4 files changed, 49 insertions(+), 153 deletions(-)

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..ffe0e57d 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)) {
+		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, monh->ctx->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..ea901c53 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,14 @@ 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_echo) {
+			nft->json_echo = json_array();
+			if (!nft->json_echo)
+				memory_allocation_error();
+		}
+		echo_monh.format = NFTNL_OUTPUT_JSON;
+	}
 
 	return netlink_events_cb(nlh, &echo_monh);
 }
diff --git a/src/parser_json.c b/src/parser_json.c
index 59347168..2453ac05 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3645,46 +3645,6 @@ static int json_verify_metainfo(struct json_ctx *ctx, json_t *root)
 	return 0;
 }
 
-struct json_cmd_assoc {
-	struct json_cmd_assoc *next;
-	const struct cmd *cmd;
-	json_t *json;
-};
-
-static struct json_cmd_assoc *json_cmd_list = NULL;
-
-static void json_cmd_assoc_free(void)
-{
-	struct json_cmd_assoc *cur;
-
-	while (json_cmd_list) {
-		cur = json_cmd_list;
-		json_cmd_list = cur->next;
-		free(cur);
-	}
-}
-
-static void json_cmd_assoc_add(json_t *json, const struct cmd *cmd)
-{
-	struct json_cmd_assoc *new = xzalloc(sizeof *new);
-
-	new->next	= json_cmd_list;
-	new->json	= json;
-	new->cmd	= cmd;
-	json_cmd_list	= new;
-}
-
-static json_t *seqnum_to_json(const uint32_t seqnum)
-{
-	const struct json_cmd_assoc *cur;
-
-	for (cur = json_cmd_list; cur; cur = cur->next) {
-		if (cur->cmd->seqnum == seqnum)
-			return cur->json;
-	}
-	return NULL;
-}
-
 static int __json_parse(struct json_ctx *ctx)
 {
 	json_t *tmp, *value;
@@ -3729,9 +3689,6 @@ static int __json_parse(struct json_ctx *ctx)
 		list_add_tail(&cmd->list, &list);
 
 		list_splice_tail(&list, ctx->cmds);
-
-		if (nft_output_echo(&ctx->nft->output))
-			json_cmd_assoc_add(value, cmd);
 	}
 
 	return 0;
@@ -3757,10 +3714,9 @@ int nft_parse_json_buffer(struct nft_ctx *nft, const char *buf,
 
 	ret = __json_parse(&ctx);
 
-	if (!nft_output_echo(&nft->output)) {
-		json_decref(nft->json_root);
-		nft->json_root = NULL;
-	}
+	json_decref(nft->json_root);
+	nft->json_root = NULL;
+
 	return ret;
 }
 
@@ -3792,103 +3748,15 @@ int nft_parse_json_filename(struct nft_ctx *nft, const char *filename,
 	return ret;
 }
 
-static int json_echo_error(struct netlink_mon_handler *monh,
-			   const char *fmt, ...)
-{
-	struct error_record *erec;
-	va_list ap;
-
-	va_start(ap, fmt);
-	erec = erec_vcreate(EREC_ERROR, int_loc, fmt, ap);
-	va_end(ap);
-	erec_queue(erec, monh->ctx->msgs);
-
-	return MNL_CB_ERROR;
-}
-
-static uint64_t handle_from_nlmsg(const struct nlmsghdr *nlh)
-{
-	struct nftnl_table *nlt;
-	struct nftnl_chain *nlc;
-	struct nftnl_rule *nlr;
-	struct nftnl_set *nls;
-	struct nftnl_obj *nlo;
-	uint64_t handle = 0;
-	uint32_t flags;
-
-	switch (NFNL_MSG_TYPE(nlh->nlmsg_type)) {
-	case NFT_MSG_NEWTABLE:
-		nlt = netlink_table_alloc(nlh);
-		handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
-		nftnl_table_free(nlt);
-		break;
-	case NFT_MSG_NEWCHAIN:
-		nlc = netlink_chain_alloc(nlh);
-		handle = nftnl_chain_get_u64(nlc, NFTNL_CHAIN_HANDLE);
-		nftnl_chain_free(nlc);
-		break;
-	case NFT_MSG_NEWRULE:
-		nlr = netlink_rule_alloc(nlh);
-		handle = nftnl_rule_get_u64(nlr, NFTNL_RULE_HANDLE);
-		nftnl_rule_free(nlr);
-		break;
-	case NFT_MSG_NEWSET:
-		nls = netlink_set_alloc(nlh);
-		flags = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
-		if (!set_is_anonymous(flags))
-			handle = nftnl_set_get_u64(nls, NFTNL_SET_HANDLE);
-		nftnl_set_free(nls);
-		break;
-	case NFT_MSG_NEWOBJ:
-		nlo = netlink_obj_alloc(nlh);
-		handle = nftnl_obj_get_u64(nlo, NFTNL_OBJ_HANDLE);
-		nftnl_obj_free(nlo);
-		break;
-	}
-	return handle;
-}
-int json_events_cb(const struct nlmsghdr *nlh, struct netlink_mon_handler *monh)
-{
-	uint64_t handle = handle_from_nlmsg(nlh);
-	json_t *tmp, *json;
-	void *iter;
-
-	if (!handle)
-		return MNL_CB_OK;
-
-	json = seqnum_to_json(nlh->nlmsg_seq);
-	if (!json)
-		return MNL_CB_OK;
-
-	tmp = json_object_get(json, "add");
-	if (!tmp)
-		tmp = json_object_get(json, "insert");
-	if (!tmp)
-		/* assume loading JSON dump */
-		tmp = json;
-
-	iter = json_object_iter(tmp);
-	if (!iter) {
-		json_echo_error(monh, "Empty JSON object in cmd list\n");
-		return MNL_CB_OK;
-	}
-	json = json_object_iter_value(iter);
-	if (!json_is_object(json) || json_object_iter_next(tmp, iter)) {
-		json_echo_error(monh, "Malformed JSON object in cmd list\n");
-		return MNL_CB_OK;
-	}
-
-	json_object_set_new(json, "handle", json_integer(handle));
-	return MNL_CB_OK;
-}
-
 void json_print_echo(struct nft_ctx *ctx)
 {
-	if (!ctx->json_root)
+	if (!ctx->json_echo)
 		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;
+	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);
 }
-- 
2.27.0

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

[Phil Sutter]

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

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

[Pablo Neira Ayuso]

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?

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

[Phil Sutter]

On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> 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.

Hmm. Maybe I miss something, I just noticed that json_print_echo()
doesn't make use of json_root anymore.

[...]
> > > @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.

We agreed that regular nft output is made for humans, not machines.
Hence why we have libnftables and JSON API. Very simple scripts may get
by with regular output, grepping for 'handle <NUM>' and ignoring the
rest. When loading more than a single command, this quickly becomes
inconvenient.

> > 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?

The less predictable echo output behaves, the harder it is to write code
that makes use of it. The whole handle semantics assumes scripts will be
able to fetch handles from echo output. With output being identical to
input apart from added handles, scripts may just replace their input
with received output and will find everything where it is supposed to
be. The alternative means extracting handles from output and updating
the stored input based on array indexes. Basically libnftables does this
updating for users as a service, and the code in commit 50b5b71ebeee3
("parser_json: Rewrite echo support") is probably more efficient than
what any Python script could do.

Maybe I am over-estimating the importance of handle usability. The fact
that people have to use a rule's handle in order to remove it means to
me that they will either find a way to get the handle of the rule they
added or fall back into a write-only usage-pattern which means dropping
the whole chain/table/ruleset for each change. Basically what iptables
does internally.

I'm assuming scripts will work directly with the Python data structures
that are later passed to libnftables as JSON. If they want to change a
rule, e.g. add a statement, it is no use if other statements disappear
or new ones are added by the commit->retrieve action.

Maybe Eric can shed some light on how Firewalld uses echo mode and
whether my concerns are relevant or not.

Cheers, Phil

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

[Eric Garver]

On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > 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.
> 
> Hmm. Maybe I miss something, I just noticed that json_print_echo()
> doesn't make use of json_root anymore.
> 
> [...]
> > > > @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.
> 
> We agreed that regular nft output is made for humans, not machines.
> Hence why we have libnftables and JSON API. Very simple scripts may get
> by with regular output, grepping for 'handle <NUM>' and ignoring the
> rest. When loading more than a single command, this quickly becomes
> inconvenient.
> 
> > > 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?
> 
> The less predictable echo output behaves, the harder it is to write code
> that makes use of it. The whole handle semantics assumes scripts will be
> able to fetch handles from echo output. With output being identical to
> input apart from added handles, scripts may just replace their input
> with received output and will find everything where it is supposed to
> be. The alternative means extracting handles from output and updating
> the stored input based on array indexes. Basically libnftables does this
> updating for users as a service, and the code in commit 50b5b71ebeee3
> ("parser_json: Rewrite echo support") is probably more efficient than
> what any Python script could do.
> 
> Maybe I am over-estimating the importance of handle usability. The fact
> that people have to use a rule's handle in order to remove it means to
> me that they will either find a way to get the handle of the rule they
> added or fall back into a write-only usage-pattern which means dropping
> the whole chain/table/ruleset for each change. Basically what iptables
> does internally.

Agreed. This would be very bad for usability.

> I'm assuming scripts will work directly with the Python data structures
> that are later passed to libnftables as JSON. If they want to change a
> rule, e.g. add a statement, it is no use if other statements disappear
> or new ones are added by the commit->retrieve action.
> 
> Maybe Eric can shed some light on how Firewalld uses echo mode and
> whether my concerns are relevant or not.

How it stands today is exactly as you described above. firewalld relies
on the output (--echo) being in the same order as the input. At the
time, and I think still today, this was the _only_ way to reliably get
the rule handles. It's mostly due to the fact that input != output.

In the past we discussed allowing a user defined cookie/handle. This
would allow applications to perform in a write only manner. They would
not need to parse back the JSON since they already know the
cookie/handle. IMO, this would be ideal for firewalld's use case.

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

[Pablo Neira Ayuso]

On Fri, Jul 31, 2020 at 10:17:42AM -0400, Eric Garver wrote:
> On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
[...]
> > I'm assuming scripts will work directly with the Python data structures
> > that are later passed to libnftables as JSON. If they want to change a
> > rule, e.g. add a statement, it is no use if other statements disappear
> > or new ones are added by the commit->retrieve action.
> > 
> > Maybe Eric can shed some light on how Firewalld uses echo mode and
> > whether my concerns are relevant or not.
> 
> How it stands today is exactly as you described above. firewalld relies
> on the output (--echo) being in the same order as the input. At the
> time, and I think still today, this was the _only_ way to reliably get
> the rule handles. It's mostly due to the fact that input != output.
> 
> In the past we discussed allowing a user defined cookie/handle. This
> would allow applications to perform in a write only manner. They would
> not need to parse back the JSON since they already know the
> cookie/handle. IMO, this would be ideal for firewalld's use case.

The question is: Is this patch breaking anything in firewalld?

And if so, what is it breaking?

I don't find a good reason why maintaining two different codepaths for
--json --echo in the codebase is needed.

Thanks.

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

[Pablo Neira Ayuso]

On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
[...]
> The less predictable echo output behaves, the harder it is to write code
> that makes use of it.

What is it making the output less predictible? The kernel should
return an input that is equal to the output plus the handle. Other
than that, it's a bug.

This is also saving quite a bit of code and streamlining this further:

 4 files changed, 49 insertions(+), 153 deletions(-)

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

[Eric Garver]

On Fri, Jul 31, 2020 at 07:19:06PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jul 31, 2020 at 10:17:42AM -0400, Eric Garver wrote:
> > On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> [...]
> > > I'm assuming scripts will work directly with the Python data structures
> > > that are later passed to libnftables as JSON. If they want to change a
> > > rule, e.g. add a statement, it is no use if other statements disappear
> > > or new ones are added by the commit->retrieve action.
> > > 
> > > Maybe Eric can shed some light on how Firewalld uses echo mode and
> > > whether my concerns are relevant or not.
> > 
> > How it stands today is exactly as you described above. firewalld relies
> > on the output (--echo) being in the same order as the input. At the
> > time, and I think still today, this was the _only_ way to reliably get
> > the rule handles. It's mostly due to the fact that input != output.
> > 
> > In the past we discussed allowing a user defined cookie/handle. This
> > would allow applications to perform in a write only manner. They would
> > not need to parse back the JSON since they already know the
> > cookie/handle. IMO, this would be ideal for firewalld's use case.
> 
> The question is: Is this patch breaking anything in firewalld?

I tried v2 and v3. Neither break firewalld.

I think this is because firewalld only relies on the input and output
_order_ being identical. That is, the Nth input json element corresponds
to the Nth output json element.

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

[Eric Garver]

On Fri, Jul 31, 2020 at 02:36:33PM -0400, Eric Garver wrote:
> On Fri, Jul 31, 2020 at 07:19:06PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jul 31, 2020 at 10:17:42AM -0400, Eric Garver wrote:
> > > On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > > > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> > [...]
> > > > I'm assuming scripts will work directly with the Python data structures
> > > > that are later passed to libnftables as JSON. If they want to change a
> > > > rule, e.g. add a statement, it is no use if other statements disappear
> > > > or new ones are added by the commit->retrieve action.
> > > > 
> > > > Maybe Eric can shed some light on how Firewalld uses echo mode and
> > > > whether my concerns are relevant or not.
> > > 
> > > How it stands today is exactly as you described above. firewalld relies
> > > on the output (--echo) being in the same order as the input. At the
> > > time, and I think still today, this was the _only_ way to reliably get
> > > the rule handles. It's mostly due to the fact that input != output.
> > > 
> > > In the past we discussed allowing a user defined cookie/handle. This
> > > would allow applications to perform in a write only manner. They would
> > > not need to parse back the JSON since they already know the
> > > cookie/handle. IMO, this would be ideal for firewalld's use case.
> > 
> > The question is: Is this patch breaking anything in firewalld?
> 
> I tried v2 and v3. Neither break firewalld.

I rescind this statement - user error on my part. Both versions break
firewalld.

It looks like the reply is in a different order than the input. So
firewalld doesn't know where to find the rule handles. I'm trying to
come up with a minimal reproducer.

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

[Phil Sutter]

On Fri, Jul 31, 2020 at 07:30:28PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> [...]
> > The less predictable echo output behaves, the harder it is to write code
> > that makes use of it.
> 
> What is it making the output less predictible? The kernel should
> return an input that is equal to the output plus the handle. Other
> than that, it's a bug.

In tests/py, I see 330 lines explicitly stating the expected output as
it differs from the input ('grep "ok;" */*.t | wc -l'). Can we fix those
bugs first before we assume what the kernel returns is identical to user
input?

Say a script manages a rule (in JSON-equivalent) of:

| ip protocol tcp tcp dport '{ 22 - 23, 24 - 25}'

Both matches are elements in an array resembling the rule's "expr"
attribute. Nftables drops the first match, so if the script wants to
edit the ports in RHS of the second match, it won't find it anymore.
Also, the two port ranges are combined into a single one, so removing
one of the two ranges turns into a non-trivial problem.

Right now a script may apply its ruleset snippet and retrieve the
handles by:

| rc, ruleset, err = nftables.json_cmd(ruleset)

If the returned ruleset is not identical (apart from added attributes),
scripts will likely resort to a fire-n-forget type of usage pattern.

> This is also saving quite a bit of code and streamlining this further:
> 
>  4 files changed, 49 insertions(+), 153 deletions(-)

Proudly presenting reduced code size by dropping functionality is
cheating. Assume nobody needs the JSON interface, easily drop 5k LoC.

Cheers, Phil

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

[Pablo Neira Ayuso]

On Sat, Aug 01, 2020 at 02:02:13AM +0200, Phil Sutter wrote:
> On Fri, Jul 31, 2020 at 07:30:28PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> > [...]
> > > The less predictable echo output behaves, the harder it is to write code
> > > that makes use of it.
> > 
> > What is it making the output less predictible? The kernel should
> > return an input that is equal to the output plus the handle. Other
> > than that, it's a bug.
> 
> In tests/py, I see 330 lines explicitly stating the expected output as
> it differs from the input ('grep "ok;" */*.t | wc -l'). Can we fix those
> bugs first before we assume what the kernel returns is identical to user
> input?

Semantically speaking those lines are equivalent, it's just that input
and the output representation differ in some scenarios because the
decompilation routine differ in the way it builds the expressions.

BTW, why do you qualify this as a bug?

> Say a script manages a rule (in JSON-equivalent) of:
> 
> | ip protocol tcp tcp dport '{ 22 - 23, 24 - 25}'
> 
> Both matches are elements in an array resembling the rule's "expr"
> attribute. Nftables drops the first match, so if the script wants to
> edit the ports in RHS of the second match, it won't find it anymore.
> Also, the two port ranges are combined into a single one, so removing
> one of the two ranges turns into a non-trivial problem.
> 
> Right now a script may apply its ruleset snippet and retrieve the
> handles by:
> 
> | rc, ruleset, err = nftables.json_cmd(ruleset)
> 
> If the returned ruleset is not identical (apart from added attributes),
> scripts will likely resort to a fire-n-forget type of usage pattern.

You mean, the user in that JSON script is comparing the input and
output strings to find the rule handle?

If so, we should explore a better way to do this, eg. expose some user
defined identifier in JSON that userspace sets on when sending the
batch to identify the object coming back from the kernel.

> > This is also saving quite a bit of code and streamlining this further:
> > 
> >  4 files changed, 49 insertions(+), 153 deletions(-)
> 
> Proudly presenting reduced code size by dropping functionality is
> cheating. Assume nobody needs the JSON interface, easily drop 5k LoC.

The existing approach ignores the kernel echo netlink message almost
entirely, it only takes the handle.

We need an unified way to deal with --json --echo, whether the input
is native nft or json syntax.

If the problem is described in the question I made above, how will
users passing native nft syntax and requesing json output will
identify the rule? They cannot make string matching comparison in that
case since there is no input JSON representation.

Thanks.

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

[Phil Sutter]

Hi Pablo,

On Sat, Aug 01, 2020 at 09:27:30PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Aug 01, 2020 at 02:02:13AM +0200, Phil Sutter wrote:
> > On Fri, Jul 31, 2020 at 07:30:28PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 31, 2020 at 03:48:28PM +0200, Phil Sutter wrote:
> > > > On Fri, Jul 31, 2020 at 02:58:25PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Fri, Jul 31, 2020 at 02:33:42PM +0200, Phil Sutter wrote:
> > > [...]
> > > > The less predictable echo output behaves, the harder it is to write code
> > > > that makes use of it.
> > > 
> > > What is it making the output less predictible? The kernel should
> > > return an input that is equal to the output plus the handle. Other
> > > than that, it's a bug.
> > 
> > In tests/py, I see 330 lines explicitly stating the expected output as
> > it differs from the input ('grep "ok;" */*.t | wc -l'). Can we fix those
> > bugs first before we assume what the kernel returns is identical to user
> > input?
> 
> Semantically speaking those lines are equivalent, it's just that input
> and the output representation differ in some scenarios because the
> decompilation routine differ in the way it builds the expressions.

Obviously, yes, but irrelevant for this discussion. A script won't be
able to identify two different looking rules as identical because of
semantics.

> BTW, why do you qualify this as a bug?

I was just picking up your argument: Above, you wrote "Other than that,
it's a bug". I assume that in "return an input that is equal to the
output plus the handle", equal really means equal and not equivalent.

> > Say a script manages a rule (in JSON-equivalent) of:
> > 
> > | ip protocol tcp tcp dport '{ 22 - 23, 24 - 25}'
> > 
> > Both matches are elements in an array resembling the rule's "expr"
> > attribute. Nftables drops the first match, so if the script wants to
> > edit the ports in RHS of the second match, it won't find it anymore.
> > Also, the two port ranges are combined into a single one, so removing
> > one of the two ranges turns into a non-trivial problem.
> > 
> > Right now a script may apply its ruleset snippet and retrieve the
> > handles by:
> > 
> > | rc, ruleset, err = nftables.json_cmd(ruleset)
> > 
> > If the returned ruleset is not identical (apart from added attributes),
> > scripts will likely resort to a fire-n-forget type of usage pattern.
> 
> You mean, the user in that JSON script is comparing the input and
> output strings to find the rule handle?

I am assuming that a script that uses echo mode wants to update the
input ruleset (snippet) with handles for later reference to the added
rules. Other than copying output to input completely, it will have to
iterate through output and extract the handle properties, identifying
it's own rules based on current index. More or less what libnftables is
doing when updating JSON input with handles.

> If so, we should explore a better way to do this, eg. expose some user
> defined identifier in JSON that userspace sets on when sending the
> batch to identify the object coming back from the kernel.

Eric suggested to accept a "cookie" property with arbitrary value to
stay in place at least for echo output. He even suggested to accept this
as an alternative to the handle for rule referencing. The latter would
need kernel support, though.

> > > This is also saving quite a bit of code and streamlining this further:
> > > 
> > >  4 files changed, 49 insertions(+), 153 deletions(-)
> > 
> > Proudly presenting reduced code size by dropping functionality is
> > cheating. Assume nobody needs the JSON interface, easily drop 5k LoC.
> 
> The existing approach ignores the kernel echo netlink message almost
> entirely, it only takes the handle.

I know, I wrote the code.

> We need an unified way to deal with --json --echo, whether the input
> is native nft or json syntax.

We don't need, but seems we want. We have JSON output and JSON echo for
a while now and code for both is distinct. I fail to see why this was OK
but is no longer. From my perspective, Jose simply failed to see that
JSON output code should be used for JSON echo if input is not JSON.

I could come up with a patch implementing that if all this is merely
about the missing feature.

> If the problem is described in the question I made above, how will
> users passing native nft syntax and requesing json output will
> identify the rule? They cannot make string matching comparison in that
> case since there is no input JSON representation.

That is not a sensible use-case. For once, I would assume native syntax
to be used by humans, so if this is combined with JSON output, the goal
is translation. If input really comes from a script, it is likely not
retaining the input for later reuse but will take whatever it receives
back.

Cheers, Phil

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

[Jose M. Guisado]

On 3/8/20 14:52, Phil Sutter wrote:
> On Sat, Aug 01, 2020 at 09:27:30PM +0200, Pablo Neira Ayuso wrote:
>> We need an unified way to deal with --json --echo, whether the input
>> is native nft or json syntax.
> 
> We don't need, but seems we want. We have JSON output and JSON echo for
> a while now and code for both is distinct. I fail to see why this was OK
> but is no longer. From my perspective, Jose simply failed to see that
> JSON output code should be used for JSON echo if input is not JSON.

I will send a v4 for this patch honoring separate cases.

Only outputting JSON command objects when input has been native instead 
of JSON, for the latter the behavior is kept intact and no 
json_cmd_assoc is touched. I think that's what we are looking for right 
now. This shouldn't interfere with firewalld, right?.

Regards.

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

[Phil Sutter]

Hi,

On Tue, Aug 04, 2020 at 12:20:05PM +0200, Jose M. Guisado wrote:
> On 3/8/20 14:52, Phil Sutter wrote:
> > On Sat, Aug 01, 2020 at 09:27:30PM +0200, Pablo Neira Ayuso wrote:
> >> We need an unified way to deal with --json --echo, whether the input
> >> is native nft or json syntax.
> > 
> > We don't need, but seems we want. We have JSON output and JSON echo for
> > a while now and code for both is distinct. I fail to see why this was OK
> > but is no longer. From my perspective, Jose simply failed to see that
> > JSON output code should be used for JSON echo if input is not JSON.
> 
> I will send a v4 for this patch honoring separate cases.
> 
> Only outputting JSON command objects when input has been native instead 
> of JSON, for the latter the behavior is kept intact and no 
> json_cmd_assoc is touched. I think that's what we are looking for right 
> now. This shouldn't interfere with firewalld, right?.

Sounds good. Unless I'm mistaken, all that's needed is to skip the call
to json_events_cb() (including the return) in netlink_echo_callback() if
input was not JSON, i.e. nft_ctx->json_root is NULL.

Cheers, Phil

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

[Jose M. Guisado Gomez]

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

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;
+	}
 }
-- 
2.27.0

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

[Pablo Neira Ayuso]

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.

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

[Jose M. Guisado]

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.

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

[Pablo Neira Ayuso]

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;
        }

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

[Phil Sutter]

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

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

[Eric Garver]

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>

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

[Jose M. Guisado]

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.

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

[Phil Sutter]

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

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

[Jose M. Guisado]

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!

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

[Pablo Neira Ayuso]

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.

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

[Pablo Neira Ayuso]

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

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

[Phil Sutter]

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

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

[Jose M. Guisado]

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.

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

[Pablo Neira Ayuso]

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.

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

[Phil Sutter]

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

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

[Pablo Neira Ayuso]

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.

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

[Phil Sutter]

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