Mailing List Archive

On Thu, Jul 01, 2004 at 04:41:50PM +0100, Paul Jakma wrote:
> Right, best I can think of is something like:
>
> #define LIST_LOOP_SAFE (LIST,VAL,THIS,NEXT) \
> for ((THIS) = (LIST)->head; (THIS) != NULL; (THIS) = (NEXT))\
> if (((VAL) = (THIS)->data) != NULL)
>
> #define LIST_LOOP_SAFE_NEXT (THIS,NEXT) \
> ((THIS)->next != NULL) ? ((NEXT) = (THIS)->next) : ((NEXT) = (THIS));
>
> usage would obviously be:
>
> LIST_LOOP_SAFE (list, val, tn, nn)
> {
> LIST_LOOP_SAFE_NEXT (tn, nn);
> <etc>
> }
>
> 1. is it worth it?
>
> 2. should we just replace existing LIST_LOOP with above (s/_SAFE//)
> and update all users?

I'm afraid that doesn't work, because you need to set nn before
you do this test:

if (((VAL) = (THIS)->data) != NULL)

(i.e. you must always set nn regardless of whether THIS->data is non-NULL)

-Andy

On Thu, 1 Jul 2004, Andrew J. Schorr wrote:

> I don't think the ternary expression is accomplishing anything there.
> How is that different from:
>
> (NEXT) = (THIS)->next;
>
> ?

that's probably just as good yes ;)

i've been fiddling with a whole bunch of ways of trying to make
LIST_LOOP safe, as I have a bit of code here where i'm using
LISTNODE_DETACH/ATTACH to move a listnode from one list to another.
Somehow that convulated way of saying above got stuck in there.

That approach at least should work. if it's worth doing i can do the
search and replace.

> -Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Graduate life: It's not just a job. It's an indenture.

On Thu, 1 Jul 2004, Andrew J. Schorr wrote:

> I'm afraid that doesn't work, because you need to set nn before
> you do this test:
>
> if (((VAL) = (THIS)->data) != NULL)
>
> (i.e. you must always set nn regardless of whether THIS->data is non-NULL)

Sigh, yes.

I give up for now.

> -Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Washington, D.C: Fifty square miles almost completely surrounded by reality.

On Mon, 21 Jun 2004, Kir Kostuchenko wrote:

> This patch works for me.

Thanks for diagnosing the problem. I'm committing a simpler fix to
CVS.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
Fortune:
Nature, to be commanded, must be obeyed.
-- Francis Bacon

Paul Jakma wrote:

> On Mon, 21 Jun 2004, Kir Kostuchenko wrote:
>
>> This patch works for me.
>
>
> Thanks for diagnosing the problem. I'm committing a simpler fix to CVS.
>
> regards,

I don't see this in the CVS...

Please.

On Wed, 4 Aug 2004, Kir Kostuchenko wrote:

> I don't see this in the CVS...
>
> Please.

ah, you're too quick. I'm proposing the following, with a nice big
comment cause I had to go reacquint myself with it all. essentially:

- compare only the list head, because all the interfaces on the
nexthop list should already have same cost

- delete the whole list if we have new < hop, but in a tidier list
loop (backward jumps are a bit evil imho, shouldnt be used unless
absolutely neccesary).

Hmm, i accidently deleted the hop->oi assert..

Index: ospf_spf.c
===================================================================
RCS file: /var/cvsroot/quagga/ospfd/ospf_spf.c,v
retrieving revision 1.9
diff -u -r1.9 ospf_spf.c
--- ospf_spf.c 8 Apr 2004 07:43:45 -0000 1.9
+++ ospf_spf.c 4 Aug 2004 15:53:17 -0000
@@ -341,31 +341,49 @@
return NULL;
}

-/* Consider supplied next-hop for inclusion to the supplied list
- * of next-hops, adjust list as neccessary
+/*
+ * Consider supplied next-hop for inclusion to the supplied list of
+ * equal-cost next-hops, adjust list as neccessary.
+ *
+ * (Discussed on GNU Zebra list 27 May 2003, [zebra 19184])
+ *
+ * Note that below is a bit of a hack, and limits ECMP to paths that go to
+ * same nexthop. Where as paths via inequal output_cost interfaces could
+ * still quite easily be ECMP due to remote cost differences.
+ *
+ * It really should be done by way of recording currently valid backlinks
+ * and determining the appropriate nexthops from the list of backlinks.
*/
void
ospf_spf_consider_nexthop (struct list *nexthops,
struct vertex_nexthop *newhop)
{
- struct listnode *nnode;
struct vertex_nexthop *hop;
+ struct listnode *ln, *nn;

- LIST_LOOP (nexthops, hop, nnode)
- {
- assert (hop->oi);
- /* weed out hops with higher cost than the newhop */
- if (hop->oi->output_cost > newhop->oi->output_cost)
- {
- /* delete the existing nexthop */
- listnode_delete (nexthops, hop);
- vertex_nexthop_free (hop);
- }
- else if (hop->oi->output_cost < newhop->oi->output_cost)
- {
+ /* nexthop list should contain only the set of nexthops that have the lowest
+ * equal cost
+ */
+ if (nexthops->head != NULL)
+ {
+ hop = getdata (nexthops->head);
+
+ /* weed out hops with higher cost than the newhop */
+ if (hop->oi->output_cost > newhop->oi->output_cost)
+ {
+ /* delete the existing nexthops */
+ for (ln = nexthops->head; ln; ln = nn)
+ {
+ nn = ln->next;
+ hop = getdata (ln);
+
+ listnode_delete (nexthops, hop);
+ vertex_nexthop_free (hop);
+ }
+ }
+ else if (hop->oi->output_cost < newhop->oi->output_cost)
return;
- }
- }
+ }

/* new hop is <= existing hops, add it */
listnode_add (nexthops, newhop);
@@ -455,7 +473,7 @@
nh = vertex_nexthop_new (v);
nh->oi = oi;
nh->router.s_addr = 0;
- listnode_add (w->nexthop, nh);
+ ospf_spf_consider_nexthop (w->nexthop, nh);
}
}
}
@@ -474,7 +492,7 @@
nh = vertex_nexthop_new (v);
nh->oi = x->oi;
nh->router = l->link_data;
- listnode_add (w->nexthop, nh);
+ ospf_spf_consider_nexthop (w->nexthop, nh);
}
return;
}

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
Fortune:
In the stairway of life, you'd best take the elevator.