Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nft] src: enable output with "nft --echo --json" and nftables syntax
@ 2020-07-30 19:53 Jose M. Guisado Gomez
  2020-07-31  0:00 ` [PATCH nft v2 0/1] " Jose M. Guisado Gomez
  2020-07-31  0:00 ` [PATCH nft v2 1/1] " Jose M. Guisado Gomez
  0 siblings, 2 replies; 16+ messages in thread
From: Jose M. Guisado Gomez @ 2020-07-30 19:53 UTC (permalink / raw)
  To: netfilter-devel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH nft v2 0/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-30 19:53 [PATCH nft] src: enable output with "nft --echo --json" and nftables syntax Jose M. Guisado Gomez
@ 2020-07-31  0:00 ` Jose M. Guisado Gomez
  2020-07-31  0:00 ` [PATCH nft v2 1/1] " Jose M. Guisado Gomez
  1 sibling, 0 replies; 16+ messages in thread
From: Jose M. Guisado Gomez @ 2020-07-31  0:00 UTC (permalink / raw)
  To: netfilter-devel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-30 19:53 [PATCH nft] src: enable output with "nft --echo --json" and nftables syntax Jose M. Guisado Gomez
  2020-07-31  0:00 ` [PATCH nft v2 0/1] " Jose M. Guisado Gomez
@ 2020-07-31  0:00 ` Jose M. Guisado Gomez
  2020-07-31  9:22   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 16+ messages in thread
From: Jose M. Guisado Gomez @ 2020-07-31  0:00 UTC (permalink / raw)
  To: netfilter-devel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31  0:00 ` [PATCH nft v2 1/1] " Jose M. Guisado Gomez
@ 2020-07-31  9:22   ` Pablo Neira Ayuso
  2020-07-31 10:49     ` [PATCH nft v3] " Jose M. Guisado Gomez
  2020-07-31 12:33     ` [PATCH nft v2 1/1] " Phil Sutter
  0 siblings, 2 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2020-07-31  9:22 UTC (permalink / raw)
  To: Jose M. Guisado Gomez; +Cc: netfilter-devel, phil


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH nft v3] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31  9:22   ` Pablo Neira Ayuso
@ 2020-07-31 10:49     ` Jose M. Guisado Gomez
  2020-07-31 12:33     ` [PATCH nft v2 1/1] " Phil Sutter
  1 sibling, 0 replies; 16+ messages in thread
From: Jose M. Guisado Gomez @ 2020-07-31 10:49 UTC (permalink / raw)
  To: netfilter-devel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31  9:22   ` Pablo Neira Ayuso
  2020-07-31 10:49     ` [PATCH nft v3] " Jose M. Guisado Gomez
@ 2020-07-31 12:33     ` Phil Sutter
  2020-07-31 12:58       ` Pablo Neira Ayuso
  1 sibling, 1 reply; 16+ messages in thread
From: Phil Sutter @ 2020-07-31 12:33 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Jose M. Guisado Gomez, netfilter-devel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 12:33     ` [PATCH nft v2 1/1] " Phil Sutter
@ 2020-07-31 12:58       ` Pablo Neira Ayuso
  2020-07-31 13:48         ` Phil Sutter
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2020-07-31 12:58 UTC (permalink / raw)
  To: Phil Sutter, Jose M. Guisado Gomez, netfilter-devel

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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 12:58       ` Pablo Neira Ayuso
@ 2020-07-31 13:48         ` Phil Sutter
  2020-07-31 14:17           ` Eric Garver
  2020-07-31 17:30           ` Pablo Neira Ayuso
  0 siblings, 2 replies; 16+ messages in thread
From: Phil Sutter @ 2020-07-31 13:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Jose M. Guisado Gomez, netfilter-devel, Eric Garver

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 13:48         ` Phil Sutter
@ 2020-07-31 14:17           ` Eric Garver
  2020-07-31 17:19             ` Pablo Neira Ayuso
  2020-07-31 17:30           ` Pablo Neira Ayuso
  1 sibling, 1 reply; 16+ messages in thread
