[PATCH nft 1/2] parser: allow ct eventmask set new,related

[PATCH nft 1/2] parser: allow ct eventmask set new,related

[Florian Westphal]

In case of bitmask types (tcp flags, ct eventmask) nft
allows to use comma operator to test multiple values, i.e.
tcp flags syn,ack   ct event new,destroy etc.

But currently nft fails to use this when used in a statement, i.e.
   ... ct eventmask set new,destroy
gives:
syntax error, unexpected comma

This allows makes this work, it is the same as
   ct eventmask set new|destroy

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 6be94a9b873f..a8448e14ef1f 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -635,8 +635,11 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { expr_free($$); }	rt_expr
 %type <val>			rt_key
 
-%type <expr>			ct_expr
-%destructor { expr_free($$); }	ct_expr
+%type <expr>			list_stmt_expr
+%destructor { expr_free($$); }	list_stmt_expr
+
+%type <expr>			ct_expr		ct_stmt_expr
+%destructor { expr_free($$); }	ct_expr		ct_stmt_expr
 %type <val>			ct_key		ct_key_dir	ct_key_dir_optional
 
 %type <expr>			fib_expr
@@ -3174,11 +3177,29 @@ ct_key_dir_optional	:	BYTES		{ $$ = NFT_CT_BYTES; }
 			|	ZONE		{ $$ = NFT_CT_ZONE; }
 			;
 
+list_stmt_expr		:	symbol_expr	COMMA	symbol_expr
+			{
+				$$ = list_expr_alloc(&@$);
+				compound_expr_add($$, $1);
+				compound_expr_add($$, $3);
+			}
+			|	list_stmt_expr	COMMA		symbol_expr
+			{
+				$1->location = @$;
+				compound_expr_add($1, $3);
+				$$ = $1;
+			}
+			;
+
+ct_stmt_expr		:	expr
+			|	list_stmt_expr
+			;
+
 ct_stmt			:	CT	ct_key		SET	expr
 			{
 				$$ = ct_stmt_alloc(&@$, $2, -1, $4);
 			}
