| Message ID | 1395094118-32580-1-git-send-email-fw@strlen.de |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > rule filter output tcp flags syn > rule filter output tcp flags == syn > > are both displayed as 'flags syn'. I believe that in other selectors: selector == value selector value are equivalent. I think it's not just that we have to fix the printing, but make it consistent. > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > src/expression.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/expression.c b/src/expression.c > index 1313925..fa14d99 100644 > --- a/src/expression.c > +++ b/src/expression.c > @@ -514,13 +514,21 @@ static void binop_arg_print(const struct expr *op, const struct expr *arg) > printf(")"); > } > > +static bool must_print_eq_op(const struct expr *expr) > +{ > + if (expr->right->dtype->basetype != NULL && > + expr->right->dtype->basetype->type == TYPE_BITMASK) > + return true; > + > + return expr->left->ops->type == EXPR_BINOP; > +} > + > static void binop_expr_print(const struct expr *expr) > { > binop_arg_print(expr, expr->left); > > if (expr_op_symbols[expr->op] && > - (expr->op != OP_EQ || > - expr->left->ops->type == EXPR_BINOP)) > + (expr->op != OP_EQ || must_print_eq_op(expr))) > printf(" %s ", expr_op_symbols[expr->op]); > else > printf(" "); > -- > 1.8.1.5 > > -- > 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 -- 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 <pablo@netfilter.org> wrote: > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > > > rule filter output tcp flags syn > > rule filter output tcp flags == syn > > > > are both displayed as 'flags syn'. > > I believe that in other selectors: > > selector == value > selector value > > are equivalent. Yes, thats true. > I think it's not just that we have to fix the printing, but make it > consistent. Not sure, this was changed recently, see 6bad82aba5d304c7a2dd1b19fe57464dca327f4a (evaluate: use flagcmp for single RHS bitmask expression). -- 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
On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > > > > > rule filter output tcp flags syn > > > rule filter output tcp flags == syn > > > > > > are both displayed as 'flags syn'. > > > > I believe that in other selectors: > > > > selector == value > > selector value > > > > are equivalent. > > Yes, thats true. > > > I think it's not just that we have to fix the printing, but make it > > consistent. > > Not sure, this was changed recently, see > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a > (evaluate: use flagcmp for single RHS bitmask expression). That change is fine, I think we only have to fix tcp flags == syn to make it equivalent to tcp flags syn. I don't find a good reason why the should behave in a different way. -- 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
On Fri, Apr 04, 2014 at 10:17:23AM +0200, Pablo Neira Ayuso wrote: > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > > > rule filter output tcp flags syn > > rule filter output tcp flags == syn > > > > are both displayed as 'flags syn'. > > I believe that in other selectors: > > selector == value > selector value > > are equivalent. No, it really depends on the base type. For bitmasks the equality relation should be printed explicitly. > I think it's not just that we have to fix the printing, but make it > consistent. What do you propose to change? -- 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
On Fri, Apr 04, 2014 at 11:44:33AM +0200, Pablo Neira Ayuso wrote: > On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > > > > > > > rule filter output tcp flags syn > > > > rule filter output tcp flags == syn > > > > > > > > are both displayed as 'flags syn'. > > > > > > I believe that in other selectors: > > > > > > selector == value > > > selector value > > > > > > are equivalent. > > > > Yes, thats true. > > > > > I think it's not just that we have to fix the printing, but make it > > > consistent. > > > > Not sure, this was changed recently, see > > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a > > (evaluate: use flagcmp for single RHS bitmask expression). > > That change is fine, I think we only have to fix tcp flags == syn to > make it equivalent to tcp flags syn. I don't find a good reason why > the should behave in a different way. Because the implicit op for bitmasks is to test for any of the given bits. tcp flags syn really should match on syn and syn/ack. If an equality relation is explicitly specified by the user, it also needs to be printed. Florian's change is all we need from what I can tell. -- 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
On Fri, Apr 04, 2014 at 02:09:48PM +0200, Patrick McHardy wrote: > On Fri, Apr 04, 2014 at 11:44:33AM +0200, Pablo Neira Ayuso wrote: > > On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote: > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > > > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > > > > > > > > > rule filter output tcp flags syn > > > > > rule filter output tcp flags == syn > > > > > > > > > > are both displayed as 'flags syn'. > > > > > > > > I believe that in other selectors: > > > > > > > > selector == value > > > > selector value > > > > > > > > are equivalent. > > > > > > Yes, thats true. > > > > > > > I think it's not just that we have to fix the printing, but make it > > > > consistent. > > > > > > Not sure, this was changed recently, see > > > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a > > > (evaluate: use flagcmp for single RHS bitmask expression). > > > > That change is fine, I think we only have to fix tcp flags == syn to > > make it equivalent to tcp flags syn. I don't find a good reason why > > the should behave in a different way. > > Because the implicit op for bitmasks is to test for any of the given bits. > tcp flags syn really should match on syn and syn/ack. That's fine with me. The problem that I see is the inconsistent interpretation depending on if the value is a flag or not. > If an equality relation is explicitly specified by the user, it also needs > to be printed. Florian's change is all we need from what I can tell. Then we have to document that in some cases key == value and key value are equivalent, and when it comes to flags it is not, which is still rare to me. nft --debug=netlink add rule filter output tcp flags == syn ip filter output [ payload load 1b @ network header + 9 => reg 1 ] [ cmp eq reg 1 0x00000006 ] [ payload load 1b @ transport header + 13 => reg 1 ] [ cmp eq reg 1 0x00000002 ] nft --debug=netlink add rule filter output tcp flags syn ip filter output [ payload load 1b @ network header + 9 => reg 1 ] [ cmp eq reg 1 0x00000006 ] [ payload load 1b @ transport header + 13 => reg 1 ] [ bitwise reg 1 = (reg=1 & 0x00000002 ) ^ 0x00000000 ] [ cmp neq reg 1 0x00000000 ] -- 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
On Fri, Apr 04, 2014 at 04:04:30PM +0200, Pablo Neira Ayuso wrote: > On Fri, Apr 04, 2014 at 02:09:48PM +0200, Patrick McHardy wrote: > > On Fri, Apr 04, 2014 at 11:44:33AM +0200, Pablo Neira Ayuso wrote: > > > On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote: > > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote: > > > > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, > > > > > > > > > > > > rule filter output tcp flags syn > > > > > > rule filter output tcp flags == syn > > > > > > > > > > > > are both displayed as 'flags syn'. > > > > > > > > > > I believe that in other selectors: > > > > > > > > > > selector == value > > > > > selector value > > > > > > > > > > are equivalent. > > > > > > > > Yes, thats true. > > > > > > > > > I think it's not just that we have to fix the printing, but make it > > > > > consistent. > > > > > > > > Not sure, this was changed recently, see > > > > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a > > > > (evaluate: use flagcmp for single RHS bitmask expression). > > > > > > That change is fine, I think we only have to fix tcp flags == syn to > > > make it equivalent to tcp flags syn. I don't find a good reason why > > > the should behave in a different way. > > > > Because the implicit op for bitmasks is to test for any of the given bits. > > tcp flags syn really should match on syn and syn/ack. > > That's fine with me. The problem that I see is the inconsistent > interpretation depending on if the value is a flag or not. I don't see it as inconsistent, its just that the implicit op can mean different things. Its basically meant to "do the right thing", which IMO is the current behaviour. For me tcp flags syn also matching on syn/ack is what I'd expect. > > If an equality relation is explicitly specified by the user, it also needs > > to be printed. Florian's change is all we need from what I can tell. > > Then we have to document that in some cases key == value and key > value are equivalent, and when it comes to flags it is not, which is > still rare to me. Sure. I think I already have it in my documentation. Its basically simply documenting what the implicit op means in which context. -- 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
On Fri, Apr 04, 2014 at 04:24:40PM +0200, Patrick McHardy wrote: > > > If an equality relation is explicitly specified by the user, it also needs > > > to be printed. Florian's change is all we need from what I can tell. > > > > Then we have to document that in some cases key == value and key > > value are equivalent, and when it comes to flags it is not, which is > > still rare to me. > > Sure. I think I already have it in my documentation. Its basically simply > documenting what the implicit op means in which context. OK, please go ahead push it, thanks. -- 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 <pablo@netfilter.org> wrote: > On Fri, Apr 04, 2014 at 04:24:40PM +0200, Patrick McHardy wrote: > > > > If an equality relation is explicitly specified by the user, it also needs > > > > to be printed. Florian's change is all we need from what I can tell. > > > > > > Then we have to document that in some cases key == value and key > > > value are equivalent, and when it comes to flags it is not, which is > > > still rare to me. > > > > Sure. I think I already have it in my documentation. Its basically simply > > documenting what the implicit op means in which context. > > OK, please go ahead push it, thanks. done. -- 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 --git a/src/expression.c b/src/expression.c index 1313925..fa14d99 100644 --- a/src/expression.c +++ b/src/expression.c @@ -514,13 +514,21 @@ static void binop_arg_print(const struct expr *op, const struct expr *arg) printf(")"); } +static bool must_print_eq_op(const struct expr *expr) +{ + if (expr->right->dtype->basetype != NULL && + expr->right->dtype->basetype->type == TYPE_BITMASK) + return true; + + return expr->left->ops->type == EXPR_BINOP; +} + static void binop_expr_print(const struct expr *expr) { binop_arg_print(expr, expr->left); if (expr_op_symbols[expr->op] && - (expr->op != OP_EQ || - expr->left->ops->type == EXPR_BINOP)) + (expr->op != OP_EQ || must_print_eq_op(expr))) printf(" %s ", expr_op_symbols[expr->op]); else printf(" ");
bitmask types default to flagcmp now, thus do not suppress OP_EQ. Else, rule filter output tcp flags syn rule filter output tcp flags == syn are both displayed as 'flags syn'. Signed-off-by: Florian Westphal <fw@strlen.de> --- src/expression.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)