[PATCH nft 0/4] nft updates

[PATCH nft 0/4] nft updates

[Pablo Neira Ayuso]

Hi,

Small oneliner fix to set byteorder in string_type. And updates for the
nft-test.py script.

If no objections, I'll push this to master before the upcoming 0.4
release.

Pablo Neira Ayuso (4):
  datatype: missing byteorder in string_type
  tests: regression: fix wrong number of test files
  tests: regression: simplify run_test_file() in case `-e' is used
  tests: regression: log.t: this works for bridge and arp since 3.17

 src/datatype.c               |    1 +
 tests/regression/any/log.t   |    9 ++++-----
 tests/regression/nft-test.py |   19 +++----------------
 3 files changed, 8 insertions(+), 21 deletions(-)

-- 
1.7.10.4

[PATCH nft 1/4] datatype: missing byteorder in string_type

[Pablo Neira Ayuso]

nft add rule filter input iifname { "lo", "eth0" } counter

Now the listing shows:

	iifname { "lo", "eth0"}

instead of:

	iifname { "", ""}

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/datatype.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/src/datatype.c b/src/datatype.c
index 4519d87..7f73077 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -324,6 +324,7 @@ const struct datatype string_type = {
 	.type		= TYPE_STRING,
 	.name		= "string",
 	.desc		= "string",
+	.byteorder	= BYTEORDER_HOST_ENDIAN,
 	.print		= string_type_print,
 	.parse		= string_type_parse,
 };
-- 
1.7.10.4

Re: [PATCH nft 1/4] datatype: missing byteorder in string_type

[Patrick McHardy]

Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>nft add rule filter input iifname { "lo", "eth0" } counter
>
>Now the listing shows:
>
>	iifname { "lo", "eth0"}
>
>instead of:
>
>	iifname { "", ""}

Again wondering what broke this. Let me check when I am at home, IIRC we have some check for strings somewhere in the netlink code that relies in this.

>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>---
> src/datatype.c |    1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/src/datatype.c b/src/datatype.c
>index 4519d87..7f73077 100644
>--- a/src/datatype.c
>+++ b/src/datatype.c
>@@ -324,6 +324,7 @@ const struct datatype string_type = {
> 	.type		= TYPE_STRING,
> 	.name		= "string",
> 	.desc		= "string",
>+	.byteorder	= BYTEORDER_HOST_ENDIAN,
> 	.print		= string_type_print,
> 	.parse		= string_type_parse,
> };

Re: [PATCH nft 1/4] datatype: missing byteorder in string_type

[Pablo Neira Ayuso]

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

On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
> Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> >nft add rule filter input iifname { "lo", "eth0" } counter
> >
> >Now the listing shows:
> >
> >	iifname { "lo", "eth0"}
> >
> >instead of:
> >
> >	iifname { "", ""}
> 
> Again wondering what broke this. Let me check when I am at home,
> IIRC we have some check for strings somewhere in the netlink code
> that relies in this.

Attached an alternative to this patch. That I can remember, this is
broken since quite some time.

Let me know, thanks.

[-- Attachment #2: 0001-datatype-fix-listing-of-string-elements.patch --]
[-- Type: text/x-diff, Size: 1845 bytes --]

>From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Sat, 13 Dec 2014 18:29:37 +0100
Subject: [PATCH] datatype: fix listing of string elements

Generalise 0451b82 ("src: generate set members using integer_type in
the appropriate byteorder") to handle string_type too, since this
datatype doesn't have any specific byteorder.

nft add rule filter input iifname { "lo", "eth0" } counter

Now the listing shows:

	iifname { "lo", "eth0"}

instead of:

	iifname { "", ""}

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/netlink_delinearize.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index e9a04dd..8762cc9 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -742,7 +742,7 @@ static void payload_dependency_store(struct rule_pp_ctx *ctx,
 	ctx->pdep  = stmt;
 }
 
-static void integer_type_postprocess(struct expr *expr)
+static void lookup_postprocess(struct expr *expr)
 {
 	struct expr *i;
 
@@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr *expr)
 	case EXPR_SET_REF:
 		list_for_each_entry(i, &expr->set->init->expressions, list) {
 			expr_set_type(i, expr->dtype, expr->byteorder);
-			integer_type_postprocess(i);
+			lookup_postprocess(i);
 		}
 		break;
 	default:
@@ -831,8 +831,12 @@ static void meta_match_postprocess(struct rule_pp_ctx *ctx,
 	case OP_LOOKUP:
 		expr_set_type(expr->right, expr->left->dtype,
 			      expr->left->byteorder);
-		if (expr->right->dtype == &integer_type)
-			integer_type_postprocess(expr->right);
+		switch (expr->right->dtype->type) {
+		case TYPE_STRING:
+		case TYPE_INTEGER:
+			lookup_postprocess(expr->right);
+			break;
+		}
 		break;
 	default:
 		break;
-- 
1.7.10.4

Re: [PATCH nft 1/4] datatype: missing byteorder in string_type

[Pablo Neira Ayuso]

Hi Patrick!

On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> > >nft add rule filter input iifname { "lo", "eth0" } counter
> > >
> > >Now the listing shows:
> > >
> > >	iifname { "lo", "eth0"}
> > >
> > >instead of:
> > >
> > >	iifname { "", ""}
> > 
> > Again wondering what broke this. Let me check when I am at home,
> > IIRC we have some check for strings somewhere in the netlink code
> > that relies in this.
> 
> Attached an alternative to this patch. That I can remember, this is
> broken since quite some time.

Any concern with this second approach?

Let me know if you prefer I keep this away from this release.

Thanks!

> From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Sat, 13 Dec 2014 18:29:37 +0100
> Subject: [PATCH] datatype: fix listing of string elements
> 
> Generalise 0451b82 ("src: generate set members using integer_type in
> the appropriate byteorder") to handle string_type too, since this
> datatype doesn't have any specific byteorder.
> 
> nft add rule filter input iifname { "lo", "eth0" } counter
> 
> Now the listing shows:
> 
> 	iifname { "lo", "eth0"}
> 
> instead of:
> 
> 	iifname { "", ""}
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/netlink_delinearize.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index e9a04dd..8762cc9 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -742,7 +742,7 @@ static void payload_dependency_store(struct rule_pp_ctx *ctx,
>  	ctx->pdep  = stmt;
>  }
>  
> -static void integer_type_postprocess(struct expr *expr)
> +static void lookup_postprocess(struct expr *expr)
>  {
>  	struct expr *i;
>  
> @@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr *expr)
>  	case EXPR_SET_REF:
>  		list_for_each_entry(i, &expr->set->init->expressions, list) {
>  			expr_set_type(i, expr->dtype, expr->byteorder);
> -			integer_type_postprocess(i);
> +			lookup_postprocess(i);
>  		}
>  		break;
>  	default:
> @@ -831,8 +831,12 @@ static void meta_match_postprocess(struct rule_pp_ctx *ctx,
>  	case OP_LOOKUP:
>  		expr_set_type(expr->right, expr->left->dtype,
>  			      expr->left->byteorder);
> -		if (expr->right->dtype == &integer_type)
> -			integer_type_postprocess(expr->right);
> +		switch (expr->right->dtype->type) {
> +		case TYPE_STRING:
> +		case TYPE_INTEGER:
> +			lookup_postprocess(expr->right);
> +			break;
> +		}
>  		break;
>  	default:
>  		break;
> -- 
> 1.7.10.4
> 

Re: [PATCH nft 1/4] datatype: missing byteorder in string_type

[Patrick McHardy]

Am 15. Dezember 2014 12:43:32 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>Hi Patrick!
>
>On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
>> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
>> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso
><pablo@netfilter.org>:
>> > >nft add rule filter input iifname { "lo", "eth0" } counter
>> > >
>> > >Now the listing shows:
>> > >
>> > >	iifname { "lo", "eth0"}
>> > >
>> > >instead of:
>> > >
>> > >	iifname { "", ""}
>> > 
>> > Again wondering what broke this. Let me check when I am at home,
>> > IIRC we have some check for strings somewhere in the netlink code
>> > that relies in this.
>> 
>> Attached an alternative to this patch. That I can remember, this is
>> broken since quite some time.
>
>Any concern with this second approach?
>
>Let me know if you prefer I keep this away from this release.

Sorry, didn't manage to look at it so far. Will manage some time this afternoon.

Cheers,
Patrick

>
>Thanks!
>
>> From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00
>2001
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Sat, 13 Dec 2014 18:29:37 +0100
>> Subject: [PATCH] datatype: fix listing of string elements
>> 
>> Generalise 0451b82 ("src: generate set members using integer_type in
>> the appropriate byteorder") to handle string_type too, since this
>> datatype doesn't have any specific byteorder.
>> 
>> nft add rule filter input iifname { "lo", "eth0" } counter
>> 
>> Now the listing shows:
>> 
>> 	iifname { "lo", "eth0"}
>> 
>> instead of:
>> 
>> 	iifname { "", ""}
>> 
>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> ---
>>  src/netlink_delinearize.c |   12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
>> index e9a04dd..8762cc9 100644
>> --- a/src/netlink_delinearize.c
>> +++ b/src/netlink_delinearize.c
>> @@ -742,7 +742,7 @@ static void payload_dependency_store(struct
>rule_pp_ctx *ctx,
>>  	ctx->pdep  = stmt;
>>  }
>>  
>> -static void integer_type_postprocess(struct expr *expr)
>> +static void lookup_postprocess(struct expr *expr)
>>  {
>>  	struct expr *i;
>>  
>> @@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr
>*expr)
>>  	case EXPR_SET_REF:
>>  		list_for_each_entry(i, &expr->set->init->expressions, list) {
>>  			expr_set_type(i, expr->dtype, expr->byteorder);
>> -			integer_type_postprocess(i);
>> +			lookup_postprocess(i);
>>  		}
>>  		break;
>>  	default:
>> @@ -831,8 +831,12 @@ static void meta_match_postprocess(struct
>rule_pp_ctx *ctx,
>>  	case OP_LOOKUP:
>>  		expr_set_type(expr->right, expr->left->dtype,
>>  			      expr->left->byteorder);
>> -		if (expr->right->dtype == &integer_type)
>> -			integer_type_postprocess(expr->right);
>> +		switch (expr->right->dtype->type) {
>> +		case TYPE_STRING:
>> +		case TYPE_INTEGER:
>> +			lookup_postprocess(expr->right);
>> +			break;
>> +		}
>>  		break;
>>  	default:
>>  		break;
>> -- 
>> 1.7.10.4
>> 

Re: [PATCH nft 1/4] datatype: missing byteorder in string_type

[Patrick McHardy]

Am 15. Dezember 2014 12:43:32 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>Hi Patrick!
>
>On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
>> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
>> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso
><pablo@netfilter.org>:
>> > >nft add rule filter input iifname { "lo", "eth0" } counter
>> > >
>> > >Now the listing shows:
>> > >
>> > >	iifname { "lo", "eth0"}
>> > >
>> > >instead of:
>> > >
>> > >	iifname { "", ""}
>> > 
>> > Again wondering what broke this. Let me check when I am at home,
>> > IIRC we have some check for strings somewhere in the netlink code
>> > that relies in this.
>> 
>> Attached an alternative to this patch. That I can remember, this is
>> broken since quite some time.
>
>Any concern with this second approach?
>
>Let me know if you prefer I keep this away from this release.

Actually the first one is fine, I thought it would affect string postprocessing in delinearization, but it's fine.

>
>Thanks!
>
>> From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00
>2001
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Sat, 13 Dec 2014 18:29:37 +0100
>> Subject: [PATCH] datatype: fix listing of string elements
>> 
>> Generalise 0451b82 ("src: generate set members using integer_type in
>> the appropriate byteorder") to handle string_type too, since this
>> datatype doesn't have any specific byteorder.
>> 
>> nft add rule filter input iifname { "lo", "eth0" } counter
>> 
>> Now the listing shows:
>> 
>> 	iifname { "lo", "eth0"}
>> 
>> instead of:
>> 
>> 	iifname { "", ""}
>> 
>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> ---
>>  src/netlink_delinearize.c |   12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
>> index e9a04dd..8762cc9 100644
>> --- a/src/netlink_delinearize.c
>> +++ b/src/netlink_delinearize.c
>> @@ -742,7 +742,7 @@ static void payload_dependency_store(struct
>rule_pp_ctx *ctx,
>>  	ctx->pdep  = stmt;
>>  }
>>  
>> -static void integer_type_postprocess(struct expr *expr)
>> +static void lookup_postprocess(struct expr *expr)
>>  {
>>  	struct expr *i;
>>  
>> @@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr
>*expr)
>>  	case EXPR_SET_REF:
>>  		list_for_each_entry(i, &expr->set->init->expressions, list) {
>>  			expr_set_type(i, expr->dtype, expr->byteorder);
>> -			integer_type_postprocess(i);
>> +			lookup_postprocess(i);
>>  		}
>>  		break;
>>  	default:
>> @@ -831,8 +831,12 @@ static void meta_match_postprocess(struct
>rule_pp_ctx *ctx,
>>  	case OP_LOOKUP:
>>  		expr_set_type(expr->right, expr->left->dtype,
>>  			      expr->left->byteorder);
>> -		if (expr->right->dtype == &integer_type)
>> -			integer_type_postprocess(expr->right);
>> +		switch (expr->right->dtype->type) {
>> +		case TYPE_STRING:
>> +		case TYPE_INTEGER:
>> +			lookup_postprocess(expr->right);
>> +			break;
>> +		}
>>  		break;
>>  	default:
>>  		break;
>> -- 
>> 1.7.10.4
>> 

Re: [PATCH nft 1/4] datatype: missing byteorder in string_type

[Pablo Neira Ayuso]

On Mon, Dec 15, 2014 at 03:16:19PM +0100, Patrick McHardy wrote:
> Am 15. Dezember 2014 12:43:32 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> >Hi Patrick!
> >
> >On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
> >> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
> >> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso
> ><pablo@netfilter.org>:
> >> > >nft add rule filter input iifname { "lo", "eth0" } counter
> >> > >
> >> > >Now the listing shows:
> >> > >
> >> > >	iifname { "lo", "eth0"}
> >> > >
> >> > >instead of:
> >> > >
> >> > >	iifname { "", ""}
> >> > 
> >> > Again wondering what broke this. Let me check when I am at home,
> >> > IIRC we have some check for strings somewhere in the netlink code
> >> > that relies in this.
> >> 
> >> Attached an alternative to this patch. That I can remember, this is
> >> broken since quite some time.
> >
> >Any concern with this second approach?
> >
> >Let me know if you prefer I keep this away from this release.
> 
> Actually the first one is fine, I thought it would affect string
> postprocessing in delinearization, but it's fine.

Thanks Patrick! I have applied the first patch.

[PATCH nft 2/4] tests: regression: fix wrong number of test files

[Pablo Neira Ayuso]

Always increment the test file counter for each test file in the list.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 tests/regression/nft-test.py |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/regression/nft-test.py b/tests/regression/nft-test.py
index 9998ab3..dea0f36 100755
--- a/tests/regression/nft-test.py
+++ b/tests/regression/nft-test.py
@@ -830,6 +830,8 @@ def main():
         file_errors = result[3]
         file_unit_run = result[4]
 
+        test_files += 1
+
         if file_warnings == 0 and file_tests == file_passed:
             files_ok += 1
         if file_tests:
@@ -837,7 +839,6 @@ def main():
             passed += file_passed
             errors += file_errors
             warnings += file_warnings
-            test_files += 1
         if force_all_family_option:
             run_total += file_unit_run
 
-- 
1.7.10.4

[PATCH nft 3/4] tests: regression: simplify run_test_file() in case `-e' is used

