[PATCH nft] expression: fix output of verdict maps

[PATCH nft] expression: fix output of verdict maps

[Pablo Neira Ayuso]

% nft list table filter
table ip filter {
	...
	chain output {
		...
		ip saddr map { 1.1.1.1 => accept}
	}
}

It displays 'map' instead of 'vmap'. Fix it by checking the mapping
type in map_expr_print().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/expression.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/expression.c b/src/expression.c
index 71154cc..97481d0 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -689,7 +689,11 @@ struct expr *mapping_expr_alloc(const struct location *loc,
 static void map_expr_print(const struct expr *expr)
 {
 	expr_print(expr->map);
-	printf(" map ");
+	if (expr->mappings->ops->type == EXPR_SET_REF &&
+	    expr->mappings->set->datatype->type == TYPE_VERDICT)
+		printf(" vmap ");
+	else
+		printf(" map ");
 	expr_print(expr->mappings);
 }
 
-- 
1.7.10.4

Re: [PATCH nft] expression: fix output of verdict maps

[Patrick McHardy]

On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> % nft list table filter
> table ip filter {
> 	...
> 	chain output {
> 		...
> 		ip saddr map { 1.1.1.1 => accept}
> 	}
> }
> 
> It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> type in map_expr_print().

The fix is fine of course, but even better would be to get rid of the
vmap keyword IMO. IIRC it only exists because I couldn't manage to get
a conflict free grammar together, but it would be worth another try.

Re: [PATCH nft] expression: fix output of verdict maps

[Patrick McHardy]

On Mon, Jan 13, 2014 at 12:42:03PM +0000, Patrick McHardy wrote:
> On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> > % nft list table filter
> > table ip filter {
> > 	...
> > 	chain output {
> > 		...
> > 		ip saddr map { 1.1.1.1 => accept}
> > 	}
> > }
> > 
> > It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> > type in map_expr_print().
> 
> The fix is fine of course, but even better would be to get rid of the
> vmap keyword IMO. IIRC it only exists because I couldn't manage to get
> a conflict free grammar together, but it would be worth another try.

Actually this works fine. Is there agreement on removing the VMAP keyword?
If so we should do it before the 0.099 release.

diff --git a/src/parser.y b/src/parser.y
index 7c18875..e5610e5 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -150,7 +150,6 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token DASH			"-"
 %token AT			"@"
 %token ARROW			"=>"
-%token VMAP			"vmap"
 
 %token INCLUDE			"include"
 %token DEFINE			"define"
@@ -1273,7 +1272,7 @@ map_expr		:	concat_expr	MAP	expr
 			}
 			;
 
-verdict_map_expr	:	concat_expr	VMAP	expr
+verdict_map_expr	:	concat_expr	MAP	expr
 			{
 				$$ = map_expr_alloc(&@$, $1, $3);
 			}
diff --git a/src/scanner.l b/src/scanner.l
index 0b8abac..5a5ec72 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -210,7 +210,6 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "$"			{ return '$'; }
 "="			{ return '='; }
 "=>"			{ return ARROW; }
-"vmap"			{ return VMAP; }
 
 "include"		{ return INCLUDE; }
 "define"		{ return DEFINE; }

Re: [PATCH nft] expression: fix output of verdict maps

[Pablo Neira Ayuso]

On Tue, Jan 14, 2014 at 09:00:09AM +0000, Patrick McHardy wrote:
> On Mon, Jan 13, 2014 at 12:42:03PM +0000, Patrick McHardy wrote:
> > On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> > > % nft list table filter
> > > table ip filter {
> > > 	...
> > > 	chain output {
> > > 		...
> > > 		ip saddr map { 1.1.1.1 => accept}
> > > 	}
> > > }
> > > 
> > > It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> > > type in map_expr_print().
> > 
> > The fix is fine of course, but even better would be to get rid of the
> > vmap keyword IMO. IIRC it only exists because I couldn't manage to get
> > a conflict free grammar together, but it would be worth another try.
> 
> Actually this works fine. Is there agreement on removing the VMAP keyword?
> If so we should do it before the 0.099 release.

