Jim,
Attached is a txt file that contains all your comments and our
responses. We had come to resolution for most of the comments in
previous email discussions. I am posting the finalized txt file with the
last set of comments that until now were unresolved ( see these:)
jdc-49, jdc-55, jdc-60, jdc-61, jdc-148, jdc-171
Thanks again for the code review
Sangeeta
jdc-1: Some global comments first:
- Please make sure your workspace passes "wx pbchk." This means
fixing up copyrights and getting the SCCS comments right.
- It would help to have standard-form SCCS comments here. I'd like
to see the references to the bugs fixed and RFEs implemented.
RESP: ACCEPT on "wx pbchk"
We are currently not using bugtraq to keep track of pre-integration
Surya internal bugs. We have been using elaborate comments to
track changes inside the projec. But we will use standard-form
SCCS comments before putback to onnv,ofcourse.
jdc-2: Reviewed, no comments:
uts/common/inet/ip/ip_rts.c
uts/common/net/Makefile
uts/common/net/if.h
RESP: ACCEPT
****************************************
cmd/ipf/Makefile.ipf
*****************************************
jdc-3: 18: this change shouldn't be necessary. If you're installing the
header files to a common location (net/radix.h), then ipf should
be able to pick them up there without an explicit -I.
RESP: ACCEPT
*****************************************
cmd/ipf/tools/Makefile.tools
*****************************************
jdc-4: 214: as an overall code organization issue, it seems odd to put the
ipf code right under $SRC/common, but put patricia under
$SRC/common/net. Why is that extra level (/net) needed, and
why aren't all of the networking things there?
RESP: We are not sure what factors went into the organization of ipf module, and
so cannot comment on why ipf is not under $SRC/common/net. But with
regards to the radix code location, we are following the same conventions
used within the kernel (i.e., using common/net for all networking
related code, just as uts/common/inet has all the kernel related
networking code. Also we decided to create $SRC/common/net/patricia
subdir as we find this layout to be more descriptive
*****************************************
common/ipf/ip_compat.h
*****************************************
jdc-5: 145: The net/radix.h feature wasn't in S10 FCS, so this change can't
be released to the open source version as it is. More
fundamentally, though, testing based on OS version number is
just plain wrong. I realize that the IPF code has this in
spades, but it's not a good design practice. Instead, this
should be a more specific test, like:
#ifdef HAS_NET_RADIX_H
#include <net/radix.h>
#endif
RESP: ACCEPT Surya will file a bug against IP Filter
This issue has been discussed with Darren Reed. Current IP Filter
defines various OS and OS-version related constants such as SOLARIS and
BSD. Modifying these in IP Filter is out of the scope of Surya, so we
are simply trying to conform to the existing choices. It is up to
IP Filter team to fix this as part of bug fix/RFE/or project work.
*****************************************
common/ipf/ip_pool.c:
*****************************************
jdc-6: 80,83,86: why are the ifndefs needed? Why
wouldn't the macro always be defined, and just have an empty
definition in the cases where it does nothing? (I don't think
ifndef/define belongs in .c files.)
RESP: ACCEPT
Moving the ifdefs to radix.h apparently wil break opensource
interoperability . Darren recommends moving the ifdefs to ip_pool.h
jdc-7: 242: this change seems inconsistent to me, because
it appears to break BSD compatibility. Either we should be
ripping out all of the non-Solaris compatibility features
in our copy of IPF, or we should just be going along with
them. (In other words, either rip out all the BSD references
globally, or add something like "|| SOLARIS2 >= 10" here.
Or better still, change it to "#ifdef NEED_RN_INIT_FINI".)
RESP: ACCEPT - *for onnv integration* we will apply the
ifdef "SOLARIS2 > 10" rule to all (and only) the Surya changes
to IP Filter code. If and when Surya integrates into S10 Update
this has to be changed to "SOLARIS2 >=10"
*****************************************
common/ipf/ip_pool.h:
*****************************************
jdc-8: 21: why not just include "SOLARIS2 < 10" as one of the cases in the
test at line 18?
RESP: DEFER until IP Filter cleanup
The define conditions in line 17 were too complex to
untangle (sun, __svr4__ or __SVR4 were defined in
un-straightforward ways) , so the Surya team made the
choice of keeping the !_KERNEL choices for Solaris as
readable as possible.
jdc-9: 27-31: is there a separate bug on this bit of ugliness?
RESP: ACCEPT Bug will be filed against IP Filter to clean this up
*****************************************
common/ipf/solaris.c:
*****************************************
jdc-10: 813,827,830: this won't work on Solaris 9. Is this code still being
synced with the open source? If so, then more ifdef
logic is needed here. If not, then we may as well rip
out the stale (and now broken) ifdefs.
RESP: ACCEPT - *for onnv integration* we will apply the
ifdef "SOLARIS2 > 10" rule to all (and only) the Surya changes
to IP Filter code. If and when Surya integrates into S10 Update
this has to be changed to "SOLARIS2 >=10"
jdc-11: 814: please remove the XXX comments.
RESP: ACCEPT
************************************
common/net/patricia/radix.c:
************************************
jdc-12: 66: this is pretty ugly. I'd suggest at least using fprintf so that
the trailing "\n" (missing from all the callers, and not added
by fputs) can be added.
RESP: ACCEPT - we will use gettext() and insert a \n. Similar to
cmd/mailx/aux.c lines 81-86
jdc-13: 102-116: there's no real difference here for user versus kernel. It
would be nice to factor this out into the definition of
R_Malloc, so that macro bears the burden. (And, really, an
allocator that takes a pointer type and a size as arguments
looks icky to me.) Same issue for 620-624.
RESP: ACCEPT
jdc-14: 104,111: I see no point to this. The existing kmem and umem
interfaces already support pretty sophisticated caching of
available memory (per-CPU, even), so why not just use that?
RESP: INVESTIGATE.
jdc-15: 122: looks like boolean_t to me.
RESP: ACCEPT
jdc-16: 178,210,et al.: please remove or refine the XXX comments.
RESP: ACCEPT
jdc-17: 397: stray semicolon.
RESP: ACCEPT
jdc-18: 459-464: is this ever enabled? Is it needed? If so, could it be
done better with sdt?
RESP: ACCEPT - will remove
jdc-19: 495,792: cast not needed.
RESP: ACCEPT
jdc-20: 522: missing blank line between local definitions and executable
code.
RESP: ACCEPT
jdc-21: 522: why are some of the ifdef RN_DEBUG statements removed, but not
others?
RESP: ACCEPT will remove
jdc-22: 563: comment appears to be unrelated to code.
RESP: ACCEPT
jdc-23: 654-660: this still looks wasteful to me, as it did last time.
RESP: ACCEPT
jdc-24: 784-786: what do the added /* parent */ comments mean ... ?
RESP: ACCEPT - will change comment.
This is the case when a new node is being attached to a previous instance.
I think that the author was emphasizing that the parent value of tt
and tt->rn_dupedkey are being assigned here
jdc-25: 789-790: in some places, the multiple statements per line problems
were fixed, but not here. Why?
RESP: ACCEPT- will remove
jdc-26: 852-858: there are too many instances of this pattern. The module
probably needs a more consistent logging mechanism.
RESP: INVESTIGATE
jdc-27: 1003,1237: negative logic is hard to read.
RESP: ACCEPT - will remove
jdc-28: 1056,1060: just what do we expect an administrator to do with these
messages? Why does he care about the internal function
names or transient pointer values?
RESP: ACCEPT - will clarify comment.
jdc-29: 1106-1108: ifdef shouldn't be needed here; the macro should expand
correctly in the right context.
RESP: ACCEPT
jdc-30: 1193: looks like boolean_t. (Also, '= 0' isn't required; .bss is
always zero-filled -- it's a required part of the C standard.)
RESP: ACCEPT
jdc-31: 1200: does this function ever get called more than once? Why?
RESP: ACCEPT - will be removed
No it does not
jdc-32: 1211-1217: why not zalloc?
RESP: ACCEPT.
jdc-33: 1237-1241: there should be no need for #ifndef here; the Free()
macro should always take two arguments and just ignore
the second in user space.
RESP: ACCEPT
*************************************
uts/common/Makefile.rules:
**************************************
jdc-34: 365,367,371,1114,1123: this is ugly; can't we fix the include files
so that hacks here aren't needed? (Note that
few things use -I directly in this file, and
the others -- gssapi and uhci -- arguably
just have bugs.)
RESP: ACCEPT -we have changed the new rule added for radix.o
to not use -I directly,
IP FIlter still uses the -I stuff. Darren has stated:
"This will eventually be cleaned up by two seperate projects.
The first use of IPFFLAG will go with pfhooks (2005/334) and the
next, IPFFLAG2, will go away with 2006/088. "
***************************************
uts/common/inet/ip.h:
****************************************
jdc-35: 636:nit: s/send a ARP/send an ARP/
RESP: ACCEPT
***************************************
uts/common/inet/ip/ip.c:
****************************************
jdc-36: 124: what is this?
RESP: ACCEPT It was used to debug some bugs. This should be removed.
jdc-37: 6754,6781,12935,13771,20030,23084:nit: stray blank lines; remove.
RESP: ACCEPT
jdc-38: 6751,8389: use NULL. (Are there any non-null uses left?)
RESP: ACCEPT L7651, L8389: should pass "NULL" instead of 0
for the dlureq_mp arg passed to ire_create_mp
No there are no other non-null uses left
jdc-39: 7744,8469: use "!" instead of "== 0"; it's clearer.
RESP: ACCEPT
jdc-40: 12478-12479: why were these reordered?
RESP: Cant remember the reason, except we may have removed one of those
lines at one point and then added it back in a different order.
jdc-41: 12483: stray blank line. Where is the martian filtering done now?
(Assume it's just fast_forward ... right?)
RESP: ACCEPT.
Yes we do Martian address checking in ip_fast_forward. We got rid
of ip_no_forward() and inlined it in ip_fast_forward at L12683 - L12697
jdc-42: 12506,12741: compare against NULL.
RESP: ACCEPT
jdc-43: 12515: remove "== B_TRUE"; booleans are booleans.
RESP: ACCEPT
jdc-44: 12637: (deleted old line 12536) -- why was this done? Isn't this
now an IRE leak path?
RESP: REJECT
The refrele is now done in ip_input at L13674. THis was done
as part of ip forwarding fastpath. So there is no leak
jdc-45: 12661-12663: delete
RESP: ACCEPT
jdc-46: 12696: remove; don't spam the system log file.
RESP: ACCEPT
jdc-47: 12721-12726: pretty hard to read.
RESP: ACCEPT We will add comments that explain the if statement
jdc-48: 12729: isn't this just "ipha"?
RESP: ACCEPT. Looking at L12680 and then greping for ipha tells me so.
jdc-49: 12741: I don't think testing q_next looks right here. What is this
code attempting to do?
RESP: This code also exists in the onnvclone-mar-5-06's
ip_rput_process_forward() and other parts of ip.c.
It tries to avoid doing a canput check for every packet. The
canput check is avoided if the entity below IP is a driver
(dev_q->q_next is NULL) and there are no messages already queued on the
driver's wq. The assumption is that we can't predict the behavior of
intermediate modules between IP and the driver, and we always do
canput check in such a case. In the case of driver, we assume
the drivers enqueue messages on the streams queue and we can
optimize by looking at q_first.
jdc-50: 12756-12757: does this code do anything? It doesn't look like it.
(I think it actually does "mp->b_rptr = rptr - hlen;"
-- but by way of unnecessary subterfuge.)
RESP: ACCEPT This fragment of code is a copy of onnvclone-mar-5-06's
ip.c(L22734) Thus we should get rid of both L12757 and L23125
jdc-51: 12770,23847:nit: s/dont/don't/
RESP: ACCEPT
jdc-52: 12820: what is this?
RESP: ACCEPT - comment should be removed. It was something
we had added during prototype days when we were unclear
on certain things.
jdc-53: 12827,12831:nit: the host byte order version of the address is used
in only one place; may as well just inline the
ntohl right there.
RESP: ACCEPT
jdc-54: 12835,12836: mangled text (will print "packet withbad src/dst");
missing space.
RESP: ACCEPT
Should also change "ip_rput_check_and_forward" to
"ip_rput_process_forward".
jdc-55: 12967: this was intentionally not done in the original FE design.
Instead, they returned a boolean to tell the caller whether
the IRE had been consumed. If you're going to change this
(which I certainly welcome seeing), then please consider
ripping out the unnecessary indirection in *irep and the
unfortunate use of the boolean return value, and just return
either (ire) or (NULL) as the function result.
RESP: ACCEPT
There seems to be only one caller of this function and that is
ip_input. This is fine to do as long as the changes in both ip_input
and ip_rput_process_broadcast are consistent
jdc-56: 13276: why was the comment deleted? Does the code still need to do
this if there are no more M_BREAK messages?
RESP: INVESTIGATE
I think this can be removed. Need investigation. Follow subject
mail thread "Question on M_BREAK cleanup and mp->b_prev detail"
jdc-57: 13550,13552: if one arm of "if" needs braces, both do.
RESP: ACCEPT
jdc-58: 13552-13554: could be simplified to just:
} else if (DB_TYPE(mp) == M_PROTO &&
*(t_uscalar_t *)mp->b_rptr == DL_UNITDATA_IND) {
RESP: ACCEPT
It should be this:
if (DB_TYPE(mp) == M_DATA)
dmp = mp;
else if (DB_TYPE(mp) == M_PROTO &&
*(t_uscalar_t *)mp->b_rptr == DL_UNITDATA_IND) {
dmp = mp->b_cont;
}
because dl->dl_primitive is just the first byte of the b_rptr anyway.
jdc-59: 13589: why is this initialized?
RESP: INVESITGATE
jdc-60: 13751-13753: what is this?
RESP: ACCEPT - The comment will be deleted
jdc-61: 13764: what does this mean?
RESP: ACCEPT - The comment will be deleted
jdc-62: 15236,15279: why are these level-0 ("spam the log file") debug
messages?
RESP: ACCEPT. Will change it to ip2dbg
jdc-63: 17941-17942: these belong in a .h file somewhere, not here.
RESP: ACCEPT - the define will be moved to inet/ip_ire.h
onnv has it defined in ip_ire.c, hence we had used extern
jdc-64: 17985: why is this "else"? This means that if MTU/ARP time
coincides with redirect time, we'll just never flush
redirects.
RESP: INVESTIGATE I think "else" should be and "if"
jdc-65: 21115: there are too many of these double-casts in the new code.
It'd be nice to have definitions of these IPP_* values that
are compatible with the way the code uses them. (I.e., put
the casts into a macro, or create a SET_BPREV_FLAG macro.)
RESP: ACCEPT- will create SET_BPREV_FLAG macro
jdc-66: 21867:nit: extra spaces in line.
RESP: ACCEPT
jdc-67: 21874: s/unersolved/unresolved/
RESP: ACCEPT
jdc-68: 21877: so why can't these be queued, at least up to some small
limit? Or is there just no point to doing so?
RESP: This was discussed in design review, and we agreed to go this way
Note that the likelihood of a forwarding packet and a wput packet
sending to the same dst at the same time and there not yet be an
ARP entry for it is small. Furthermore, if this actually happens, it
might be likely that wput would generate multiple packets (and
forwarding would also have a train of packets) for that destination. If
this the case, some of them would have been dropped pre-Surya anyway,
since ARP only queues a few packets..
It was agreed upon early on that ip_xmit_v4() itself should not
handle fragmentation. One could argue that ip_wput_frag() should
be rewhacked, to actually fragment the packets and then send each fragment
to ip_xmit_v4(), but post-Yosemite putback, ip_wput_frag() now calls
ip_wput_frag_mdt() as well.
In the long term ip_xmit_v4() can be changed
to handle M_MULTIDATA, but we have not done this as part of Surya. Even
if we do that there is no guarantee that all the fragments will
make it (note that the nd queue size is 4 packets).
jdc-69: 22149: indeed. What is this? (Note: references to some other
company, especially in potentially disparaging ways, should
not find their way into the code or comments.) (Actual code
seems to deal with link layers that don't require headers,
such as SLIP, though it's not clear.)
RESP: ACCEPT Comment will be removed
jdc-70: 23871:nit: s/its/it's/
RESP: ACCEPT
jdc-71: 24751-24752,26928-26929,26951: what are these?
RESP: ACCEPT
NOTE: These were counters added to debug performance with Smartbits. Ideally
it would be nice to preserve this somehow ( if not in this format). Trying
to use dtrace to debug with Smartbits tests running results in tput to
go down so, we preferred having counters that we could look at via
mdb post every tput run.
jdc-72: 25291: why is this freeb rather than freemsg?
RESP: ACCEPT
jdc-73: 25340: cast unnecessary.
RESP: ACCEPT
jdc-74: 25375-25376: garbled message (prints "from arpire 0x")
RESP: ACCEPT Will add a space at L25376
jdc-75: 25384-25385: exactly equivalent to just "freeb(mp);"
RESP: ACCEPT
jdc-76: 26933: this doesn't appear to be true; see line 26999.
RESP: ACCEPT
The comment on L26933 is not well-written. It should say
26933 * must set mp->b_prev to one of these values:
{0, IPP_FWD_OUT, IPP_LOCAL_OUT}"
jdc-77: 26955: initialization unnecessary.
RESP: ACCEPT
jdc-78: 26959: appears to be boolean.
RESP: ACCEPT Will change to boolean_t
jdc-79: 26974: if statement doesn't appear to do much (code inside works
fine when arpnce->nce_qd_mp is NULL).
RESP: INVESTIGATE
Need to see if skipping over
26976-26980 in case nce_qd_mp was null helps perf any.
jdc-80: 26976: how do we get here with mp == NULL?
RESP: we get here form:
ip.c ip_wput_nondata 25406 (void ) ip_xmit_v4(NULL, ire, NULL);
ip.c ip_wput_frag 21872 (void ) ip_xmit_v4(NULL, ire, NULL);
The ip_wput_nondata call is to just flush out the queued mp's for a newly
resolved entry (the packet was already queued before sending the
IRE_ARPRESOLVE_TYPE to arp).
jdc-81: 26985: stray semicolon.
RESP:ACCEPT
jdc-82: 26987-26989: reformat.
RESP:ACCEPT
jdc-83: 27014-27058: indenting is out of hand here; please clean up.
RESP: INVESTIGATE as to how to reorganise the if else statement
jdc-84: 27081: what is this?
RESP: ACCEPT - will be removed.
THis was a question we had. The answer came to us via the
solution for CR 6391685
jdc-85: 27093-27094: consider rewriting comment; hard to read.
RESP:ACCEPT
jdc-86: 27100: why isn't this an assertion? If this happens, it looks to me
like it's a bug in the code, not just an operational error.
RESP: INVESTIGATE
******************************************
uts/common/inet/ip/ip_ftable.c:
******************************************
jdc-87: 69,742,1086,1100,1107: stray blank lines.
RESP: ACCEPT
jdc-88: 70,72: these belong in header files.
RESP: ACCEPT
jdc-89: 181-182: comments don't seem to match code.
RESP: ACCEPT
jdc-90: 186-187: does this still need to be default only?
RESP: We have not enabled round-robinning/ecmp for the
general case because, as discussed during design review,
enabling round-robin for all routes would require more research
to be done on ECMP algorithms, and elaborate testing with
IPMP to make sure the load-balancing works correctly wrt
IPMP. We plan to address these issues in Phase 2
jdc-100: 189,806: missing required blank line.
RESP: ACCEPT
jdc-101: 193: under what conditions would ire_round_robin return NULL, and
yet we still want to proceed with the original ire?
RESP: INVESTIGATE Looking into this, it seems we may have some
bug in ftable_lookup code.
jdc-102: 196: stray semicolon.
RESP: ACCEPT
jdc-103: 199: when is DEBUG_LOOKUP set?
RESP: ACCEPT - will remove
Was being used for prototype testing
jdc-104: 499,518: why not declare ihandle as uint32_t?
RESP: ACCEPT
jdc-105: 509: cast isn't needed.
RESP: ACCEPT
jdc-106: 514: ASSERT can never fail.
RESP: ACCEPT
jdc-107: 590-593: this could be replaced by just "return (ih.ire);" -- the
structure is initialized, so ih.ire always has the right
value. (And this means both err and IH_SUCCESS are
unnecessary.)
RESP: ACCEPT
jdc-108: 682-689,821-835: please remove. (And no company names in comments,
please.)
RESP:ACCEPT
jdc-109: 695: s/wont/won't/
RESP:ACCEPT
jdc-110: 745-748: how does this happen?
RESP: INVESTIGATE.
I believe this code is plagerized from onnv's ip_newroute()
logic - see L6952-6956 of onnvclone-mar-5-06's ip.c
jdc-111: 763: text is mangled; has extra spaces and "-handling".
RESP: ACCEPT
jdc-112: 765: can sire be NULL here?
RESP: ACCEPT - will fix. Yes it can be NULL here.
jdc-113: 767: so that ... what?
RESP: ACCEPT - will fix comment
jdc-114: 882,883: dyadic operator goes at end of line.
RESP: ACCEPT
jdc-115: 923: ?
RESP: INVESTIGATE. I think we inlined it for perf boost
jdc-116: 927: use MBLKL
RESP: ACCEPT
jdc-117: 976: NULL?
RESP: ACCEPT
jdc-118: 1083: initialized value is never used.
RESP: ACCEPT
Why does full nightly no pick this flaw up?
jdc-119: 1094-1096: looks like an assertion to me.
RESP: ACCEPT
jdc-120: 1120: clean up XXX comments.
RESP: ACCEPT Need to figure out if this is needed.
jdc-121: 1130: "&" by itself here is strange.
RESP: ACCEPT - will distribute the line better
jdc-122: 1139: s/).ifindex/). ifindex/
RESP: ACCEPT
jdc-123: 1141: "off of?"
RESP: ACCEPT
jdc-124: 1142: s/follwoing/following/
RESP: ACCEPT
jdc-125: 1176: if one arm of 'if' needs braces, both do.
RESP: ACCEPT
jdc-126: 1177: is this just an ASSERT?
RESP: ACCEPT
jdc-127: 1211,1225: design complete?
RESP: ACCEPT. Will remove the comment
jdc-128: 1246: please simplify; "||" evaluation is short-cut in C, so testing
'ire' against NULL once is more than enough.
RESP: ACCEPT
jdc-129: 1256: possible leaks ire and sire reference. Please
ire-review this function carefully to make sure you're not
dropping references more than once or failing to drop references that
need to be dropped. are serious bugs.
RESP: INVESTIGATE - possible bug here
jdc-130: 1391: indenting is off.
RESP: ACCEPT
jdc-131: 1392: instead of casting so many times, create a local variable that
has the casted 'mp' value.
RESP: ACCEPT
jdc-132: 1444: cast not needed.
RESP: ACCEPT
****************************************
uts/common/inet/ip/ip_if.c:
*****************************************
jdc-134: 5618-5621,5628-5631: why are we going out of our way to add these?
Perhaps sdts would be better. Note that, as a
performance issue, this change causes
ill_is_quiescent (and ipif_is_quiescent) to
become non-leaf routines on production code.
RESP: ACCEPT- will remove the debug lines. They were being added to
track ire_refcnt leaks.
jdc-135: 6424:nit: stray blank line.
RESP:ACCEPT
jdc-136: 6428,9894,14062,14066,18561: compare against NULL.
RESP:ACCEPT
jdc-137: 8837: restore deleted blank line; required between function
definitions.
RESP: ACCEPT
jdc-138: 9979: stray semicolon.
RESP: ACCEPT
jdc-139: 14356: missing required blank line.
RESP: ACCEPT
jdc-140: 14397: if one side of 'if' needs curly braces, then both do.
RESP: ACCEPT
******************************************
uts/common/inet/ip/ip_ire.c:
******************************************
jdc-141: 75,1994: what are these?
RESP: ACCEPT - will remove. Was used to debug
jdc-142: 367: belongs in a .h file.
RESP: ACCEPT - will move to ip_ire.h
jdc-143: 671: initialization not used.
RESP: ACCEPT
jdc-144: 702: when you run into ire_next == NULL, what resets irb_rr_origin
back to the first one? (I'd have expected this at 693, but
it's not there.)
RESP: ACCEPT
irb_rr_origin should be reset at ire_delete (when the ire is delete)
If the origin is null, then ire_round_robin will set it to the first
one
jdc-145: 707: stray semicolon.
RESP: ACCEPT
jdc-146: 1832: s/propogate/propagate/
RESP: ACCEPT
jdc-147: 1844,1846: lock does nothing; store is atomic.
RESP: ACCEPT
jdc-148: 1852-1853: open design question?
RESP: ACCEPT- comment can be removed. design issue is resolved.
jdc-149: 2094-2095,2667,3831,3873,6472,6480,6599-6600,6838,6866: stray blank
lines.
RESP:ACCEPT
jdc-150: 2456,2480,2720: what's a "YYY" comment?
RESP: ACCEPT "YYY" phase should be removed.
I think the YYY comments in ire_walk_ipvers(), ire_walk_ill()
ire_walk_ill_tables() was used to highlight the code change that
resulted in differing ftable walking in V6 and V4 world.
Note in Surya, for V4 the BSD radix tree's rnh_walktree is being
used, where as the V6 continues to use onnv's ftable walk scheme.
So Surya's ire_walk_ipvers(), passes the "ipftable"
argument to be either 0 or ip_forwarding_table_v6. If the argument is
0, rnh_walktree() code execution occurs, otherwise its a V6 case and
onnv's ftable walk scheme is executed.
jdc-151: 2671: cast unnecessary.
RESP: ACCEPT accept
jdc-152: 2680: ASSERT does nothing; it's impossible for "&" to return NULL
here.
RESP: ACCEPT
jdc-153: 3210: when is DEBUG_ADD set? Is this needed?
RESP: ACCEPT L3210- L3212 should be removed
jdc-154: 3255: ip0dbg looks no more valid to me than the original printf.
This should be an assertion or just a panic case.
RESP: INVESTIGATE, though prefer not to change existing
behaviour in this case.
jdc-155: 3274,3601: prom_printf?! Please remove.
RESP: ACCEPT L3272 - L3276 should be removed
jdc-156: 3859,3866: "0x%lx" doesn't look right for a pointer; use %p.
RESP: ACCEPT
jdc-157: 3880: missing required blank line.
RESP: ACCEPT
jdc-158: 3880: when is DEBUG_DEL set?
RESP: ACCEPT L3880- L3885 should be removed.
jdc-159: 4038: is this right? Why won't the freeb callback do this?
RESP: this is for the SO_DONTROUTE/IRE_MARK_NOADD case
jdc-160: 4990: missing rn_fini, and destructors for v4ll_g_lock and
rt_entry_cache. Note: should be in reverse order of
construction, at least for readability.
RESP: INVESTIGATE on creating rn_fini for kernel.
ACCEPT on doing mutex_destroy of v4ll_g_lock and rt_entry_cache
jdc-161: 5303: open design question?
RESP: Accept - will remove the comment.
jdc-162: 6103-6111: could be simplified by bottom-factoring dupb/copyb.
There's duplicate logic here for the dupb/copyb calls. The code can
be simplified and made a bit more readable by fetching the mblk to be
copied into a variable first and then making a single set of
dupb/copyb calls.
RESP: ACCEPT
jdc-163: 6522,6533: leaks dlureq_mp.
RESP: ACCEPT.
jdc-164 6536: isn't this exactly "buf"? When would b_rptr not be "buf?"
RESP: ACCEPT - will fix
jdc-165: 6557: this assertion can trip if the copyb at line 6489 fails.
RESP: ACCEPT - we need to bail if the copyb at line 6489 fails
jdc-166: 6585: remove.
jdc-167: 6668-6676: wow.
RESP: ACCEPT - will simplify this debug statement
jdc-168: 6670-6675: what's with all the "\" here?
RESP: ACCEPT - will simplify this debug statement
jdc-169: 6751: what is this?
RESP: ACCEPT - will remove. Yet another counter for Smartbits testing
jdc-170: 6804,6806,6810,6812,6835,6837,6847,6849: locks do nothing here.
RESP: ACCEPT
jdc-171: 6868: clean up XXX comments.
RESP: ACCEPT
Comment can be removed though we need to look at ire_round_robin code
needs to be looked at as per thiru-25
jdc-172: 6919-6920: consider putting into the "next" part of for-loop, so
that code can use "continue" rather than "goto."
RESP: INVESTIGATE
jdc-173: 6922: could just be "return (ire)" and then use break rather than
return in the loop.
RESP: INVESTIGATE
**********************************************
uts/common/inet/ip/ip_ndp.c:
***********************************************
jdc-174: 119,121,et cetera: consider gathering up the hash table items (table
itself, thread counter, lock, cleanup flag) into
a single structure and having one structure for
each of V4 and V6. Don't make everyone deal with
if (v4) x; else y. Just use a table pointer.
This is both a performance issue (you're forced to have if-else
clauses buried in all the lock macros) and a code cleanliness issue.
The current structure isn't good because the design is
incomplete, and this is likely to result in maintenance bugs.
Please consider the issue further and don't dismiss as merely a
comment on the code style. It's a design issue.
RESP: ACCEPT
jdc-175: 163,173,345,1008: missing required blank line.
RESP: ACCEPT.
jdc-176: 223: stray semicolon.
RESP: ACCEPT.
jdc-177: 237: can never be false.
RESP: ACCEPT.
jdc-178: 484-487: ugh.
RESP: ACCEPT ( same issue as jdc-174)
jdc-179: 895: compare against NULL.
RESP: ACCEPT
jdc-180: 1499,1589: why not reuse nnce? This doesn't match the usage pattern
elsewhere.
RESP: ACCEPT.
jdc-181: 2075: s/Dont/Don't/
RESP: ACCEPT.
jdc-182: 3184,3261-3262: stray blank lines.
RESP: ACCEPT.
jdc-183: 3191: what is this?
RESP: ACCEPT - will be removed
counter was added for debugging.
jdc-184: 3278-3281: why isn't this just an ASSERT?
How can we be adding an NCE that's already
condemned? And if it were something that could happen, why would we
want to force a message on the system log with ip0dbg?
This still looks like either an assertion case or a missing
bit of design.
RESP: INVESTIGATE - need to find out what these flags are exactly
jdc-185: 3279: shouldn't be ip0dbg.
RESP: ACCEPT.
jdc-186: 3302: compare against NULL.
RESP: ACCEPT.
jdc-187: 3365: unused label. (Lint?)
RESP: ACCEPT. was not caught in full nightly builds!
jdc-188: 3366: this is always zero; err is never set.
RESP: ACCEPT.
***********************************************
uts/common/inet/ip_ftable.h
***********************************************
jdc-189: 2: missing CDDL.
RESP: ACCEPT.
jdc-190: 15: why is this here?
I suppose this is ok, but it seems a little counter-intuitive to me
RESP: to avoid collision with other definitions like that of
in.routed's rt_entry
jdc-191: 29: I have a slight perference for making header files (even
internal ones) stand-alone by making sure that we have the
right types defined where needed.
RESP: ACCEPT - Will add #include ip_ire.h in ip_ftable.h
**************************************************
uts/common/inet/ip_impl.h
**************************************************
jdc-192: 135: compare against NULL.
RESP: ACCEPT.
**************************************************
uts/common/inet/ip_ire.h
**************************************************
jdc-193: 337: stray blank line.
RESP: ACCEPT.
**********************************************
uts/common/inet/ip_ndp.h
**********************************************
jdc-194: 65-76: why is this the right factoring? Why have the two separate
locks?
I don't think that's really much of an intentional design.
So, why are there exactly two locks? Why is one lock not enough? Why
aren't 100 locks better? Why do we believe that parallelism between
v4 and v6 is the necessary feature?
It seems like it may be right, but it also seems a bit arbitrary
RESP: There are examples of both ways in IP. We simply picked this route.
jdc-195: 312: this probably belongs with its friends; maybe near line 304.
RESP: ACCEPT
***********************************************
uts/common/inet/ipf/pkt.c
************************************************
jdc-196: 242: this isn't going to work on Solaris 8, 9, or 10FCS.
RESP: ACCEPT - *for onnv integration* we will apply the
ifdef "SOLARIS2 > 10" rule to all (and only) the Surya changes
to IP Filter code. If and when Surya integrates into S10 Update
this has to be changed to "SOLARIS2 >=10"
jdc-197: 246: clean up XXX comments.
RESP: ACCEPT will remove
***********************************************
uts/common/inet/ipf/qif.c
***********************************************
jdc-198: 271: this isn't going to work on Solaris 8, 9, or 10FCS.
RESP: ACCEPT - *for onnv integration* we will apply the
ifdef "SOLARIS2 > 10" rule to all (and only) the Surya changes
to IP Filter code. If and when Surya integrates into S10 Update
this has to be changed to "SOLARIS2 >=10"
****************************************
uts/common/inet/tcp/tcp.c
****************************************
jdc-199: 18025: s/follwoing/following/ -- consider rewriting this comment.
RESP: ACCEPT.
jdc-200: 18030-18037: becoming unreadable.
RESP: ACCEPT - will refactor.
jdc-201: 18033: compare against NULL.
RESP: ACCEPT.
*****************************************
uts/common/inet/udp/udp.c
*******************************************
jdc-202: 6068-6076: this is getting to be pretty much unreadable.
RESP: ACCEPT - will refactor.
jdc-203: 6071: compare against NULL.
RESP: ACCEPT.
****************************************
uts/common/net/radix.h
*****************************************
jdc-204: 150: negative logic is hard to read.
RESP: ACCEPT
jdc-205: 152-158: does this belong here?
RESP: ACCEPT
jdc-206: 160-163,190-196: these macros should match in terms of arguments so
that the caller doesn't have to muck about with
"ifdef KERNEL".
RESP:ACCEPT
jdc-207: 165: this guy misses his friends.
RESP: ACCEPT
jdc-208: 204-210: these need to exist in both kernel and non-kernel.
RESP:ACCEPT
jdc-209: 214-225: these don't belong here.
These things are already defined in sys/sysmacros.h. Why
not just use those? We already have something like
7 different implementations of these generic and
commonly-used macros floating around in delivered
header files; and who knows how many others squirreled away somewhere.
It'd be nice to avoid adding yet another copy to the pile.
RESP: INVESTIGATE if we can use the existing ones in sys/sysmacros.h
jdc-210: 227: stray blank line.
RESP: ACCEPT
_______________________________________________
networking-discuss mailing list
[email protected]
