On Wed, 2009-08-12 at 13:54 -0700, Darren Reed wrote:
> 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.
All variable names should be aligned at a tab boundary, and initialized
in the declaration where possible. That's the style used throughout the
file. There may be a couple of exceptions (i.e. mistakes), but that's
not reason enough to change the entire file to use an entirely new
style. If there's a function that isn't using this style, then let's
fix it rather than fix every other function. e.g.
netstack_t *ns = netstack_find_by_zoneid(ifp->if_zoneid);
ip_stack_t *ipst = ns->netstack_ip;
ipnet_t *ipnet;
char name[32];
int error = 0;
>
> >
> > 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.
I would think that the cost of the atomic_inc_*() APIs are quite minimal
(and they're relatively widely used in IP), but I can't say that I care
very much either way. Having an accurate error count would seem like an
important thing, and performance when recording an error would seem like
a secondary concern (and again, I doubt that atomic operations would
affect performance of ipnet).
> >
> > * 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().
I don't follow. ipnet_acceptfn should only be set to ipnet_loaccept()
if /dev/lo0 is opened, and in no other case. Therefore the check you
added in the function itself: "if (getminor(ipnet->ipnet_if->if_dev) ==
IPNET_MINOR_LO)" is redundant. If ipnet_loaccept() is getting called
when it shouldn't be, then something else is wrong.
> >
> > * 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.
In what case, and what does it mean? Remember that /dev/lo0
and /dev/ipnet/lo0 have different semantics, and that ipnet_loaccept()
is only strictly implementing the semantics of /dev/lo0.
> > * 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).
Ah, I see.
> 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.
Okay.
> > * 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.
Yep, I think so.
-Seb
