On Mon, May 19, 2014 at 10:38:38AM -0700, Jarno Rajahalme wrote: > > 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.
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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
