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
_______________________________________________
networking-discuss mailing list
[email protected]
