LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings
@ 2004-05-15 13:05 Thomas Gleixner
  2004-05-17 10:47 ` Andries Brouwer
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2004-05-15 13:05 UTC (permalink / raw)
  To: linux-kernel


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) {


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings
  2004-05-15 13:05 [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings Thomas Gleixner
@ 2004-05-17 10:47 ` Andries Brouwer
  2004-05-17 12:47   ` Thomas Gleixner
  0 siblings, 1 reply; 5+ messages in thread
From: Andries Brouwer @ 2004-05-17 10:47 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings
  2004-05-17 10:47 ` Andries Brouwer
@ 2004-05-17 12:47   ` Thomas Gleixner
  2004-05-17 13:36     ` Andries Brouwer
  2004-05-17 15:40     ` Valdis.Kletnieks
  0 siblings, 2 replies; 5+ messages in thread
From: Thomas Gleixner @ 2004-05-17 12:47 UTC (permalink / raw)
  To: Andries Brouwer; +Cc: linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings
  2004-05-17 12:47   ` Thomas Gleixner
@ 2004-05-17 13:36     ` Andries Brouwer
  2004-05-17 15:40     ` Valdis.Kletnieks
  1 sibling, 0 replies; 5+ messages in thread
From: Andries Brouwer @ 2004-05-17 13:36 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andries Brouwer, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings 
  2004-05-17 12:47   ` Thomas Gleixner
  2004-05-17 13:36     ` Andries Brouwer
@ 2004-05-17 15:40     ` Valdis.Kletnieks
  1 sibling, 0 replies; 5+ messages in thread
From: Valdis.Kletnieks @ 2004-05-17 15:40 UTC (permalink / raw)
  To: tglx; +Cc: Andries Brouwer, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2004-05-17 15:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-05-15 13:05 [PATCH 2.4.26] drivers/char/vt.c fix compiler warnings Thomas Gleixner
2004-05-17 10:47 ` Andries Brouwer
2004-05-17 12:47   ` Thomas Gleixner
2004-05-17 13:36     ` Andries Brouwer
2004-05-17 15:40     ` Valdis.Kletnieks

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).