From: Eric Garver @ 2020-07-31 14:17 UTC (permalink / raw)
  To: Phil Sutter, Pablo Neira Ayuso, Jose M. Guisado Gomez,
	netfilter-devel, 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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 14:17           ` Eric Garver
@ 2020-07-31 17:19             ` Pablo Neira Ayuso
  2020-07-31 18:36               ` Eric Garver
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2020-07-31 17:19 UTC (permalink / raw)
  To: Eric Garver, Phil Sutter, Jose M. Guisado Gomez, netfilter-devel

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 13:48         ` Phil Sutter
  2020-07-31 14:17           ` Eric Garver
@ 2020-07-31 17:30           ` Pablo Neira Ayuso
  2020-08-01  0:02             ` Phil Sutter
  1 sibling, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2020-07-31 17:30 UTC (permalink / raw)
  To: Phil Sutter, Jose M. Guisado Gomez, netfilter-devel, 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:
[...]
> 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(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 17:19             ` Pablo Neira Ayuso
@ 2020-07-31 18:36               ` Eric Garver
  2020-07-31 20:14                 ` Eric Garver
  0 siblings, 1 reply; 16+ messages in thread
From: Eric Garver @ 2020-07-31 18:36 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Phil Sutter, Jose M. Guisado Gomez, netfilter-devel

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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 18:36               ` Eric Garver
@ 2020-07-31 20:14                 ` Eric Garver
  0 siblings, 0 replies; 16+ messages in thread
From: Eric Garver @ 2020-07-31 20:14 UTC (permalink / raw)
  To: Pablo Neira Ayuso, Phil Sutter, Jose M. Guisado Gomez, netfilter-devel

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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-07-31 17:30           ` Pablo Neira Ayuso
@ 2020-08-01  0:02             ` Phil Sutter
  2020-08-01 19:27               ` Pablo Neira Ayuso
  0 siblings, 1 reply; 16+ messages in thread
From: Phil Sutter @ 2020-08-01  0:02 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Jose M. Guisado Gomez, netfilter-devel, Eric Garver

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-08-01  0:02             ` Phil Sutter
@ 2020-08-01 19:27               ` Pablo Neira Ayuso
  2020-08-03 12:52                 ` Phil Sutter
  0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2020-08-01 19:27 UTC (permalink / raw)
  To: Phil Sutter, Jose M. Guisado Gomez, netfilter-devel, Eric Garver

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH nft v2 1/1] src: enable output with "nft --echo --json" and nftables syntax
  2020-08-01 19:27               ` Pablo Neira Ayuso
@ 2020-08-03 12:52                 ` Phil Sutter
  0 siblings, 0 replies; 16+ messages in thread
From: Phil Sutter @ 2020-08-03 12:52 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Jose M. Guisado Gomez, netfilter-devel, Eric Garver

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 19:53 [PATCH nft] src: enable output with "nft --echo --json" and nftables syntax Jose M. Guisado Gomez
2020-07-31  0:00 ` [PATCH nft v2 0/1] " Jose M. Guisado Gomez
2020-07-31  0:00 ` [PATCH nft v2 1/1] " Jose M. Guisado Gomez
2020-07-31  9:22   ` Pablo Neira Ayuso
2020-07-31 10:49     ` [PATCH nft v3] " Jose M. Guisado Gomez
2020-07-31 12:33     ` [PATCH nft v2 1/1] " Phil Sutter
2020-07-31 12:58       ` Pablo Neira Ayuso
2020-07-31 13:48         ` Phil Sutter
2020-07-31 14:17           ` Eric Garver
2020-07-31 17:19             ` Pablo Neira Ayuso
2020-07-31 18:36               ` Eric Garver
2020-07-31 20:14                 ` Eric Garver
2020-07-31 17:30           ` Pablo Neira Ayuso
2020-08-01  0:02             ` Phil Sutter
2020-08-01 19:27               ` Pablo Neira Ayuso
2020-08-03 12:52                 ` Phil Sutter

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git