Sangeeta Misra writes:
> file:///net/npt.sfbay/export/surya/surya-gate-review/webrev/index.html
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.
Reviewed, no comments:
uts/common/inet/ip/ip_rts.c
uts/common/net/Makefile
uts/common/net/if.h
Comments:
cmd/ipf/Makefile.ipf:
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.
cmd/ipf/tools/Makefile.tools:
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?
common/ipf/ip_compat.h
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
common/ipf/ip_pool.c:
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.)
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".)
common/ipf/ip_pool.h:
21: why not just include "SOLARIS2 < 10" as one of the cases in the
test at line 18?
27-31: is there a separate bug on this bit of ugliness?
common/ipf/solaris.c:
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.
814: please remove the XXX comments.
common/net/patricia/radix.c:
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.
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.
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?
122: looks like boolean_t to me.
178,210,et al.: please remove or refine the XXX comments.
397: stray semicolon.
459-464: is this ever enabled? Is it needed? If so, could it be
done better with sdt?
495,792: cast not needed.
522: missing blank line between local definitions and executable
code.
522: why are some of the ifdef RN_DEBUG statements removed, but not
others?
563: comment appears to be unrelated to code.
654-660: this still looks wasteful to me, as it did last time.
784-786: what do the added /* parent */ comments mean ... ?
789-790: in some places, the multiple statements per line problems
were fixed, but not here. Why?
852-858: there are too many instances of this pattern. The module
probably needs a more consistent logging mechanism.
1003,1237: negative logic is hard to read.
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?
1106-1108: ifdef shouldn't be needed here; the macro should expand
correctly in the right context.
1193: looks like boolean_t. (Also, '= 0' isn't required; .bss is
always zero-filled -- it's a required part of the C standard.)
1200: does this function ever get called more than once? Why?
1211-1217: why not zalloc?
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.
uts/common/Makefile.rules:
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.)
uts/common/inet/ip.h:
636:nit: s/send a ARP/send an ARP/
uts/common/inet/ip/ip.c:
124: what is this?
6754,6781,12935,13771,20030,23084:nit: stray blank lines; remove.
6751,8389: use NULL. (Are there any non-null uses left?)
7744,8469: use "!" instead of "== 0"; it's clearer.
12478-12479: why were these reordered?
12483: stray blank line. Where is the martian filtering done now?
(Assume it's just fast_forward ... right?)
12506,12741: compare against NULL.
12515: remove "== B_TRUE"; booleans are booleans.
12637: (deleted old line 12536) -- why was this done? Isn't this
now an IRE leak path?
12661-12663: delete
12696: remove; don't spam the system log file.
12721-12726: pretty hard to read.
12729: isn't this just "ipha"?
12741: I don't think testing q_next looks right here. What is this
code attempting to do?
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.)
12770,23847:nit: s/dont/don't/
12820: what is this?
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.
12835,12836: mangled text (will print "packet withbad src/dst");
missing space.
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.
13276: why was the comment deleted? Does the code still need to do
this if there are no more M_BREAK messages?
13550,13552: if one arm of "if" needs braces, both do.
13552-13554: could be simplified to just:
} else if (DB_TYPE(mp) == M_PROTO &&
*(t_uscalar_t *)mp->b_rptr == DL_UNITDATA_IND) {
13589: why is this initialized?
13751-13753: what is this?
13764: what does this mean?
15236,15279: why are these level-0 ("spam the log file") debug
messages?
17941-17942: these belong in a .h file somewhere, not here.
17985: why is this "else"? This means that if MTU/ARP time
coincides with redirect time, we'll just never flush
redirects.
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.)
21867:nit: extra spaces in line.
21874: s/unersolved/unresolved/
21877: so why can't these be queued, at least up to some small
limit? Or is there just no point to doing so?
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.)
23871:nit: s/its/it's/
24751-24752,26928-26929,26951: what are these?
25291: why is this freeb rather than freemsg?
25340: cast unnecessary.
25375-25376: garbled message (prints "from arpire 0x")
25384-25385: exactly equivalent to just "freeb(mp);"
26933: this doesn't appear to be true; see line 26999.
26955: initialization unnecessary.
26959: appears to be boolean.
26974: if statement doesn't appear to do much (code inside works
fine when arpnce->nce_qd_mp is NULL).
26976: how do we get here with mp == NULL?
26985: stray semicolon.
26987-26989: reformat.
27014-27058: indenting is out of hand here; please clean up.
27081: what is this?
27093-27094: consider rewriting comment; hard to read.
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.
uts/common/inet/ip/ip_ftable.c:
69,742,1086,1100,1107: stray blank lines.
70,72: these belong in header files.
181-182: comments don't seem to match code.
186-187: does this still need to be default only?
189,806: missing required blank line.
193: under what conditions would ire_round_robin return NULL, and
yet we still want to proceed with the original ire?
196: stray semicolon.
199: when is DEBUG_LOOKUP set?
499,518: why not declare ihandle as uint32_t?
509: cast isn't needed.
514: ASSERT can never fail.
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.)
682-689,821-835: please remove. (And no company names in comments,
please.)
695: s/wont/won't/
745-748: how does this happen?
763: text is mangled; has extra spaces and "-handling".
765: can sire be NULL here?
767: so that ... what?
882,883: dyadic operator goes at end of line.
923: ?
927: use MBLKL
976: NULL?
1083: initialized value is never used.
1094-1096: looks like an assertion to me.
1120: clean up XXX comments.
1130: "&" by itself here is strange.
1139: s/).ifindex/). ifindex/
1141: "off of?"
1142: s/follwoing/following/
1176: if one arm of 'if' needs braces, both do.
1177: is this just an ASSERT?
1211,1225: design complete?
1246: please simplify; "||" evaluation is short-cut in C, so testing
'ire' against NULL once is more than enough.
1256: leaks ire and sire reference.
1391: indenting is off.
1392: instead of casting so many times, create a local variable that
has the casted 'mp' value.
1444: cast not needed.
uts/common/inet/ip/ip_if.c:
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.
6424:nit: stray blank line.
6428,9894,14062,14066,18561: compare against NULL.
8837: restore deleted blank line; required between function
definitions.
9979: stray semicolon.
14356: missing required blank line.
14397: if one side of 'if' needs curly braces, then both do.
uts/common/inet/ip/ip_ire.c:
75,1994: what are these?
367: belongs in a .h file.
671: initialization not used.
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.)
707: stray semicolon.
1832: s/propogate/propagate/
1844,1846: lock does nothing; store is atomic.
1852-1853: open design question?
2094-2095,2667,3831,3873,6472,6480,6599-6600,6838,6866: stray blank
lines.
2456,2480,2720: what's a "YYY" comment?
2671: cast unnecessary.
2680: ASSERT does nothing; it's impossible for "&" to return NULL
here.
3210: when is DEBUG_ADD set? Is this needed?
3255: ip0dbg looks no more valid to me than the original printf.
This should be an assertion or just a panic case.
3274,3601: prom_printf?! Please remove.
3859,3866: "0x%lx" doesn't look right for a pointer; use %p.
3880: missing required blank line.
3880: when is DEBUG_DEL set?
4038: is this right? Why won't the freeb callback do this?
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.
5303: open design question?
6103-6111: could be simplified by bottom-factoring dupb/copyb.
6522,6533: leaks dlureq_mp.
6536: isn't this exactly "buf"? When would b_rptr not be "buf?"
6557: this assertion can trip if the copyb at line 6489 fails.
6585: remove.
6668-6676: wow.
6670-6675: what's with all the "\" here?
6751: what is this?
6804,6806,6810,6812,6835,6837,6847,6849: locks do nothing here.
6868: clean up XXX comments.
6919-6920: consider putting into the "next" part of for-loop, so
that code can use "continue" rather than "goto."
6922: could just be "return (ire)" and then use break rather than
return in the loop.
uts/common/inet/ip/ip_ndp.c:
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.
163,173,345,1008: missing required blank line.
223: stray semicolon.
237: can never be false.
484-487: ugh.
895: compare against NULL.
1499,1589: why not reuse nnce? This doesn't match the usage pattern
elsewhere.
2075: s/Dont/Don't/
3184,3261-3262: stray blank lines.
3191: what is this?
3278-3281: why isn't this just an ASSERT?
3279: shouldn't be ip0dbg.
3302: compare against NULL.
3365: unused label. (Lint?)
3366: this is always zero; err is never set.
uts/common/inet/ip_ftable.h
2: missing CDDL.
15: why is this here?
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.
uts/common/inet/ip_impl.h
135: compare against NULL.
uts/common/inet/ip_ire.h
337: stray blank line.
uts/common/inet/ip_ndp.h
65-76: why is this the right factoring? Why have the two separate
locks?
312: this probably belongs with its friends; maybe near line 304.
uts/common/inet/ipf/pkt.c
242: this isn't going to work on Solaris 8, 9, or 10FCS.
246: clean up XXX comments.
uts/common/inet/ipf/qif.c
271: this isn't going to work on Solaris 8, 9, or 10FCS.
uts/common/inet/tcp/tcp.c
18025: s/follwoing/following/ -- consider rewriting this comment.
18030-18037: becoming unreadable.
18033: compare against NULL.
uts/common/inet/udp/udp.c
6068-6076: this is getting to be pretty much unreadable.
6071: compare against NULL.
uts/common/net/radix.h
150: negative logic is hard to read.
152-158: does this belong here?
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".
165: this guy misses his friends.
204-210: these need to exist in both kernel and non-kernel.
214-225: these don't belong here.
227: stray blank line.
--
James Carlson, KISS Network <[EMAIL PROTECTED]>
Sun Microsystems / 1 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
networking-discuss mailing list
[email protected]
