2.4.21-pre4 comparison bugs

2.4.21-pre4 comparison bugs

[Oleg Drokin]

Hello!

   Thanks to whoever mentioned "gcc -W", it's *sweet*  ;)
   Looking at it's output I found few cases where error checking
   does not work.
   Though nothing too serious it seems (except maybe IDE setup-pci stuff,
   I just do not know about that, and may be in that case
   we actually want to change all the functions to return signed
   value, though my fix is certainly less intrusive ;) )
   Most of the patched stuff in here assigns signed value to unsigned
   variable and then checks if it is less than zero which does not work
   for obvious reasons ;)
   I decided taht in most cases simple casting to int would be best
   and least intrusive resolution of a problem.
   The only exception is fs/isofs/inode.c, there we have unsigned int
   (so it is unsigned not depending on any arch) and so '> some num'
   stuff will also check for former negative numbers anyway. So
   I removed one extra comparison in that case.
   See the patch below.

Bye,
    Oleg

===== drivers/ide/setup-pci.c 1.2 vs edited =====
--- 1.2/drivers/ide/setup-pci.c	Thu Dec 12 04:09:23 2002
+++ edited/drivers/ide/setup-pci.c	Sat Feb  8 19:06:13 2003
@@ -582,7 +582,7 @@
 
 	index.all = 0xf0f0;
 
