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

Ben Pfaff blp at nicira.com
Wed Nov 3 18:03:14 UTC 2010


On Tue, Nov 02, 2010 at 11:55:40PM -0700, Justin Pettit wrote:
> 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?

Yes.

The parenthetical sentence isn't actually true anymore, though, so I
just deleted it.

> > 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?

They have to exist if 'rule' is indeed in 'cls'.  If 'rule' isn't in
'cls' then we'll get a friendly segfault anyway.

> > +/* 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.

OK, added.

> >  * 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.

Right, thanks.

> > +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()?

OK, I made that change.

> > {
> > ....
> > +            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.

Thanks, fixed.

> > +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?

Ooh, good spotting.  That's a potentially serious bug and I guess this
only worked because offsetof(cls_table, hmap_node) is 0.

> > +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.

You're right.  I guess an earlier version must have anticipated an
external user.  Changed.

> > 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".

Sure, done.

> > + * 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.

Later commits introduce more functions that modify "cls_rule"s, that are
not initializers, that do maintain the invariants.

> > 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"?

Like this?  OK.

/* Information on wildcards for a flow, as a supplement to "struct flow".




More information about the dev mailing list