-			|	CT	STRING		SET	expr
+			|	CT	STRING		SET	ct_stmt_expr
 			{
 				struct error_record *erec;
 				unsigned int key;
-- 
2.13.0

[PATCH nft 2/2] src: rename ct eventmask to event

[Florian Westphal]

ct status isn't named 'statusmask' either.

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Pablo, we could still change kernel API and rename
 NFT_CT_EVENTMASK to NFT_CT_EVENT, let me know (or
 do so yourself), it just has to hit Linus tree before 4.12.

 The needed libnftnl patch is trivial, i'll just push it
 out if you don't NAK this one.

 doc/nft.xml               |  6 +++---
 src/ct.c                  |  2 +-
 tests/py/any/ct.t         | 10 +++++-----
 tests/py/any/ct.t.payload | 16 ++++++++--------
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/doc/nft.xml b/doc/nft.xml
index f613f69cb764..6f5b8bf90c6f 100644
--- a/doc/nft.xml
+++ b/doc/nft.xml
@@ -3864,7 +3864,7 @@ ip6 filter output log flags all
 					<command>ct</command>
 					<group choice="req">
 						<arg>mark</arg>
-						<arg>eventmask</arg>
+						<arg>event</arg>
 						<arg>label</arg>
 						<arg>zone</arg>
 					</group>
@@ -3895,7 +3895,7 @@ ip6 filter output log flags all
 						</thead>
 						<tbody>
 							<row>
-								<entry>eventmask</entry>
+								<entry>event</entry>
 								<entry>conntrack event bits</entry>
 								<entry>bitmask, integer (32 bit)</entry>
 							</row>
@@ -3950,7 +3950,7 @@ table inet raw {
 			<example>
 					<title>restrict events reported by ctnetlink</title>
 				<programlisting>
-ct eventmask set new or related or destroy
+ct event set new,related,destroy
 				</programlisting>
 			</example>
 
diff --git a/src/ct.c b/src/ct.c
index 5014265a3427..ab50a1668404 100644
--- a/src/ct.c
+++ b/src/ct.c
@@ -264,7 +264,7 @@ static const struct ct_template ct_templates[] = {
 					      BYTEORDER_HOST_ENDIAN, 64),
 	[NFT_CT_ZONE]		= CT_TEMPLATE("zone", &integer_type,
 					      BYTEORDER_HOST_ENDIAN, 16),
-	[NFT_CT_EVENTMASK]	= CT_TEMPLATE("eventmask", &ct_event_type,
+	[NFT_CT_EVENTMASK]	= CT_TEMPLATE("event", &ct_event_type,
 					      BYTEORDER_HOST_ENDIAN, 32),
 };
 
diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
index 667126e656ae..1c90ec1c769d 100644
--- a/tests/py/any/ct.t
+++ b/tests/py/any/ct.t
@@ -100,11 +100,11 @@ ct original mark 42;fail
 # swapped key and direction
 ct mark original;fail
 
-ct eventmask set new;ok
-ct eventmask set new or related or destroy or foobar;fail
-ct eventmask set 'new | related | destroy | label';ok;ct eventmask set new | related | destroy | label
-ct eventmask set 1;ok;ct eventmask set new
-ct eventmask set 0x0;ok
+ct event set new;ok
+ct event set new or related or destroy or foobar;fail
+ct event set 'new | related | destroy | label';ok;ct event set new | related | destroy | label
+ct event set 1;ok;ct event set new
+ct event set 0x0;ok
 
 ct label 127;ok
 ct label set 127;ok
diff --git a/tests/py/any/ct.t.payload b/tests/py/any/ct.t.payload
index c5fa7c8d49e4..dea985a3016b 100644
--- a/tests/py/any/ct.t.payload
+++ b/tests/py/any/ct.t.payload
@@ -398,25 +398,25 @@ ip test-ip4 output
   [ bitwise reg 1 = (reg=1 & 0x00000020 ) ^ 0x00000000 ]
   [ cmp neq reg 1 0x00000000 ]
 
-# ct eventmask set new
+# ct event set new
 ip test-ip4 output
   [ immediate reg 1 0x00000001 ]
-  [ ct set eventmask with reg 1 ]
+  [ ct set event with reg 1 ]
 
-# ct eventmask set 'new | related | destroy | label'
+# ct event set 'new | related | destroy | label'
 ip test-ip4 output
   [ immediate reg 1 0x00000407 ]
-  [ ct set eventmask with reg 1 ]
+  [ ct set event with reg 1 ]
 
-# ct eventmask set 1
+# ct event set 1
 ip test-ip4 output
   [ immediate reg 1 0x00000001 ]
-  [ ct set eventmask with reg 1 ]
+  [ ct set event with reg 1 ]
 
-# ct eventmask set 0x0
+# ct event set 0x0
 ip test-ip4 output
   [ immediate reg 1 0x00000000 ]
-  [ ct set eventmask with reg 1 ]
+  [ ct set event with reg 1 ]
 
 # ct label 127
 ip test-ip4 output
-- 
2.13.0

Re: [PATCH nft 1/2] parser: allow ct eventmask set new,related

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 12:24:09PM +0200, Florian Westphal wrote:
> In case of bitmask types (tcp flags, ct eventmask) nft
> allows to use comma operator to test multiple values, i.e.
> tcp flags syn,ack   ct event new,destroy etc.
> 
> But currently nft fails to use this when used in a statement, i.e.
>    ... ct eventmask set new,destroy
> gives:
> syntax error, unexpected comma
> 
> This allows makes this work, it is the same as
>    ct eventmask set new|destroy
> 
> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Much nicer! Thanks for working on this!

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [PATCH nft 2/2] src: rename ct eventmask to event

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 12:24:10PM +0200, Florian Westphal wrote:
> diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
> index 667126e656ae..1c90ec1c769d 100644
> --- a/tests/py/any/ct.t
> +++ b/tests/py/any/ct.t
> @@ -100,11 +100,11 @@ ct original mark 42;fail
[...]
> +ct event set new or related or destroy or foobar;fail
> +ct event set 'new | related | destroy | label';ok;ct event set new | related | destroy | label

I would replace this by the new syntax in the tests too.

So anyone looking at test for example relies on this new one, it is
more compact and readable IMO.

Thanks!

Re: [PATCH nft 2/2] src: rename ct eventmask to event

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 12:38:20PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 07, 2017 at 12:24:10PM +0200, Florian Westphal wrote:
> > diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
> > index 667126e656ae..1c90ec1c769d 100644
> > --- a/tests/py/any/ct.t
> > +++ b/tests/py/any/ct.t
> > @@ -100,11 +100,11 @@ ct original mark 42;fail
> [...]
> > +ct event set new or related or destroy or foobar;fail
> > +ct event set 'new | related | destroy | label';ok;ct event set new | related | destroy | label
> 
> I would replace this by the new syntax in the tests too.
> 
> So anyone looking at test for example relies on this new one, it is
> more compact and readable IMO.

Apart from that small detail, LGTM.

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [PATCH nft 2/2] src: rename ct eventmask to event

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jun 07, 2017 at 12:24:10PM +0200, Florian Westphal wrote:
> > diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
> > index 667126e656ae..1c90ec1c769d 100644
> > --- a/tests/py/any/ct.t
> > +++ b/tests/py/any/ct.t
> > @@ -100,11 +100,11 @@ ct original mark 42;fail
> [...]
> > +ct event set new or related or destroy or foobar;fail
> > +ct event set 'new | related | destroy | label';ok;ct event set new | related | destroy | label
> 
> I would replace this by the new syntax in the tests too.
> 
> So anyone looking at test for example relies on this new one, it is
> more compact and readable IMO.

Good point, we still print
ct event set new | related | destroy | label

because we lack the OP_FLAGCMP postprocessing that relational expression
does (it converts the rhs binops into list in case of OP_FLAGCMP).

Flagcmp is also a bit different thing:
   tcp flags syn,ack
is short-hand for
   'tcp flags & (syn|ack) != 0'

but when using 'ct event set foo,bar'
its same as
ct event set foo|bar.

This gets ugly... I see no way to autodetect which output format
we should use.

I could of course stick a check for the key type into
netlink_delinerize.c but thats ugly.

Alterntively I could hook into ct_stmt_print and dissect there.

Any idea/preference?

Re: [PATCH nft 2/2] src: rename ct eventmask to event

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 01:07:51PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Jun 07, 2017 at 12:24:10PM +0200, Florian Westphal wrote:
> > > diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
> > > index 667126e656ae..1c90ec1c769d 100644
> > > --- a/tests/py/any/ct.t
> > > +++ b/tests/py/any/ct.t
> > > @@ -100,11 +100,11 @@ ct original mark 42;fail
> > [...]
> > > +ct event set new or related or destroy or foobar;fail
> > > +ct event set 'new | related | destroy | label';ok;ct event set new | related | destroy | label
> > 
> > I would replace this by the new syntax in the tests too.
> > 
> > So anyone looking at test for example relies on this new one, it is
> > more compact and readable IMO.
> 
> Good point, we still print
> ct event set new | related | destroy | label
> 
> because we lack the OP_FLAGCMP postprocessing that relational expression
> does (it converts the rhs binops into list in case of OP_FLAGCMP).
> 
> Flagcmp is also a bit different thing:
>    tcp flags syn,ack
> is short-hand for
>    'tcp flags & (syn|ack) != 0'
> 
> but when using 'ct event set foo,bar'
> its same as
> ct event set foo|bar.
> 
> This gets ugly... I see no way to autodetect which output format
> we should use.
>
> I could of course stick a check for the key type into
> netlink_delinerize.c but thats ugly.
> 
> Alterntively I could hook into ct_stmt_print and dissect there.
> 
> Any idea/preference?

I suggest you always use the comma separated one to print this.

This assymmetry is not a problem, what matters if that the internal
AST representation end up being the same, which is what matters to the
delete by name (if that is your primary concern with this). So it's
not that we need the same syntax in both directions specifically, but
the same internal representation for both.

So we just need that the evaluation transform these or syntax to
OP_FLAGCMP.

Actually, I remember to have discussed with Laura ideas to kill
OP_FLAGCMP and just convert this to binary, specifically, I need to
look back at the archive, but I think the problem is to check for
flaglist mismatch.

Anyway, at this stage, I would suggest you just update this to print
it in comma separated output.

Re: [PATCH nft 2/2] src: rename ct eventmask to event

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jun 07, 2017 at 01:07:51PM +0200, Florian Westphal wrote:
> > I could of course stick a check for the key type into
> > netlink_delinerize.c but thats ugly.
> > 
> > Alterntively I could hook into ct_stmt_print and dissect there.
> > 
> > Any idea/preference?
> 
> I suggest you always use the comma separated one to print this.
 
I've pushed a revised version of this patch + the 'use comma' patch
to master, thanks!