[quagga-dev 5416] Re: [PATCH] RFC 2328, chap 8.1:

Joakim Tjernlund joakim.tjernlund at transmode.se
Mon Jun 2 12:55:04 BST 2008


> > +/*
> > + * Used for creating a temporary ordered list of pt-to-pt OSPF
> > + * interfaces pointing at a common node.
> > + * Sorted after increasing output interface cost.
> > + */
> > +static
> > +int ospf_output_cost_cmp(void *v1, void *v2)
> > +{
> > +  struct ospf_interface *a1 = v1, *a2 = v2;
> > +
> > +  if (a1->output_cost > a2->output_cost)
> > +    return 1;
> > +  if (a1->output_cost < a2->output_cost)
> > +    return -1;
> > +  return 0;
> > +}
> 
> This would raise concerns.. Why do we need a temporary list to 
> determine priority? SPF is supposed to take care of finding the best 
> edge between two vertices..
> 
> > /* 16.1.1.  Calculate nexthop from root through V (parent) to
> >  * vertex W (destination), with given distance from root->W.
> > @@ -573,24 +589,50 @@ ospf_nexthop_calculation (struct ospf_area *area, struct vertex *v,
> >               else
> >                 {
> >                   /* l is a regular point-to-point link.
> > -                     Look for a link from W to V.
> > +                     Look for a links from W to V.
> >                    */
> > -                  while ((l2 = ospf_get_next_link (w, v, l2)))
> > -                    {
> > -                      oi = ospf_if_is_configured (area->ospf,
> > -                                                  &(l2->link_data));
> > -
> > -                      if (oi == NULL)
> > -                        continue;
> > -
> > -                      if (!IPV4_ADDR_SAME (&oi->address->u.prefix4,
> > -                                           &l->link_data))
> > -                        continue;
> 
> Note that this, in-part, a sanity check, to ensure that there is 
> 2-way communication between the parent V and the candidate W*, 
> appropriate for V's given link of l (note: 2-way, not symmetric). 
> Note that we don't care at all what the best link is, only that one 
> exists. You seem to have dropped this part of the check.
> 
> The second reason for this code is to find the appropriate IP address 
> to use as nexthop for (V,l)->W. This is obviously not possible if the 
> /other/ side is advertising an unnumbered link.
> 
> > +		  cost = OSPF_OUTPUT_COST_INFINITE;
> > +		  /* Only add the cheapest i/f's */
> > +		  for (ALL_LIST_ELEMENTS_RO(oiflist, node, tmp_oi))
> > +		    {
> > +		      if (tmp_oi->output_cost > cost)
> > +			break; /* Cost too high, drop out */
> > +		      cost = tmp_oi->output_cost;
> 
> 
> If I get the sense of this right, you're looking through the links of 
> V to find the lowest link to W. This isn't appropriate for nexthop 
> calculation - the SPF algorithm has already determined the link to be 
> "l" (i.e. the l argument to nexthop_calculation).

Yes, find all links that have the same lowest cost and add these links.

> 
> What you want is, presuming the other side is unnumbered:
> 
> - verify there is a /any/ link l2 for (W,l2)->V
> 
>    (because, AFAICT, you can't reconcile the unnumbered link back
>     to an exact local link, as you can with addressed links)
> 
> - setup a 'struct nexthop' using the oi corresponding to the
>    given (V,l), and the router-ID of W.
> 
> You shouldn't need to modify the /existing/ Point-to-Point nexthop 
> chunk of code at all, other than to make it conditional so as to only 
> apply where (W,l2) is numbered.
> 
> I **guess** you want something like the following psuedo-ish 
> additions to the existing code:

Started with something similar as you suggests but ...

> 
>  	           /* l is a regular point-to-point link.
>                       Look for a link from W to V.
>                     */
>                    while ((l2 = ospf_get_next_link (w, v, l2)))
>                      {
>  		      if (!ospf_link_is_unnumbered (l2))
>                          {
>                            oi = ospf_if_is_configured (area->ospf,
>                                                      &(l2->link_data));
> 
>                            if (oi == NULL)
>                              continue;
> 
>                            if (!IPV4_ADDR_SAME (&oi->address->u.prefix4,
>                                                 &l->link_data))
>                            continue;
> 
>                            break;
>                         }
>                       else
>                         {
>                           unnumbered_l2 = l2;
>                           unnumbered_oi = <fill in>;

... here is were I got into trouble, how do I find the right oi? Perhaps
any oi with matching routerID will do? Or NULL oi should be used
instead?

As you can see in my patch I tried to use nh->router.s_addr = 0 but
that won't work if I don't have the correct oi, I guess.

>                         }
>                      }
> 
>                      if (oi && l2)
>                        ....
>                      else if (unnumbered_l2)
>                        <nh appropriate for unnumbered, using W's ID I
>                         guess>
> 
> I guess :)
> 
> * This check is slightly redundant, as we have a similar check at a
>    higher-layer. The check in nexthop calculation only is done for W's
>    that are attached to V though.
> 
> regards,



More information about the Quagga-dev mailing list