[quagga-dev 10352] Re: [PATCH] ospfd: restore nexthop IP for p2p interfaces

David Lamparter equinox at opensourcerouting.org
Wed Mar 20 19:00:47 GMT 2013


On Wed, Mar 20, 2013 at 06:57:35PM +0100, Joakim Tjernlund wrote:
> <equinox at diac24.net> wrote on 2013/03/20 17:28:46:
> > 
> > From: Christian Franke <chris at opensourcerouting.org>
> > 
> > commit c81ee5c... "ospfd: Optimize and improve SPF nexthop calculation"
> > subtly changed semantics of routes calculated over pointopoint links by
> > removing the nexthop IP address and instead using an ifindex route.
> > 
> > This breaks calculation of AS-Ext routes with a forwarding address since
> > in ospf_ase_complete_direct_routes() this will be hit:
> >     if (op->nexthop.s_addr == 0)
> >       op->nexthop.s_addr = nexthop.s_addr;
> > thus turning the route unusable by having an invalid nexthop.
> > 
> > Fix by restoring the nexthop IP on routes over PtP links.  This also
> > allows running multi-access (Ethernet) interfaces in PtP mode again.
> > 
> > This bug is a regression against 0.99.21 and only present in 0.99.22.
> 
> OOPS, I guess my patch was dropped on the floor, I thought it had
> gone in long time ago. I am very surprised nobody noticed until now.

The patch was merged last July, but 0.99.22 has only been released a
little while ago.  Sadly, none of the tests caught this;  in the end it
broke Christian's home network, so he went and investigated it...

> Anyhow, this version is much better. Using the neighbour list is far
> superior, BIRD already does this.
> 
> One question though, will this lookup use nbrs which nbr state is != FULL? 

Not sure right off, but I don't see how a non-FULL neighbor can end up
on the nexthop calculation?  Better to double-check I guess.

As a sidenote, there shouldn't really be more than one neighbor on the
oi->nbrs list in that code snippet, since, well, we're talking about a
ptp interface.  However, ospf_nbr_lookup_by_routerid seemed like the
safest and cleanest way.


-David


> >           if (oi->type == OSPF_IFTYPE_POINTOPOINT)
> >        {
> > -        added = 1;
> > -        nexthop.s_addr = 0; /* Nexthop not required */
> > +        /* Having nexthop = 0 is tempting, but NOT acceptable.
> > +           It breaks AS-External routes with a forwarding address,
> > +           since ospf_ase_complete_direct_routes() will mistakenly
> > +           assume we've reached the last hop and should place the
> > +           forwarding address as nexthop.
> > +           Also, users may configure multi-access links in p2p mode,
> > +           so we need the IP to ARP the nexthop.
> > +        */
> > +        struct ospf_neighbor *nbr_w;
> > +
> > +        nbr_w = ospf_nbr_lookup_by_routerid (oi->nbrs, &l->link_id);
> > +        if (nbr_w != NULL)
> > +          {
> > +            added = 1;
> > +            nexthop = nbr_w->src;
> > +          }
> >        }





More information about the Quagga-dev mailing list