[PATCH 1/2 nft RFC] expression: default to print binary operations using nominal representation

[PATCH 1/2 nft RFC] expression: default to print binary operations using nominal representation

[Pablo Neira Ayuso]

Since b59e65c ("scanner: add aliases to symbols for easier
interaction with most shells") we have nominal aliases, default to
these for the output representation.

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

diff --git a/src/expression.c b/src/expression.c
index 71154cc..6da5c10 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -351,17 +351,17 @@ const char *expr_op_symbols[] = {
 	[OP_INVALID]	= "invalid",
 	[OP_HTON]	= "hton",
 	[OP_NTOH]	= "ntoh",
-	[OP_AND]	= "&",
-	[OP_OR]		= "|",
-	[OP_XOR]	= "^",
-	[OP_LSHIFT]	= "<<",
-	[OP_RSHIFT]	= ">>",
+	[OP_AND]	= "and",
+	[OP_OR]		= "or",
+	[OP_XOR]	= "xor",
+	[OP_LSHIFT]	= "lshift",
+	[OP_RSHIFT]	= "rshift",
 	[OP_EQ]		= NULL,
-	[OP_NEQ]	= "!=",
-	[OP_LT]		= "<",
-	[OP_GT]		= ">",
-	[OP_LTE]	= "<=",
-	[OP_GTE]	= ">=",
+	[OP_NEQ]	= "ne",
+	[OP_LT]		= "lt",
+	[OP_GT]		= "gt",
+	[OP_LTE]	= "le",
+	[OP_GTE]	= "ge",
 	[OP_RANGE]	= "within range",
 	[OP_LOOKUP]	= NULL,
 };
-- 
1.7.10.4

[PATCH 2/2] expression: fix printing of binary operation

[Pablo Neira Ayuso]

This patch adds a special print function for the relational case in
which == is assumed, so it's not printed. It also fixes the output of
binary operations from:

& 0x00000003 0x00000001

to:

and 0x00000003 == 0x00000001

An example output of how thing are left after this patch follows:

% nft list table filter
table ip filter {
        chain output {
                 type filter hook output priority 0;
                 meta mark and 0x00000003 == 0x00000001 counter packets 0 bytes 0
                 meta mark 0x00000003 counter packets 0 bytes 0
        }
}

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

diff --git a/src/expression.c b/src/expression.c
index 6da5c10..452b0d7 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -411,7 +411,9 @@ static void binop_expr_print(const struct expr *expr)
 		printf(" %s ", expr_op_symbols[expr->op]);
 	else
 		printf(" ");
+
 	expr_print(expr->right);
+	printf(" ==");
 }
 
 static void binop_expr_clone(struct expr *new, const struct expr *expr)
@@ -447,10 +449,17 @@ struct expr *binop_expr_alloc(const struct location *loc, enum ops op,
 	return expr;
 }
 
+static void relational_expr_print(const struct expr *expr)
+{
+	expr_print(expr->left);
+	printf(" ");
+	expr_print(expr->right);
+}
+
 static const struct expr_ops relational_expr_ops = {
 	.type		= EXPR_RELATIONAL,
 	.name		= "relational",
-	.print		= binop_expr_print,
+	.print		= relational_expr_print,
 	.destroy	= binop_expr_destroy,
 };
 
-- 
1.7.10.4

Re: [PATCH 2/2] expression: fix printing of binary operation

[Patrick McHardy]

On Wed, Jan 15, 2014 at 12:09:27PM +0100, Pablo Neira Ayuso wrote:
> This patch adds a special print function for the relational case in
> which == is assumed, so it's not printed. It also fixes the output of
> binary operations from:
> 
> & 0x00000003 0x00000001
> 
> to:
> 
> and 0x00000003 == 0x00000001
> 
> diff --git a/src/expression.c b/src/expression.c
> index 6da5c10..452b0d7 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -411,7 +411,9 @@ static void binop_expr_print(const struct expr *expr)
>  		printf(" %s ", expr_op_symbols[expr->op]);
>  	else
>  		printf(" ");
> +
>  	expr_print(expr->right);
> +	printf(" ==");

That doesn't look right, binops can also occur outside of relational
expressions. I'd suggest to special case OP_EQ and not print it by
default unless the LHS is an EXPR_BINOP.

