From owner-freebsd-net@FreeBSD.ORG Thu Dec 29 18:21:30 2011 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 320661065687; Thu, 29 Dec 2011 18:21:30 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id E3A528FC18; Thu, 29 Dec 2011 18:21:29 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 8136F46B2C; Thu, 29 Dec 2011 13:21:29 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id EB129B93F; Thu, 29 Dec 2011 13:21:28 -0500 (EST) From: John Baldwin To: Sergey Kandaurov Date: Thu, 29 Dec 2011 12:12:52 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <201112231446.38057.jhb@freebsd.org> <4EF6752C.4040109@FreeBSD.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112291212.52682.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 29 Dec 2011 13:21:29 -0500 (EST) Cc: Bjoern Zeeb , net@freebsd.org Subject: Re: [PATCH] Minor fixes to netinet6/icmp6.c X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Dec 2011 18:21:30 -0000 On Sunday, December 25, 2011 4:45:55 am Sergey Kandaurov wrote: > On 25 December 2011 04:58, John Baldwin wrote: > > On 12/23/11 6:38 PM, Sergey Kandaurov wrote: > >> > >> On 23 December 2011 23:46, John Baldwin 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