diff mbox

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

Message ID 20140115112717.GC17728@macbook.localnet
State Accepted
Headers show

Commit Message

Patrick McHardy Jan. 15, 2014, 11:27 a.m. UTC
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:


--
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

Comments

Pablo Neira Ayuso Jan. 15, 2014, 11:41 a.m. UTC | #1
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...
--
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
Patrick McHardy Jan. 15, 2014, 11:49 a.m. UTC | #2
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 :)
--
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
Pablo Neira Ayuso Jan. 15, 2014, 11:59 a.m. UTC | #3
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.
--
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
diff mbox

Patch

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(" ");