ovsdb: Correctly implement conditions that include multiple clauses.

Multiple-clause conditions in OVSDB operations with "where" clauses are
supposed to be conjunctions, that is, the condition is true only if every
clause is true.  In fact, the implementation only checked a single clause
(not necessarily the first one) and ignored the rest.  This fixes the
problem and adds test coverage for multiple-clause conditions.

Reported-by: Shih-Hao Li <shli@nicira.com>

diff --git a/ovsdb/condition.c b/ovsdb/condition.c
index 59f742c952be91a90600e831819520ee32b7c726..c57e419785c393c28aceee21cb2232b3ca7dde18 100644 (file)
--- a/ovsdb/condition.c
+++ b/ovsdb/condition.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010 Nicira Networks
+/* Copyright (c) 2009, 2010, 2011 Nicira Networks
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -217,6 +217,54 @@ ovsdb_condition_to_json(const struct ovsdb_condition *cnd)
     return json_array_create(clauses, cnd->n_clauses);
 }
 
+static bool
+ovsdb_clause_evaluate(const struct ovsdb_row *row,
+                      const struct ovsdb_clause *c)
+{
+    const struct ovsdb_datum *field = &row->fields[c->column->index];
+    const struct ovsdb_datum *arg = &c->arg;
+    const struct ovsdb_type *type = &c->column->type;
+
+    if (ovsdb_type_is_scalar(type)) {
+        int cmp = ovsdb_atom_compare_3way(&field->keys[0], &arg->keys[0],
+                                          type->key.type);
+        switch (c->function) {
+        case OVSDB_F_LT:
+            return cmp < 0;
+        case OVSDB_F_LE:
+            return cmp <= 0;
+        case OVSDB_F_EQ:
+        case OVSDB_F_INCLUDES:
+            return cmp == 0;
+        case OVSDB_F_NE:
+        case OVSDB_F_EXCLUDES:
+            return cmp != 0;
+        case OVSDB_F_GE:
+            return cmp >= 0;
+        case OVSDB_F_GT:
+            return cmp > 0;
+        }
+    } else {
+        switch (c->function) {
+        case OVSDB_F_EQ:
+            return ovsdb_datum_equals(field, arg, type);
+        case OVSDB_F_NE:
+            return !ovsdb_datum_equals(field, arg, type);
+        case OVSDB_F_INCLUDES:
+            return ovsdb_datum_includes_all(arg, field, type);
+        case OVSDB_F_EXCLUDES:
+            return ovsdb_datum_excludes_all(arg, field, type);
+        case OVSDB_F_LT:
+        case OVSDB_F_LE:
+        case OVSDB_F_GE:
+        case OVSDB_F_GT:
+            NOT_REACHED();
+        }
+    }
+
+    NOT_REACHED();
+}
+
 bool
 ovsdb_condition_evaluate(const struct ovsdb_row *row,
                          const struct ovsdb_condition *cnd)
@@ -224,48 +272,9 @@ ovsdb_condition_evaluate(const struct ovsdb_row *row,
     size_t i;
 
     for (i = 0; i < cnd->n_clauses; i++) {
-        const struct ovsdb_clause *c = &cnd->clauses[i];
-        const struct ovsdb_datum *field = &row->fields[c->column->index];
-        const struct ovsdb_datum *arg = &cnd->clauses[i].arg;
-        const struct ovsdb_type *type = &c->column->type;
-
-        if (ovsdb_type_is_scalar(type)) {
-            int cmp = ovsdb_atom_compare_3way(&field->keys[0], &arg->keys[0],
-                                              type->key.type);
-            switch (c->function) {
-            case OVSDB_F_LT:
-                return cmp < 0;
-            case OVSDB_F_LE:
-                return cmp <= 0;
-            case OVSDB_F_EQ:
-            case OVSDB_F_INCLUDES:
-                return cmp == 0;
-            case OVSDB_F_NE:
-            case OVSDB_F_EXCLUDES:
-                return cmp != 0;
-            case OVSDB_F_GE:
-                return cmp >= 0;
-            case OVSDB_F_GT:
-                return cmp > 0;
-            }
-        } else {
-            switch (c->function) {
-            case OVSDB_F_EQ:
-                return ovsdb_datum_equals(field, arg, type);
-            case OVSDB_F_NE:
-                return !ovsdb_datum_equals(field, arg, type);
-            case OVSDB_F_INCLUDES:
-                return ovsdb_datum_includes_all(arg, field, type);
-            case OVSDB_F_EXCLUDES:
-                return ovsdb_datum_excludes_all(arg, field, type);
-            case OVSDB_F_LT:
-            case OVSDB_F_LE:
-            case OVSDB_F_GE:
-            case OVSDB_F_GT:
-                NOT_REACHED();
-            }
+        if (!ovsdb_clause_evaluate(row, &cnd->clauses[i])) {
+            return false;
         }
-        NOT_REACHED();
     }
 
     return true;

diff --git a/tests/ovsdb-condition.at b/tests/ovsdb-condition.at
index 80961cc5d6a314b87267b9507d2b7f79b49347a0..d3d7d83c8fa3e5b93f3148f3536ed021e3d84b1b 100644 (file)
--- a/tests/ovsdb-condition.at
+++ b/tests/ovsdb-condition.at
@@ -203,7 +203,8 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on integers],
       [["i", ">=", 1]],
       [["i", ">", 1]],
       [["i", "includes", 1]],
-      [["i", "excludes", 1]]]' \
+      [["i", "excludes", 1]],
+      [["i", ">", 0], ["i", "<", 2]]]' \
     '[{"i": 0},
       {"i": 1},
       {"i": 2}']]],
@@ -214,7 +215,8 @@ condition  3: T-T
 condition  4: -TT
 condition  5: --T
 condition  6: -T-
-condition  7: T-T], [condition])
+condition  7: T-T
+condition  8: -T-], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating conditions on reals],
   [[evaluate-conditions \
@@ -226,7 +228,8 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on reals],
       [["r", ">=", 5.0]],
       [["r", ">", 5.0]],
       [["r", "includes", 5.0]],
-      [["r", "excludes", 5.0]]]' \
+      [["r", "excludes", 5.0]],
+      [["r", "!=", 0], ["r", "!=", 5.1]]]' \
     '[{"r": 0},
       {"r": 5.0},
       {"r": 5.1}']]],
@@ -237,7 +240,8 @@ condition  3: T-T
 condition  4: -TT
 condition  5: --T
 condition  6: -T-
-condition  7: T-T], [condition])
+condition  7: T-T
+condition  8: -T-], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating conditions on booleans],
   [[evaluate-conditions \
@@ -249,7 +253,8 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on booleans],
       [["b", "==", false]],
       [["b", "!=", false]],
       [["b", "includes", false]],
-      [["b", "excludes", false]]]' \
+      [["b", "excludes", false]],
+      [["b", "==", true], ["b", "==", false]]]' \
     '[{"b": true},
       {"b": false}']]],
   [condition  0: T-
@@ -259,7 +264,8 @@ condition  3: -T
 condition  4: -T
 condition  5: T-
 condition  6: -T
-condition  7: T-], [condition])
+condition  7: T-
+condition  8: --], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating conditions on strings],
   [[evaluate-conditions \
@@ -271,7 +277,8 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on strings],
       [["s", "==", "foo"]],
       [["s", "!=", "foo"]],
       [["s", "includes", "foo"]],
-      [["s", "excludes", "foo"]]]' \
+      [["s", "excludes", "foo"]],
+      [["s", "!=", "foo"], ["s", "!=", ""]]]' \
     '[{"s": ""},
       {"s": "foo"},
       {"s": "xxx"}']]],
@@ -282,7 +289,8 @@ condition  3: -TT
 condition  4: -T-
 condition  5: T-T
 condition  6: -T-
-condition  7: T-T], [condition])
+condition  7: T-T
+condition  8: --T], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating conditions on UUIDs],
   [[evaluate-conditions \
@@ -294,7 +302,9 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on UUIDs],
       [["u", "==", ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]]],
       [["u", "!=", ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]]],
       [["u", "includes", ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]]],
-      [["u", "excludes", ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]]]]' \
+      [["u", "excludes", ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]]],
+      [["u", "!=", ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]],
+       ["u", "!=", ["uuid", "cb160ed6-92a6-4503-a6aa-a09a09e01f0d"]]]]' \
     '[{"u": ["uuid", "8a1dbdb8-416f-4ce9-affa-3332691714b6"]},
       {"u": ["uuid", "06151f9d-62d6-4f59-8504-e9765107faa9"]},
       {"u": ["uuid", "00000000-0000-0000-0000-000000000000"]}']]],
@@ -305,7 +315,8 @@ condition  3: -TT
 condition  4: -T-
 condition  5: T-T
 condition  6: -T-
-condition  7: T-T], [condition])
+condition  7: T-T
+condition  8: T-T], [condition])
 
 OVSDB_CHECK_POSITIVE([evaluating conditions on sets],
   [[evaluate-conditions \
@@ -341,7 +352,9 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on sets],
       [["i", "excludes", ["set", [2]]]],
       [["i", "excludes", ["set", [2, 0]]]],
       [["i", "excludes", ["set", [2, 1]]]],
-      [["i", "excludes", ["set", [2, 1, 0]]]]]' \
+      [["i", "excludes", ["set", [2, 1, 0]]]],
+      [["i", "includes", ["set", [0]]],
+       ["i", "includes", ["set", [1]]]]]' \
     '[{"i": ["set", []]},
       {"i": ["set", [0]]},
       {"i": ["set", [1]]},
@@ -382,7 +395,8 @@ condition 27: T---T ---
 condition 28: TTTT- ---
 condition 29: T-T-- ---
 condition 30: TT--- ---
-condition 31: T---- ---], [condition])
+condition 31: T---- ---
+condition 32: ---T- --T], [condition])
 
 # This is the same as the "set" test except that it adds values,
 # all of which always match.
@@ -423,7 +437,9 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on maps (1)],
       [["i", "excludes", ["map", [[2, true]]]]],
       [["i", "excludes", ["map", [[2, true], [0, true]]]]],
       [["i", "excludes", ["map", [[2, true], [1, false]]]]],
-      [["i", "excludes", ["map", [[2, true], [1, false], [0, true]]]]]]' \
+      [["i", "excludes", ["map", [[2, true], [1, false], [0, true]]]]],
+      [["i", "includes", ["map", [[0, true]]]],
+       ["i", "includes", ["map", [[1, false]]]]]]' \
     '[{"i": ["map", []]},
       {"i": ["map", [[0, true]]]},
       {"i": ["map", [[1, false]]]},
@@ -464,7 +480,8 @@ condition 27: T---T ---
 condition 28: TTTT- ---
 condition 29: T-T-- ---
 condition 30: TT--- ---
-condition 31: T---- ---], [condition])
+condition 31: T---- ---
+condition 32: ---T- --T], [condition])
 
 # This is the same as the "set" test except that it adds values,
 # and those values don't always match.
@@ -505,7 +522,9 @@ OVSDB_CHECK_POSITIVE([evaluating conditions on maps (2)],
       [["i", "excludes", ["map", [[2, true]]]]],
       [["i", "excludes", ["map", [[2, true], [0, true]]]]],
       [["i", "excludes", ["map", [[2, true], [1, false]]]]],
-      [["i", "excludes", ["map", [[2, true], [1, false], [0, true]]]]]]' \
+      [["i", "excludes", ["map", [[2, true], [1, false], [0, true]]]]],
+      [["i", "includes", ["map", [[0, true]]]],
+       ["i", "includes", ["map", [[1, false]]]]]]' \
     '[{"i": ["map", []]},
       {"i": ["map", [[0, true]]]},
       {"i": ["map", [[0, false]]]},
@@ -555,4 +574,5 @@ condition 27: T-T-T --TT- --T--
 condition 28: TTTTT TT-T- T----
 condition 29: T-TTT ---T- -----
 condition 30: TTT-T -T-T- T----
-condition 31: T-T-T ---T- -----], [condition])
+condition 31: T-T-T ---T- -----
+condition 32: ----- T---- ---T-], [condition])