[PATCH 2.4.26] drivers/char/vt.c fix compiler warnings

[PATCH 2.4.26] drivers/char/vt.c fix compiler warnings

[Thomas Gleixner]

The patch fixes the following warnings, produced by gcc3.3.3:

vt.c: In function `do_kdsk_ioctl':
vt.c:166: warning: comparison is always false due to limited range of data 
type
vt.c: In function `do_kdgkb_ioctl':
vt.c:283: warning: comparison is always false due to limited range of data 
type

s is a unsigned char, which can never be >= MAX_NR_KEYMAPS, as MAX_NR_KEYMAPS 
= 256

tmp.kb_func is an unsigned char, which can never be > MAX_NR_FUNC, which is = 
256

Maybe it is neccecary to change the types from unsigned char to uint8_t ?

-- 
Thomas
________________________________________________________________________
"Free software" is a matter of liberty, not price. To understand the concept,
you should think of "free" as in "free speech,'' not as in "free beer".
________________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: tglx@linutronix.de

--- linux-2.4.26.org/drivers/char/vt.c	2002-11-29 00:53:12.000000000 +0100
+++ linux-2.4.26/drivers/char/vt.c	2004-05-15 15:02:05.000000000 +0200
@@ -163,7 +163,7 @@
 
 	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
 		return -EFAULT;
-	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
+	if (i >= NR_KEYS)
 		return -EINVAL;	
 
 	switch (cmd) {
@@ -280,8 +280,6 @@
 	if (copy_from_user(&tmp, user_kdgkb, sizeof(struct kbsentry)))
 		return -EFAULT;
 	tmp.kb_string[sizeof(tmp.kb_string)-1] = '\0';
-	if (tmp.kb_func >= MAX_NR_FUNC)
-		return -EINVAL;
 	i = tmp.kb_func;
 
 	switch (cmd) {

Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings

[Andries Brouwer]

4~On Sat, May 15, 2004 at 03:05:23PM +0200, Thomas Gleixner wrote:

> The patch fixes the following warnings, produced by gcc3.3.3:
> 
> s is a unsigned char, which can never be >= MAX_NR_KEYMAPS, as MAX_NR_KEYMAPS 
> = 256

> -	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
> +	if (i >= NR_KEYS)
>  		return -EINVAL;	

You see that you break thew source in order to avoid a compiler warning. Bad.

(MAX_NR_KEYMAPS is a #define, often 256, on tiny systems people tend to pick 16.
The above test is not superfluous. MAX_NR_KEYMAPS is not an absolute constant.)

Andries

Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings

[Thomas Gleixner]

On Monday 17 May 2004 12:47, Andries Brouwer wrote:
> 4~On Sat, May 15, 2004 at 03:05:23PM +0200, Thomas Gleixner wrote:
> > The patch fixes the following warnings, produced by gcc3.3.3:
> >
> > s is a unsigned char, which can never be >= MAX_NR_KEYMAPS, as
> > MAX_NR_KEYMAPS = 256
> >
> > -	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
> > +	if (i >= NR_KEYS)
> >  		return -EINVAL;
>
> You see that you break thew source in order to avoid a compiler warning.
> Bad.
>
> (MAX_NR_KEYMAPS is a #define, often 256, on tiny systems people tend to
> pick 16. The above test is not superfluous. MAX_NR_KEYMAPS is not an
> absolute constant.)

Ooops, did not think about that. Was just annoyed from the compiler warnings.
What about:

--- linux-2.4.26-preempt/drivers/char/vt.c	2002-11-29 00:53:12.000000000 +0100
+++ linux-2.4.26-rc2-work/drivers/char/vt.c	2004-05-17 14:44:27.000000000 
+0200
@@ -163,7 +163,11 @@
 
 	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
 		return -EFAULT;
+#if MAX_NR_KEYMAPS < 256		
 	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
+#else
+	if (i >= NR_KEYS)
+#endif	
 		return -EINVAL;	
 
 	switch (cmd) {

-- 
Thomas
________________________________________________________________________
Steve Ballmer quotes the statistic that IT pros spend 70 percent of their 
time managing existing systems. That couldn’t have anything to do with 
the fact that 99 percent of these systems run Windows, could it?
________________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: tglx@linutronix.de

Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings

[Andries Brouwer]

On Mon, May 17, 2004 at 02:47:56PM +0200, Thomas Gleixner wrote:

> > (MAX_NR_KEYMAPS is a #define, often 256, on tiny systems people tend to
> > pick 16. The above test is not superfluous. MAX_NR_KEYMAPS is not an
> > absolute constant.)
> 
> Ooops, did not think about that. Was just annoyed from the compiler warnings.
> What about:
> 
>  	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
>  		return -EFAULT;
> +#if MAX_NR_KEYMAPS < 256		
>  	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
> +#else
> +	if (i >= NR_KEYS)
> +#endif	

(i) My muttering was not just about this case - also elsewhere
you removed tests that are only superfluous under a particular
constellation of #define-s.
(ii) Introducing conditional compilation certainly would not
make anybody happier.

I don't have the code nearby, but probably you could use an (unsigned) integer
variable.

Andries

Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On Mon, 17 May 2004 14:47:56 +0200, Thomas Gleixner said:

> Ooops, did not think about that. Was just annoyed from the compiler warnings.
> What about:

> +#if MAX_NR_KEYMAPS < 256		
>  	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
> +#else
> +	if (i >= NR_KEYS)
> +#endif	
>  		return -EINVAL;	

Speaking as somebody who's had stuff rejected for doing this sort of ifdef'ing,
the *right* fix is to go back and look at the definitions of everything, and
see if there's a reason why the compiler is tossing the warning.

Canonical example is, of course the userland:

	char c;
	if ((c=getc()) != EOF) {....

(I don't have a 2.4 tree handy to double-check, but maybe the variable 's'
should be something wider?  It's *quite* possible that there is/will be some
keyboard of the Chinese/Japanese/Korean persuasion which actually ends up with
more than 256 keymap entries.....




[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]