Re: [PATCH 2/2] expression: fix printing of binary operation

[Pablo Neira Ayuso]

On Wed, Jan 15, 2014 at 11:21:29AM +0000, Patrick McHardy wrote:
> On Wed, Jan 15, 2014 at 12:09:27PM +0100, Pablo Neira Ayuso wrote:
> > This patch adds a special print function for the relational case in
> > which == is assumed, so it's not printed. It also fixes the output of
> > binary operations from:
> > 
> > & 0x00000003 0x00000001
> > 
> > to:
> > 
> > and 0x00000003 == 0x00000001
> > 
> > diff --git a/src/expression.c b/src/expression.c
> > index 6da5c10..452b0d7 100644
> > --- a/src/expression.c
> > +++ b/src/expression.c
> > @@ -411,7 +411,9 @@ static void binop_expr_print(const struct expr *expr)
> >  		printf(" %s ", expr_op_symbols[expr->op]);
> >  	else
> >  		printf(" ");
> > +
> >  	expr_print(expr->right);
> > +	printf(" ==");
> 
> That doesn't look right, binops can also occur outside of relational
> expressions. I'd suggest to special case OP_EQ and not print it by
> default unless the LHS is an EXPR_BINOP.

Indeed, this can be !=.

Re: [PATCH 2/2] expression: fix printing of binary operation

[Patrick McHardy]

On Wed, Jan 15, 2014 at 12:25:06PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 15, 2014 at 11:21:29AM +0000, Patrick McHardy wrote:
> > On Wed, Jan 15, 2014 at 12:09:27PM +0100, Pablo Neira Ayuso wrote:
> > > This patch adds a special print function for the relational case in
> > > which == is assumed, so it's not printed. It also fixes the output of
> > > binary operations from:
> > > 
> > > & 0x00000003 0x00000001
> > > 
> > > to:
> > > 
> > > and 0x00000003 == 0x00000001
> > > 
> > > diff --git a/src/expression.c b/src/expression.c
> > > index 6da5c10..452b0d7 100644
> > > --- a/src/expression.c
> > > +++ b/src/expression.c
> > > @@ -411,7 +411,9 @@ static void binop_expr_print(const struct expr *expr)
> > >  		printf(" %s ", expr_op_symbols[expr->op]);
> > >  	else
> > >  		printf(" ");
> > > +
> > >  	expr_print(expr->right);
> > > +	printf(" ==");
> > 
> > That doesn't look right, binops can also occur outside of relational
> > expressions. I'd suggest to special case OP_EQ and not print it by
> > default unless the LHS is an EXPR_BINOP.
> 
> Indeed, this can be !=.

Yep, that as well. The patch I just sent should handle it correctly.

Re: [PATCH 2/2] expression: fix printing of binary operation

[Patrick McHardy]

On Wed, Jan 15, 2014 at 11:21:29AM +0000, Patrick McHardy wrote:
> On Wed, Jan 15, 2014 at 12:09:27PM +0100, Pablo Neira Ayuso wrote:
> > This patch adds a special print function for the relational case in
> > which == is assumed, so it's not printed. It also fixes the output of
> > binary operations from:
> > 
> > & 0x00000003 0x00000001
> > 
> > to:
> > 
> > and 0x00000003 == 0x00000001
> > 
> > diff --git a/src/expression.c b/src/expression.c
> > index 6da5c10..452b0d7 100644
> > --- a/src/expression.c
> > +++ b/src/expression.c
> > @@ -411,7 +411,9 @@ static void binop_expr_print(const struct expr *expr)
> >  		printf(" %s ", expr_op_symbols[expr->op]);
> >  	else
> >  		printf(" ");
> > +
> >  	expr_print(expr->right);
> > +	printf(" ==");
> 
> That doesn't look right, binops can also occur outside of relational
> expressions. I'd suggest to special case OP_EQ and not print it by
> default unless the LHS is an EXPR_BINOP.

Something like this:


