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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
