Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Valdis.Kletnieks]

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

On Fri, 07 Feb 2003 12:23:22 EST, Frank Davis said:
>
> +++ linux/drivers/net/fc/iph5526.c	2003-02-07 02:13:43.000000000 -0500

> -	for (i = 0; i < clone_list[i].vendor_id != 0; i++)
> +	for (i = 0; ((i < clone_list[i].vendor_id) && (clone_list[i].vendor_id 
!= 0)); i++)

'i < clone_list[i].vendor_id'?

Currently, clone_list only has 1 vendor listed:
#define PCI_VENDOR_ID_INTERPHASE        0x107e

I suspect this was intended:

	for (i = 0; (clone_list[i].vendor_id != 0); i++)
-- 
				Valdis Kletnieks
				Computer Systems Senior Engineer
				Virginia Tech


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

[PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Frank Davis]

Hello all,
   The following patch addresses buzilla bug # 323, and removes a double 
logical issue. Please review for inclusion.

Regards,
Frank

--- linux/drivers/net/fc/iph5526.c.old	2003-01-16 21:21:45.000000000 -0500
+++ linux/drivers/net/fc/iph5526.c	2003-02-07 02:13:43.000000000 -0500
@@ -3769,7 +3769,7 @@
 	for (i = 0; i <= MAX_FC_CARDS; i++) 
 		fc[i] = NULL;
 
-	for (i = 0; i < clone_list[i].vendor_id != 0; i++)
+	for (i = 0; ((i < clone_list[i].vendor_id) && (clone_list[i].vendor_id != 0)); i++)
 	while ((pdev = pci_find_device(clone_list[i].vendor_id, clone_list[i].device_id, pdev))) {
 		unsigned short pci_command;
 		if (pci_enable_device(pdev))

Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Rusty Russell]

In message <Pine.LNX.4.44.0302071221490.6917-100000@master> you write:
> --- linux/drivers/net/fc/iph5526.c.old	2003-01-16 21:21:45.000000000 -
0500
> +++ linux/drivers/net/fc/iph5526.c	2003-02-07 02:13:43.000000000 -0500
> @@ -3769,7 +3769,7 @@
>  	for (i = 0; i <= MAX_FC_CARDS; i++) 
>  		fc[i] = NULL;
>  
> -	for (i = 0; i < clone_list[i].vendor_id != 0; i++)
> +	for (i = 0; ((i < clone_list[i].vendor_id) && (clone_list[i].vendor_id != 0)); i++)
>  	while ((pdev = pci_find_device(clone_list[i].vendor_id, clone_list[i].device_id, pdev))) {
>  		unsigned short pci_command;
>  		if (pci_enable_device(pdev))
> 

Once again, up to the author.  Vineet?

Thanks,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Horst von Brand]

Rusty Russell <rusty@rustcorp.com.au> said:

[...]

> > -	for (i = 0; i < clone_list[i].vendor_id != 0; i++)

i < clone_list[i].vendor_id != 0 is (i < clone_list[i].vendor_id) != 0 is
just i < clone_list[i].vendor_id, so the for is done for i = 0 and possibly
for 1. Getting this effect (if desired) with an if is a load clearer.

Having i twice in the condition is also highly suspicious... is this code
ever executed? It looks like a bad screwup in the condition that went
unnoticed because it was never hit.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Valdis.Kletnieks]

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

On Mon, 10 Feb 2003 14:01:13 +0100, Horst von Brand said:
> Rusty Russell <rusty@rustcorp.com.au> said:
> 
> [...]
> 
> > > -	for (i = 0; i < clone_list[i].vendor_id != 0; i++)
> 
> i < clone_list[i].vendor_id != 0 is (i < clone_list[i].vendor_id) != 0 is
> just i < clone_list[i].vendor_id, so the for is done for i = 0 and possibly
> for 1. Getting this effect (if desired) with an if is a load clearer.

However, looking at the definition of clone_list[], it's pretty obvious
that this was intended:

    for (i=0; clone_list[i].vendor_id != 0; i++) {...

It's searching through a zero-terminated table of vendor_id's.

It's possible it started off life as
     i < sizeof(clone_list) && clone_list[i].vendor_id != 0

or some such.  
-- 
				Valdis Kletnieks
				Computer Systems Senior Engineer
				Virginia Tech


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

Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Horst von Brand]

Valdis.Kletnieks@vt.edu said:
> On Mon, 10 Feb 2003 14:01:13 +0100, Horst von Brand said:
> > Rusty Russell <rusty@rustcorp.com.au> said:
> > 
> > [...]
> > 
> > > > -	for (i = 0; i < clone_list[i].vendor_id != 0; i++)
> > 
> > i < clone_list[i].vendor_id != 0 is (i < clone_list[i].vendor_id) != 0 is
> > just i < clone_list[i].vendor_id, so the for is done for i = 0 and possibly
> > for 1. Getting this effect (if desired) with an if is a load clearer.
> 
> However, looking at the definition of clone_list[], it's pretty obvious
> that this was intended:
> 
>     for (i=0; clone_list[i].vendor_id != 0; i++) {...
> 
> It's searching through a zero-terminated table of vendor_id's.

It isn't, as written. Something is very wrong in any case, as it should
have blown up somewhere before.

In any case, the != 0 is redundant, idiomatic C is to just go:

     for (i=0; clone_list[i].vendor_id; i++) {...
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] 2.5.59 : drivers/net/fc/iph5526.c

[Valdis.Kletnieks]

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

On Tue, 11 Feb 2003 10:01:37 +0100, Horst von Brand said:

> > > > > -	for (i = 0; i < clone_list[i].vendor_id != 0; i++)

> It isn't, as written. Something is very wrong in any case, as it should
> have blown up somewhere before.

Well, currently, there's only like 3 entries in the table - two clones
and a null. So for i=0 and i=1, 'i<vendor_id' happens to be true, and we
compare that to a zero. Then for i=2, i is greater than the terminating
null in the table, and we compare THAT to zero.

> In any case, the != 0 is redundant, idiomatic C is to just go:
> 
>      for (i=0; clone_list[i].vendor_id; i++) {...

True.  Notice I said "what was intended", not "what is idiomatic". ;)
-- 
				Valdis Kletnieks
				Computer Systems Senior Engineer
				Virginia Tech


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