Looks good to me.
Ethan
On Wed, Nov 16, 2011 at 14:39, Ben Pfaff <b...@nicira.com> wrote:
> 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 <s...@nicira.com>
> ---
> ovsdb/condition.c | 93
> +++++++++++++++++++++++++---------------------
> tests/ovsdb-condition.at | 52 ++++++++++++++++++--------
> 2 files changed, 87 insertions(+), 58 deletions(-)
>
> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
> index 59f742c..c57e419 100644
> --- 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 80961cc..d3d7d83 100644
> --- 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])
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
