Jim, thanks for the code review. I've updated the webrevs with all of your comments including:
- updated description in CR 4616660 and evaluation/state of CR 6599424/CR 6630932 - s/L2MAXADDRLEN/L2MAXADDRSTRLEN - cleanup DEBUG cruft in modules/ip/ip.c - update format of ire_format so that it matches the md_walk_cb_t - don't ignore error returns (the only case where we do actually want to do this is in nce_cb(), where we want the walk to continue in case the nce turned bad while we were in the middle of the walk, and we don't want to abort the walk for that reason). I'm also working on checking with the CGTP group to make sure they don't issue ND_GET/ND_SET directly to get to the ip_cgtp_filter value. Meanwhile, here's the updated webrev, in case you (or anyone else!) want to take another look: http://cr.opensolaris.org/~sowmini/nce_walker/ thanks Sowmini Jim's review comments below: > 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.)
