Mailing List Archive

On Thu, 13 May 2004, Michele 'mydecay' Marchetto wrote:

> On Thu, 2004-05-13 at 16:46, Aravindhan S - CTD, Chennai. wrote:
> > Hi,
> > This may not be an abnormal behavior.
> > Excerpt from RFC 2453: Page 28
> >
> > "If the new metric is the same as the old one, it is simplest to do
> > nothing further (beyond re-initializing the timeout, as specified
> > above); but, there is a heuristic which could be applied. Normally,
> > it is senseless to replace a route if the new route has the same
> > metric as the existing route; this would cause the route to bounce
> > back and forth, which would generate an intolerable number of
> > triggered updates. However, if the existing route is showing signs
> > of timing out, it may be better to switch to an equally-good
> > alternative route immediately, rather than waiting for the timeout to
> > happen. Therefore, if the new metric is the same as the old one,
> > examine the timeout for the existing route. If it is at least
> > halfway to the expiration point, switch to the new route. This
> > heuristic is optional, but highly recommended."

This is logical, we should not switch to a different route if it is not
better than current one. So, if there are two routers that announce same
network with same metric it is wrong idea to change anything. But if the
router, which previously announced that route to as, changes nexthop, it
is possible that this is an important imformation which we should not
ignore.

> > Please correct me if i am wrong. Or am i missing something?
>
> imho in a router's routing table cannot co-exist two identical routes
> with only different nexthop
Why? They can. However, in most cases, only one route will be used so only
this one is expected to be announced.

> so one router cannot send such packets.
True, RIP routers should send only one (the best one) route for one
destination.


OK, please read the "4.4 Next Hop" and consider following scenario:

1. Three routers in one network: A=192.168.0.1, B=192.168.0.2,
C=192.168.0.3 and one host H=192.168.0.10

Routers A & B provide uplink to main network (Internet for example) but
can't speak RIP nor any other dynamic routing protocol.

Router C has static routing 0.0.0.0/0 via router A that speaks RIP and has
redistribute static enabled. Host H also speaks RIP so it receives packet
from C with 0.0.0.0/0 and nexthop=192.168.0.1 and sends all packet
directly to the "A" router.

2. Lets assume that router A fails, I can switch manually routing on "C"
for the 0.0.0.0/0 to 192.168.0.2 (B). Router is expected to send new
announce and "H" will switch immediately to the B router.

That's it - the same metric, different nexthop and if we ignore this
update on H we will stuck without uplink.

Maybe this is not a perfect example, but I think it shows why we need
to update this route immediately.

Best regards,

Krzysztof Olêdzki

Hi,
My comments are inline.

> -----Original Message-----
> From: Krzysztof Oledzki [mailto:oleq@ans.pl]
> Sent: Friday, May 14, 2004 3:54 AM
> To: Michele 'mydecay' Marchetto
> Cc: Aravindhan S - CTD, Chennai.; quagga-dev@lists.quagga.net
> Subject: Re: [quagga-dev 1174] Re: [zebra 21412] possible ripd bug
>
>
>
>
> On Thu, 13 May 2004, Michele 'mydecay' Marchetto wrote:
>
> > On Thu, 2004-05-13 at 16:46, Aravindhan S - CTD, Chennai. wrote:
> > > Hi,
> > > This may not be an abnormal behavior.
> > > Excerpt from RFC 2453: Page 28
> > >
> > > "If the new metric is the same as the old one, it is
> simplest to do
> > > nothing further (beyond re-initializing the timeout,
> as specified
> > > above); but, there is a heuristic which could be
> applied. Normally,
> > > it is senseless to replace a route if the new route
> has the same
> > > metric as the existing route; this would cause the
> route to bounce
> > > back and forth, which would generate an intolerable number of
> > > triggered updates. However, if the existing route is
> showing signs
> > > of timing out, it may be better to switch to an equally-good
> > > alternative route immediately, rather than waiting for
> the timeout to
> > > happen. Therefore, if the new metric is the same as
> the old one,
> > > examine the timeout for the existing route. If it is at least
> > > halfway to the expiration point, switch to the new route. This
> > > heuristic is optional, but highly recommended."
>
> This is logical, we should not switch to a different route if
> it is not
> better than current one. So, if there are two routers that
> announce same
> network with same metric it is wrong idea to change anything.
> But if the
> router, which previously announced that route to as, changes
> nexthop, it
> is possible that this is an important imformation which we should not
> ignore.
>
> > > Please correct me if i am wrong. Or am i missing something?
> >
> > imho in a router's routing table cannot co-exist two
> identical routes
> > with only different nexthop
> Why? They can. However, in most cases, only one route will be
> used so only
> this one is expected to be announced.
>