-	if((autodma = ide_setup_pci_controller(dev, d, noisy, &tried_config)) < 0)
+	if((int)(autodma = ide_setup_pci_controller(dev, d, noisy, &tried_config)) < 0)
 		return index;
 
 	/*
@@ -613,7 +613,7 @@
 	} else {
 		if (d->init_chipset)
 		{
-			if(d->init_chipset(dev, d->name) < 0)
+			if((int)d->init_chipset(dev, d->name) < 0)
 				return index;
 		}
 		if (noisy)
===== drivers/net/tun.c 1.8 vs edited =====
--- 1.8/drivers/net/tun.c	Mon Apr 22 21:38:08 2002
+++ edited/drivers/net/tun.c	Sat Feb  8 19:07:28 2003
@@ -188,7 +188,7 @@
 	size_t len = count;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if ((int)(len -= sizeof(pi)) < 0)
 			return -EINVAL;
 
 		memcpy_fromiovec((void *)&pi, iv, sizeof(pi));
===== fs/isofs/inode.c 1.9 vs edited =====
--- 1.9/fs/isofs/inode.c	Mon Jul  1 02:25:33 2002
+++ edited/fs/isofs/inode.c	Sat Feb  8 19:15:41 2003
@@ -343,13 +343,13 @@
 		if (!strcmp(this_char,"session") && value) {
 			char * vpnt = value;
 			unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
-			if(ivalue < 0 || ivalue >99) return 0;
+			if(ivalue > 99) return 0;
 			popt->session=ivalue+1;
 		}
 		if (!strcmp(this_char,"sbsector") && value) {
 			char * vpnt = value;
 			unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0);
-			if(ivalue < 0 || ivalue >660*512) return 0;
+			if(ivalue > 660*512) return 0;
 			popt->sbsector=ivalue;
 		}
 		else if (!strcmp(this_char,"check") && value) {
===== net/sunrpc/pmap_clnt.c 1.3 vs edited =====
--- 1.3/net/sunrpc/pmap_clnt.c	Wed Apr 24 12:58:19 2002
+++ edited/net/sunrpc/pmap_clnt.c	Sat Feb  8 19:09:20 2003
@@ -149,7 +149,7 @@
 	struct sockaddr_in	sin;
 	struct rpc_portmap	map;
 	struct rpc_clnt		*pmap_clnt;
-	unsigned int		error = 0;
+	int			error = 0;
 
 	dprintk("RPC: registering (%d, %d, %d, %d) with portmapper.\n",
 			prog, vers, prot, port);

Re: 2.4.21-pre4 comparison bugs

[J.A. Magallon]

On 2003.02.08 Oleg Drokin wrote:
> Hello!
> 
>    Thanks to whoever mentioned "gcc -W", it's *sweet*  ;)
>    Looking at it's output I found few cases where error checking
>    does not work.
>    Though nothing too serious it seems (except maybe IDE setup-pci stuff,
>    I just do not know about that, and may be in that case
>    we actually want to change all the functions to return signed
>    value, though my fix is certainly less intrusive ;) )
>    Most of the patched stuff in here assigns signed value to unsigned
>    variable and then checks if it is less than zero which does not work
>    for obvious reasons ;)
>    I decided taht in most cases simple casting to int would be best
>    and least intrusive resolution of a problem.
>    The only exception is fs/isofs/inode.c, there we have unsigned int
>    (so it is unsigned not depending on any arch) and so '> some num'
>    stuff will also check for former negative numbers anyway. So
>    I removed one extra comparison in that case.
>    See the patch below.
> 

So:

unsgined f()
{
	return -1;
}

if ((int)f()<0)
??

Wouldn't you get killed by some kind of bit/sign extension in the return ?
Just to be sure, probably the answer is just 'go learn C internals'...

-- 
J.A. Magallon <jamagallon@able.es>      \                 Software is like sex:
werewolf.able.es                         \           It's better when it's free
Mandrake Linux release 9.1 (Cooker) for i586
Linux 2.4.21-pre4-jam1 (gcc 3.2.1 (Mandrake Linux 9.1 3.2.1-5mdk))

Re: 2.4.21-pre4 comparison bugs

[Bill Davidsen]

On Sun, 9 Feb 2003, J.A. Magallon wrote:

> So:
> 
> unsgined f()
> {
> 	return -1;
> }
> 
> if ((int)f()<0)
> ??
> 
> Wouldn't you get killed by some kind of bit/sign extension in the return ?
> Just to be sure, probably the answer is just 'go learn C internals'...

Fuzzy thinking and non-portable. I think the answer is that someone didn't
put enough thought into the error returns. This is trickery to avoid
defining an error value to return. One of those "it works on most
compilers and computers" things. Definitely low human readability. If the
return value is always small enough to be positive if cast to (int), why
not return int? Can't say without looking at actual code rather than a
general example.

Because it mostly works, I'm not sure what priority a more readable return
code would have, though.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: 2.4.21-pre4 comparison bugs

[Alan Cox]

On Sat, 2003-02-08 at 17:18, Oleg Drokin wrote:
> -	if((autodma = ide_setup_pci_controller(dev, d, noisy, &tried_config)) < 0)
> +	if((int)(autodma = ide_setup_pci_controller(dev, d, noisy, &tried_config)) < 0)
>  		return index;

Well caught. I don't like your fix. I'd prefer to do the job properly
and either make it return a signed value or split error/value reporting
in these various cases.

I'll fix them for the next -ac

Re: 2.4.21-pre4 comparison bugs (More of those)

[Oleg Drokin]

Hello!

On Sun, Feb 09, 2003 at 12:58:40AM +0000, Alan Cox wrote:
> > -	if((autodma = ide_setup_pci_controller(dev, d, noisy, &tried_config)) < 0)
> > +	if((int)(autodma = ide_setup_pci_controller(dev, d, noisy, &tried_config)) < 0)
> >  		return index;
> Well caught. I don't like your fix. I'd prefer to do the job properly
> and either make it return a signed value or split error/value reporting
> in these various cases.
> I'll fix them for the next -ac

Ok, here is some more for you ;)
This time I changed the type of variable to signed type whenever
I felt it was appropriate.
When I was not sure (or unsigned type was in some commonly used
structure), I still used a cast just to highlight a problem, so that someone
more knowledgeable created better fix.
See the patch.
Mostly we do incorrect stuff on errors. Sigh, nobody likes errors ;)

Bye,
    Oleg 
===== drivers/char/mwave/mwavedd.c 1.3 vs edited =====
--- 1.3/drivers/char/mwave/mwavedd.c	Wed Feb 13 15:43:48 2002
+++ edited/drivers/char/mwave/mwavedd.c	Sun Feb  9 20:13:45 2003
@@ -500,7 +500,7 @@
 {
 	int i;
 	int retval = 0;
-	unsigned int resultMiscRegister;
+	int resultMiscRegister;
 	pMWAVE_DEVICE_DATA pDrvData = &mwave_s_mdd;
 
 	memset(&mwave_s_mdd, 0, sizeof(MWAVE_DEVICE_DATA));
===== drivers/isdn/hisax/st5481_usb.c 1.8 vs edited =====
--- 1.8/drivers/isdn/hisax/st5481_usb.c	Mon Jan 27 23:49:41 2003
+++ edited/drivers/isdn/hisax/st5481_usb.c	Sun Feb  9 20:21:32 2003
@@ -576,7 +576,7 @@
 	     pipd < pend; 
 	     pipd++) {
 		
-		if (pipd->status < 0) {
+		if ((int)pipd->status < 0) {
 			return (pipd->status);
 		}
 	
===== drivers/message/fusion/mptbase.c 1.7 vs edited =====
--- 1.7/drivers/message/fusion/mptbase.c	Wed Nov 20 23:27:21 2002
+++ edited/drivers/message/fusion/mptbase.c	Sun Feb  9 20:25:57 2003
@@ -1801,7 +1801,7 @@
 {
 	if (this != NULL) {
 		int sz;
-		u32 state;
+		int state;
 
 		/* Disable the FW */
 		state = mpt_GetIocState(this, 1);
