Darren, On Mon, 2009-07-27 at 19:01 -0700, Darren Reed wrote: > http://cr.opensolaris.org/~darrenr/onnv-pcapture/ >
I reviewed the changes to ipnet and ip: General: There are quite a bit of changes related to the style of local variable declaration that was changed, thus adding to the volume of the review. In general, it would be preferable to restrict the set of changes to the functionality being added, and to cstyle and lint issues found while making these changes. Otherwise, sticking with the existing style of the file without making sweeping changes to style based on personal preference should be the modus operandi. I won't comment on each instance, I've restricted my specific comments to things not related to local variable declarations. :-) uts/common/inet/ipnet.h * 58: This may need to be atomic (atomic_inc_64()) * 58: Wrap _y in parens uts/common/inet/ipnet/ipnet.c * 216: s/instnace/instance * 539: Please restore ipnet_sap code as discussed on clearview-discuss. * 572: This is a spurious change, please restore the code as it was. ips is derived from ns->netstack_ipnet at 532. Why go through ips to get at ns again when ns can ge used directly? * 787: As we discussed on clearview-discuss, the ipnet SAP space was fine. * 1168: What's the purpose of this addition? This function only gets called for ipnet_t streams that are on the /dev/lo0 device. * 1327-1328,2300-2304: This is not correct. The IPNETIF_LOOPBACK code breaks the semantics defined in PSARC 2006/475 as we talked about on clearview-discuss. * 1959: ipobs_bounce_func() isn't obvious as far as names go. How about ipobs_hook_cb()? The convention of _cb() for a callback is already used elsewhere in the file. * 1959: What is the reason for this callback having to duplicate every packet passed to it? Why wouldn't the code that dispatches packets to callbacks always simply pass a duplicate? An observability hook should always have those semantics (it should never manipulate the original or block while processing it), so hard-coding such semantics for all NH_OBSERVE hooks would make sense to me. * 2203-2232: datalink_id_t is not the same as IP interface index. They don't represent the same objects, and they don't exist at the same layer. if_index is an IP interface index, and a datalink_id_t is a linkid as handled in the GLDv3 framework. This code has nothing to do with GLDv3, and is strictly contained at the IP-layer, so it shouldn't be handling datalink_id_t. -Seb