True.

> > so one router cannot send such packets.
> True, RIP routers should send only one (the best one) route for one
> destination.
>

True.

>
> OK, please read the "4.4 Next Hop" and consider following scenario:
>
> 1. Three routers in one network: A=192.168.0.1, B=192.168.0.2,
> C=192.168.0.3 and one host H=192.168.0.10
>
> Routers A & B provide uplink to main network (Internet for
> example) but
> can't speak RIP nor any other dynamic routing protocol.
>
> Router C has static routing 0.0.0.0/0 via router A that
> speaks RIP and has
> redistribute static enabled. Host H also speaks RIP so it
> receives packet
> from C with 0.0.0.0/0 and nexthop=192.168.0.1 and sends all packet
> directly to the "A" router.
>
> 2. Lets assume that router A fails, I can switch manually
> routing on "C"
> for the 0.0.0.0/0 to 192.168.0.2 (B). Router is expected to send new
> announce and "H" will switch immediately to the B router.
>
> That's it - the same metric, different nexthop and if we ignore this
> update on H we will stuck without uplink.
>


Yes, but we will be stuck without uplink only till the route 0.0.0.0/0 and
nexthop = 192.168.0.1 is timed out.
Here it is assumed that the implementation has taken care of this scenario.

If it is not there is a problem here as you said.
If H times out the route by looking just the 0.0.0.0/0 and metric and the
address of the router from which the datagram came then timeout will never
happen.
The reason is the route is announced with the new nexthop which the
implementation ignores and refreshes the timeout timer for that route.
So host H will be stuck without uplink.

In other words if the implementation checks prefix, metric, nexthop and the
address of the router from which the datagram came for refreshing the route
timeout timer, the duration for which H will be stuck without uplink is the
timeout interval (180 seconds). (please note that the intention of checking
the nexthop is only to refresh the timer and not to update the nexthop)

Coming to updating the route with a new nexthop, yes we "can" do this.
But this helps a misbehaving router to keep changing the nexthops making the
"route to bounce back and forth" as said in RFC.

Imho it would be better not to make the routes bounce back and forth by
updating the route with the new nexthop from the same router immediatedly as
we give a chance for a misbehaving router to make the routes bounce back and
forth as said in the RFC.

But still this leaves the host H waiting till the route 0.0.0.0/0 and
nexthop = 192.168.0.1 to be timed out for regaining the connectivity to the
main network (uplink) through router B.But there is a work around for this
problem.

The admistrator can create a routemap for 0.0.0.0/0 and nexthop =
192.168.0.1 and do "set metric 16" for this.
This makes the router C to announce the route with nexthop A as unreachable.
Thus the route can be removed from service from H and after making sure that
the update has been sent to the H the administrator can safely switch to the
new route by changing the static entry 0.0.0.0/0 and next hop = 192.168.0.2
in C.



> Maybe this is not a perfect example, but I think it shows why we need
> to update this route immediately.
>

I agree i couldn't think of any other scenario right now. May be giving it a
deep thought would uncover more scenarios. Please mail me if you find
similar scenarios which would make "updating the route with the new
nexthop", "absolutely necessary".

> Best regards,
>
> Krzysztof Olêdzki
>


Thanks and Regards,
Aravindhan.

On Fri, 14 May 2004, Aravindhan S - CTD, Chennai. wrote:
<CIACH>
> > That's it - the same metric, different nexthop and if we ignore this
> > update on H we will stuck without uplink.
> >
>
>
> Yes, but we will be stuck without uplink only till the route 0.0.0.0/0 and
> nexthop = 192.168.0.1 is timed out.
<CIACH>
> The reason is the route is announced with the new nexthop which the
> implementation ignores and refreshes the timeout timer for that route.

