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

[Ben Pfaff]

On Mon, May 19, 2014 at 10:38:38AM -0700, Jarno Rajahalme wrote:
> 
> 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.

I would find that more straightforward because it does not imply that
the subtable pointers can be null, raising questions.

> > 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 was about to suggest that.

> I?ll send a v2.

Thanks,

Ben.