[prev in list] [next in list] [prev in thread] [next in thread] 

List:       opensolaris-networking-discuss
Subject:    Re: [networking-discuss] Code Review 6806928 (followup on ndd
From:       James Carlson <james.d.carlson () Sun ! COM>
Date:       2009-02-27 17:01:28
Message-ID: 18856.7272.405033.500166 () gargle ! gargle ! HOWL
[Download RAW message or body]

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.)

Reviewed; no comments:

  usr/src/cmd/mdb/common/modules/genunix/net.h
  usr/src/uts/common/inet/ip_if.h
  usr/src/uts/intel/ip/ip.global-objs.debug64
  usr/src/uts/intel/ip/ip.global-objs.obj64
  usr/src/uts/sparc/ip/ip.global-objs.debug64
  usr/src/uts/sparc/ip/ip.global-objs.obj64

Missing (should be changed, but not in webrev):

  Need to remove orphaned 'tcps_ndd_get_info_interval' from
  tcp_impl.h.

  Need to remove orphaned 'us_last_ndd_get_info_time' and
  'us_ndd_get_info_interval' from udp_impl.h.

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.

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?

  1680,1690: leaks allocated 'iw' structure.

  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.)

  1713-1714: not needed; just initialize 'ret = WALK_DONE;', and the
  loop will fall through.

  1858,2143: not sure why the return value is casted away.

  1876: c-style: missing blank line between functions.

  1876: why does this function always return WALK_NEXT?

  1879: cast shouldn't be needed; make '*ill' const.

  1883,1901,1919: c-style: stray blank lines in auto variable
  declarations.

  1922-1931,1951-1959: this looks far too complicated.  Why not just
  this?

	switch (ill->ill_type) {
	case IFT_ETHER:
		typebuf = "ETHER";
		break;
	case IFT_OTHER:
		typebuf = "OTHER";
		break;
	case IFT_LOOPBACK:
		typebuf = "LOOPBACK";
		break;
	default:
		typebuf = NULL;
		break;
	}

  1964,1992: simpler as "typebuf == NULL".

  1964-1974,1992-2002: I suggest printing as multiple pieces to avoid
  all the duplication.

  1982-1990: there might be enough of these to make a small common
  function instead.

  2008,2294,2365,2391,2429,2437: could be static.

  2032,2139: don't compare booleans against TRUE or FALSE.  Just
  assign.

  2042: this should be returning DCMD_* values, not a WALK_* value.

  2043,2196,2334: nit: stray blank line.

  2051,2342: c-style nit: usual style would put this inside the 'else'
  block.

  2154,2160: why are these fields 80 columns wide?  Isn't that a
  little large?

  2168: initialization not used.

  2170: c style: missing blank line between declarations and
  executable code.

  2172: c style: if one arm of the 'if' needs braces, both do.

  2180: nit: remove extra level of parenthesis.

  2190: cast shouldn't be needed, and 'ipif' should be const.

  2232,2382-2386: why discard errors here?

  2235: nit: shouldn't add ":%d" if ipif_id == 0.

  2237: it doesn't look to me like this works.  This is an snprintf
  call ... but where's the format string?  Was it tested?

  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?

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.

-- 
James Carlson, Solaris Networking              <james.d.carlson@sun.com>
Sun Microsystems / 35 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
networking-discuss@opensolaris.org
[prev in list] [next in list] [prev in thread] [next in thread]