Sowmini.Varadhan at Sun.COM writes:
> Fixed.. the internal webrev at
> http://zhadum.east.sun.com/export/ws/sowmini/6599424/webrev/
> has been updated..
General comments:
- Please update CR 4616660. The 'comments' field of this bug should
be moved into the 'description' field (it's in pre-opensolaris
format), and it looks like you're fixing _some_ of this bug by
ripping out the IRE and NCE ndd junk.
- CR 6599424 should be in "Fix in Progress" state (not "Accepted"),
and there should be an 'Evaluation.'
- CR 6630932 should also be put into "Fix in Progress" state.
modules/ip/ip.c
56: this appears to be used as the printable (ASCII string) size of
the address, not the size of the raw L2 address itself. The macro
name should probably reflect that.
454: there's a lot of this leftover 'debug' stuff here; suggest
removing.
491,997: I'd prefer to see ire_format using the void * types
declared for callbacks, and then locally copying into appropriate
variables instead of casting the function pointer. The current
casting trick will simply paper over any errors -- if the prototype
is changed, the compiler will ignore the change and incorrect code
may result.
597: should probably use boolean_t for 'verbose' and
'ire_cbdata_t.verbose' throughout.
597,608: not sure if comparing against a hard-coded decimal '6' is
the right answer here.
954,956,957: can be simplified by getting rid of 'verbose' and
'ipversion', and just using ire_cb.verbose and ire_cb.ire_ipversion
directly.
1352: why no 'const' on ill?
1377-1378: this doesn't look right. You're reading b_wptr-b_rptr
(MBLKL) bytes, but you're starting at an offset location -- b_rptr
plus NCE_LL_ADDR_OFFSET. That's guaranteed to overflow out of the
mblk, isn't it?
You're also trusting that either ill_nd_lla_len or MBLKL(&mp) are
"reasonable" values, and in a corrupted dump, there's no guarantee
that this is true.
I'd suggest something like this instead:
if (ill->ill_nd_lla_len == 0)
return ("None");
mblen = mp.b_wptr - mp.b_rptr;
if (mblen > sizeof (dl_unitdata_req_t) + MAX_REASONABLE_SIZE ||
ill->ill_nd_lla_len > MAX_REASONABLE_SIZE ||
NCE_LL_ADDR_OFFSET(ill) + ill->ill_nd_lla_len > mblen) {
return ("Truncated");
}
mbptr = mdb_alloc(mblen, UM_SLEEP);
if (mdb_vread(mbptr, mblen, (uintptr_t)mp.b_rptr) == -1) {
mdb_warn("failed to read hw addr at %p",
mbptr);
return ("Unknown");
}
mdb_mac_addr(mbptr + NCE_LL_ADDR_OFFSET(ill), ill->ill_nd_lla_len,
addr_buf, sizeof (addr_buf));
1435: is this missing "\n"? Should it be mdb_warn instead?
1439,1479: why discard the error?
1482: is this needed?
1549: what does the '4' in 'ips_ndp4' mean for this message?
(Shouldn't this be a '6' in this case?)
1565: what's a "local walk?" Perhaps this should say that the
nce_stack walker requires an ndp_g_s address.
1625: should this return errors?
inet/ip/ip.c
917: I'd suggest double checking with the CGTP people. I don't see
anything in the existing CGTP contracts that suggests they'd be
affected by this, but we've been surprised in the past. (The
PSARC/2000/539 contract specifies ip_cgtp_filter as an ndd variable
under contract, but not how they'll use it.)
--
James Carlson, Solaris Networking <james.d.carlson at 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
