From owner-svn-src-all@FreeBSD.ORG Fri Feb 27 07:59:09 2009 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DB3811065672; Fri, 27 Feb 2009 07:59:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail02.syd.optusnet.com.au (mail02.syd.optusnet.com.au [211.29.132.183]) by mx1.freebsd.org (Postfix) with ESMTP id 7055A8FC19; Fri, 27 Feb 2009 07:59:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-107-120-227.carlnfd1.nsw.optusnet.com.au (c122-107-120-227.carlnfd1.nsw.optusnet.com.au [122.107.120.227]) by mail02.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n1R7x6Ng005452 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 27 Feb 2009 18:59:07 +1100 Date: Fri, 27 Feb 2009 18:59:06 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Bruce Evans In-Reply-To: <20090227184405.Q13337@delplex.bde.org> Message-ID: <20090227185048.J13337@delplex.bde.org> References: <200902261212.n1QCCYI6027315@svn.freebsd.org> <20090226232352.S53478@maildrop.int.zabbadoz.net> <20090227184405.Q13337@delplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, "Bjoern A. Zeeb" , svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Ed Schouten Subject: Re: svn commit: r189066 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Feb 2009 07:59:10 -0000 On Fri, 27 Feb 2009, I wrote: > On Thu, 26 Feb 2009, Bjoern A. Zeeb wrote: > >> On Thu, 26 Feb 2009, Ed Schouten wrote: >>> Log: >>> Remove redundant assignment of `p'. >>> >>> `p' is already initialized with `td->td_proc'. Because td is always >>> curthread, it is safe to initialize it without any locks. >>> >>> Found by: LLVM's scan-build >>> >>> Modified: >>> head/sys/kern/subr_prf.c >>> >>> Modified: head/sys/kern/subr_prf.c >>> ============================================================================== >>> --- head/sys/kern/subr_prf.c Thu Feb 26 12:06:46 2009 (r189065) >>> +++ head/sys/kern/subr_prf.c Thu Feb 26 12:12:34 2009 (r189066) >>> @@ -137,7 +137,6 @@ uprintf(const char *fmt, ...) >>> return (0); >>> >>> sx_slock(&proctree_lock); >>> - p = td->td_proc; >>> PROC_LOCK(p); >>> if ((p->p_flag & P_CONTROLT) == 0) { >>> PROC_UNLOCK(p); >> >> >> I think this one is wrong. You should probably have removed the >> assignment from declaration time as we are checking for td != NULL >> just above that so it could possibly be a NULL pointer deref in the >> initial assigment or the NULL check is redundant. > > Almost all of them are wrong, since they keep the assignment with the > style bug (initialization in declaration) and remove the one without > the style bug. In libkern this also increases the divergence from the > libc versions, since the libc versions are missing the style bug. However, this one is just a style bug, not a runtime bug. Since td is always curthread, it not only doesn't need locking; it is guaranteed to be non-NULL and the check that it is NULL is just garbage. My very old fix for style bugs in uprintf() removes the check as well as removing the correct initialization: % Index: subr_prf.c % =================================================================== % RCS file: /home/ncvs/src/sys/kern/subr_prf.c,v % retrieving revision 1.111 % diff -u -2 -r1.111 subr_prf.c % --- subr_prf.c 18 Jun 2004 20:12:42 -0000 1.111 % +++ subr_prf.c 20 Jun 2004 04:40:46 -0000 % @@ -123,13 +119,13 @@ % uprintf(const char *fmt, ...) % { % - struct thread *td = curthread; % - struct proc *p = td->td_proc; % va_list ap; % struct putchar_arg pca; % + struct proc *p; % + struct thread *td; % int retval; % % - if (td == NULL || td == PCPU_GET(idlethread)) % + td = curthread; % + if (td == PCPU_GET(idlethread)) % return (0); % - % p = td->td_proc; % PROC_LOCK(p); % @@ -148,5 +144,4 @@ % retval = kvprintf(fmt, putchar, &pca, 10, ap); % va_end(ap); % - % return (retval); % } Other style bugs fixed here: - declarations were disordered (td before p to facilitate the style bugs in the initializations, and pointers before structs for no reason) - another initialization in declaration - extra blank lines Bruce