* [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type @ 2014-03-17 22:08 Florian Westphal 2014-04-04 8:17 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Florian Westphal @ 2014-03-17 22:08 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal 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(-) 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-03-17 22:08 [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type Florian Westphal @ 2014-04-04 8:17 ` Pablo Neira Ayuso 2014-04-04 8:33 ` Florian Westphal 2014-04-04 12:07 ` Patrick McHardy 0 siblings, 2 replies; 10+ messages in thread From: Pablo Neira Ayuso @ 2014-04-04 8:17 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel, kaber 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 8:17 ` Pablo Neira Ayuso @ 2014-04-04 8:33 ` Florian Westphal 2014-04-04 9:44 ` Pablo Neira Ayuso 2014-04-04 12:07 ` Patrick McHardy 1 sibling, 1 reply; 10+ messages in thread From: Florian Westphal @ 2014-04-04 8:33 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel, kaber 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). ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 8:33 ` Florian Westphal @ 2014-04-04 9:44 ` Pablo Neira Ayuso 2014-04-04 12:09 ` Patrick McHardy 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2014-04-04 9:44 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel, kaber 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 9:44 ` Pablo Neira Ayuso @ 2014-04-04 12:09 ` Patrick McHardy 2014-04-04 14:04 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Patrick McHardy @ 2014-04-04 12:09 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 12:09 ` Patrick McHardy @ 2014-04-04 14:04 ` Pablo Neira Ayuso 2014-04-04 14:24 ` Patrick McHardy 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2014-04-04 14:04 UTC (permalink / raw) To: Patrick McHardy; +Cc: Florian Westphal, netfilter-devel 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 ] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 14:04 ` Pablo Neira Ayuso @ 2014-04-04 14:24 ` Patrick McHardy 2014-04-04 15:23 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Patrick McHardy @ 2014-04-04 14:24 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 14:24 ` Patrick McHardy @ 2014-04-04 15:23 ` Pablo Neira Ayuso 2014-04-04 15:39 ` Florian Westphal 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2014-04-04 15:23 UTC (permalink / raw) To: Patrick McHardy; +Cc: Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 15:23 ` Pablo Neira Ayuso @ 2014-04-04 15:39 ` Florian Westphal 0 siblings, 0 replies; 10+ messages in thread From: Florian Westphal @ 2014-04-04 15:39 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Patrick McHardy, Florian Westphal, netfilter-devel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type 2014-04-04 8:17 ` Pablo Neira Ayuso 2014-04-04 8:33 ` Florian Westphal @ 2014-04-04 12:07 ` Patrick McHardy 1 sibling, 0 replies; 10+ messages in thread From: Patrick McHardy @ 2014-04-04 12:07 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-04-04 15:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-17 22:08 [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type Florian Westphal 2014-04-04 8:17 ` Pablo Neira Ayuso 2014-04-04 8:33 ` Florian Westphal 2014-04-04 9:44 ` Pablo Neira Ayuso 2014-04-04 12:09 ` Patrick McHardy 2014-04-04 14:04 ` Pablo Neira Ayuso 2014-04-04 14:24 ` Patrick McHardy 2014-04-04 15:23 ` Pablo Neira Ayuso 2014-04-04 15:39 ` Florian Westphal 2014-04-04 12:07 ` Patrick McHardy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).