svn commit: r189066 - head/sys/kern

[Bruce Evans]

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