[ovs-dev] [nxm 17/42] classifier: Rewrite.

Justin Pettit jpettit at nicira.com
Wed Nov 3 06:55:40 UTC 2010


Thanks, Ben.  This classifier is significantly easier to understand than the previous one.  In-line are some comments...

> +/* Returns the first rule in 'table', or a null pointer if 'table' is NULL.
> + *
> + * (The classifier never creates empty tables and destroys tables as soon as
> + * they become empty, so if 'table' is null then the return value should always
> + * be nonnull.  However, the implementation of this function is robust
> + * anyhow.) */
> +struct cls_rule *
> +cls_table_first_rule(const struct cls_table *table)
> +{
> +    return table ? cls_rule_from_hmap_node(hmap_first(&table->rules)) : NULL;
> +}

I understand the function, but I found the first sentence in that parenthetical description confusing.  Did you mean that if "table" is *not* null, then the return value should always be nonnull?

> void
> classifier_remove(struct classifier *cls, struct cls_rule *rule)
> {
> +    struct cls_rule *head;
> +    struct cls_table *table;
> 
> +    table = find_table(cls, &rule->wc);
> +    head = find_equal(table, &rule->flow, rule->hmap_node.hash);

Might it be a good idea to assert that "table" and "head" exist?

> +/* Finds and returns a rule in 'cls' with exactly the same priority and
> + * matching criteria as 'target'.  Returns a null pointer if 'cls' doesn't
> + * contain an exact match.
> + *
> + * Priority is ignored for exact-match rules. */

It would be nice to explain that the reason is that OpenFlow <= 1.0, exact match rules have the highest priority.

>  * If 'include' is CLS_INC_EXACT then CLASSIFIER_FOR_EACH_EXACT_RULE(_SAFE) is
>  * probably easier to use. */

I don't think the SAFE version exists anymore.

> +classifier_for_each(const struct classifier *cls_, int include,
>                     void (*callback)(struct cls_rule *, void *aux),
>                     void *aux)

Is there a reason not to use "cls_cb_func" for the callback definition, like in classifier_for_each_match()?

> {
> ....
> +            next_table = classifier_next_table(cls, table);
> +            if (!--table->n_refs && !table->n_table_rules) {
> +                hmap_remove(&cls->tables, &table->hmap_node);
> +                free(table);
> +            }

Is there a reason not to call classifier_destroy_table() here?  It seems like we'll leak "table->rules" otherwise.

> +static struct cls_table *
> +find_table(const struct classifier *cls, const struct flow_wildcards *wc)
> +{
> +    struct cls_table *table;
> 
> +    HMAP_FOR_EACH_IN_BUCKET (table, hmap_node, flow_wildcards_hash(wc),
> +                             &cls->tables) {
> +        if (flow_wildcards_equal(wc, &table->wc)) {
> +            return table;
>         }
>     }
> +    return table;

Shouldn't this return NULL?

> +void
> +classifier_destroy_table(struct classifier *cls, struct cls_table *table)
> +{
> +    hmap_remove(&cls->tables, &table->hmap_node);
> +    hmap_destroy(&table->rules);
> +    free(table);
> +}

Is there a reason not to declare this static and drop the "classifier_"?  I didn't see any external callers of it.

> diff --git a/lib/classifier.h b/lib/classifier.h
> ...
> +/* A set of rules that all have the same wildcarded fields. */

I think it would be clearer to say "the same fields wildcarded".

> + * The cls_rule_*() functions below maintain the following important
> + * invariant that the classifier depends on:

More specifically, isn't it one of the cls_rule_from_*() functions, since others that match that description don't seem like initializers.

> diff --git a/lib/flow.h b/lib/flow.h
> index 7667cb0..67f4827 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> 
> -/* Information on wildcards for a flow, as a supplement to struct flow. */
> +/* Information on wildcards for a flow, as a supplement to struct flow.

I'm embarrassed to even suggest this, but do you mind quoting that second "flow"?

Thanks,

--Justin






More information about the dev mailing list