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