| Message ID | 20240320145806.4167-1-phil@nwl.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [nft] json: Accept more than two operands in binary expressions | expand |
On Wed, Mar 20, 2024 at 03:58:06PM +0100, Phil Sutter wrote: > The most common use case is ORing flags like > > | syn | ack | rst > > but nft seems to be fine with less intuitive stuff like > > | meta mark set ip dscp << 2 << 3 This is equivalent to: meta mark set ip dscp << 5 userspace is lacking the code to simplify this, just like it does for: meta mark set ip dscp | 0x8 | 0xf0 results in: meta mark set ip dscp | 0xf8 > so support all of them. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > doc/libnftables-json.adoc | 18 ++-- > src/json.c | 19 +++- > src/parser_json.c | 12 +++ > tests/py/inet/tcp.t.json | 50 +--------- > tests/py/inet/tcp.t.json.output | 34 ++----- > .../dumps/0012different_defines_0.json-nft | 8 +- > .../sets/dumps/0055tcpflags_0.json-nft | 98 +++++-------------- > 7 files changed, 75 insertions(+), 164 deletions(-) > > diff --git a/doc/libnftables-json.adoc b/doc/libnftables-json.adoc > index 3948a0bad47c1..e3b24cc4ed60d 100644 > --- a/doc/libnftables-json.adoc > +++ b/doc/libnftables-json.adoc > @@ -1343,15 +1343,17 @@ Perform kernel Forwarding Information Base lookups. > > === BINARY OPERATION > [verse] > -*{ "|": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* > -*{ "^": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* > -*{ "&": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* > -*{ "+<<+": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* > -*{ ">>": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* > - > -All binary operations expect an array of exactly two expressions, of which the > +*{ "|": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* > +*{ "^": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* > +*{ "&": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* > +*{ "+<<+": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* > +*{ ">>": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* > +'EXPRESSIONS' := 'EXPRESSION' | 'EXPRESSION'*,* 'EXPRESSIONS' > + > +All binary operations expect an array of at least two expressions, of which the > first element denotes the left hand side and the second one the right hand > -side. > +side. Extra elements are accepted in the given array and appended to the term > +accordingly. > > === VERDICT > [verse] > diff --git a/src/json.c b/src/json.c > index 29fbd0cfdba28..3753017169930 100644 > --- a/src/json.c > +++ b/src/json.c > @@ -540,11 +540,24 @@ json_t *flagcmp_expr_json(const struct expr *expr, struct output_ctx *octx) > "right", expr_print_json(expr->flagcmp.value, octx)); > } > > +static json_t * > +__binop_expr_json(int op, const struct expr *expr, struct output_ctx *octx) > +{ > + json_t *a = json_array(); > + > + if (expr->etype == EXPR_BINOP && expr->op == op) { > + json_array_extend(a, __binop_expr_json(op, expr->left, octx)); > + json_array_extend(a, __binop_expr_json(op, expr->right, octx)); > + } else { > + json_array_append_new(a, expr_print_json(expr, octx)); > + } > + return a; > +} > + > json_t *binop_expr_json(const struct expr *expr, struct output_ctx *octx) > { > - return json_pack("{s:[o, o]}", expr_op_symbols[expr->op], > - expr_print_json(expr->left, octx), > - expr_print_json(expr->right, octx)); > + return json_pack("{s:o}", expr_op_symbols[expr->op], > + __binop_expr_json(expr->op, expr, octx)); > } > > json_t *relational_expr_json(const struct expr *expr, struct output_ctx *octx) > diff --git a/src/parser_json.c b/src/parser_json.c > index 04255688ca04c..55d65c415bf5c 100644 > --- a/src/parser_json.c > +++ b/src/parser_json.c > @@ -1204,6 +1204,18 @@ static struct expr *json_parse_binop_expr(struct json_ctx *ctx, > return NULL; > } > > + if (json_array_size(root) > 2) { > + left = json_parse_primary_expr(ctx, json_array_get(root, 0)); > + right = json_parse_primary_expr(ctx, json_array_get(root, 1)); > + left = binop_expr_alloc(int_loc, thisop, left, right); > + for (i = 2; i < json_array_size(root); i++) { > + jright = json_array_get(root, i); > + right = json_parse_primary_expr(ctx, jright); > + left = binop_expr_alloc(int_loc, thisop, left, right); > + } > + return left; > + } > + > if (json_unpack_err(ctx, root, "[o, o!]", &jleft, &jright)) > return NULL; > > diff --git a/tests/py/inet/tcp.t.json b/tests/py/inet/tcp.t.json > index 8439c2b5931dd..9a1b158e7ac0b 100644 > --- a/tests/py/inet/tcp.t.json > +++ b/tests/py/inet/tcp.t.json > @@ -954,12 +954,12 @@ > } > }, > { > - "|": [ "fin", { "|": [ "syn", { "|": [ "rst", { "|": [ "psh", { "|": [ "ack", { "|": [ "urg", { "|": [ "ecn", "cwr" ] } ] } ] } ] } ] } ] } ] > + "|": [ "fin", "syn", "rst", "psh", "ack", "urg", "ecn", "cwr" ] > } > ] > }, > "op": "==", > - "right": { "|": [ "fin", { "|": [ "syn", { "|": [ "rst", { "|": [ "psh", { "|": [ "ack", { "|": [ "urg", { "|": [ "ecn", "cwr" ] } ] } ] } ] } ] } ] } ] } > + "right": { "|": [ "fin", "syn", "rst", "psh", "ack", "urg", "ecn", "cwr" ] } > } > } > ] > @@ -1395,55 +1395,15 @@ > "protocol": "tcp" > } > }, > - { > - "|": [ > - { > - "|": [ > - { > - "|": [ > - { > - "|": [ > - { > - "|": [ > - "fin", > - "syn" > - ] > - }, > - "rst" > - ] > - }, > - "psh" > - ] > - }, > - "ack" > - ] > - }, > - "urg" > - ] > - } > + { "|": [ "fin", "syn", "rst", "psh", "ack", "urg" ] } > ] > }, > "op": "==", > "right": { > "set": [ > - { > - "|": [ > - { > - "|": [ > - "fin", > - "psh" > - ] > - }, > - "ack" > - ] > - }, > + { "|": [ "fin", "psh", "ack" ] }, > "fin", > - { > - "|": [ > - "psh", > - "ack" > - ] > - }, > + { "|": [ "psh", "ack" ] }, > "ack" > ] > } > diff --git a/tests/py/inet/tcp.t.json.output b/tests/py/inet/tcp.t.json.output > index c471e8d8dcef5..5a16714e9145d 100644 > --- a/tests/py/inet/tcp.t.json.output > +++ b/tests/py/inet/tcp.t.json.output > @@ -155,27 +155,11 @@ > }, > { > "|": [ > - { > - "|": [ > - { > - "|": [ > - { > - "|": [ > - { > - "|": [ > - "fin", > - "syn" > - ] > - }, > - "rst" > - ] > - }, > - "psh" > - ] > - }, > - "ack" > - ] > - }, > + "fin", > + "syn", > + "rst", > + "psh", > + "ack", > "urg" > ] > } > @@ -187,12 +171,8 @@ > "fin", > { > "|": [ > - { > - "|": [ > - "fin", > - "psh" > - ] > - }, > + "fin", > + "psh", > "ack" > ] > }, > diff --git a/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft b/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft > index 8f3f3a81a9bc8..1b2e342047f4b 100644 > --- a/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft > +++ b/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft > @@ -169,12 +169,8 @@ > }, > "right": { > "|": [ > - { > - "|": [ > - "established", > - "related" > - ] > - }, > + "established", > + "related", > "new" > ] > } > diff --git a/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft b/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft > index cd39f0909e120..6a3511515f785 100644 > --- a/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft > +++ b/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft > @@ -27,39 +27,23 @@ > "elem": [ > { > "|": [ > - { > - "|": [ > - { > - "|": [ > - "fin", > - "psh" > - ] > - }, > - "ack" > - ] > - }, > + "fin", > + "psh", > + "ack", > "urg" > ] > }, > { > "|": [ > - { > - "|": [ > - "fin", > - "psh" > - ] > - }, > + "fin", > + "psh", > "ack" > ] > }, > { > "|": [ > - { > - "|": [ > - "fin", > - "ack" > - ] > - }, > + "fin", > + "ack", > "urg" > ] > }, > @@ -71,39 +55,23 @@ > }, > { > "|": [ > - { > - "|": [ > - { > - "|": [ > - "syn", > - "psh" > - ] > - }, > - "ack" > - ] > - }, > + "syn", > + "psh", > + "ack", > "urg" > ] > }, > { > "|": [ > - { > - "|": [ > - "syn", > - "psh" > - ] > - }, > + "syn", > + "psh", > "ack" > ] > }, > { > "|": [ > - { > - "|": [ > - "syn", > - "ack" > - ] > - }, > + "syn", > + "ack", > "urg" > ] > }, > @@ -116,39 +84,23 @@ > "syn", > { > "|": [ > - { > - "|": [ > - { > - "|": [ > - "rst", > - "psh" > - ] > - }, > - "ack" > - ] > - }, > + "rst", > + "psh", > + "ack", > "urg" > ] > }, > { > "|": [ > - { > - "|": [ > - "rst", > - "psh" > - ] > - }, > + "rst", > + "psh", > "ack" > ] > }, > { > "|": [ > - { > - "|": [ > - "rst", > - "ack" > - ] > - }, > + "rst", > + "ack", > "urg" > ] > }, > @@ -161,12 +113,8 @@ > "rst", > { > "|": [ > - { > - "|": [ > - "psh", > - "ack" > - ] > - }, > + "psh", > + "ack", > "urg" > ] > }, > -- > 2.43.0 >
On Wed, Mar 20, 2024 at 06:17:18PM +0100, Pablo Neira Ayuso wrote: > On Wed, Mar 20, 2024 at 03:58:06PM +0100, Phil Sutter wrote: > > The most common use case is ORing flags like > > > > | syn | ack | rst > > > > but nft seems to be fine with less intuitive stuff like > > > > | meta mark set ip dscp << 2 << 3 > > This is equivalent to: > > meta mark set ip dscp << 5 > > userspace is lacking the code to simplify this, just like it does for: > > meta mark set ip dscp | 0x8 | 0xf0 > > results in: > > meta mark set ip dscp | 0xf8 You're right, of course. Simplifying the input is a different task though, I merely made sure that JSON input/output matches what regular syntax supports as well. Input optimization should happen in eval phase (or so) anyway and thus independent of where input was parsed from. Cheers, Phil
diff --git a/doc/libnftables-json.adoc b/doc/libnftables-json.adoc index 3948a0bad47c1..e3b24cc4ed60d 100644 --- a/doc/libnftables-json.adoc +++ b/doc/libnftables-json.adoc @@ -1343,15 +1343,17 @@ Perform kernel Forwarding Information Base lookups. === BINARY OPERATION [verse] -*{ "|": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* -*{ "^": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* -*{ "&": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* -*{ "+<<+": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* -*{ ">>": [* 'EXPRESSION'*,* 'EXPRESSION' *] }* - -All binary operations expect an array of exactly two expressions, of which the +*{ "|": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* +*{ "^": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* +*{ "&": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* +*{ "+<<+": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* +*{ ">>": [* 'EXPRESSION'*,* 'EXPRESSIONS' *] }* +'EXPRESSIONS' := 'EXPRESSION' | 'EXPRESSION'*,* 'EXPRESSIONS' + +All binary operations expect an array of at least two expressions, of which the first element denotes the left hand side and the second one the right hand -side. +side. Extra elements are accepted in the given array and appended to the term +accordingly. === VERDICT [verse] diff --git a/src/json.c b/src/json.c index 29fbd0cfdba28..3753017169930 100644 --- a/src/json.c +++ b/src/json.c @@ -540,11 +540,24 @@ json_t *flagcmp_expr_json(const struct expr *expr, struct output_ctx *octx) "right", expr_print_json(expr->flagcmp.value, octx)); } +static json_t * +__binop_expr_json(int op, const struct expr *expr, struct output_ctx *octx) +{ + json_t *a = json_array(); + + if (expr->etype == EXPR_BINOP && expr->op == op) { + json_array_extend(a, __binop_expr_json(op, expr->left, octx)); + json_array_extend(a, __binop_expr_json(op, expr->right, octx)); + } else { + json_array_append_new(a, expr_print_json(expr, octx)); + } + return a; +} + json_t *binop_expr_json(const struct expr *expr, struct output_ctx *octx) { - return json_pack("{s:[o, o]}", expr_op_symbols[expr->op], - expr_print_json(expr->left, octx), - expr_print_json(expr->right, octx)); + return json_pack("{s:o}", expr_op_symbols[expr->op], + __binop_expr_json(expr->op, expr, octx)); } json_t *relational_expr_json(const struct expr *expr, struct output_ctx *octx) diff --git a/src/parser_json.c b/src/parser_json.c index 04255688ca04c..55d65c415bf5c 100644 --- a/src/parser_json.c +++ b/src/parser_json.c @@ -1204,6 +1204,18 @@ static struct expr *json_parse_binop_expr(struct json_ctx *ctx, return NULL; } + if (json_array_size(root) > 2) { + left = json_parse_primary_expr(ctx, json_array_get(root, 0)); + right = json_parse_primary_expr(ctx, json_array_get(root, 1)); + left = binop_expr_alloc(int_loc, thisop, left, right); + for (i = 2; i < json_array_size(root); i++) { + jright = json_array_get(root, i); + right = json_parse_primary_expr(ctx, jright); + left = binop_expr_alloc(int_loc, thisop, left, right); + } + return left; + } + if (json_unpack_err(ctx, root, "[o, o!]", &jleft, &jright)) return NULL; diff --git a/tests/py/inet/tcp.t.json b/tests/py/inet/tcp.t.json index 8439c2b5931dd..9a1b158e7ac0b 100644 --- a/tests/py/inet/tcp.t.json +++ b/tests/py/inet/tcp.t.json @@ -954,12 +954,12 @@ } }, { - "|": [ "fin", { "|": [ "syn", { "|": [ "rst", { "|": [ "psh", { "|": [ "ack", { "|": [ "urg", { "|": [ "ecn", "cwr" ] } ] } ] } ] } ] } ] } ] + "|": [ "fin", "syn", "rst", "psh", "ack", "urg", "ecn", "cwr" ] } ] }, "op": "==", - "right": { "|": [ "fin", { "|": [ "syn", { "|": [ "rst", { "|": [ "psh", { "|": [ "ack", { "|": [ "urg", { "|": [ "ecn", "cwr" ] } ] } ] } ] } ] } ] } ] } + "right": { "|": [ "fin", "syn", "rst", "psh", "ack", "urg", "ecn", "cwr" ] } } } ] @@ -1395,55 +1395,15 @@ "protocol": "tcp" } }, - { - "|": [ - { - "|": [ - { - "|": [ - { - "|": [ - { - "|": [ - "fin", - "syn" - ] - }, - "rst" - ] - }, - "psh" - ] - }, - "ack" - ] - }, - "urg" - ] - } + { "|": [ "fin", "syn", "rst", "psh", "ack", "urg" ] } ] }, "op": "==", "right": { "set": [ - { - "|": [ - { - "|": [ - "fin", - "psh" - ] - }, - "ack" - ] - }, + { "|": [ "fin", "psh", "ack" ] }, "fin", - { - "|": [ - "psh", - "ack" - ] - }, + { "|": [ "psh", "ack" ] }, "ack" ] } diff --git a/tests/py/inet/tcp.t.json.output b/tests/py/inet/tcp.t.json.output index c471e8d8dcef5..5a16714e9145d 100644 --- a/tests/py/inet/tcp.t.json.output +++ b/tests/py/inet/tcp.t.json.output @@ -155,27 +155,11 @@ }, { "|": [ - { - "|": [ - { - "|": [ - { - "|": [ - { - "|": [ - "fin", - "syn" - ] - }, - "rst" - ] - }, - "psh" - ] - }, - "ack" - ] - }, + "fin", + "syn", + "rst", + "psh", + "ack", "urg" ] } @@ -187,12 +171,8 @@ "fin", { "|": [ - { - "|": [ - "fin", - "psh" - ] - }, + "fin", + "psh", "ack" ] }, diff --git a/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft b/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft index 8f3f3a81a9bc8..1b2e342047f4b 100644 --- a/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft +++ b/tests/shell/testcases/nft-f/dumps/0012different_defines_0.json-nft @@ -169,12 +169,8 @@ }, "right": { "|": [ - { - "|": [ - "established", - "related" - ] - }, + "established", + "related", "new" ] } diff --git a/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft b/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft index cd39f0909e120..6a3511515f785 100644 --- a/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft +++ b/tests/shell/testcases/sets/dumps/0055tcpflags_0.json-nft @@ -27,39 +27,23 @@ "elem": [ { "|": [ - { - "|": [ - { - "|": [ - "fin", - "psh" - ] - }, - "ack" - ] - }, + "fin", + "psh", + "ack", "urg" ] }, { "|": [ - { - "|": [ - "fin", - "psh" - ] - }, + "fin", + "psh", "ack" ] }, { "|": [ - { - "|": [ - "fin", - "ack" - ] - }, + "fin", + "ack", "urg" ] }, @@ -71,39 +55,23 @@ }, { "|": [ - { - "|": [ - { - "|": [ - "syn", - "psh" - ] - }, - "ack" - ] - }, + "syn", + "psh", + "ack", "urg" ] }, { "|": [ - { - "|": [ - "syn", - "psh" - ] - }, + "syn", + "psh", "ack" ] }, { "|": [ - { - "|": [ - "syn", - "ack" - ] - }, + "syn", + "ack", "urg" ] }, @@ -116,39 +84,23 @@ "syn", { "|": [ - { - "|": [ - { - "|": [ - "rst", - "psh" - ] - }, - "ack" - ] - }, + "rst", + "psh", + "ack", "urg" ] }, { "|": [ - { - "|": [ - "rst", - "psh" - ] - }, + "rst", + "psh", "ack" ] }, { "|": [ - { - "|": [ - "rst", - "ack" - ] - }, + "rst", + "ack", "urg" ] }, @@ -161,12 +113,8 @@ "rst", { "|": [ - { - "|": [ - "psh", - "ack" - ] - }, + "psh", + "ack", "urg" ] },
The most common use case is ORing flags like | syn | ack | rst but nft seems to be fine with less intuitive stuff like | meta mark set ip dscp << 2 << 3 so support all of them. Signed-off-by: Phil Sutter <phil@nwl.cc> --- doc/libnftables-json.adoc | 18 ++-- src/json.c | 19 +++- src/parser_json.c | 12 +++ tests/py/inet/tcp.t.json | 50 +--------- tests/py/inet/tcp.t.json.output | 34 ++----- .../dumps/0012different_defines_0.json-nft | 8 +- .../sets/dumps/0055tcpflags_0.json-nft | 98 +++++-------------- 7 files changed, 75 insertions(+), 164 deletions(-)