Date: Thu, 29 Dec 2011 12:12:52 -0500 From: John Baldwin <jhb@freebsd.org> To: Sergey Kandaurov <pluknet@gmail.com> Cc: Bjoern Zeeb <bz@freebsd.org>, net@freebsd.org Subject: Re: [PATCH] Minor fixes to netinet6/icmp6.c Message-ID: <201112291212.52682.jhb@freebsd.org> In-Reply-To: <CAE-mSOJ722UNqkVNs%2B8_LQ4pVt4VTmSkDhr3re5aDhsHPfQffQ@mail.gmail.com> References: <201112231446.38057.jhb@freebsd.org> <4EF6752C.4040109@FreeBSD.org> <CAE-mSOJ722UNqkVNs%2B8_LQ4pVt4VTmSkDhr3re5aDhsHPfQffQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sunday, December 25, 2011 4:45:55 am Sergey Kandaurov wrote:
> On 25 December 2011 04:58, John Baldwin <jhb@freebsd.org> wrote:
> > On 12/23/11 6:38 PM, Sergey Kandaurov wrote:
> >>
> >> On 23 December 2011 23:46, John Baldwin<jhb@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201112291212.52682.jhb>