True:
/* If there is an existing route, compare the next hop address
to the address of the router from which the datagram came.
If this datagram is from the same router as the existing
route, reinitialize the timeout. */
same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
&& (rinfo->ifindex == ifp->ifindex));

So we just need to make "same" match not only ifindex & router IP but also
nexthop, right? So..

same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr) &&
IPV4_ADDR_SAME (&rinfo->nexthop, &nexthop) &&
(rinfo->ifindex == ifp->ifindex));

will do the trick?

Best regards,


Krzysztof Olêdzki

On Fri, 14 May 2004, Aravindhan S - CTD, Chennai. wrote:
<CIACH>
> Coming to updating the route with a new nexthop, yes we "can" do this.
> But this helps a misbehaving router to keep changing the nexthops making the
> "route to bounce back and forth" as said in RFC.
>
> Imho it would be better not to make the routes bounce back and forth by
> updating the route with the new nexthop from the same router immediatedly as
> we give a chance for a misbehaving router to make the routes bounce back and
> forth as said in the RFC.
Anyway, this "bounce back and forth" should never occure with quagga - if
nexthop from received datagram is not directly reachable, ripd changes it
to 0.0.0.0. And... if it is directly reachable then switching it
immediately still makes sense.

<CIACH>
> The admistrator can create a routemap for 0.0.0.0/0 and nexthop =
> 192.168.0.1 and do "set metric 16" for this.
> This makes the router C to announce the route with nexthop A as unreachable.
> Thus the route can be removed from service from H and after making sure that
> the update has been sent to the H the administrator can safely switch to the
> new route by changing the static entry 0.0.0.0/0 and next hop = 192.168.0.2
> in C.
OK, but this could cause connections to brake out.


Best regards,

Krzysztof Olêdzki

On Fri, 2004-05-14 at 10:11, Krzysztof Oledzki wrote:
> So we just need to make "same" match not only ifindex & router IP but also
> nexthop, right? So..
>
> same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr) &&
> IPV4_ADDR_SAME (&rinfo->nexthop, &nexthop) &&
> (rinfo->ifindex == ifp->ifindex));
>
> will do the trick?

finally we've reached the perfect solution :) but i think we should
check even the tag field, shouldn't we?

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Sat, 15 May 2004, Michele 'mydecay' Marchetto wrote:

> On Fri, 2004-05-14 at 10:11, Krzysztof Oledzki wrote:
> > So we just need to make "same" match not only ifindex & router IP but also
> > nexthop, right? So..
> >
> > same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr) &&
> > IPV4_ADDR_SAME (&rinfo->nexthop, &nexthop) &&
> > (rinfo->ifindex == ifp->ifindex));
> >
> > will do the trick?
>
> finally we've reached the perfect solution :) but i think we should
> check even the tag field, shouldn't we?

IMHO no. Tag is used mainly to prevent routing loops when redistributing
routes beetwen other protocols and RIP. Anyway, AFAIK, RFC doesn't say
anything about that, does it?

Best regards,

Krzysztof Olêdzki

On Sat, 2004-05-15 at 20:00, Krzysztof Oledzki wrote:
> IMHO no. Tag is used mainly to prevent routing loops when redistributing
> routes beetwen other protocols and RIP. Anyway, AFAIK, RFC doesn't say
> anything about that, does it?

i'm not sure about that..

from rfc 2453:

"Routers supporting protocols other than RIP should be configurable to
allow the Route Tag to be configured for routes imported from
different sources. For example, routes imported from EGP or BGP
should be able to have their Route Tag either set to an arbitrary
value, or at least to the number of the Autonomous System from which
the routes were learned."

i was thinking about the possibility that a route change his external source..
in this case that route should be updated...

RFC clear enough for this scenario and so we must decide by ourselves how to
implement route tag.

what do you think?



--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Sun, 16 May 2004, Michele 'mydecay' Marchetto wrote:

> On Sat, 2004-05-15 at 20:00, Krzysztof Oledzki wrote:
> > IMHO no. Tag is used mainly to prevent routing loops when redistributing
> > routes beetwen other protocols and RIP. Anyway, AFAIK, RFC doesn't say
> > anything about that, does it?
>
> i'm not sure about that..
>
> from rfc 2453:
>
> "Routers supporting protocols other than RIP should be configurable to
> allow the Route Tag to be configured for routes imported from
> different sources. For example, routes imported from EGP or BGP
> should be able to have their Route Tag either set to an arbitrary
> value, or at least to the number of the Autonomous System from which
> the routes were learned."
Yes, I know this paragraph :) But this is all about Tag in the rfc.