[Pablo Neira Ayuso]

Avoid copy&paste coding style pattern by simplifying the code that
handles the `-e' option that allows us to run known broken tests.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 tests/regression/nft-test.py |   16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/tests/regression/nft-test.py b/tests/regression/nft-test.py
index dea0f36..7e5b475 100755
--- a/tests/regression/nft-test.py
+++ b/tests/regression/nft-test.py
@@ -692,23 +692,9 @@ def run_test_file(filename, force_all_family_option, specific_file):
                 continue
             if need_fix_option:
                 rule[0] = rule[0].rstrip()[1:].strip()
-                result = rule_add(rule, table_list, chain_list, filename,
-                                  lineno, force_all_family_option)
-                tests += 1
-                warning = result[1]
-                ret = result[0]
-                total_warning += warning
-                total_error += result[2]
-                total_unit_run += result[3]
-
-                if ret != 0:
-                    total_test_passed = False
-                elif warning == 0:
-                    passed += 1
-                continue
             else:
                 continue
-        if need_fix_option:
+        elif need_fix_option:
             continue
 
         result = rule_add(rule, table_list, chain_list, filename, lineno,
-- 
1.7.10.4

[PATCH nft 4/4] tests: regression: log.t: this works for bridge and arp since 3.17

[Pablo Neira Ayuso]

So tests log statement for those two families too and remove the
tests/selectors that are ip and ip6 specific, they don't belong here.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 tests/regression/any/log.t |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/regression/any/log.t b/tests/regression/any/log.t
index 2bc2543..0eed580 100644
--- a/tests/regression/any/log.t
+++ b/tests/regression/any/log.t
@@ -1,11 +1,10 @@
 *ip;test-ip4
 *ip6;test-ip6
 *inet;test-inet
-- *arp;test-arp
-- *bridge;test-bridge
+*arp;test-arp
+*bridge;test-bridge
 :output;type filter hook output priority 0
 
-ct direction original log;ok
 log;ok
 log level emerg;ok
 log level alert;ok
@@ -19,9 +18,9 @@ log level debug;ok
 log level emerg group 2;fail
 log level alert group 2 prefix "log test2";fail
 
-udp dport 200 log prefix aaaaa-aaaaaa group 2 snaplen 33;ok;udp dport 200 log prefix "aaaaa-aaaaaa" group 2 snaplen 33
+log prefix aaaaa-aaaaaa group 2 snaplen 33;ok;log prefix "aaaaa-aaaaaa" group 2 snaplen 33
 # TODO: Add an exception: 'queue-threshold' attribute needs 'group' attribute
 # The correct rule is log group 2 queue-threshold 2
 log group 2 queue-threshold 2;ok
 log group 2 snaplen 33;ok
-tcp dport 300 log group 2 prefix \"IPTABLES-NFT SSH\";ok;tcp dport 300 log prefix "IPTABLES-NFT SSH" group 2
+log group 2 prefix \"nft-test: \";ok;log prefix "nft-test: " group 2
-- 
1.7.10.4