Date: Fri, 27 Feb 2009 18:59:06 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net>, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Ed Schouten <ed@FreeBSD.org> Subject: Re: svn commit: r189066 - head/sys/kern Message-ID: <20090227185048.J13337@delplex.bde.org> In-Reply-To: <20090227184405.Q13337@delplex.bde.org> References: <200902261212.n1QCCYI6027315@svn.freebsd.org> <20090226232352.S53478@maildrop.int.zabbadoz.net> <20090227184405.Q13337@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090227185048.J13337>