> i was thinking about the possibility that a route change his external source..
> in this case that route should be updated...
Exactly.

> RFC clear enough for this scenario and so we must decide by ourselves how to
> implement route tag.
It is alredy implemented. IMHO we shouldn't change it :)

Best regards,

Krzysztof Olêdzki

On Fri, 14 May 2004, Krzysztof Oledzki wrote:

> <CIACH>
> > The admistrator can create a routemap for 0.0.0.0/0 and nexthop =
> > 192.168.0.1 and do "set metric 16" for this.
> > This makes the router C to announce the route with nexthop A as unreachable.
> > Thus the route can be removed from service from H and after making sure that
> > the update has been sent to the H the administrator can safely switch to the
> > new route by changing the static entry 0.0.0.0/0 and next hop = 192.168.0.2
> > in C.
> OK, but this could cause connections to brake out.
... and finally functionality is the same as we simply treat routes with
different nexthops as "same" but we have to send two updates insted of
only one.

So, I think the best solution is to add new configuration option which
allows to choose which behavior is preferred and set default value
according to rfc hints. Something like "same-route nexthop"...

What do you think?

Best regards,

Krzysztof Olêdzki

Hi,
My comments are inline.

> -----Original Message-----
> From: Krzysztof Oledzki [mailto:oleq@ans.pl]
> Sent: Monday, May 17, 2004 4:14 AM
> To: Aravindhan S - CTD, Chennai.
> Cc: quagga-dev@lists.quagga.net; Michele 'mydecay' Marchetto
> Subject: Re: [quagga-dev 1180] Re: [zebra 21412] possible ripd bug
>
>
>
>
> On Fri, 14 May 2004, Krzysztof Oledzki wrote:
>
> > <CIACH>
> > > The admistrator can create a routemap for 0.0.0.0/0 and nexthop =
> > > 192.168.0.1 and do "set metric 16" for this.
> > > This makes the router C to announce the route with
> nexthop A as unreachable.
> > > Thus the route can be removed from service from H and
> after making sure that
> > > the update has been sent to the H the administrator can
> safely switch to the
> > > new route by changing the static entry 0.0.0.0/0 and next
> hop = 192.168.0.2
> > > in C.
> > OK, but this could cause connections to brake out.


> ... and finally functionality is the same as we simply treat
> routes with
> different nexthops as "same" but we have to send two updates insted of
> only one.

True. This is a work around. But this works without any modification in the
implementation.

>
> So, I think the best solution is to add new configuration option which
> allows to choose which behavior is preferred and set default value
> according to rfc hints. Something like "same-route nexthop"...
>
> What do you think?

I think providing a configuration option is better.
>
> Best regards,
>
> Krzysztof Olêdzki
>

Thanks and Regards,
Aravindhan.

On Mon, 2004-05-17 at 00:43, Krzysztof Oledzki wrote:
> So, I think the best solution is to add new configuration option which
> allows to choose which behavior is preferred and set default value
> according to rfc hints. Something like "same-route nexthop"...
>
> What do you think?

imho that's a great solution...

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Thu, 2004-05-20 at 14:46, Aravindhan S - CTD, Chennai. wrote:
> To be frank i don't know. Sorry for the not so prompt reply. I am busy.

eventually i've made the patch.
as decided it adds a new options that treat as same route, routes with
different nexthops.
The default behaviour is to treat them as different routes as stated in
rfc.

There are some trouble to insert two identical routes (with different
nexthops) in the rt in order to send updates that contain both of them.
It is because route_node_get doesen't take into account the nexthop.
This function is used even in ospfd bgpd and others. I think that
inserting two identical route can create them some trouble, but i'm not
sure about this.

So, actually, the first route must time out in order to insert the
second one..

Future discussions, and patch should fix this.

I've tested the patch in a small environment, if you can test it more
massively, it would be appreciated.

i can make even some documentation about this new option to put in the
website.

apply against cvs version

regards.

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Thu, 27 May 2004, Michele 'mydecay' Marchetto wrote:

