> >> webrev: http://cr.opensolaris.org/~meem/uv-tweaks > >> cscope: /net/atlantic.east/export/ws/meem/uv-tweaks/usr/src
I've updated the webrev and cscope. It also includes the dl_primstr() changes. I also found and removed one more homegrown dl_info() routine. > > sundlpi.c > > - line222 > > why ETIME is different compared to other errno and needs special case? It's not strictly needed and mostly a holdover from the original code. However, I think it's likely the most common failure and thus I thought it was useful to give a pleasant error message rather than just an errno. > - line438 > > !MBLKIN(mp, addroff, addrlen + ABS(dliap->dl_sap_length) can be changed to > !MBLKIN(mp, addroff, dliap->dl_addr_length) Yes, that's simpler -- thanks :-) > - You forgot to freemsg() in some successful case in dl_info(). Yes, thanks for catching that. > nfs_dlinet.c > > - Line1551, 1557, 1563 > > explicitly check if (rc != 0)? I considered that, but I decided it's more important to be consistent with the style of that file, and most places (especially near those calls) don't do that test. > dld_drv.c > > - Line 668 > > should be "diap->dia_npush > MAXAPUSH", not >=. Yep. > - Line 683 > > you assign mod_hash_key_t to a mod_hash_val_t? and mod_hash_find() and > mod_hash_insert() > should use key not val as the second argument. Yes, the variable name should be "key" rather than "val" and should be declared as a mod_hash_key_t. (But despite those naming issues, the code appears semantically correct -- "val" is actually a key and passed where the key should be.) (The compiler apparently doesn't get upset because both types are actually void *.) > softmac_main.c > > - Line 611 > > What if dl_notify() returns 0? Note that softmac->smac_notifications should > only be set > when dl_notify() returns 0. Agreed. > softmac_ctl.c > > - Line 214 > > should be "!MBLKIN(mp, addroff, dlp->dl_addr_length)"? Sadly, I think it's safer as-is -- the SAP is not really included even though the length says it is. IIRC, some code only sets the length to include it but doesn't actually allocate the packet with that space -- and since we're not going to access that that part of the payload anyway, it's more robust to not require it. Yes, the whole thing with the SAP length is really broken and someone needs to fix it. > - Line 222 > > As we discussed, this is not needed, right? I'd like to wait for that conversation to converge before modifying this part of the code. (As it is, it matches what you originally have except that it actually calls mac_unicst_update(); if you want me to remove that call for now, I will.) > - Line 276-279, 286-290 are not needed as the length check is done in > softmac_process_dlpi(). They are unfortunately necessary because we must examine dlp as part of passing the expected primitive to softmac_process_dlpi(). -- meem
