[openib-general] Re: [Ipoverib] [PATCH] IPv6: Add correct padding to IPoIB link addr option

[Vivek Kashyap]

I suggest that the implementation discussion not be reflected on ipoverib. 
I've bcc'd to ipoverib for this mail. Those interested in the details can 
follow it on netdev/openib.

Thanks,
	Vivek

On Wed, 19 Jan 2005, Krishna Kumar wrote:

> 
> 
> 
> 
> 
> Hi Roland,
> 
> Couple of minor comments on this :
> 
> 1 : In  "+static int ndisc_addr_option_pad(unsigned short type)", change
> the function
>      name to ndisc_lladdr_option_pad() and the argument name to addr_type,
> to be
>      more precise ? addr_option seems confusing, unless it is known that
> this is a LL
>      address option (not a big deal, though).
> 
> 2. I know memset has optimized check for count, but isn't it better to
> check for pad
>     before this code since most of time it is going to be zero (unless you
> are doing only
>     IB traffic) ?
> 
> +           if (pad) {
>                   memset(opt + 2, 0, pad);
>                   opt   += pad;
>                   space -= pad;
> +           }
> 
> 3. I guess there is no clean way to avoid changing all consumers of ll_addr
> to not worry
>     about the padding after the length and before the LL address, but it
> would be nice if
>     that were possible (eg via the ndisc_parse_options).
> 
> Also, Dave/Yoshifuji, can't ndisc_options have just nd_opt_array[5] instead
> of nd_opt_array[7],
> with indices-1 being used to store/access the options ? In any case, I was
> expecting 6 rather
> than 7 in the current code, since there are 5 options with the array[0]
> being unused.
> 
> thanks,
> 
> - KK
> 
> 
> 
>                                                                        
>              YOSHIFUJI Hideaki                                         
>              / $B5HF#1QL@(B                                      
>              <yoshfuji at linux-i                                          To
>              pv6.org>                  roland at topspin.com,             
>              Sent by:                  davem at davemloft.net             
>              netdev-bounce at oss                                          cc
>              .sgi.com                  netdev at oss.sgi.com,             
>                                        ipoverib at ietf.org,              
>                                        openib-general at openib.org       
>              01/19/2005 05:50                                      Subject
>              AM                        Re: [Ipoverib] [PATCH] IPv6: Add
>                                        correct padding to IPoIB link addr
>                                        option                          
>                                                                        
>                                                                        
>                                                                        
>                                                                        
>                                                                        
>                                                                        
> 
> 
> 
> 
> David, let me tkink about this.
> Thanks.
> 
> In article <528y6qej5w.fsf at topspin.com> (at Tue, 18 Jan 2005 09:33:47
> -0800), Roland Dreier <roland at topspin.com> says:
> 
> > +++ linux-bk/net/ipv6/ndisc.c            2005-01-14 21:20:55.736745091
> -0800
> > @@ -169,12 +169,33 @@
> >
> >  #define NDISC_OPT_SPACE(len) (((len)+2+7)&~7)
> >
> > -static u8 *ndisc_fill_option(u8 *opt, int type, void *data, int
> data_len)
> > +/*
> > + * Return the padding between the option length and the start of the
> > + * link addr.  Currently only IP-over-InfiniBand needs this, although
> > + * if RFC 3831 IPv6-over-Fibre Channel is ever implemented it may
> > + * also need a pad of 2.
> > + */
> > +static int ndisc_addr_option_pad(unsigned short type)
> > +{
> > +          switch (type) {
> > +          case ARPHRD_INFINIBAND: return 2;
> > +          default:                return 0;
> > +          }
> > +}
> > +
> > +static u8 *ndisc_fill_addr_option(u8 *opt, int type, void *data, int
> data_len,
> > +                                                unsigned short
> addr_type)
> >  {
> >            int space = NDISC_OPT_SPACE(data_len);
> > +          int pad   = ndisc_addr_option_pad(addr_type);
> >
> >            opt[0] = type;
> >            opt[1] = space>>3;
> > +
> > +          memset(opt + 2, 0, pad);
> > +          opt   += pad;
> > +          space -= pad;
> > +
> >            memcpy(opt+2, data, data_len);
> >            data_len += 2;
> >            opt += data_len;
> 
> --
> Hideaki YOSHIFUJI @ USAGI Project <yoshfuji at linux-ipv6.org>
> GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA
> 
>