On Wed, Jun 06, 2018 at 08:17:54PM +0200, Remi Locherer wrote:
> On Wed, Jun 06, 2018 at 09:01:49AM +0200, Claudio Jeker wrote:
> > On Wed, Jun 06, 2018 at 08:06:30AM +0200, Remi Locherer wrote:
> > > Hi,
> > > 
> > > RfC 5340 says that for intra area prefix LSAs metric should be set to 0
> > > in case of point-to-multipoint  or loopback interfaces. Otherwise metric
> > > should be set to the value of the interfaces output cost.
> > > 
> > > ospf6d currently sends intra area prefix LSAs *always* with metric 0.
> > > 
> > > Below diff fixes this.
> > > 
> > > OK?
> > > 
> > > Remi
> > > 
> > > 
> > > Index: rde.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/ospf6d/rde.c,v
> > > retrieving revision 1.72
> > > diff -u -p -r1.72 rde.c
> > > --- rde.c 12 Aug 2017 16:27:50 -0000      1.72
> > > +++ rde.c 5 Jun 2018 20:45:03 -0000
> > > @@ -1301,7 +1301,6 @@ append_prefix_lsa(struct lsa **lsa, u_in
> > >   /* Append prefix to LSA. */
> > >   copy = (struct lsa_prefix *)(new_lsa + *len);
> > >   memcpy(copy, prefix, lsa_prefix_len);
> > > - copy->metric = 0;
> > >  
> > >   *lsa = (struct lsa *)new_lsa;
> > >   *len = new_len;
> > > @@ -1514,6 +1513,7 @@ orig_intra_lsa_rtr(struct area *area, st
> > >                   if (iface->type == IF_TYPE_POINTOMULTIPOINT ||
> > >                       iface->state & IF_STA_LOOPBACK) {
> > >                           lsa_prefix->prefixlen = 128;
> > > +                         lsa_prefix->metric = 0;
> > 
> > This is not strictly needed because there is a bzero done before.
> 
> I thought its easier to spot that metric is 0 on purpose.
> Same in the updated diff below in prefix_tree_add (calloc()).
> 
> What is more common in such a case?
> 
> > 
> > >                   } else {
> > >                           lsa_prefix->prefixlen = ia->prefixlen;
> > >                           lsa_prefix->metric = htons(iface->metric);
> > > 
> > 
> > What about the append_prefix_lsa() in orig_intra_lsa_net()?
> > Currently the metric is set to 0 there but now it wont.
> > What should the behaviour be there?
> 
> Metric should be set to 0.
> 
> > I guess it would be possible to change prefix_tree_add() to set metric to
> > 0 there. Also I think prefix_tree_add() is leaking memory.
> > The if (!(IN6_IS_ADDR_LINKLOCAL(&addr)) && ... should have an
> > else free(new); block to make sure we don't leak new if it is not added to
> > the prefix_tree.
> 
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospf6d/rde.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 rde.c
> --- rde.c     12 Aug 2017 16:27:50 -0000      1.72
> +++ rde.c     6 Jun 2018 16:29:46 -0000
> @@ -1301,7 +1301,6 @@ append_prefix_lsa(struct lsa **lsa, u_in
>       /* Append prefix to LSA. */
>       copy = (struct lsa_prefix *)(new_lsa + *len);
>       memcpy(copy, prefix, lsa_prefix_len);
> -     copy->metric = 0;
>  
>       *lsa = (struct lsa *)new_lsa;
>       *len = new_len;
> @@ -1354,6 +1353,8 @@ prefix_tree_add(struct prefix_tree *tree
>               memcpy(&addr, new->prefix + 1,
>                   LSA_PREFIXSIZE(new->prefix->prefixlen));
>  
> +             new->prefix->metric = 0;
> +
>               if (!(IN6_IS_ADDR_LINKLOCAL(&addr)) &&
>                   (new->prefix->options & OSPF_PREFIX_NU) == 0 &&
>                   (new->prefix->options & OSPF_PREFIX_LA) == 0) {
> @@ -1362,7 +1363,8 @@ prefix_tree_add(struct prefix_tree *tree
>                               old->prefix->options |= new->prefix->options;
>                               free(new);
>                       }
> -             }
> +             } else
> +                     free(new);
>  
>               cur_prefix = cur_prefix + len;
>       }
> @@ -1514,6 +1516,7 @@ orig_intra_lsa_rtr(struct area *area, st
>                       if (iface->type == IF_TYPE_POINTOMULTIPOINT ||
>                           iface->state & IF_STA_LOOPBACK) {
>                               lsa_prefix->prefixlen = 128;
> +                             lsa_prefix->metric = 0;
>                       } else {
>                               lsa_prefix->prefixlen = ia->prefixlen;
>                               lsa_prefix->metric = htons(iface->metric);
> 

Looks good. Maybe split the memleak fix from the metric bits and commit
that independently. OK claudio@

-- 
:wq Claudio

Reply via email to