On Sat, Jan 16, 2016 at 12:16 AM, Liran Schour <lir...@il.ibm.com> wrote:
> <condition> now is a 3-element json array or a boolean value, see
> ovsdb-server(1) man page. This functions will be used for
> conditional monitoring sessions.
>
> Signed-off-by: Liran Schour <lir...@il.ibm.com>
> ---
> ovsdb/condition.c | 270
> +++++++++++++++++++++++++++++++++++++++++++++--
> ovsdb/condition.h | 28 ++++-
> ovsdb/query.c | 4 +-
> tests/ovsdb-condition.at | 35 +++++-
> tests/test-ovsdb.c | 29 ++++-
> 5 files changed, 348 insertions(+), 18 deletions(-)
>
> From the last review:
> Should any columns within schema be allowed in condition? or only
> the ones being monitored?
> From protocol viewpoint, it would make sense to allow any column.
>
Yes, any columns should be allowed in condition.
I did not find the implementation to match this. The values used to
conditional comparison is coming
from the monitored table rows, this is required to be part of the monitored
columns? Also tested and verified
that monitor_cond does not work if where clause contains columns not being
monitored.
It would be nice to clearly define which columns are allowed in the where
clause.
I suppose if the implementation actually allows any column, than there may
not be a need for
'columns_index_map' introduced from the last patch.
The commit message is slightly misleading since ovsdb-server(1) changes are
not contained within this patch,
but in a later patch.
> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
> index 4baf1bb..3e1cc19 100644
> --- a/ovsdb/condition.c
> +++ b/ovsdb/condition.c
> @@ -19,6 +19,7 @@
>
> #include <limits.h>
>
> +#include "bitmap.h"
> #include "column.h"
> #include "json.h"
> #include "ovsdb-error.h"
> @@ -64,6 +65,17 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema
> *ts,
> const char *column_name;
> struct ovsdb_type type;
>
> + if (json->type == JSON_TRUE || json->type == JSON_FALSE) {
> + function_name = (json->type == JSON_TRUE) ? "true" : "false";
> + error = ovsdb_function_from_string(function_name,
> &clause->function);
> +
> + /* column and arg fields are not being used with boolean function
> + * use dummy values */
> + clause->column = ovsdb_table_schema_get_column(ts, "_uuid");
> + ovsdb_datum_init_default(&clause->arg, &clause->column->type);
> + return error;
> + }
> +
>
It seems we can simply drop any FALSE if there are more than one clauses,
because
of the or conditions. On the other hand, the presence to True can be
reduced to an empty
condition..
To handle condition changes, you may still want to finish the JSON
conversions, or define the protocol
in such way that the modification of a TRUE condition the same as modifying
an empty condition.
If JSON_TRUE and JSON_FALSE has been removed, this function can just expect
a JSON_ARRAY.
> if (json->type != JSON_ARRAY
> || json->u.array.n != 3
> || json->u.array.elems[0]->type != JSON_STRING
> @@ -109,7 +121,8 @@ ovsdb_clause_from_json(const struct ovsdb_table_schema
> *ts,
> return error;
> }
> break;
> -
> + case OVSDB_F_TRUE:
> + case OVSDB_F_FALSE:
> case OVSDB_F_EQ:
> case OVSDB_F_NE:
> break;
> @@ -164,6 +177,19 @@ compare_clauses_3way(const void *a_, const void *b_)
> }
> }
>
> +static int
> +compare_clauses_3way_with_data(const void *a_, const void *b_)
> +{
> + const struct ovsdb_clause *a = a_;
> + const struct ovsdb_clause *b = b_;
> + int res;
> +
> + res = compare_clauses_3way(a, b);
> + return res ? res : ovsdb_datum_compare_3way(&a->arg,
> + &b->arg,
> + &a->column->type);
> + }
> +
> struct ovsdb_error *
> ovsdb_condition_from_json(const struct ovsdb_table_schema *ts,
> const struct json *json,
>
Should we make sure there is not duplicated clauses here?
> @@ -173,7 +199,7 @@ ovsdb_condition_from_json(const struct
> ovsdb_table_schema *ts,
> const struct json_array *array = json_array(json);
> size_t i;
>
> - cnd->clauses = xmalloc(array->n * sizeof *cnd->clauses);
> + cnd->clauses = xzalloc(array->n * sizeof *cnd->clauses);
> cnd->n_clauses = 0;
> for (i = 0; i < array->n; i++) {
> struct ovsdb_error *error;
> @@ -198,10 +224,16 @@ ovsdb_condition_from_json(const struct
> ovsdb_table_schema *ts,
> static struct json *
> ovsdb_clause_to_json(const struct ovsdb_clause *clause)
> {
> - return json_array_create_3(
> - json_string_create(clause->column->name),
> - json_string_create(ovsdb_function_to_string(clause->function)),
> - ovsdb_datum_to_json(&clause->arg, &clause->column->type));
> + if (clause->function != OVSDB_F_TRUE &&
> + clause->function != OVSDB_F_FALSE) {
> + return json_array_create_3(
> + json_string_create(clause->column->name),
> +
> json_string_create(ovsdb_function_to_string(clause->function)),
> + ovsdb_datum_to_json(&clause->arg, &clause->column->type));
> + }
> +
> + return json_boolean_create(clause->function == OVSDB_F_TRUE ?
> + true : false);
> }
>
> struct json *
> @@ -218,13 +250,22 @@ ovsdb_condition_to_json(const struct ovsdb_condition
> *cnd)
> }
>
> static bool
> -ovsdb_clause_evaluate(const struct ovsdb_row *row,
> - const struct ovsdb_clause *c)
> +ovsdb_clause_evaluate(const struct ovsdb_datum *fields,
> + const struct ovsdb_clause *c,
> + const unsigned int columns_index_map[])
> {
> - const struct ovsdb_datum *field = &row->fields[c->column->index];
> + if (c->function == OVSDB_F_TRUE || c->function == OVSDB_F_FALSE) {
> + return c->function == OVSDB_F_TRUE ? true : false;
> + }
> +
> + const struct ovsdb_datum *field;
> const struct ovsdb_datum *arg = &c->arg;
> const struct ovsdb_type *type = &c->column->type;
>
> + field = !columns_index_map ?
> + &fields[c->column->index] :
> + &fields[columns_index_map[c->column->index]];
> +
> if (ovsdb_type_is_optional_scalar(type) && field->n == 0) {
> switch (c->function) {
> case OVSDB_F_LT:
> @@ -237,6 +278,9 @@ ovsdb_clause_evaluate(const struct ovsdb_row *row,
> 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)) {
> @@ -257,6 +301,9 @@ ovsdb_clause_evaluate(const struct ovsdb_row *row,
> return cmp >= 0;
> case OVSDB_F_GT:
> return cmp > 0;
> + case OVSDB_F_TRUE:
> + case OVSDB_F_FALSE:
> + OVS_NOT_REACHED();
> }
> } else {
> switch (c->function) {
> @@ -272,6 +319,8 @@ ovsdb_clause_evaluate(const struct ovsdb_row *row,
> case OVSDB_F_LE:
> case OVSDB_F_GE:
> case OVSDB_F_GT:
> + case OVSDB_F_TRUE:
> + case OVSDB_F_FALSE:
> OVS_NOT_REACHED();
> }
> }
> @@ -279,14 +328,42 @@ ovsdb_clause_evaluate(const struct ovsdb_row *row,
> OVS_NOT_REACHED();
> }
>
> +static int
> +ovsdb_clause_exists(const struct ovsdb_condition *cnd,
> + const struct ovsdb_clause *clause)
> +{
> + size_t i;
> +
> + for (i=0; i < cnd->n_clauses; i++) {
> + if(!compare_clauses_3way_with_data(&cnd->clauses[i], clause)) {
> + return i;
> + }
> + }
> +
> + return -1;
> +}
> +
> +static void
> +ovsdb_clone_clause(struct ovsdb_clause *new, struct ovsdb_clause *old)
> +{
> + new->function = old->function;
> + new->column = old->column;
> + ovsdb_datum_clone(&new->arg,
> + &old->arg,
> + &old->column->type);
> +}
> +
> bool
> ovsdb_condition_evaluate(const struct ovsdb_row *row,
> - const struct ovsdb_condition *cnd)
> + const struct ovsdb_condition *cnd,
> + const unsigned int columns_index_map[])
> {
> size_t i;
>
> for (i = 0; i < cnd->n_clauses; i++) {
> - if (!ovsdb_clause_evaluate(row, &cnd->clauses[i])) {
> + if (!ovsdb_clause_evaluate(row->fields,
> + &cnd->clauses[i],
> + columns_index_map)) {
> return false;
> }
> }
> @@ -294,6 +371,28 @@ ovsdb_condition_evaluate(const struct ovsdb_row *row,
> return true;
> }
>
> +bool
> +ovsdb_condition_evaluate_or_datum(const struct ovsdb_datum *row_datum,
> + const struct ovsdb_condition *cnd,
> + const unsigned int columns_index_map[])
>
The name of this function is not clear about its intention. The
implementation also
duplicate in many parts with ovsdb_condition_evaluate() above.
> +{
> + size_t i;
> +
> + if (cnd->n_clauses == 0) {
> + return true;
> + }
> +
> + for (i = 0; i < cnd->n_clauses; i++) {
> + if (ovsdb_clause_evaluate(row_datum,
> + &cnd->clauses[i],
> + columns_index_map)) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> void
> ovsdb_condition_destroy(struct ovsdb_condition *cnd)
> {
> @@ -303,4 +402,153 @@ ovsdb_condition_destroy(struct ovsdb_condition *cnd)
> ovsdb_clause_free(&cnd->clauses[i]);
> }
> free(cnd->clauses);
>
Should cnd->clauses be set to NULL?
> + cnd->n_clauses = 0;
> +}
> +
> +void ovsdb_condition_init(struct ovsdb_condition *cnd)
> +{
> + cnd->clauses = NULL;
> + cnd->n_clauses = 0;
> +}
> +
> +bool ovsdb_condition_empty(const struct ovsdb_condition *cnd)
> +{
> + return cnd->n_clauses == 0;
> +}
> +
> +int ovsdb_condition_cmp(const struct ovsdb_condition *a,
> + const struct ovsdb_condition *b)
> +{
> + size_t i;
> + int res;
> +
> + if (a->n_clauses != b->n_clauses) {
> + return a->n_clauses - b->n_clauses;
> + }
> +
> + /* We assume clauses are sorted */
> + for (i = 0; i < a->n_clauses; i++) {
> + res = (compare_clauses_3way_with_data(&a->clauses[i],
> &b->clauses[i]));
> + if (res != 0) {
> + return res;
> + }
> + }
> +
> + return 0;
> +}
> +
> +void
> +ovsdb_condition_clone(struct ovsdb_condition *to,
> + const struct ovsdb_condition *from)
> +{
> + size_t i;
> +
> + to->clauses = xzalloc(from->n_clauses * sizeof *to->clauses);
> +
> + for (i = 0; i < from->n_clauses; i++) {
> + ovsdb_clone_clause(&to->clauses[i], &from->clauses[i]);
> + }
> + to->n_clauses = from->n_clauses;
> +}
> +
>
With the way it is implemented, the order of when "add" and "remove' are
applied
is important. May be it should be documented.
Another possibility is to check and make sure clauses in "add" and "remove"
are orthogonal.
Reject the change if they are not.
+void
> +ovsdb_condition_add(struct ovsdb_condition *to,
> + const struct ovsdb_condition *add)
> +{
> + size_t i, count = 0;
> + struct ovsdb_clause *clauses;
> + unsigned long int *clause_map =
> xzalloc(bitmap_n_bytes(add->n_clauses));
> + int index = to->n_clauses;;
> +
> + for (i = 0; i < add->n_clauses; i++) {
> + if (ovsdb_clause_exists(to, &add->clauses[i]) == -1) {
> + bitmap_set1(clause_map, i);
> + count++;
> + }
> + }
> +
> + if (!count) {
> + free(clause_map);
> + return;
> + }
> + clauses = xzalloc((to->n_clauses + count) * sizeof *clauses);
> +
> + for (i = 0; i < to->n_clauses; i++) {
> + ovsdb_clone_clause(&clauses[i], &to->clauses[i]);
> + ovsdb_clause_free(&to->clauses[i]);
> + }
> +
> + for (i = 0; i < add->n_clauses; i++) {
> + if (bitmap_is_set(clause_map, i)) {
> + ovsdb_clone_clause(&clauses[index++], &add->clauses[i]);
> + }
> + }
>
Should we keep the clauses sorted?
If yes, It may be more efficient to blindly add the new clauses, using
xreallc()
sort them, then only check for duplicates with adjacent clauses.
> +
> + free(to->clauses);
> + free(clause_map);
> + to->clauses = clauses;
> + to->n_clauses += count;
> +}
> +
> +void
> +ovsdb_condition_remove(struct ovsdb_condition *from,
> + const struct ovsdb_condition *remove)
> +{
> + size_t i, count = 0;
> + int j;
> + struct ovsdb_clause *clauses;
> + unsigned long int *clause_map =
> xmalloc(bitmap_n_bytes(from->n_clauses));
> + int index = 0;
> +
> + if (remove->n_clauses == 0) {
> + free(clause_map);
> + return;
> + }
> +
> + for (i = 0; i < from->n_clauses; i++) {
> + bitmap_set1(clause_map, i);
> + }
> +
> + for (i = 0; i < remove->n_clauses; i++) {
> + j = ovsdb_clause_exists(from, &remove->clauses[i]);
> + if (j >= 0) {
> + bitmap_set0(clause_map, j);
> + count++;
> + }
> + }
> +
> + if (count == 0) {
> + free(clause_map);
> + return;
> + }
> +
> + clauses = xzalloc((from->n_clauses - count) * sizeof *clauses);
> + for (i = 0; i < from->n_clauses; i++) {
> + if (bitmap_is_set(clause_map, i)) {
> + ovsdb_clone_clause(&clauses[index++], &from->clauses[i]);
> + }
> + }
> +
>
Should clauses still be sorted after removal? Since we make sure all
removed
clauses exist in current condition at message parsing time
(ovsdb_monitor_table_condition_change()),
sort then remove should also work.
> + free(from->clauses);
> + free(clause_map);
> + from->clauses = clauses;
> + from->n_clauses -= count;
> +}
> +
> +/* Returns if a + b includes c */
> +bool
> +ovsdb_conditions_includes(const struct ovsdb_condition *a,
> + const struct ovsdb_condition *b,
> + const struct ovsdb_condition *c)
> +{
> + size_t i;
> +
> + for (i = 0; i < c->n_clauses; i++) {
> + if(ovsdb_clause_exists(a, &c->clauses[i]) == -1 &&
> + ovsdb_clause_exists(b, &c->clauses[i]) == -1) {
> + return false;
> + }
> + }
> +
> + return true;
> }
> diff --git a/ovsdb/condition.h b/ovsdb/condition.h
> index 620757f..4d223d4 100644
> --- a/ovsdb/condition.h
> +++ b/ovsdb/condition.h
> @@ -27,6 +27,8 @@ struct ovsdb_row;
> /* These list is ordered in ascending order of the fraction of tables row
> that
> * they are (heuristically) expected to leave in query results. */
> #define OVSDB_FUNCTIONS \
> + OVSDB_FUNCTION(OVSDB_F_FALSE, "false") \
> + OVSDB_FUNCTION(OVSDB_F_TRUE, "true") \
> OVSDB_FUNCTION(OVSDB_F_EQ, "==") \
> OVSDB_FUNCTION(OVSDB_F_INCLUDES, "includes") \
> OVSDB_FUNCTION(OVSDB_F_LE, "<=") \
> @@ -60,6 +62,8 @@ struct ovsdb_condition {
>
> #define OVSDB_CONDITION_INITIALIZER { NULL, 0 }
>
> +void ovsdb_condition_init(struct ovsdb_condition *);
> +bool ovsdb_condition_empty(const struct ovsdb_condition *);
> struct ovsdb_error *ovsdb_condition_from_json(
> const struct ovsdb_table_schema *,
> const struct json *, struct ovsdb_symbol_table *,
> @@ -67,6 +71,28 @@ struct ovsdb_error *ovsdb_condition_from_json(
> struct json *ovsdb_condition_to_json(const struct ovsdb_condition *);
> void ovsdb_condition_destroy(struct ovsdb_condition *);
> bool ovsdb_condition_evaluate(const struct ovsdb_row *,
> - const struct ovsdb_condition *);
> + const struct ovsdb_condition *,
> + const unsigned int columns_index_map[]);
> +bool ovsdb_condition_evaluate_or_datum(const struct ovsdb_datum *,
> + const struct ovsdb_condition *,
> + const unsigned int
> columns_index_map[]);
> +int ovsdb_condition_cmp(const struct ovsdb_condition *a,
> + const struct ovsdb_condition *b);
> +
> +void ovsdb_condition_clone(struct ovsdb_condition *to,
> + const struct ovsdb_condition *from);
> +
> +void
> +ovsdb_condition_add(struct ovsdb_condition *to,
> + const struct ovsdb_condition *add);
> +
> +void
> +ovsdb_condition_remove(struct ovsdb_condition *from,
> + const struct ovsdb_condition *remove);
> +
> +bool
> +ovsdb_conditions_includes(const struct ovsdb_condition *a,
> + const struct ovsdb_condition *b,
> + const struct ovsdb_condition *c);
>
> #endif /* ovsdb/condition.h */
> diff --git a/ovsdb/query.c b/ovsdb/query.c
> index e288020..5d3f9b1 100644
> --- a/ovsdb/query.c
> +++ b/ovsdb/query.c
> @@ -34,7 +34,7 @@ ovsdb_query(struct ovsdb_table *table, const struct
> ovsdb_condition *cnd,
> const struct ovsdb_row *row;
>
> row = ovsdb_table_get_row(table,
> &cnd->clauses[0].arg.keys[0].uuid);
> - if (row && row->table == table && ovsdb_condition_evaluate(row,
> cnd)) {
> + if (row && row->table == table && ovsdb_condition_evaluate(row,
> cnd, NULL)) {
> output_row(row, aux);
> }
> } else {
> @@ -42,7 +42,7 @@ ovsdb_query(struct ovsdb_table *table, const struct
> ovsdb_condition *cnd,
> const struct ovsdb_row *row, *next;
>
> HMAP_FOR_EACH_SAFE (row, next, hmap_node, &table->rows) {
> - if (ovsdb_condition_evaluate(row, cnd) && !output_row(row,
> aux)) {
> + if (ovsdb_condition_evaluate(row, cnd, NULL) &&
> !output_row(row, aux)) {
> break;
> }
> }
> diff --git a/tests/ovsdb-condition.at b/tests/ovsdb-condition.at
> index ab54b1c..285c3d6 100644
> --- a/tests/ovsdb-condition.at
> +++ b/tests/ovsdb-condition.at
> @@ -181,8 +181,21 @@ OVSDB_CHECK_POSITIVE([condition sorting],
> ["i", "<", 4],
> ["i", ">", 6],
> ["i", ">=", 5],
> - ["_uuid", "==", ["uuid",
> "d50e85c6-8ae7-4b16-b69e-4395928bd9be"]]]']],
> -
> [[[["_uuid","==",["uuid","d50e85c6-8ae7-4b16-b69e-4395928bd9be"]],["i","==",1],["i","includes",2],["i","<=",3],["i","<",4],["i",">=",5],["i",">",6],["i","excludes",7],["i","!=",8]]]])
> + ["_uuid", "==", ["uuid", "d50e85c6-8ae7-4b16-b69e-4395928bd9be"]],
> + true]']],
> +
> [[[true,["_uuid","==",["uuid","d50e85c6-8ae7-4b16-b69e-4395928bd9be"]],["i","==",1],["i","includes",2],["i","<=",3],["i","<",4],["i",">=",5],["i",">",6],["i","excludes",7],["i","!=",8]]]])
> +
> +OVSDB_CHECK_POSITIVE([boolean condition],
> + [[parse-conditions \
> + '{"columns": {"name": {"type": "string"}}}' \
> + '[true]']],
> + [[[true]]])
> +
> +OVSDB_CHECK_POSITIVE([boolean condition],
> + [[parse-conditions \
> + '{"columns": {"name": {"type": "string"}}}' \
> + '[false]']],
> + [[[false]]])
>
> OVSDB_CHECK_POSITIVE([evaluating null condition],
> [[evaluate-conditions \
> @@ -657,3 +670,21 @@ condition 5: --T-
> condition 6: -T--
> condition 7: T-TT
> condition 8: -T-T], [condition])
> +
> +OVSDB_CHECK_POSITIVE([evaluating false boolean condition],
> + [[evaluate-conditions \
> + '{"columns": {"i": {"type": "integer"}}}' \
> + '[[false,["i","==",0]]]' \
> + '[{"i": 0},
> + {"i": 1},
> + {"i": 2}']]],
> + [condition 0: ---])
> +
> +OVSDB_CHECK_POSITIVE([evaluating true boolean condition],
> + [[evaluate-conditions-or \
> + '{"columns": {"i": {"type": "integer"}}}' \
> + '[[true,["i","==",0]]]' \
> + '[{"i": 0},
> + {"i": 1},
> + {"i": 2}']]],
> + [condition 0: TTT])
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 15f41b0..e924532 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -851,8 +851,11 @@ do_parse_conditions(struct ovs_cmdl_context *ctx)
> exit(exit_code);
> }
>
> +#define OVSDB_CONDITION_AND 0
> +#define OVSDB_CONDITION_OR 1
> +
> static void
> -do_evaluate_conditions(struct ovs_cmdl_context *ctx)
> +do_evaluate_condition__(struct ovs_cmdl_context *ctx, int mode)
> {
> struct ovsdb_table_schema *ts;
> struct ovsdb_table *table;
> @@ -900,7 +903,16 @@ do_evaluate_conditions(struct ovs_cmdl_context *ctx)
> for (i = 0; i < n_conditions; i++) {
> printf("condition %2"PRIuSIZE":", i);
> for (j = 0; j < n_rows; j++) {
> - bool result = ovsdb_condition_evaluate(rows[j],
> &conditions[i]);
> + bool result;
> + if (mode == OVSDB_CONDITION_AND) {
> + result = ovsdb_condition_evaluate(rows[j],
> + &conditions[i],
> + NULL);
> + } else {
> + result =
> ovsdb_condition_evaluate_or_datum(rows[j]->fields,
> + &conditions[i],
> + NULL);
> + }
> if (j % 5 == 0) {
> putchar(' ');
> }
> @@ -921,6 +933,18 @@ do_evaluate_conditions(struct ovs_cmdl_context *ctx)
> }
>
> static void
> +do_evaluate_conditions(struct ovs_cmdl_context *ctx)
> +{
> + do_evaluate_condition__(ctx, OVSDB_CONDITION_AND);
> +}
> +
> +static void
> +do_evaluate_conditions_or(struct ovs_cmdl_context *ctx)
> +{
> + do_evaluate_condition__(ctx, OVSDB_CONDITION_OR);
> +}
> +
> +static void
> do_parse_mutations(struct ovs_cmdl_context *ctx)
> {
> struct ovsdb_table_schema *ts;
> @@ -2191,6 +2215,7 @@ static struct ovs_cmdl_command all_commands[] = {
> { "compare-rows", NULL, 2, INT_MAX, do_compare_rows },
> { "parse-conditions", NULL, 2, INT_MAX, do_parse_conditions },
> { "evaluate-conditions", NULL, 3, 3, do_evaluate_conditions },
> + { "evaluate-conditions-or", NULL, 3, 3, do_evaluate_conditions_or },
> { "parse-mutations", NULL, 2, INT_MAX, do_parse_mutations },
> { "execute-mutations", NULL, 3, 3, do_execute_mutations },
> { "query", NULL, 3, 3, do_query },
> --
> 2.1.4
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
