I fixed all the comments and the incremental webrev is at:
http://cr.opensolaris.org/~vassun08/mdbmacro-webrev-r2/
The old webrev is at the same location
http://cr.opensolaris.org/~vassun08/mdbmacro-webrev/
Please see my comments below.
James Carlson wrote:
Vasumathi Sundaram writes:
In addition to this CR, I have also added the fix for "6796360
tcp_hsp_lookup* code is orphanned after 6737341".
That's a nice find.
The webrev is available at:
http://cr.opensolaris.org/~vassun08/mdbmacro-webrev/
Design:
I don't understand the new "-a", "-b", "-c" and "-l" options on the
"::netstat" dcmd. Why are these actually needed? Why not just
always display these tables and include a field to indicate status?
I don't mind some difference between the user-space command and the
dcmd -- that's to be expected -- but it's unclear to me why this
particular difference is needed. (If the idea is to mimic the old
ndd commands exactly, then don't. The separation in tables that
they showed wasn't so necessary.)
I have removed these new options. The output for all of the following
ndd commands can be obtained through '::netstat -a -P <protocol>' dcmd
within mdb.
tcp_status
tcp_bind_hash
tcp_listen_hash
tcp_conn_hash
tcp_acceptor_hash
udp_status
udp_bind_hash
Comments:
usr/src/cmd/mdb/common/modules/genunix/genunix.c
4674,4769: nit: these look like stray blank lines.
usr/src/cmd/mdb/common/modules/genunix/net.c
(Due to design comments above, I didn't review this file very
thoroughly.)
707-716,738-745: too complicated; use switch or simple table indexed
on udp.udp_state instead. See comments for ip.c:1922 below.
1238-1244: seems incomplete, given current "::netstat" command.
I removed all the changes I made in these files, except for the
formatting fixes.
usr/src/cmd/mdb/common/modules/ip/ip.c
143,148,153,158,163: could be static. (And probably const as well.)
1349,1351: difference between these walkers is unclear given just
the summary line.
1378,1382,2057-2059,2266,2269,2348-2358,2456-2458: nit: "\" not
needed at end of line in C. (Only the preprocessor needs that.)
1644: I suggest just passing in ipcl_hash_walk_data_t, rather than
forcing callers to pass in each element separately. This function
is only called one way.
1658: the WALK_ERR symbol isn't an address; did you mean NULL?
Fixed.
1680,1690: leaks allocated 'iw' structure.
'iw' is stored in wsp->walk_data and freed in ipcl_hash_walk_fini()
1711: careful: check the size of the structure. If it's "big," then
allocate with mdb_alloc instead of using on the stack. (This is a
general mdb/kmdb issue; make sure you inspect all of your functions
for excessive stack usage, because they can panic the system if used
in kmdb, as I learned recently and rather painfully.)
The size of conn_t is large (>1K). I changed it use mdb_alloc.
1713-1714: not needed; just initialize 'ret = WALK_DONE;', and the
loop will fall through.
..........
Fixed all of these.
2237: it doesn't look to me like this works. This is an snprintf
call ... but where's the format string? Was it tested?
I must have removed the format string by mistake. I had tested before
sending for review. Fixed it.
2430: nit: use ANSI "(void)" instead.
usr/src/uts/common/inet/ip/ip.c
775,28358-28385: more should be removed to get code orphaned by this
change.
917-918: shouldn't this go as well?
The data structure srcid_map_t (ip_stack_t:ip_srcid_head) is opaque,
defined and used only within ip_srcid.c
I could not pull this to mdb.
usr/src/uts/common/inet/ip/ip_if.c
437-474: more stuff potentially orphaned by this change, but ...
15595: removal of that table will require modifying (or just plain
removing) this one lonely debug print statement.
usr/src/uts/common/inet/tcp/tcp.c
631-636: these are orphaned now.
1188,1248-1251: orphaned.
usr/src/uts/common/inet/tcp_stack.h
214-221: orphaned.
usr/src/uts/common/inet/udp/udp.c
136-139: orphaned.
406: remove.
Fixed.
I fixed another problem in function mask_to_prefixlen(). I introduced a
macro to get host-byte-ordered mask values.
Thanks
Vasumathi
_______________________________________________
networking-discuss mailing list
[email protected]
