> > > * 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.
Considering that IP has the same requirement in ip_ll_subnet_defaults(), I suspect all driver writers have just adjusted to this Solaris requirement (it's not a hard one to satisfy anyway). > > > 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". After looking through the revision history, I agree with your assessment; I'll update the comment. (As an side, I wonder if the non-DL_VERSION_2 code has any purpose, given that snoop, dhcpagent, and other userland programs won't work with them. Maybe we should just torch this cruft?) -- meem
