On Sun, Sep 10, 2017 at 11:10:58AM -0700, Han Zhou wrote: > 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!
You're welcome. I applied this to master, branch-2.8, branch-2.7, and branch-2.6. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
