On May 19, 2014, at 7:48 AM, Ben Pfaff <b...@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 <jrajaha...@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 <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
