Date: Sat, 1 Jul 2006 03:33:26 +0300 From: Giorgos Keramidas <keramida@freebsd.org> To: Yar Tikhiy <yar@freebsd.org> 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 Message-ID: <20060701003326.GA41947@gothmog.pc> In-Reply-To: <200606291922.k5TJM5ev007314@repoman.freebsd.org> References: <200606291922.k5TJM5ev007314@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2006-06-29 19:22, Yar Tikhiy <yar@freebsd.org> 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 :)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060701003326.GA41947>