On Mon, 2006-10-02 at 14:40 +0800, Peter Memishian wrote: > Thanks for the review, Seb -- my responses follow: > > > 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? > > No, because there was only one flag. Clearly, it's stupid to have a field > called "arl_flags" that can only set or clear one flag, but there's no > actual bug. In my Clearview workspace, there's an additional ARL_F_IPMP > flag, so I have to fix this -- and I'd rather do it sooner than later.
Got it. > > > * 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? > > The arl_link_up field is now tracked in the arlphy_t as ap_link_down. > I reversed the sense since it's always used to check if the link is down, > rather than if the link is up. As before, we assume the link is up, so > ap_link_down defaults to B_FALSE, which is handled automatically by the > kmem_zalloc(). Okay. > > > 2180: I'd rename "up". It's sole purpose at this point is to point at > > the broadcast address in the dl_info_ack. > > The old code had `up2' which had the same exact purpose, so I'm not sure I > follow your rationale. It's just a throwaway temporary variable like `i' > whose meaning is obvious from the code. But if you feel strongly, please > suggest a name and I'll use it as long as it doesn't create hideous line > wraps. It doesn't matter to me. > > * 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... > > What you're missing is that the comment is simply rephrased and moved from > the old code (and TODO was changed to XXX to reflect the fact that it > represents unfinished existing work). The code has always relied on > ap_saplen (or, previously, arl_sap_length) being correct in > ar_ll_set_defaults(), even though DLPI does not guarantee it. That issue > is independent of my changes, so I have simply left it as an issue. That's fine. It's surprising that there hasn't been a driver which has broken this in the lifetime of this code. > > 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.(?) > > This is unchanged from the old code; check out ar_m_tbl[]. Ah. > > > 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. > > It makes like simpler to guarantee that the arlphy_t is always fully > allocated, and it's also the way the code has always worked. Having it be > all zeroes before the DL_BIND_ACK comes seems reasonable to me. I don't > see any harm in getting the physical address length from the broadcast > address length (which, again, is how it's worked for 15 years); we don't > support cases where those don't match. This isn't your change's problem, and that's fine. > > > 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. > > It's possible to defer until DL_BIND_ACK, but it would kick up a bunch of > dust and is unnecessary for my changes. Okay. > > > 2224: This comment isn't accurate. The code only assumes a broadcast > > address of "all ones" if dl_version isn't 2. > > This comment is unchanged from the original code -- I left it alone > because I figured it was referring to the fact that we did not directly > set a multicast address here in arl_ll_subnet_defaults(), rather than > referring to the setting of the broadcast address. It doesn't matter much to me. I just thought I'd mention that the comment was incorrect in case you feel compelled to fix it. The person who added support for DLPIv2 (jypeng) forgot to change the comment to state something like, "we'll use the DLPI-supplied broadcast address, or all ones if the driver doesn't supply one". > > 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. > > The fact is that ap_arp_addr is allocated to be ap_hw_addrlen bytes in > length -- and we should use the size of the destination buffer to bound > our bcopy(), not the size of the source buffer -- even if the size of the > source buffer was used to initialize the size of the destination buffer > (that is, ap_arp_hw_addrlen was initialized to dl_brdcst_addr_length). > The way I have the code written also makes the if/else clauses symmetric > (they both use ap_hw_addrlen), which I prefer. > > Yes, we could have a separate ap_arp_addrlen field, but it would always be > the same as arp_hw_addrlen, so it feels a bit pedantic to me. Okay. > > > 2829: Can ap_saplen ever be zero after we've bound with the DLPI > > provider? > > I don't think so, but I didn't see the need to rock that boat. That's fine. Thanks, -Seb
