[PATCH nft 0/3] flowtable json fixes

[PATCH nft 0/3] flowtable json fixes

[Florian Westphal]

json doesn't export the flow statement.
First patch resolves this, the other two patches resolve
import problems with flowtable json format.

Florian Westphal (3):
  json: add flow statement json export + parser
  parser_json: fix flowtable device datatype
  parser_json: permit empty device list

 include/json.h    |  2 ++
 src/ct.c          |  1 +
 src/json.c        |  7 ++++++
 src/parser_json.c | 54 ++++++++++++++++++++++++++++++++++++-----------
 4 files changed, 52 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH nft 1/3] json: add flow statement json export + parser

[Florian Westphal]

flow statement has no export, its shown as:
".. }, "flow add @ft" ] } }"

With this patch:

".. }, {"flow": {"op": "add", "flowtable": "@ft"}}]}}"

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/json.h    |  2 ++
 src/ct.c          |  1 +
 src/json.c        |  7 +++++++
 src/parser_json.c | 23 +++++++++++++++++++++++
 4 files changed, 33 insertions(+)

diff --git a/include/json.h b/include/json.h
index 3db9f2782d11..a753f359aa52 100644
--- a/include/json.h
+++ b/include/json.h
@@ -69,6 +69,7 @@ json_t *uid_type_json(const struct expr *expr, struct output_ctx *octx);
 json_t *gid_type_json(const struct expr *expr, struct output_ctx *octx);
 
 json_t *expr_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
+json_t *flow_offload_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 json_t *payload_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 json_t *exthdr_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
 json_t *quota_stmt_json(const struct stmt *stmt, struct output_ctx *octx);
@@ -169,6 +170,7 @@ EXPR_PRINT_STUB(uid_type)
 EXPR_PRINT_STUB(gid_type)
 
 STMT_PRINT_STUB(expr)
+STMT_PRINT_STUB(flow_offload)
 STMT_PRINT_STUB(payload)
 STMT_PRINT_STUB(exthdr)
 STMT_PRINT_STUB(quota)
diff --git a/src/ct.c b/src/ct.c
index 6049157198ad..e246d3039240 100644
--- a/src/ct.c
+++ b/src/ct.c
@@ -578,6 +578,7 @@ static const struct stmt_ops flow_offload_stmt_ops = {
 	.name		= "flow_offload",
 	.print		= flow_offload_stmt_print,
 	.destroy	= flow_offload_stmt_destroy,
+	.json		= flow_offload_stmt_json,
 };
 
 struct stmt *flow_offload_stmt_alloc(const struct location *loc,
diff --git a/src/json.c b/src/json.c
index 63b325afc8d1..4f800c908c66 100644
--- a/src/json.c
+++ b/src/json.c
@@ -1122,6 +1122,13 @@ json_t *expr_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
 	return expr_print_json(stmt->expr, octx);
 }
 