diff --git a/src/expression.c b/src/expression.c
index 71154cc..518f71c 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -356,7 +356,7 @@ const char *expr_op_symbols[] = {
 	[OP_XOR]	= "^",
 	[OP_LSHIFT]	= "<<",
 	[OP_RSHIFT]	= ">>",
-	[OP_EQ]		= NULL,
+	[OP_EQ]		= "==",
 	[OP_NEQ]	= "!=",
 	[OP_LT]		= "<",
 	[OP_GT]		= ">",
@@ -407,7 +407,9 @@ struct expr *unary_expr_alloc(const struct location *loc,
 static void binop_expr_print(const struct expr *expr)
 {
 	expr_print(expr->left);
-	if (expr_op_symbols[expr->op] != NULL)
+	if (expr_op_symbols[expr->op] &&
+	    (expr->op != OP_EQ ||
+	     expr->left->ops->type == EXPR_BINOP))
 		printf(" %s ", expr_op_symbols[expr->op]);
 	else
 		printf(" ");

Re: [PATCH 2/2] expression: fix printing of binary operation

[Pablo Neira Ayuso]

On Wed, Jan 15, 2014 at 11:27:17AM +0000, Patrick McHardy wrote:
> On Wed, Jan 15, 2014 at 11:21:29AM +0000, Patrick McHardy wrote:
> > On Wed, Jan 15, 2014 at 12:09:27PM +0100, Pablo Neira Ayuso wrote:
> > > This patch adds a special print function for the relational case in
> > > which == is assumed, so it's not printed. It also fixes the output of
> > > binary operations from:
> > > 
> > > & 0x00000003 0x00000001
> > > 
> > > to:
> > > 
> > > and 0x00000003 == 0x00000001
> > > 
> > > diff --git a/src/expression.c b/src/expression.c
> > > index 6da5c10..452b0d7 100644
> > > --- a/src/expression.c
> > > +++ b/src/expression.c
> > > @@ -411,7 +411,9 @@ static void binop_expr_print(const struct expr *expr)
> > >  		printf(" %s ", expr_op_symbols[expr->op]);
> > >  	else
> > >  		printf(" ");
> > > +
> > >  	expr_print(expr->right);
> > > +	printf(" ==");
> > 
> > That doesn't look right, binops can also occur outside of relational
> > expressions. I'd suggest to special case OP_EQ and not print it by
> > default unless the LHS is an EXPR_BINOP.
> 
> Something like this:
> 
> 
> diff --git a/src/expression.c b/src/expression.c
> index 71154cc..518f71c 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -356,7 +356,7 @@ const char *expr_op_symbols[] = {
>  	[OP_XOR]	= "^",
>  	[OP_LSHIFT]	= "<<",
>  	[OP_RSHIFT]	= ">>",
> -	[OP_EQ]		= NULL,
> +	[OP_EQ]		= "==",
>  	[OP_NEQ]	= "!=",
>  	[OP_LT]		= "<",
>  	[OP_GT]		= ">",
> @@ -407,7 +407,9 @@ struct expr *unary_expr_alloc(const struct location *loc,
>  static void binop_expr_print(const struct expr *expr)
>  {
>  	expr_print(expr->left);
> -	if (expr_op_symbols[expr->op] != NULL)
> +	if (expr_op_symbols[expr->op] &&
> +	    (expr->op != OP_EQ ||
> +	     expr->left->ops->type == EXPR_BINOP))
>  		printf(" %s ", expr_op_symbols[expr->op]);
>  	else
>  		printf(" ");

This looks a bit more complicated. To my understanding, the right-hand
side of the relational tree contains the value. The left-hand side
contains the binop tree, whose left-hand side is the meta mark and the
right-hand side is the value to apply the operation. The print
function doesn't have context to know what's on the right-hand side of
the upper relational expression. Thinking how to fix this...

Re: [PATCH 2/2] expression: fix printing of binary operation

[Patrick McHardy]

On Wed, Jan 15, 2014 at 12:41:34PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 15, 2014 at 11:27:17AM +0000, Patrick McHardy wrote:
> > On Wed, Jan 15, 2014 at 11:21:29AM +0000, Patrick McHardy wrote:
> > > 
> > > That doesn't look right, binops can also occur outside of relational
> > > expressions. I'd suggest to special case OP_EQ and not print it by
> > > default unless the LHS is an EXPR_BINOP.
> > 
> > Something like this:
> > 
> > 
> > diff --git a/src/expression.c b/src/expression.c
> > index 71154cc..518f71c 100644
> > --- a/src/expression.c
> > +++ b/src/expression.c
> > @@ -356,7 +356,7 @@ const char *expr_op_symbols[] = {
> >  	[OP_XOR]	= "^",
> >  	[OP_LSHIFT]	= "<<",
> >  	[OP_RSHIFT]	= ">>",
> > -	[OP_EQ]		= NULL,
> > +	[OP_EQ]		= "==",
> >  	[OP_NEQ]	= "!=",
> >  	[OP_LT]		= "<",
> >  	[OP_GT]		= ">",
> > @@ -407,7 +407,9 @@ struct expr *unary_expr_alloc(const struct location *loc,
> >  static void binop_expr_print(const struct expr *expr)
> >  {
> >  	expr_print(expr->left);
> > -	if (expr_op_symbols[expr->op] != NULL)
> > +	if (expr_op_symbols[expr->op] &&
> > +	    (expr->op != OP_EQ ||
> > +	     expr->left->ops->type == EXPR_BINOP))
> >  		printf(" %s ", expr_op_symbols[expr->op]);
> >  	else
> >  		printf(" ");
> 
> This looks a bit more complicated. To my understanding, the right-hand
> side of the relational tree contains the value. The left-hand side
> contains the binop tree, whose left-hand side is the meta mark and the
> right-hand side is the value to apply the operation. The print
> function doesn't have context to know what's on the right-hand side of
> the upper relational expression. Thinking how to fix this...

This OP_EQ case is the upper relational expression. Try it, it works fine :)

Re: [PATCH 2/2] expression: fix printing of binary operation

[Pablo Neira Ayuso]

On Wed, Jan 15, 2014 at 11:49:39AM +0000, Patrick McHardy wrote:
> On Wed, Jan 15, 2014 at 12:41:34PM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Jan 15, 2014 at 11:27:17AM +0000, Patrick McHardy wrote:
> > > On Wed, Jan 15, 2014 at 11:21:29AM +0000, Patrick McHardy wrote:
> > > > 
> > > > That doesn't look right, binops can also occur outside of relational
> > > > expressions. I'd suggest to special case OP_EQ and not print it by
> > > > default unless the LHS is an EXPR_BINOP.
> > > 
> > > Something like this:
> > > 
> > > 
> > > diff --git a/src/expression.c b/src/expression.c
> > > index 71154cc..518f71c 100644
> > > --- a/src/expression.c
> > > +++ b/src/expression.c
> > > @@ -356,7 +356,7 @@ const char *expr_op_symbols[] = {
> > >  	[OP_XOR]	= "^",
> > >  	[OP_LSHIFT]	= "<<",
> > >  	[OP_RSHIFT]	= ">>",
> > > -	[OP_EQ]		= NULL,
> > > +	[OP_EQ]		= "==",
> > >  	[OP_NEQ]	= "!=",
> > >  	[OP_LT]		= "<",
> > >  	[OP_GT]		= ">",
> > > @@ -407,7 +407,9 @@ struct expr *unary_expr_alloc(const struct location *loc,
> > >  static void binop_expr_print(const struct expr *expr)
> > >  {
> > >  	expr_print(expr->left);
> > > -	if (expr_op_symbols[expr->op] != NULL)
> > > +	if (expr_op_symbols[expr->op] &&
> > > +	    (expr->op != OP_EQ ||
> > > +	     expr->left->ops->type == EXPR_BINOP))
> > >  		printf(" %s ", expr_op_symbols[expr->op]);
> > >  	else
> > >  		printf(" ");
> > 
> > This looks a bit more complicated. To my understanding, the right-hand
> > side of the relational tree contains the value. The left-hand side
> > contains the binop tree, whose left-hand side is the meta mark and the
> > right-hand side is the value to apply the operation. The print
> > function doesn't have context to know what's on the right-hand side of
> > the upper relational expression. Thinking how to fix this...
> 
> This OP_EQ case is the upper relational expression. Try it, it works fine :)

Indeed, thanks Patrick.

Re: [PATCH 1/2 nft RFC] expression: default to print binary operations using nominal representation

[Patrick McHardy]

On Wed, Jan 15, 2014 at 12:09:26PM +0100, Pablo Neira Ayuso wrote:
> Since b59e65c ("scanner: add aliases to symbols for easier
> interaction with most shells") we have nominal aliases, default to
> these for the output representation.

I was thinking about this before, but didn't mention it since you
hadn't included it in your first patch. I'd prefer to stick to the
IMO more readable existing form. Since we don't support deleting
rules by rule specification, there should be no reason to copy and
paste the output to the command line again.

> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/expression.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/expression.c b/src/expression.c
> index 71154cc..6da5c10 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -351,17 +351,17 @@ const char *expr_op_symbols[] = {
>  	[OP_INVALID]	= "invalid",
>  	[OP_HTON]	= "hton",
>  	[OP_NTOH]	= "ntoh",
> -	[OP_AND]	= "&",
> -	[OP_OR]		= "|",
> -	[OP_XOR]	= "^",
> -	[OP_LSHIFT]	= "<<",
> -	[OP_RSHIFT]	= ">>",
> +	[OP_AND]	= "and",
> +	[OP_OR]		= "or",
> +	[OP_XOR]	= "xor",
> +	[OP_LSHIFT]	= "lshift",
> +	[OP_RSHIFT]	= "rshift",
>  	[OP_EQ]		= NULL,
> -	[OP_NEQ]	= "!=",
> -	[OP_LT]		= "<",
> -	[OP_GT]		= ">",
> -	[OP_LTE]	= "<=",
> -	[OP_GTE]	= ">=",
> +	[OP_NEQ]	= "ne",
> +	[OP_LT]		= "lt",
> +	[OP_GT]		= "gt",
> +	[OP_LTE]	= "le",
> +	[OP_GTE]	= "ge",
>  	[OP_RANGE]	= "within range",
>  	[OP_LOOKUP]	= NULL,
>  };
> -- 
> 1.7.10.4

Re: [PATCH 1/2 nft RFC] expression: default to print binary operations using nominal representation

[Arturo Borrero Gonzalez]

On 15 January 2014 12:18, Patrick McHardy <kaber@trash.net> wrote:
> On Wed, Jan 15, 2014 at 12:09:26PM +0100, Pablo Neira Ayuso wrote:
>> Since b59e65c ("scanner: add aliases to symbols for easier
>> interaction with most shells") we have nominal aliases, default to
>> these for the output representation.
>
> I was thinking about this before, but didn't mention it since you
> hadn't included it in your first patch. I'd prefer to stick to the
> IMO more readable existing form. Since we don't support deleting
> rules by rule specification, there should be no reason to copy and
> paste the output to the command line again.

What about a file to be loaded with `nft -f'?
In fact, I think most of users will end with a config file to load
with `nft -f'.

It may be annoying getting the output different to what you wrote.

I think this patch means that the symbol-free syntax is the default,
and I personally like that.
-- 
Arturo Borrero González
--
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 1/2 nft RFC] expression: default to print binary operations using nominal representation

[Patrick McHardy]

On Wed, Jan 15, 2014 at 12:32:36PM +0100, Arturo Borrero Gonzalez wrote:
> On 15 January 2014 12:18, Patrick McHardy <kaber@trash.net> wrote:
> > On Wed, Jan 15, 2014 at 12:09:26PM +0100, Pablo Neira Ayuso wrote:
> >> Since b59e65c ("scanner: add aliases to symbols for easier
> >> interaction with most shells") we have nominal aliases, default to
> >> these for the output representation.
> >
> > I was thinking about this before, but didn't mention it since you
> > hadn't included it in your first patch. I'd prefer to stick to the
> > IMO more readable existing form. Since we don't support deleting
> > rules by rule specification, there should be no reason to copy and
> > paste the output to the command line again.
> 
> What about a file to be loaded with `nft -f'?
> In fact, I think most of users will end with a config file to load
> with `nft -f'.

Indeed, and those users don't need the long syntax since the shell is
not involved.

> It may be annoying getting the output different to what you wrote.

That may happen for other reasons anyways, especially with binops we
optimize away redundant expressions and calculate constants in userspace.

> I think this patch means that the symbol-free syntax is the default,
> and I personally like that.

I don't, its longer and harder to read. It has its justification for
parsing on the command line, but as you say, most users won't use it
that way.