Sure, go ahead. I like it :) and it's consistent with other data maps.

Re: [PATCH nft] expression: fix output of verdict maps

[Patrick McHardy]

On Tue, Jan 14, 2014 at 12:29:51PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 14, 2014 at 09:00:09AM +0000, Patrick McHardy wrote:
> > On Mon, Jan 13, 2014 at 12:42:03PM +0000, Patrick McHardy wrote:
> > > On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> > > > % nft list table filter
> > > > table ip filter {
> > > > 	...
> > > > 	chain output {
> > > > 		...
> > > > 		ip saddr map { 1.1.1.1 => accept}
> > > > 	}
> > > > }
> > > > 
> > > > It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> > > > type in map_expr_print().
> > > 
> > > The fix is fine of course, but even better would be to get rid of the
> > > vmap keyword IMO. IIRC it only exists because I couldn't manage to get
> > > a conflict free grammar together, but it would be worth another try.
> > 
> > Actually this works fine. Is there agreement on removing the VMAP keyword?
> > If so we should do it before the 0.099 release.
> 
> Sure, go ahead. I like it :) and it's consistent with other data maps.

Crap, I missed that it actually does introduce conflicts. Will try to solve
them or revert that patch.

Re: [PATCH nft] expression: fix output of verdict maps

[Pablo Neira Ayuso]

On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> % nft list table filter
> table ip filter {
> 	...
> 	chain output {
> 		...
> 		ip saddr map { 1.1.1.1 => accept}
> 	}
> }
> 
> It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> type in map_expr_print().

Spinning over the list of pending stuff in my notepad before the
release.

I think it's not so bad if we keep the 'vmap' for verdict maps. This
can just become a synonym of 'map' if we ever find the way to make it
work without ambiguity complains from the parser.

> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/expression.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/expression.c b/src/expression.c
> index 71154cc..97481d0 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -689,7 +689,11 @@ struct expr *mapping_expr_alloc(const struct location *loc,
>  static void map_expr_print(const struct expr *expr)
>  {
>  	expr_print(expr->map);
> -	printf(" map ");
> +	if (expr->mappings->ops->type == EXPR_SET_REF &&
> +	    expr->mappings->set->datatype->type == TYPE_VERDICT)
> +		printf(" vmap ");
> +	else
> +		printf(" map ");
>  	expr_print(expr->mappings);
>  }
>  
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nft] expression: fix output of verdict maps

[Patrick McHardy]

On Thu, Jan 16, 2014 at 05:53:37PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> > % nft list table filter
> > table ip filter {
> > 	...
> > 	chain output {
> > 		...
> > 		ip saddr map { 1.1.1.1 => accept}
> > 	}
> > }
> > 
> > It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> > type in map_expr_print().
> 
> Spinning over the list of pending stuff in my notepad before the
> release.
> 
> I think it's not so bad if we keep the 'vmap' for verdict maps. This
> can just become a synonym of 'map' if we ever find the way to make it
> work without ambiguity complains from the parser.

Yes, absolutely. We should just add your patch to fix the output for now.

Re: [PATCH nft] expression: fix output of verdict maps

[Pablo Neira Ayuso]

On Thu, Jan 16, 2014 at 04:57:53PM +0000, Patrick McHardy wrote:
> On Thu, Jan 16, 2014 at 05:53:37PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 13, 2014 at 01:39:16PM +0100, Pablo Neira Ayuso wrote:
> > > % nft list table filter
> > > table ip filter {
> > > 	...
> > > 	chain output {
> > > 		...
> > > 		ip saddr map { 1.1.1.1 => accept}
> > > 	}
> > > }
> > > 
> > > It displays 'map' instead of 'vmap'. Fix it by checking the mapping
> > > type in map_expr_print().
> > 
> > Spinning over the list of pending stuff in my notepad before the
> > release.
> > 
> > I think it's not so bad if we keep the 'vmap' for verdict maps. This
> > can just become a synonym of 'map' if we ever find the way to make it
> > work without ambiguity complains from the parser.
> 
> Yes, absolutely. We should just add your patch to fix the output for now.

Pushed to master.