+json_t *flow_offload_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
+{
+	return json_pack("{s:{s:s, s:s+}}", "flow",
+			 "op", "add", "flowtable",
+			 "@", stmt->flow.table_name);
+}
+
 json_t *payload_stmt_json(const struct stmt *stmt, struct output_ctx *octx)
 {
 	return json_pack("{s: {s:o, s:o}}", "mangle",
diff --git a/src/parser_json.c b/src/parser_json.c
index 2fad308f7783..f07b798adecd 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1903,6 +1903,28 @@ out_err:
 	return NULL;
 }
 
+static struct stmt *json_parse_flow_offload_stmt(struct json_ctx *ctx,
+						 const char *key, json_t *value)
+{
+	const char *opstr, *flowtable;
+
+	if (json_unpack_err(ctx, value, "{s:s, s:s}",
+			    "op", &opstr, "flowtable", &flowtable))
+		return NULL;
+
+	if (strcmp(opstr, "add")) {
+		json_error(ctx, "Unknown flow offload statement op '%s'.", opstr);
+		return NULL;
+	}
+
+	if (flowtable[0] != '@') {
+		json_error(ctx, "Illegal flowtable reference in flow offload statement.");
+		return NULL;
+	}
+
+	return flow_offload_stmt_alloc(int_loc, xstrdup(flowtable + 1));
+}
+
 static struct stmt *json_parse_notrack_stmt(struct json_ctx *ctx,
 					const char *key, json_t *value)
 {
@@ -2647,6 +2669,7 @@ static struct stmt *json_parse_stmt(struct json_ctx *ctx, json_t *root)
 		{ "mangle", json_parse_mangle_stmt },
 		{ "quota", json_parse_quota_stmt },
 		{ "limit", json_parse_limit_stmt },
+		{ "flow", json_parse_flow_offload_stmt },
 		{ "fwd", json_parse_fwd_stmt },
 		{ "notrack", json_parse_notrack_stmt },
 		{ "dup", json_parse_dup_stmt },
-- 
2.34.1

Re: [PATCH nft 1/3] json: add flow statement json export + parser

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> flow statement has no export, its shown as:
> ".. }, "flow add @ft" ] } }"
> 
> With this patch:
> 
> ".. }, {"flow": {"op": "add", "flowtable": "@ft"}}]}}"

This is based on the 'set' statement.  If you prefer the @ to
be removed let me know.

Re: [PATCH nft 1/3] json: add flow statement json export + parser

[Pablo Neira Ayuso]

On Mon, Feb 07, 2022 at 02:29:15PM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > flow statement has no export, its shown as:
> > ".. }, "flow add @ft" ] } }"
> > 
> > With this patch:
> > 
> > ".. }, {"flow": {"op": "add", "flowtable": "@ft"}}]}}"
> 
> This is based on the 'set' statement.  If you prefer the @ to
> be removed let me know.

Then, it is consistent with the existing syntax. So either we consider
deprecating the @ on the 'set' statement (while retaining backward
compatibility) or flowtable also includes it as in your patch.

Re: [PATCH nft 1/3] json: add flow statement json export + parser

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Feb 07, 2022 at 02:29:15PM +0100, Florian Westphal wrote:
> > Florian Westphal <fw@strlen.de> wrote:
> > > flow statement has no export, its shown as:
> > > ".. }, "flow add @ft" ] } }"
> > > 
> > > With this patch:
> > > 
> > > ".. }, {"flow": {"op": "add", "flowtable": "@ft"}}]}}"
> > 
> > This is based on the 'set' statement.  If you prefer the @ to
> > be removed let me know.
> 
> Then, it is consistent with the existing syntax. So either we consider
> deprecating the @ on the 'set' statement (while retaining backward
> compatibility) or flowtable also includes it as in your patch.

Ok, then lets keep it as-is; the @ might help humans to find a
set/flowtable name more quickly this way.

Re: [PATCH nft 1/3] json: add flow statement json export + parser

[Pablo Neira Ayuso]

On Mon, Feb 07, 2022 at 03:53:13PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Feb 07, 2022 at 02:29:15PM +0100, Florian Westphal wrote:
> > > Florian Westphal <fw@strlen.de> wrote:
> > > > flow statement has no export, its shown as:
> > > > ".. }, "flow add @ft" ] } }"
> > > > 
> > > > With this patch:
> > > > 
> > > > ".. }, {"flow": {"op": "add", "flowtable": "@ft"}}]}}"
> > > 
> > > This is based on the 'set' statement.  If you prefer the @ to
> > > be removed let me know.
> > 
> > Then, it is consistent with the existing syntax. So either we consider
> > deprecating the @ on the 'set' statement (while retaining backward
> > compatibility) or flowtable also includes it as in your patch.
> 
> Ok, then lets keep it as-is; the @ might help humans to find a
> set/flowtable name more quickly this way.

OK.

Re: [PATCH nft 1/3] json: add flow statement json export + parser

[Phil Sutter]

Hi,

On Mon, Feb 07, 2022 at 03:04:16PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Feb 07, 2022 at 02:29:15PM +0100, Florian Westphal wrote:
> > Florian Westphal <fw@strlen.de> wrote:
> > > flow statement has no export, its shown as:
> > > ".. }, "flow add @ft" ] } }"
> > > 
> > > With this patch:
> > > 
> > > ".. }, {"flow": {"op": "add", "flowtable": "@ft"}}]}}"
> > 
> > This is based on the 'set' statement.  If you prefer the @ to
> > be removed let me know.
> 
> Then, it is consistent with the existing syntax. So either we consider
> deprecating the @ on the 'set' statement (while retaining backward
> compatibility) or flowtable also includes it as in your patch.

ACK, we should strive for internal consistency. I admittedly don't
recall why I added the '@' prefix in output, parser even demands it.
Dropping (i.e., omitting in output and accepting non-prefixed input) is
fine with me!

Thanks, Phil

[PATCH nft 2/3] parser_json: fix flowtable device datatype

[Florian Westphal]

Failed with: BUG: invalid expresion type symbol

Fixes: 78bbe7f7a55be489 ("mnl: do not use expr->identifier to fetch device name")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_json.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index f07b798adecd..2ab0196461e2 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3125,7 +3125,9 @@ static struct expr *json_parse_flowtable_devs(struct json_ctx *ctx,
 	size_t index;
 
 	if (!json_unpack(root, "s", &dev)) {
-		tmp = symbol_expr_alloc(int_loc, SYMBOL_VALUE, NULL, dev);
+		tmp = constant_expr_alloc(int_loc, &string_type,
+					  BYTEORDER_HOST_ENDIAN,
+					  strlen(dev) * BITS_PER_BYTE, dev);
 		compound_expr_add(expr, tmp);
 		return expr;
 	}
@@ -3141,7 +3143,9 @@ static struct expr *json_parse_flowtable_devs(struct json_ctx *ctx,
 			expr_free(expr);
 			return NULL;
 		}
-		tmp = symbol_expr_alloc(int_loc, SYMBOL_VALUE, NULL, dev);
+		tmp = constant_expr_alloc(int_loc, &string_type,
+					  BYTEORDER_HOST_ENDIAN,
+					  strlen(dev) * BITS_PER_BYTE, dev);
 		compound_expr_add(expr, tmp);
 	}
 	return expr;
-- 
2.34.1

[PATCH nft 3/3] parser_json: permit empty device list

[Florian Westphal]

Normal input parser allows flowtables without 'devices' token, which
makes the json export part elide 'dev' entirely, this then breaks on
re-import:

$ nft -j -f json.dump
/tmp/json_1:1:14-14: Error: Object item not found: dev

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_json.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 2ab0196461e2..4913260434f4 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3158,7 +3158,7 @@ static struct cmd *json_parse_cmd_add_flowtable(struct json_ctx *ctx,
 	const char *family, *hook, *hookstr;
 	struct flowtable *flowtable;
 	struct handle h = { 0 };
-	json_t *devs;
+	json_t *devs = NULL;
 	int prio;
 
 	if (json_unpack_err(ctx, root, "{s:s, s:s}",
@@ -3187,14 +3187,15 @@ static struct cmd *json_parse_cmd_add_flowtable(struct json_ctx *ctx,
 	if (op == CMD_DELETE || op == CMD_LIST)
 		return cmd_alloc(op, cmd_obj, &h, int_loc, NULL);
 
-	if (json_unpack_err(ctx, root, "{s:s, s:I, s:o}",
+	if (json_unpack_err(ctx, root, "{s:s, s:I}",
 			    "hook", &hook,
-			    "prio", &prio,
-			    "dev", &devs)) {
+			    "prio", &prio)) {
 		handle_free(&h);
 		return NULL;
 	}
 
+	json_unpack(root, "{s:o}", &devs);
+
 	hookstr = chain_hookname_lookup(hook);
 	if (!hookstr) {
 		json_error(ctx, "Invalid flowtable hook '%s'.", hook);
@@ -3209,12 +3210,14 @@ static struct cmd *json_parse_cmd_add_flowtable(struct json_ctx *ctx,
 				    BYTEORDER_HOST_ENDIAN,
 				    sizeof(int) * BITS_PER_BYTE, &prio);
 
-	flowtable->dev_expr = json_parse_flowtable_devs(ctx, devs);
-	if (!flowtable->dev_expr) {
-		json_error(ctx, "Invalid flowtable dev.");
-		flowtable_free(flowtable);
-		handle_free(&h);
-		return NULL;
+	if (devs) {
+		flowtable->dev_expr = json_parse_flowtable_devs(ctx, devs);
+		if (!flowtable->dev_expr) {
+			json_error(ctx, "Invalid flowtable dev.");
+			flowtable_free(flowtable);
+			handle_free(&h);
+			return NULL;
+		}
 	}
 	return cmd_alloc(op, cmd_obj, &h, int_loc, flowtable);
 }
-- 
2.34.1

Re: [PATCH nft 0/3] flowtable json fixes

[Phil Sutter]

On Mon, Feb 07, 2022 at 02:28:13PM +0100, Florian Westphal wrote:
> json doesn't export the flow statement.
> First patch resolves this, the other two patches resolve
> import problems with flowtable json format.
> 
> Florian Westphal (3):
>   json: add flow statement json export + parser
>   parser_json: fix flowtable device datatype
>   parser_json: permit empty device list

Series:

Acked-by: Phil Sutter <phil@nwl.cc>

Thanks, Phil