ehci tests unsigned variables against 0

ehci tests unsigned variables against 0

[Matthew Wilcox]

On Fri, Apr 18, 2008 at 09:08:55PM +0200, Julia Lawall wrote:
> I found 63 occurrences of this problem with the following semantic match
> (http://www.emn.fr/x-info/coccinelle/):
> 
> @@ unsigned int i; @@
> 
> * i < 0
> 
> I looked through all of the results by hand, and they all seem to be 
> problems.  In many cases, it seems like the variable should not be 
> unsigned as it is used to hold the return value of a function that might 
> return a negative error code, but I haven't looked into this in detail.
> 
> In the output below, the lines that begin with a single start contain a 
> test of whether an unsigned variable or structure field is less than 0.
> The output is actually generated with diff, but I converted the -s to *s 
> to avoid confusion.

> diff -u -p a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> *** a/drivers/usb/host/ehci-dbg.c 2008-03-12 14:13:14.000000000 +0100
> @@ -454,7 +454,7 @@ static void qh_lines (
>  				(scratch >> 16) & 0x7fff,
>  				scratch,
>  				td->urb);
> *		if (temp < 0)
>  			temp = 0;
>  		else if (size < temp)
>  			temp = size;
> @@ -465,7 +465,7 @@ static void qh_lines (
>  	}
>  
>  	temp = snprintf (next, size, "\n");
> *	if (temp < 0)
>  		temp = 0;
>  	else if (size < temp)
>  		temp = size;

These tests will never trigger and should simply be removed.  Linux's
kernel snprintf function conforms to C99 and never returns 0.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: ehci tests unsigned variables against 0

[David Brownell]

On Saturday 19 April 2008, Matthew Wilcox wrote:
> These tests will never trigger and should simply be removed.  Linux's
> kernel snprintf function conforms to C99 and never returns 0.

I think you mean "never returns negative" ?

-ENOPATCH

... although if that's the case, I'd think that the *snprintf()
signatures are incorrect:  they should return "unsigned" not "int".
If that were done, I think even GCC could be made to report such
issues; one wouldn't need less-common tools like coccinelle.

(What I've seen of coccinelle makes me glad it's being applied to
the kernel sources, by the way.)

- Dave
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ehci tests unsigned variables against 0

[Matthew Wilcox]

On Mon, Apr 21, 2008 at 12:26:10PM -0700, David Brownell wrote:
> On Saturday 19 April 2008, Matthew Wilcox wrote:
> > These tests will never trigger and should simply be removed. ?Linux's
> > kernel snprintf function conforms to C99 and never returns 0.
> 
> I think you mean "never returns negative" ?

True.

> -ENOPATCH

Yeah ... I didn't bother with one.  Let's try now:

diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
index 64ebfc5..55a2c73 100644
--- a/drivers/usb/host/ehci-dbg.c
+++ b/drivers/usb/host/ehci-dbg.c
@@ -454,9 +454,7 @@ static void qh_lines (
 				(scratch >> 16) & 0x7fff,
 				scratch,
 				td->urb);
-		if (temp < 0)
-			temp = 0;
-		else if (size < temp)
+		if (size < temp)
 			temp = size;
 		size -= temp;
 		next += temp;
@@ -465,9 +463,7 @@ static void qh_lines (
 	}
 
 	temp = snprintf (next, size, "\n");
-	if (temp < 0)
-		temp = 0;
-	else if (size < temp)
+	if (size < temp)
 		temp = size;
 	size -= temp;
 	next += temp;

> ... although if that's the case, I'd think that the *snprintf()
> signatures are incorrect:  they should return "unsigned" not "int".
> If that were done, I think even GCC could be made to report such
> issues; one wouldn't need less-common tools like coccinelle.

We can take it up with ANSI, but I'm not sure they'll be interested in
changing the return type of snprintf ...

> (What I've seen of coccinelle makes me glad it's being applied to
> the kernel sources, by the way.)

Me too!

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: ehci tests unsigned variables against 0

[David Brownell]

On Monday 21 April 2008, Matthew Wilcox wrote:
> > -ENOPATCH
> 
> Yeah ... I didn't bother with one.  Let's try now:

If you send this as a proper patch to linux-usb@vger,
you can add

Acked-by: David Brownell <dbrownell@users.sourceforge.net>

> 
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index 64ebfc5..55a2c73 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -454,9 +454,7 @@ static void qh_lines (
>  				(scratch >> 16) & 0x7fff,
>  				scratch,
>  				td->urb);
> -		if (temp < 0)
> -			temp = 0;
> -		else if (size < temp)
> +		if (size < temp)
>  			temp = size;
>  		size -= temp;
>  		next += temp;
> @@ -465,9 +463,7 @@ static void qh_lines (
>  	}
>  
>  	temp = snprintf (next, size, "\n");
> -	if (temp < 0)
> -		temp = 0;
> -	else if (size < temp)
> +	if (size < temp)
>  		temp = size;
>  	size -= temp;
>  	next += temp;
> 
> > ... although if that's the case, I'd think that the *snprintf()
> > signatures are incorrect:  they should return "unsigned" not "int".
> > If that were done, I think even GCC could be made to report such
> > issues; one wouldn't need less-common tools like coccinelle.
> 
> We can take it up with ANSI, but I'm not sure they'll be interested in
> changing the return type of snprintf ...

I don't care so much about ANSI as about <linux/kernel.h>,
at least in this context...

- Dave