[PATCH] Minor fixes to netinet6/icmp6.c

[John Baldwin]

On Sunday, December 25, 2011 4:45:55 am Sergey Kandaurov wrote:
> On 25 December 2011 04:58, John Baldwin <jhb at freebsd.org> wrote:
> > On 12/23/11 6:38 PM, Sergey Kandaurov wrote:
> >>
> >> On 23 December 2011 23:46, John Baldwin<jhb at freebsd.org>  wrote:
> >>>
> >>> I found these nits while working on the patches to convert if_addr_mtx to
> >>> an
> >>> rwlock.  The first change is cosmetic, it just un-inlines a
> >>> TAILQ_FOREACH().
> >>> The second change is an actual bug.  The code is currently reading
> >>> TAILQ_FIRST(&V_ifnet) without holding the appropriate lock.
> >>>
> >>> Index: icmp6.c
> >>> ===================================================================
> >>> --- icmp6.c     (revision 228777)
> >>> +++ icmp6.c     (working copy)
> >>> @@ -1780,7 +1780,7 @@ ni6_addrs(struct icmp6_nodeinfo *ni6, struct mbuf
> >>>        }
> >>>
> >>>        IFNET_RLOCK_NOSLEEP();
> >>> -       for (ifp = TAILQ_FIRST(&V_ifnet); ifp; ifp = TAILQ_NEXT(ifp,
> >>> if_list)) {
> >>> +       TAILQ_FOREACH(ifp,&V_ifnet, if_list) {
> >>>                addrsofif = 0;
> >>>                IF_ADDR_LOCK(ifp);
> >>>                TAILQ_FOREACH(ifa,&ifp->if_addrhead, ifa_link) {
> >>
> >>
> >> FWIW, there are much more of them in netinet6.
> >> Some time ago I started to un-expand them to queue(3).
> >> [not unfinished yet..]
> >
> >
> > I went ahead and did a sweep for queue(3) changes in netinet6.  I went a bit
> Great, thank you! This sweep is long overdue.
> 
> > further and removed things like the ndpr_next hack, etc.  This only includes
> > queue(3) changes though, not your other fixes like moving common code out of
> Oops, yeah. This is an unrelated change.
> 
> > blocks.  I also fixed a few places to use *_EMPTY() instead of checking
> > *_FIRST() against NULL.  There should be no functional change.
> >
> > http://www.FreeBSD.org/~jhb/patches/inet6_queue.patch
> 
> Looks good. Please, commit with two notes:
> 
> a) You changed a loop with precondition
>         while (i < DRLSTSIZ) { ... }
> into
>         if (i >= DRLSTSIZ)
> and moved it below i++ increment, which effectively became
> a loop with post-condition like do { ...} while ().
> To preserve the current behavior I would move this check up
> right under *_FOREACH() loop, like this:
> 
>                 TAILQ_FOREACH(dr, &V_nd_defrouter, dr_entry) {
>                                                 if (i >= DRLSTSIZ)
>                                                          break;

Note that i is initialized to 0 before the loop starts (albeit that is
harder to see because of the style bug of it being initialized in its
declaration).  I considered instead rewriting this as:

	for (i = 0, dr = TAILQ_FIRST(&V_nd_defrouter); dr && i < DRLSTSIZ; i++, dr = TAILQ_NEXT(dr, dr_entry)) {

However, that is a bit verbose.  I can move it up, the compiler probably
"knows" that i is zero on the first loop and might skip the check anyway.

> b)
> It should be safe to use non-SAFE() FOREACH() variants of
> queue(3) macros for most occurrences. SAFE() versions are
> usually only used when you need to add/remove an element
> on list w/o need to subsequent restart the *_FOREACH() loop.

I only used them where the existing code was already using that idiom.
I just checked them all and each instance is potentially freeing or
deleting the current item while iterating the list.

-- 
John Baldwin