On Thu, May 22, 2014 at 06:38:53PM -0700, Ethan Jackson wrote:
> From: Jarno Rajahalme <[email protected]>
>
> Do not cache the 'tag' and 'max_priority' in the subtable array. This
> makes later changes to classifier easier.
>
> Change CLS_SUBTABLES_FOR_EACH to iterate with a regular array index.
>
> Makes the 'cls_subtables*' functions to always leave the subtables
> array in a consistent state. This includes the new
> cls_subtables_insert() function and removal of the old
> cls_subtables_push_back() function.
>
> Finally, using qsort() instead of our hand rolled sort routines. This
> simplifies the code significantly.
>
> Signed-off-by: Jarno Rajahalme <[email protected]>
> Co-authored-by: Ethan Jackson <[email protected]>
> Signed-off-by: Ethan Jackson <[email protected]>
> ---
>
> Jarno wrote the bulk of this patch, but I folded in the switch to use the
> qsort() function. I was planning to write that on top of this, but it turned
> out to be much easier to fold it in. Someone other than either of us should
> probably review it.
Some of the new functions in classifier.c are marked explicitly
inline. This is usually a bad idea because it suppresses warnings
about functions that are never called (and because it usually doesn't
help code generation).
I don't see a reason to change the cls_subtables member 'count' from
size_t to int. (The previous versions of the patch did have a reason,
but it doesn't appear valid anymore.) Similarly, I would use size_t
for the index parameter to cls_subtables_remove().
This new comment in cls_subtables_insert() seems like a non sequitur:
> + /* Lowest priority subtables at the end. */
> + cls->subtables.array[cls->subtables.count++] = a;
> + sort_subtables(&cls->subtables);
> }
Now that CodingStyle allows variable declarations in 'for' statements,
CLS_SUBTABLES_FOR_EACH can be made easier to use. Instead of:
#define CLS_SUBTABLES_FOR_EACH(SUBTABLE, INDEX, SUBTABLES) \
for ((INDEX) = 0; \
(INDEX) < (SUBTABLES)->count \
&& ((SUBTABLE) = (SUBTABLES)->array[INDEX], true); \
(INDEX)++)
one can write:
#define CLS_SUBTABLES_FOR_EACH(SUBTABLE, SUBTABLES) \
for (size_t index_ = 0; \
index_ < (SUBTABLES)->count \
&& ((SUBTABLE) = (SUBTABLES)->array[index_], true); \
index_++)
eliminating the INDEX macro argument.
cmp_subtable() could use this really simple way to get a return value
for qsort(), which has been my favorite for a long time:
return a > b ? -1 : a < b;
Acked-by: Ben Pfaff <[email protected]>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
