On 12/08/09 06:37 AM, Sebastien Roy wrote: > 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. :-)
As we discussed, i want there to be *1* style for the file. I have no interest in which style that is. I just don't like working on source code with mixed styles because it isn't clear which one should be used going forward... Is it to be: <type><tab><variable> or <type><n tabs><variables> .. where all the variables are aligned and an arbitrary number of tabs are inserted. The difference is (what I've currently done): struct ip ip; int foo; one tab between type and name, vs: struct ip ip int foo; where short type names have extra tabs. At present, compare _init()'s variables (space between type and variable name) with those in ipnet_if_init(). _init()'s use the latter of the above - multiple tabs to bring variables out to the same position, which is different to what I interpreted your earlier suggestion (single tab) as being. > > uts/common/inet/ipnet.h > > * 58: This may need to be atomic (atomic_inc_64()) Discuss. In a similar vein to many of the counters in IP not being atomic increments, so to is this. There's a real cost for using atomic_() ops and it seems that unless there is an absolute need for it to be 100% reliable (reference count, etc), the "danger" in just using "++" for general statistics seems to be accepted. Compare with MIB_BUMP() and other _BUMP() macros in $SRC/uts/common/inet/*.h. > * 58: Wrap _y in parens Reject. Does not work - code will not compile like that, I wish it did. Not good, I know, but that's life. > uts/common/inet/ipnet/ipnet.c > > * 216: s/instnace/instance Accept. > * 539: Please restore ipnet_sap code as discussed on > clearview-discuss. Accept. > * 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? Accept. > > * 787: As we discussed on clearview-discuss, the ipnet SAP space was fine. Accept. > > * 1168: What's the purpose of this addition? This function only gets > called for ipnet_t streams that are on the /dev/lo0 device. Discuss. ipnet_loaccept() is used twice: once in ipnet_open() and once in ipnet_promisc_add(). > > * 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. Accept. IPNETIF_LOOPBACK is now only used by the BPF code path. > * 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. Accept. > * 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. Discuss. The duplication here has been moved from ip.c`ipobs_hook() to here. Whereas ipobs_hook() was previously just walking a list of functions to call and doing the duplication there, as required, now the list walking is separated (pfhooks) from the packet duplication. Thus the dispatching comes out of pfhooks and jumps through ipobs_bounce_func_cb() to do the duplication for the real recipient (func). But, if your comments above are correct, then dupmsg() should be removed and only copymsg() used, otherwise the observe is presented with the real packet data (in dblk_t's off of its own mblk_t.) There is a performance cost with this, though: it enforces a deep copy of all observed packets, copying all of the dblk_t's, not just a shallow one (creating new mblk_t's.) The current implementation only does a deep copy if the shallow one fails. > * 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. Discuss. The index used is the index from IP, but seeing as there is no ipindex_t, I used datalink_id_t to put the index in. It would seem that using datalink_id_t as a type here is cause for confusion so it might be better to just use uint_t. Darren -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/crossbow-discuss/attachments/20090812/e0adc720/attachment.html>
