From owner-cvs-all@FreeBSD.ORG Sat Jul 1 00:33:54 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 61CF116A412; Sat, 1 Jul 2006 00:33:54 +0000 (UTC) (envelope-from keramida@freebsd.org) Received: from igloo.linux.gr (igloo.linux.gr [62.1.205.36]) by mx1.FreeBSD.org (Postfix) with ESMTP id 709BB44313; Sat, 1 Jul 2006 00:33:53 +0000 (GMT) (envelope-from keramida@freebsd.org) Received: from gothmog.pc (host5.bedc.ondsl.gr [62.103.39.229]) (authenticated bits=128) by igloo.linux.gr (8.13.7/8.13.7/Debian-1) with ESMTP id k610XZaj018765 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Sat, 1 Jul 2006 03:33:37 +0300 Received: from gothmog.pc (gothmog [127.0.0.1]) by gothmog.pc (8.13.7/8.13.7) with ESMTP id k610XQtP042007; Sat, 1 Jul 2006 03:33:27 +0300 (EEST) (envelope-from keramida@freebsd.org) Received: (from giorgos@localhost) by gothmog.pc (8.13.7/8.13.7/Submit) id k610XQWb042006; Sat, 1 Jul 2006 03:33:26 +0300 (EEST) (envelope-from keramida@freebsd.org) Date: Sat, 1 Jul 2006 03:33:26 +0300 From: Giorgos Keramidas To: Yar Tikhiy Message-ID: <20060701003326.GA41947@gothmog.pc> References: <200606291922.k5TJM5ev007314@repoman.freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200606291922.k5TJM5ev007314@repoman.freebsd.org> X-Hellug-MailScanner: Found to be clean X-Hellug-MailScanner-SpamCheck: not spam, SpamAssassin (score=-4.331, required 5, autolearn=not spam, ALL_TRUSTED -1.80, AWL 0.07, BAYES_00 -2.60) X-Hellug-MailScanner-From: keramida@freebsd.org X-Spam-Status: No Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/net if.c if_atmsubr.c if_stf.c if_tun.c src/sys/netinet if_ether.c ip_divert.c ip_fw2.c src/sys/netinet6 in6.c in6_var.h src/sys/nfsclient bootp_subr.c nfs_diskless.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Jul 2006 00:33:54 -0000 On 2006-06-29 19:22, Yar Tikhiy wrote: > yar 2006-06-29 19:22:05 UTC > > FreeBSD src repository > > Modified files: > sys/net if.c if_atmsubr.c if_stf.c if_tun.c > sys/netinet if_ether.c ip_divert.c ip_fw2.c > sys/netinet6 in6.c in6_var.h > sys/nfsclient bootp_subr.c nfs_diskless.c > Log: > There is a consensus that ifaddr.ifa_addr should never be NULL, > except in places dealing with ifaddr creation or destruction; and > in such special places incomplete ifaddrs should never be linked > to system-wide data structures. Therefore we can eliminate all the > superfluous checks for "ifa->ifa_addr != NULL" and get ready > to the system crashing honestly instead of masking possible bugs. This is probably silly, but it was the first thing I thought about when I saw the NULL checks removed. Since we assume that ifa->ifa_addr != NULL, does it make sense to add KASSERT() calls in the places where we do so? Something like the following: % === sys/netinet6/in6.c % ================================================================== % --- sys/netinet6/in6.c (revision 149) % +++ sys/netinet6/in6.c (local) % @@ -1,4 +1,4 @@ % -/* $FreeBSD: src/sys/netinet6/in6.c,v 1.61 2006/06/08 00:31:17 gnn Exp $ */ % +/* $FreeBSD: src/sys/netinet6/in6.c,v 1.62 2006/06/29 19:22:05 yar Exp $ */ % /* $KAME: in6.c,v 1.259 2002/01/21 11:37:50 keiichi Exp $ */ % % /*- % @@ -1696,8 +1696,6 @@ % * and to validate the address if necessary. % */ % TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) { % - if (ifa->ifa_addr == NULL) % - continue; /* just for safety */ % if (ifa->ifa_addr->sa_family != AF_INET6) % continue; % ifacount++; would become then: TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) { KASSERT(ifa->ifa_addr == NULL, ("ifa %p has no ifa_addr", ifa)); if (ifa->ifa_addr->sa_family != AF_INET6) continue; ifacount++; This shouldn't really be slower than the original NULL check, but it is a relatively useful sort of `inline documentation' of the assumption and it may also help a bit in debugging the crash :)