On Thu, Feb 18, 2016 at 11:46:40AM +0000, Liran Schour wrote:
> Change ovsdb_condition to be a 3-element json array or a boolean value.
> Conditions utilities will be used later for conditional monitoring.
>
> Signed-off-by: Liran Schour <[email protected]>
>
> ---
> v3->v4:
> * Pass index_map only to ovsdb_condition_evaluate_or_datum()
> * Added ovsdb_condition_is_[true|false]()
> * Added ovsdb_condition_get_max_function()
> * Added ovsdb_condition_diff()
>
> v2->v3:
> * Remove condition_add() and condition_remove() and all sub-functions
> * Allow single bollean value XOR <condition>
Thanks for working on this.
The naming here starts to be confusing. The difference between
ovsdb_condition_evaluate() and ovsdb_condition_evaluate_or_datum() is
not obvious. I guess that the former is for determining whether every
clause is true and the other whether any clause is true; names like
ovsdb_condition_is_every_clause_true() and
ovsdb_condition_is_any_clause_true() would be easier to understand.
It's totally unclear why the latter needs an extra index_map parameter
or what goes in it. I think that a comment needs to explain that too.
A lot of the function declarations here violate the coding style.
Instead of:
void ovsdb_condition_init(struct ovsdb_condition *cnd)
please write:
void
ovsdb_condition_init(struct ovsdb_condition *cnd)
The new "functions" violate the comment right above them:
/* 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") \
It seems strange that ovsdb_condition_evaluate_or_datum() returns true
if there are no clauses. What's the rationale?
I'm not comfortable with tests like this:
+ if (clause->function > OVSDB_F_TRUE) {
and this:
+ if (c->function <= OVSDB_F_TRUE) {
because they break unexpectedly if someone reorders the enumeration.
This test is a really bad idea because n_clauses is unsigned:
+ if (a->n_clauses != b->n_clauses) {
+ return a->n_clauses - b->n_clauses;
+ }
I think that bug demonstrates that there need to be some more tests.
ovsdb_condition_cmp() should probably be named
ovsdb_condition_cmp_3way() to match the convention elsewhere.
ovsdb_conditon_diff() misspells the word "condition".
Naming in diff functions can sometimes be tricky. The
ovsdb_conditon_diff() parameters might be easier to understand if they
were named a_only and b_only, that is, a_only received the clauses that
were only in a and b_only received the clauses that were only in b.
ovsdb_condition_is_true() assumes the condition is evaluated in one
particular way (I guess by ovsdb_condition_evaluate_or_datum()). This
should probably be documented in a comment, at least.
I don't understand the purpose of ovsdb_condition_max_function().
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
