[ovs-dev] [PATCH 2/3] lib/classifier: Simplify subtable array.

Jarno Rajahalme jrajahalme at nicira.com
Mon May 19 17:38:38 UTC 2014


On May 19, 2014, at 7:48 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Fri, May 16, 2014 at 02:44:40PM -0700, Jarno Rajahalme wrote:
>> Do not cache the 'tag' and 'max_priority' in the subtable array.  This
>> makes later changes to classifier easier.
>> 
>> Also 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.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> The FOR_EACH macros caught my eye.  Each of them checks for a nonnull
> pointer in the subtables array.  Are null pointers really possible?  I
> took a quick look at the functions that modify the array and I did not
> see how any of them could leave a null pointer in it.
> 

I need to dereference the subtables array pointer only after the bounds check, otherwise I would be dereferencing a NULL pointer in case of an empty array, or potentially invalid memory after the end of the array. The dereference needs to produce a boolean true to keep the loop condition true, but maybe I could change it to the following instead:

#define CLS_SUBTABLES_FOR_EACH(SUBTABLE, ITER, SUBTABLES)  \
    for ((ITER) = (SUBTABLES)->array;                      \
         (ITER) < &(SUBTABLES)->array[(SUBTABLES)->count]  \
             && ((SUBTABLE) = *(ITER), true);          \
         ++(ITER))

i.e., ITER is dereferenced only after the successful bounds check, but the assigned value itself is not tested.

> The code in cls_subtables_change_priority() has almost parallel code for
> the two cases, except that in the new_priority < old_priority case it
> does the iter++ before the break and in the other case it does the
> iter-- after the loop.  I would prefer to make both cases the same.
> 

To make both cases similar, I need to change the FOR_EACH macros to iterate using array indices, which turns out to be simpler anyway :-) I’ll send a v2.

  Jarno

> Acked-by: Ben Pfaff <blp at nicira.com>




More information about the dev mailing list