> On Thu, 2004-05-20 at 14:46, Aravindhan S - CTD, Chennai. wrote:
> > To be frank i don't know. Sorry for the not so prompt reply. I am busy.
>
> eventually i've made the patch.
> as decided it adds a new options that treat as same route, routes with
> different nexthops.
> The default behaviour is to treat them as different routes as stated in
> rfc.
>
> There are some trouble to insert two identical routes (with different
> nexthops) in the rt in order to send updates that contain both of them.
> It is because route_node_get doesen't take into account the nexthop.
> This function is used even in ospfd bgpd and others. I think that
> inserting two identical route can create them some trouble, but i'm not
> sure about this.
>
> So, actually, the first route must time out in order to insert the
> second one..
>
> Future discussions, and patch should fix this.
>
> I've tested the patch in a small environment, if you can test it more
> massively, it would be appreciated.
>
> i can make even some documentation about this new option to put in the
> website.
>
> apply against cvs version

Nice :) Some comments:

- route, reinitialize the timeout. */
+ route and same-route nexthop is not set, reinitialize the
+ timeout. */

Looks for me like reversed logic?

+ /* Same route nexthop */
+DEFUN(ip_rip_same_route_nexthop,
+ ip_rip_same_route_nexthop_cmd,
+ "ip rip same-route nexthop",
+ IP_STR
+ "Routing Information Protocol\n"
+ "Interpret as same route routes with different nexthop\n")

Shouldn't it be:

/* If there is an existing route, compare the next hop address
* to the address of the router from which the datagram came.
* If same-route nexthop is not set compare also announced nexthop.
* If this datagram is from the same router as the existing
* route and nexthop matches or is ignored, reinitialize the
* timeout.
*/


+ if (!ri->same_route_nexthop)
+ {
+ same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
+ && (rinfo->ifindex == ifp->ifindex)
+ && IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop));
+ }
+ else
+ {
same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
&& (rinfo->ifindex == ifp->ifindex));
+ }

Anyway, this "same=" looks strange without spaces/tabs. Hm... How about
this:

same = (IPV4_ADDR_SAME(&rinfo->from, &from->sin_addr) &&
rinfo->ifindex == ifp->ifindex &&
(ri->same_route_nexthop==RIP_NO_SAME_ROUTE_NEXTHOP)?
IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop):1);

Best regards,


Krzysztof Olêdzki

Ah, and one more cosmetic fix:

+2004-05-27 Michele 'mydecay' Marchetto <smarchetto1@tin.it>
+ Krzysztof Oledzki <oleq@ans.pl>
+
+ * Added new option "ip rip same-route nexthop" that allows
+ ripd to interpret routes with different nexthop as same route.
+

How about:
* Added new option "ip rip same-route nexthop" that allows
ripd to interpret routes with different nexthop as same route.
* Do not ignore updates of routes with different nexthop only.
* Small code cleanup (route compare in rip_rte_process).

Best regards,

Krzysztof Olêdzki

On Thu, 2004-05-27 at 23:19, Krzysztof Oledzki wrote:
> Shouldn't it be:
>
> /* If there is an existing route, compare the next hop address
> * to the address of the router from which the datagram came.
> * If same-route nexthop is not set compare also announced nexthop.
> * If this datagram is from the same router as the existing
> * route and nexthop matches or is ignored, reinitialize the
> * timeout.
> */

that's fine.

> Anyway, this "same=" looks strange without spaces/tabs. Hm... How about
> this:
>
> same = (IPV4_ADDR_SAME(&rinfo->from, &from->sin_addr) &&
> rinfo->ifindex == ifp->ifindex &&
> (ri->same_route_nexthop==RIP_NO_SAME_ROUTE_NEXTHOP)?
> IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop):1);
>

i've had some trouble with indentation, sorry...

the two solutions are identical. I think mine is cleaner, but also this
one is correct, let paul decide :)

--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Thu, 2004-05-27 at 23:44, Krzysztof Oledzki wrote:
> Ah, and one more cosmetic fix:
> How about:
> * Added new option "ip rip same-route nexthop" that allows
> ripd to interpret routes with different nexthop as same route.
> * Do not ignore updates of routes with different nexthop only.
> * Small code cleanup (route compare in rip_rte_process).

