diff mbox series

[ovs-dev,1/3] condition: Fix ==, !=, includes, excludes on optional scalars.

Message ID 20180907023012.12752-1-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev,1/3] condition: Fix ==, !=, includes, excludes on optional scalars. | expand

Commit Message

Ben Pfaff Sept. 7, 2018, 2:30 a.m. UTC 
Open vSwitch 2.4 introduced an OVSDB extension in which a column with
type optional integer or real could be compared with the operators <,
<=, >, and >=.  At the same time, it broke the implementation of the
operators ==, !=, includes, and excludes on columns with the same types.
This fixes the problem.

Reported-by: Hans Ole Rafaelsen <hrafaelsen@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-September/047356.html
CC: Terry Wilson <twilson@redhat.com>
Fixes: 09e256031a62 ("ovsdb: Allow comparison on optional scalar types")
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/condition.c        | 29 +++++++++++------------------
 tests/ovsdb-condition.at | 24 ++++++++++++++++++++----
 2 files changed, 31 insertions(+), 22 deletions(-)

Comments

Justin Pettit Oct. 3, 2018, 11:03 p.m. UTC | #1 
> On Sep 6, 2018, at 7:30 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Open vSwitch 2.4 introduced an OVSDB extension in which a column with
> type optional integer or real could be compared with the operators <,
> <=, >, and >=.  At the same time, it broke the implementation of the
> operators ==, !=, includes, and excludes on columns with the same types.
> This fixes the problem.
> 
> Reported-by: Hans Ole Rafaelsen <hrafaelsen@gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-September/047356.html
> CC: Terry Wilson <twilson@redhat.com>
> Fixes: 09e256031a62 ("ovsdb: Allow comparison on optional scalar types")
> Signed-off-by: Ben Pfaff <blp@ovn.org>

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Oct. 4, 2018, 1:22 a.m. UTC | #2 
On Thu, Sep 06, 2018 at 07:30:10PM -0700, Ben Pfaff wrote:
> Open vSwitch 2.4 introduced an OVSDB extension in which a column with
> type optional integer or real could be compared with the operators <,
> <=, >, and >=.  At the same time, it broke the implementation of the
> operators ==, !=, includes, and excludes on columns with the same types.
> This fixes the problem.
> 
> Reported-by: Hans Ole Rafaelsen <hrafaelsen@gmail.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-September/047356.html
> CC: Terry Wilson <twilson@redhat.com>
> Fixes: 09e256031a62 ("ovsdb: Allow comparison on optional scalar types")
> Signed-off-by: Ben Pfaff <blp@ovn.org>

Hans, thanks again for the bug report.  This is now fixed on master and
branch-2.10.
diff mbox series

Patch

diff --git a/ovsdb/condition.c b/ovsdb/condition.c
index f2213ca11f1b..06126d2922e0 100644
--- a/ovsdb/condition.c
+++ b/ovsdb/condition.c
@@ -338,24 +338,17 @@  ovsdb_clause_evaluate(const struct ovsdb_datum *fields,
         c->function == OVSDB_F_FALSE) {
         return c->function == OVSDB_F_TRUE;
     }
-    if (ovsdb_type_is_optional_scalar(type) && field->n == 0) {
-        switch (c->function) {
-            case OVSDB_F_LT:
-            case OVSDB_F_LE:
-            case OVSDB_F_EQ:
-            case OVSDB_F_GE:
-            case OVSDB_F_GT:
-            case OVSDB_F_INCLUDES:
-                return false;
-            case OVSDB_F_NE:
-            case OVSDB_F_EXCLUDES:
-                return true;
-            case OVSDB_F_TRUE:
-            case OVSDB_F_FALSE:
-                OVS_NOT_REACHED();
-        }
-    } else if (ovsdb_type_is_scalar(type)
-               || ovsdb_type_is_optional_scalar(type)) {
+    if (ovsdb_type_is_optional_scalar(type)
+        && field->n == 0
+        && (c->function == OVSDB_F_LT ||
+            c->function == OVSDB_F_LE ||
+            c->function == OVSDB_F_GT ||
+            c->function == OVSDB_F_GE)) {
+        return false;
+    } else if ((ovsdb_type_is_scalar(type)
+                || ovsdb_type_is_optional_scalar(type))
+               && field->n == 1
+               && arg->n == 1) {
         int cmp = ovsdb_atom_compare_3way(&field->keys[0], &arg->keys[0],
                                           type->key.type);
         switch (c->function) {
diff --git a/tests/ovsdb-condition.at b/tests/ovsdb-condition.at
index 4b613e5f62bf..58ff32ebbb16 100644
--- a/tests/ovsdb-condition.at
+++ b/tests/ovsdb-condition.at
@@ -600,7 +600,11 @@  OVSDB_CHECK_POSITIVE([evaluating conditions on optional integers],
       [["i", ">", 1]],
       [["i", "includes", 1]],
       [["i", "excludes", 1]],
-      [["i", ">", 0], ["i", "<", 2]]]' \
+      [["i", ">", 0], ["i", "<", 2]],
+      [["i", "==", ["set", []]]],
+      [["i", "!=", ["set", []]]],
+      [["i", "includes", ["set", []]]],
+      [["i", "excludes", ["set", []]]]]' \
     '[{"i": ["set", []]},
       {"i": ["set", [0]]},
       {"i": ["set", [1]]},
@@ -614,7 +618,11 @@  condition  4: --TT
 condition  5: ---T
 condition  6: --T-
 condition  7: TT-T
-condition  8: --T-], [condition])
+condition  8: --T-
+condition  9: T---
+condition 10: -TTT
+condition 11: TTTT
+condition 12: TTTT], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating conditions on optional strings],
   [[evaluate-conditions \
@@ -654,7 +662,11 @@  OVSDB_CHECK_POSITIVE([evaluating conditions on optional reals],
       [["r", ">", 5.0]],
       [["r", "includes", 5.0]],
       [["r", "excludes", 5.0]],
-      [["r", "!=", 0], ["r", "!=", 5.1]]]' \
+      [["r", "!=", 0], ["r", "!=", 5.1]],
+      [["r", "==", ["set", []]]],
+      [["r", "!=", ["set", []]]],
+      [["r", "includes", ["set", []]]],
+      [["r", "excludes", ["set", []]]]]' \
     '[{"r": ["set", [0]]},
       {"r": ["set", [5.0]]},
       {"r": ["set", [5.1]]},
@@ -668,7 +680,11 @@  condition  4: -TT-
 condition  5: --T-
 condition  6: -T--
 condition  7: T-TT
-condition  8: -T-T], [condition])
+condition  8: -T-T
+condition  9: ---T
+condition 10: TTT-
+condition 11: TTTT
+condition 12: TTTT], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating false boolean condition],
   [[evaluate-conditions-any \