On Fri, 2006-09-29 at 23:25 +0800, Peter Memishian wrote: > As part of my IPMP work, I've had to move the arl_t goo related to > physical links into a separate arlphy_t data structure (which is NULL for > IPMP arl_t's). While this split doesn't benefit the current ON bits, it > also doesn't seem to hurt (on the whole, it's one less line of code), and > putting it back separately to ON would reduce my merge hell. > > Here's the webrev[1]: > > http://cr.grommit.com/~meem/arp-phy/
The change looks harmless. A couple of comments: * 2052 and 2073: Did the clearing of other arl_flags in these cases cause actual problems. Can we file a bug? * What's the impact of the removal of "arl->arl_link_up = B_TRUE;" at 2162? Was this causing a problem previously? Similarly as the last comment, if so, can we file a bug? 2180: I'd rename "up". It's sole purpose at this point is to point at the broadcast address in the dl_info_ack. * 2204: I'm not sure I fully understand this comment (does the XXX mean that there's some problem to be resolved as part of this code-review?). Are you saying that we're most definitely not bound at this point, and that dl_sap_length may be 0? If so, then why bother setting ap_saplen at this point if we're going to correctly set it when we do become bound? I actually don't see where we set ap_saplen when we do receive the DL_BIND_ACK, which would be a problem if it weren't for the fact that I'm so unfamiliar with this code that I must be missing something... 2209: I can't for the life of me figure out where ar_mac_hw_addr_length gets set... Actually, I can't figure out where any of the ar_m_t members get set. I'm guessing they're getting indirectly set while cast to a different type.(?) 2217: Why don't we allocate this when we set it? That way, we can get the actual physical address length as part of the SAP length instead of from the broadcast address length on 2206. 2218: Similarly, if we're not going to have all of the information required from the DLPI provider until after we get a DL_BIND_ACK, then I don't see the point in allocating nor setting a lot of this stuff at this point. 2224: This comment isn't accurate. The code only assumes a broadcast address of "all ones" if dl_version isn't 2. 2229, 2231: Not using "dlia->dl_brdcst_addr_length" obfuscates the purpose of this code, which is to set the broadcast address. Yes, I realize that ap_hw_addrlen is the same value for every DLPI provider that supports broadcast. 2829: Can ap_saplen ever be zero after we've bound with the DLPI provider? > > Anyone have an opinion on whether I should put it back separately or leave > it in my IPMP gate? Normally, if it's something that has no real benefit in the gate without the overall project, I'd say keep it in the project gate. In this case, however, I sympathize with the fact that this won't be putback for a long time, and time wasted due to "merge hell" seems to increase as time passes on large and lengthy projects. It's your call (or your RTI advocate's). :-) -Seb