===== drivers/mtd/devices/slram.c 1.6 vs edited =====
--- 1.6/drivers/mtd/devices/slram.c	Sat Jan 25 03:25:20 2003
+++ edited/drivers/mtd/devices/slram.c	Sun Feb  9 20:30:10 2003
@@ -246,8 +246,8 @@
 int parse_cmdline(char *devname, char *szstart, char *szlength)
 {
 	char *buffer;
-	unsigned long devstart;
-	unsigned long devlength;
+	long devstart;
+	long devlength;
 	
 	if ((!devname) || (!szstart) || (!szlength)) {
 		unregister_devices();
===== drivers/net/acenic.c 1.27 vs edited =====
--- 1.27/drivers/net/acenic.c	Fri Sep 20 03:49:29 2002
+++ edited/drivers/net/acenic.c	Sun Feb  9 20:34:09 2003
@@ -1157,8 +1157,8 @@
 	struct pci_dev *pdev;
 	unsigned long myjif;
 	u64 tmp_ptr;
-	u32 tig_ver, mac1, mac2, tmp, pci_state;
-	int board_idx, ecode = 0;
+	u32 tig_ver, mac1, mac2, pci_state;
+	int board_idx, ecode = 0, tmp;
 	short i;
 	unsigned char cache_size;
 
===== drivers/net/wan/8253x/8253xini.c 1.1 vs edited =====
--- 1.1/drivers/net/wan/8253x/8253xini.c	Thu Apr  4 23:05:10 2002
+++ edited/drivers/net/wan/8253x/8253xini.c	Sun Feb  9 20:31:37 2003
@@ -2196,7 +2196,7 @@
 	SAB_BOARD *boardptr;
 	SAB_PORT *portptr;
 	struct net_device *dev;
-	unsigned int result;
+	int result;
 	unsigned int namelength;
 	unsigned int portno;
 	int intr_val;
===== drivers/net/wan/8253x/8253xtty.c 1.1 vs edited =====
--- 1.1/drivers/net/wan/8253x/8253xtty.c	Thu Apr  4 23:05:10 2002
+++ edited/drivers/net/wan/8253x/8253xtty.c	Sun Feb  9 20:32:38 2003
@@ -135,7 +135,7 @@
 	register unsigned int slopspace;
 	register int sendsize;
 	unsigned int totaltransmit;
-	unsigned fifospace;
+	int  fifospace;
 	unsigned loadedcount;
 	struct tty_struct *tty = port->tty;
 	
===== drivers/scsi/osst.c 1.10 vs edited =====
--- 1.10/drivers/scsi/osst.c	Tue Feb  5 17:06:58 2002
+++ edited/drivers/scsi/osst.c	Sun Feb  9 20:38:01 2003
@@ -4680,7 +4680,7 @@
 	 unsigned int cmd_in, unsigned long arg)
 {
 	int i, cmd_nr, cmd_type, retval = 0;
-	unsigned int blk;
+	int blk;
 	OS_Scsi_Tape *STp;
 	ST_mode *STm;
 	ST_partstat *STps;
===== drivers/scsi/aacraid/aachba.c 1.3 vs edited =====
--- 1.3/drivers/scsi/aacraid/aachba.c	Mon Jul 29 16:58:43 2002
+++ edited/drivers/scsi/aacraid/aachba.c	Sun Feb  9 20:35:01 2003
@@ -233,7 +233,8 @@
 int aac_get_containers(struct aac_dev *dev)
 {
 	struct fsa_scsi_hba *fsa_dev_ptr;
-	u32 index, status = 0;
+	u32 index;
+	int status = 0;
 	struct aac_query_mount *dinfo;
 	struct aac_mount *dresp;
 	struct fib * fibptr;
===== drivers/usb/hcd/ehci-sched.c 1.7 vs edited =====
--- 1.7/drivers/usb/hcd/ehci-sched.c	Fri Dec 20 10:33:27 2002
+++ edited/drivers/usb/hcd/ehci-sched.c	Sun Feb  9 20:49:44 2003
@@ -549,7 +549,7 @@
 	u64		temp;
 	u32		buf1;
 	unsigned	i, epnum, maxp, multi;
-	unsigned	length;
+	int	length;
 	int		is_input;
 
 	itd->hw_next = EHCI_LIST_END;
===== fs/intermezzo/psdev.c 1.7 vs edited =====
--- 1.7/fs/intermezzo/psdev.c	Fri Oct 11 02:24:51 2002
+++ edited/fs/intermezzo/psdev.c	Sun Feb  9 20:44:48 2003
@@ -605,7 +605,7 @@
             if (req->rq_flags & REQ_WRITE) {
                     out = (struct izo_upcall_resp *)req->rq_data;
                     /* here we map positive Lento errors to kernel errors */
-                    if ( out->result < 0 ) {
+                    if ( (int)out->result < 0 ) {
                             CERROR("Tell Peter: Lento returns negative error %d, for oc %d!\n",
                                    out->result, out->opcode);
                           out->result = EINVAL;
===== fs/intermezzo/super.c 1.4 vs edited =====
--- 1.4/fs/intermezzo/super.c	Fri Oct 11 02:24:51 2002
+++ edited/fs/intermezzo/super.c	Sun Feb  9 20:45:35 2003
@@ -200,7 +200,7 @@
         char *fileset = NULL;
         char *channel = NULL;
         int err; 
-        unsigned int minor;
+        int minor;
 
         ENTRY;
 
===== net/decnet/af_decnet.c 1.12 vs edited =====
--- 1.12/net/decnet/af_decnet.c	Tue Aug 13 00:43:21 2002
+++ edited/net/decnet/af_decnet.c	Sun Feb  9 20:47:24 2003
@@ -1180,7 +1180,7 @@
 	struct sock *sk = sock->sk;
 	struct dn_scp *scp = DN_SK(sk);
 	int err = -EOPNOTSUPP;
-	unsigned long amount = 0;
+	long amount = 0;
 	struct sk_buff *skb;
 	int val;
 
===== net/ipv4/netfilter/ip_conntrack_irc.c 1.5 vs edited =====
--- 1.5/net/ipv4/netfilter/ip_conntrack_irc.c	Thu Aug  8 18:49:17 2002
+++ edited/net/ipv4/netfilter/ip_conntrack_irc.c	Sun Feb  9 20:48:02 2003
@@ -37,7 +37,7 @@
 static int ports[MAX_PORTS];
 static int ports_c = 0;
 static int max_dcc_channels = 8;
-static unsigned int dcc_timeout = 300;
+static int dcc_timeout = 300;
 
 MODULE_AUTHOR("Harald Welte <laforge@gnumonks.org>");
 MODULE_DESCRIPTION("IRC (DCC) connection tracking module");

Re: 2.4.21-pre4 comparison bugs (Even More Again)

[Oleg Drokin]

Hello!

    Ok. In addition to "unsigned_var < 0" kind of error checks that
    never work, there is different non-working kind of checks:
    "pointer < 0".
    We can see these at:
drivers/char/joystick/tmdc.c:318	if (tmdc->abs[i] < 0) continue;
drivers/char/epca.c:3758		if (board.port <= 0)
drivers/char/epca.c:3770		if (board.membase <= 0)
drivers/media/radio/radio-cadet.c:541	if(request_region(io,2, "cadet-probe")>=0) {
drivers/net/wan/dscc4.c:1760		if (dscc4_init_dummy_skb(dpriv) < 0)

     Given the fact that you seem not to like casts to signed int,
     how do you propose to fix these?

Bye,
    Oleg

Re: 2.4.21-pre4 comparison bugs (Even More Again)

[Alan Cox]

On Sun, 2003-02-09 at 18:22, Oleg Drokin wrote:
> Hello!
> 
>     Ok. In addition to "unsigned_var < 0" kind of error checks that
>     never work, there is different non-working kind of checks:
>     "pointer < 0".
>     We can see these at:
> drivers/char/joystick/tmdc.c:318	if (tmdc->abs[i] < 0) continue;
> drivers/char/epca.c:3758		if (board.port <= 0)
> drivers/char/epca.c:3770		if (board.membase <= 0)
> drivers/media/radio/radio-cadet.c:541	if(request_region(io,2, "cadet-probe")>=0) {
> drivers/net/wan/dscc4.c:1760		if (dscc4_init_dummy_skb(dpriv) < 0)
> 
>      Given the fact that you seem not to like casts to signed int,
>      how do you propose to fix these?

By actually going and reading the drivers to see why the errors occur
and if they are meaningful. You need to know the possible returns and
the intent of those returns.

Eg radio-cadet request region appears not be a cast problem but a
complete misunderstanding of the return codes. Likewise epca it looks
like the board.port/board.membase are just overbroad checks and in fact
harmless.

Re: 2.4.21-pre4 comparison bugs (More of those)

[Alan Cox]

On Sun, 2003-02-09 at 17:53, Oleg Drokin wrote:
> This time I changed the type of variable to signed type whenever
> I felt it was appropriate.
> When I was not sure (or unsigned type was in some commonly used
> structure), I still used a cast just to highlight a problem, so that someone
> more knowledgeable created better fix.
> See the patch.
> Mostly we do incorrect stuff on errors. Sigh, nobody likes errors ;)

Hiding them is even worse than having them there visible and unfixed.
Changing the sign on stuff holding physical addresses is actually
introducing real bugs

Re: 2.4.21-pre4 comparison bugs (More of those)

[Oleg Drokin]

Hello!

On Sun, Feb 09, 2003 at 10:01:30PM +0000, Alan Cox wrote:
> > This time I changed the type of variable to signed type whenever
> > I felt it was appropriate.
> > When I was not sure (or unsigned type was in some commonly used
> > structure), I still used a cast just to highlight a problem, so that someone
> > more knowledgeable created better fix.
> > See the patch.
> > Mostly we do incorrect stuff on errors. Sigh, nobody likes errors ;)
> Hiding them is even worse than having them there visible and unfixed.
> Changing the sign on stuff holding physical addresses is actually
> introducing real bugs

I assume you are speaking of slram stuff here.
I thought that slram was not designed to work with parts of RAM past 2G border.
(as far as I remember, slram was used on old x86 HW to convert uncached RAM
beyond 64M (256M for some systems?) into kind of a ramdisk.)

Bye,
    Oleg

2.4.21-pre4 more extra semicolons bugs

[Oleg Drokin]

Hello!

    It is a boring sunday here today, so I decided to play with gcc -W
    on a 2.4.21-pre4 (from current bitkeeper tree) with all the stuff
    possible compiled in.
    Seems that megaraid and 8253xtty have extra semicolons. Nothing
    too serious, though.

Bye,
    Oleg
===== drivers/net/wan/8253x/8253xtty.c 1.1 vs edited =====
--- 1.1/drivers/net/wan/8253x/8253xtty.c	Thu Apr  4 23:05:10 2002
+++ edited/drivers/net/wan/8253x/8253xtty.c	Sun Feb  9 19:32:58 2003
@@ -2131,7 +2131,7 @@
 	/* Check whether or not the port is open in SYNC mode */
 	if(port->open_type == OPEN_SYNC_NET)
 	{
-		if(port->dev && netif_carrier_ok(port->dev));
+		if(port->dev && netif_carrier_ok(port->dev))
 		{
 			port->tty= NULL;	/* Don't bother with open counting here
 						   but make sure the tty field is NULL*/
===== drivers/scsi/megaraid.c 1.21 vs edited =====
--- 1.21/drivers/scsi/megaraid.c	Fri Dec 13 12:29:59 2002
+++ edited/drivers/scsi/megaraid.c	Sun Feb  9 19:39:12 2003
@@ -4936,7 +4936,7 @@
 		if( ioc.mbox[0] == MEGA_MBOXCMD_PASSTHRU ) {
 			put_user( scsicmd->result, &uioc->pthru.scsistatus );
 			if (copy_to_user( uioc->pthru.reqsensearea, scsicmd->sense_buffer,
-							  MAX_REQ_SENSE_LEN ));
+							  MAX_REQ_SENSE_LEN ))
 				ret= -EFAULT;
 		} else {
 			put_user(1, &uioc->mbox[16]);	/* numstatus */

2.4.21-pre4 - two simple compile fixes

[Oleg Drokin]

Hello!

  With this trivial patch below I am able to compile with support
  for come Cadet radio card and NatSemi SCx200

Bye,
    Oleg

===== drivers/char/Makefile 1.30 vs edited =====
--- 1.30/drivers/char/Makefile	Tue Jan  7 18:03:18 2003
+++ edited/drivers/char/Makefile	Sun Feb  9 17:48:17 2003
@@ -24,7 +24,7 @@
 export-objs     :=	busmouse.o console.o keyboard.o sysrq.o \
 			misc.o pty.o random.o selection.o serial.o \
 			sonypi.o tty_io.o tty_ioctl.o generic_serial.o \
-			au1000_gpio.o hp_psaux.o nvram.o
+			au1000_gpio.o hp_psaux.o nvram.o scx200.o
 
 mod-subdirs	:=	joystick ftape drm drm-4.0 pcmcia
 
===== drivers/media/radio/radio-cadet.c 1.5 vs edited =====
--- 1.5/drivers/media/radio/radio-cadet.c	Fri Jan 31 15:03:08 2003
+++ edited/drivers/media/radio/radio-cadet.c	Sun Feb  9 17:59:13 2003
@@ -558,7 +558,7 @@
 static int __init cadet_init(void)
 {
 
-	spin_lock_init(&cadet_lock);
+	spin_lock_init(&cadet_io_lock);
 	
 	/*
 	 *	If a probe was requested then probe ISAPnP first (safest)

Re: 2.4.21-pre4 - two simple compile fixes

[Alan Cox]

On Sun, 2003-02-09 at 16:59, Oleg Drokin wrote:
> Hello!
> 
>   With this trivial patch below I am able to compile with support
>   for come Cadet radio card and NatSemi SCx200
> 

I sent Marcelo the cadet one already and its fine, the scx200 one looks right too
I'll check that in -ac