On Sun, Sep 10, 2017 at 10:56 AM, Ben Pfaff <[email protected]> wrote: > > On Sat, Sep 09, 2017 at 10:58:25PM -0700, Han Zhou wrote: > > When an address set is empty, current implementation will generate > > an ovs flow that matches random things (and in most cases matching > > everything) due to a problem in expression parser of constant set. > > This patch fixes it by replacing the expression by a boolean false > > when the set is empty, and adds tests cases accordingly. > > > > Reported-by: Guru Shetty <[email protected]> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html > > Signed-off-by: Han Zhou <[email protected]> > > --- > > ovn/lib/expr.c | 4 ++++ > > tests/ovn.at | 9 +++++++++ > > tests/test-ovn.c | 2 ++ > > 3 files changed, 15 insertions(+) > > > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > > index 060e9ee..6a731de 100644 > > --- a/ovn/lib/expr.c > > +++ b/ovn/lib/expr.c > > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx, > > } > > } > > > > + if (!cs->n_values) { > > + e = expr_create_boolean(false); > > + goto exit; > > + } > > Thanks for working on this. I agree with everyone that this is an > important fix. > > I think that it overlooks the behavior of !=. Presumably, == and != > should yield opposite results. To get that behavior, we want: > e = expr_create_boolean(r == EXPR_R_NE); > instead of: > e = expr_create_boolean(false); > > Here's a v2 that includes this change plus tests for !=. I added myself > as coauthor; I hope that's OK. > > Thanks, > > Ben. > > --8<--------------------------cut here-------------------------->8-- > > From: Han Zhou <[email protected]> > Date: Sat, 9 Sep 2017 22:58:25 -0700 > Subject: [PATCH] ovn-controller: Fix empty address set parsing problem. > > When an address set is empty, current implementation will generate > an ovs flow that matches random things (and in most cases matching > everything) due to a problem in expression parser of constant set. > This patch fixes it by replacing the expression by a boolean false > when the set is empty, and adds tests cases accordingly. > > Reported-by: Guru Shetty <[email protected]> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338441.html > Signed-off-by: Han Zhou <[email protected]> > Co-authored-by: Ben Pfaff <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > ovn/lib/expr.c | 4 ++++ > tests/ovn.at | 32 ++++++++++++++++++++++++++++++++ > tests/test-ovn.c | 2 ++ > 3 files changed, 38 insertions(+) > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > index 060e9ee3d088..79ff45762f65 100644 > --- a/ovn/lib/expr.c > +++ b/ovn/lib/expr.c > @@ -612,6 +612,10 @@ make_cmp(struct expr_context *ctx, > } > } > > + if (!cs->n_values) { > + e = expr_create_boolean(r == EXPR_R_NE); > + goto exit; > + } > e = make_cmp__(f, r, &cs->values[0]); > for (size_t i = 1; i < cs->n_values; i++) { > e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR : EXPR_T_AND, > diff --git a/tests/ovn.at b/tests/ovn.at > index bb9999ce0b1b..f2035290f66e 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -623,6 +623,38 @@ dl_src=00:00:00:00:00:02 > dl_src=00:00:00:00:00:03 > dl_src=ba:be:be:ef:de:ad > ]) > +AT_CHECK([expr_to_flow 'ip4.src == {$set4}'], [0], [dnl > +(no flows) > +]) > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set4}'], [0], [dnl > +ip,nw_src=1.2.3.4 > +]) > +AT_CHECK([expr_to_flow 'ip4.src == 1.2.3.4 || ip4.src == {$set4}'], [0], [dnl > +ip,nw_src=1.2.3.4 > +]) > +AT_CHECK([expr_to_flow 'ip4.src != {$set4}'], [0], [dnl > + > +]) > +AT_CHECK([expr_to_flow 'ip4.src != {1.0.0.0/8, $set4}'], [0], [dnl > +ip,nw_src=0.0.0.0/1.0.0.0 > +ip,nw_src=128.0.0.0/1 > +ip,nw_src=16.0.0.0/16.0.0.0 > +ip,nw_src=2.0.0.0/2.0.0.0 > +ip,nw_src=32.0.0.0/32.0.0.0 > +ip,nw_src=4.0.0.0/4.0.0.0 > +ip,nw_src=64.0.0.0/64.0.0.0 > +ip,nw_src=8.0.0.0/8.0.0.0 > +]) > +AT_CHECK([expr_to_flow 'ip4.src != 1.0.0.0/8 && ip4.src != {$set4}'], [0], [dnl > +ip,nw_src=0.0.0.0/1.0.0.0 > +ip,nw_src=128.0.0.0/1 > +ip,nw_src=16.0.0.0/16.0.0.0 > +ip,nw_src=2.0.0.0/2.0.0.0 > +ip,nw_src=32.0.0.0/32.0.0.0 > +ip,nw_src=4.0.0.0/4.0.0.0 > +ip,nw_src=64.0.0.0/64.0.0.0 > +ip,nw_src=8.0.0.0/8.0.0.0 > +]) > AT_CLEANUP > > AT_SETUP([ovn -- action parsing]) > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 694bc794a923..148ce122d385 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -202,10 +202,12 @@ create_addr_sets(struct shash *addr_sets) > static const char *const addrs3[] = { > "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03", > }; > + static const char *const addrs4[] = {}; > > expr_addr_sets_add(addr_sets, "set1", addrs1, 3); > expr_addr_sets_add(addr_sets, "set2", addrs2, 3); > expr_addr_sets_add(addr_sets, "set3", addrs3, 3); > + expr_addr_sets_add(addr_sets, "set4", addrs4, 0); > } > > static bool > -- > 2.10.2 >
Ahh, I missed the case of "!=". Thanks Ben for the quick fix! Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
