Re: [PATCH] kdb: match return value to function signature

[Nicholas Mc Guire <der.herr@hofr.at>]

On Tue, 12 May 2015, Daniel Thompson wrote:

> On 12/05/15 13:46, Nicholas Mc Guire wrote:
>> kdb_task_state_string() introduced in the initial commit 5d5314d6795f
>> ("kdb: core for kgdb back end (1 of 2)") returns unsigned long (a bit
>> array of states) but intermediately it is being assigned to a long which
>> make static code checkers unhappy and also does not not help readability
>> (technically there is no reason to use a signed type here).
>>
>> Type-checking coccinelle spatches are being used to locate type mismatches
>> between function signatures and return values in this case it produced:
>> ./kernel/debug/kdb/kdb_support.c:611 WARNING: return of wrong type
>>                                   unsigned long != long
>>
>> Patch was compile tested with x86_64_defconfig + CONFIG_KGDB=y,
>> CONFIG_KGDB_KDB=y
>>
>> Patch is against 4.1-rc3 (localversion-next is -next-20150512)
>>
>> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
>> ---
>>   kernel/debug/kdb/kdb_support.c |   14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> All (5) call-sites of kdb_task_state_string() were checked and all are
>> expecting an unsigned long as the function signature provides - so this
>> change should have no effect as automatic type conversion did not make
>> the signed type visible externally and internally the signed nature was
>> also not in use.
>>
>> Doc fixup: long -> unsigned long only (and some reformatting this caused)
>>
>> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
>> index d35cc2d..0bb3b81 100644
>> --- a/kernel/debug/kdb/kdb_support.c
>> +++ b/kernel/debug/kdb/kdb_support.c
>> @@ -544,12 +544,12 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
>>    * Returns:
>>    *	Mask for process state.
>>    * Notes:
>> - *	The mask folds data from several sources into a single long value, so
>> - *	be careful not to overlap the bits.  TASK_* bits are in the LSB,
>> - *	special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1 there
>> - *	is no overlap between TASK_* and EXIT_* but that may not always be
>> - *	true, so EXIT_* bits are shifted left 16 bits before being stored in
>> - *	the mask.
>> + *	The mask folds data from several sources into a single unsigned long
>> + *      value, so be careful not to overlap the bits.  TASK_* bits are in the
>> + *      LSB, special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1
>> + *      there is no overlap between TASK_* and EXIT_* but that may not always
>> + *      be true, so EXIT_* bits are shifted left 16 bits before being stored
>> + *      in the mask.
>
> The ragged alignment here looks like a spurious replacement of tabs with  
> spaces.
>
> BTW having the type of the variable int he comment is pointless anyway.  
> If you simply remove the type from the comment ("folds data into several  
> sources into a single value") then you don't have to reflow the comment  
> in the first place.
>

makes sense - will resubmit without the noise around the doc fix.

>>    */
>>
>>   /* unrunnable is < 0 */
>> @@ -560,7 +560,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
>>
>>   unsigned long kdb_task_state_string(const char *s)
>>   {
>> -	long res = 0;
>> +	unsigned long res = 0;
>>   	if (!s) {
>>   		s = kdbgetenv("PS");
>>   		if (!s)
>>
>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>

thx!
hofrat