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))
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
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.
Rusty Russell <[email protected]> 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
On Mon, 10 Feb 2003 14:01:13 +0100, Horst von Brand said:
> Rusty Russell <[email protected]> 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
[email protected] said:
> On Mon, 10 Feb 2003 14:01:13 +0100, Horst von Brand said:
> > Rusty Russell <[email protected]> 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
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