perfect :)
--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Fri, 28 May 2004, Michele 'mydecay' Marchetto wrote:

> perfect :)

Excellent.. so what is the end-result? :)

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
You would if you could but you can't so you won't.

On Mon, 31 May 2004, Paul Jakma wrote:

> On Fri, 28 May 2004, Michele 'mydecay' Marchetto wrote:
>
> > perfect :)
>
> Excellent.. so what is the end-result? :)

AFAIK it depends on:

[quagga-dev 1222] Re: [zebra 21412] possible ripd bug
[quagga-dev 1224] Re: [zebra 21412] possible ripd bug

Best Regards,


Krzysztof Olêdzki

On Mon, 2004-05-31 at 16:43, Paul Jakma wrote:
> Excellent.. so what is the end-result? :)

that's the complete patch..

> regards,

bye :)
--
mydecay
S.P.I.N.E. Group - http://www.spine-group.org
Key Fingerprint: 667A 4E73 EA53 66AC E2AB D0CA 2908 1484 1F26 4C40
GnuPG Key: http://www.spine-group.org/keys/mydecay.asc

On Mon, 31 May 2004, Michele 'mydecay' Marchetto wrote:

> On Mon, 2004-05-31 at 16:43, Paul Jakma wrote:
> > Excellent.. so what is the end-result? :)
>
> that's the complete patch..

Heh:


@@ -664,6 +666,8 @@
ri->split_horizon = RIP_NO_SPLIT_HORIZON;
ri->split_horizon_default = RIP_NO_SPLIT_HORIZON;

+ ri->same_route_nexthop = RIP_NO_SAME_ROUTE_NEXTHOP;
+
ri->list[RIP_FILTER_IN] = NULL;
ri->list[RIP_FILTER_OUT] = NULL;

Why three (3) tabs?


- /* If there is an existing route, compare the next hop address
- to the address of the router from which the datagram came.
- If this datagram is from the same router as the existing
- route, reinitialize the timeout. */
- same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
- && (rinfo->ifindex == ifp->ifindex));
+ /* If there is an existing route, compare the next hop address
+ * to the address of the router from which the datagram came.
+ * If same-route nexthop is not set compare also announced nexthop.
+ * If this datagram is from the same router as the existing
+ * route and nexthop matches or is ignored, reinitialize the
+ * timeout.
+ */


I'm afraid you are using wrong lenght of tab. Current zebra/quaga code
seems to have standard like:

4 spaces
1 tab (8 spaces)
1 tab + 4 spaces
2 tabs
...

IHMO, we should keep with this standard or change it everywhere.

Anyway, if you insist that this one:
+ if (!ri->same_route_nexthop)
+ {
+ same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
+ && (rinfo->ifindex == ifp->ifindex)
+ && IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop));
+ }
+ else
+ {
+ same = (IPV4_ADDR_SAME (&rinfo->from, &from->sin_addr)
+ && (rinfo->ifindex == ifp->ifindex));
+ }

is better than this one:
same = (IPV4_ADDR_SAME(&rinfo->from, &from->sin_addr) &&
rinfo->ifindex == ifp->ifindex &&
(ri->same_route_nexthop==RIP_NO_SAME_ROUTE_NEXTHOP)?
IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop):1);

than you should at least consider removing this unnecessary
"!" in "!ri->same_route_nexthop" and change the order of if/else or
changing it into "ri->same_route_nexthop==RIP_NO_SAME_ROUTE_NEXTHOP" ;-)


+ /* Next, compare the existing route with new datagram. If the
+ datagram is from the same router as the existing route and
+ the distance, metric, tag or nexthop has been changed; or
+ if there is a route with a lower administrave distance or
+ metric; do the following actions: */
+ if ((same && (rinfo->distance != rip_distance_apply (rinfo) ||
+ rinfo->metric != rte->metric ||
+ ntohs(rinfo->tag) != rte->tag ||
+ !IPV4_ADDR_SAME(&rinfo->nexthop, &rte->nexthop))) ||
+ (rinfo->distance > rip_distance_apply (&rinfotmp)) ||
+ (rinfo->metric > rte->metric))
+ {

Again, tabs/spaces. :(

Best regards,

Krzysztof Olêdzki