[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