[email protected] wrote: > On (10/05/09 21:33), James Carlson wrote: >> Erik Nordmark wrote: >>> The webrev for usr/src is at >>> http://cr.opensolaris.org/~nordmark/refactor-review/ >> ibcm_arp_link.c: >> >> 154: this ASSERT doesn't do much (the reference on line 155 will do >> fine), but it's unclear to me why it's correct to assume that >> the lookup will never fail. Is that really true? Even with >> the edge cases that cause "in transition" routes? Even when >> there's no configured default route? >> >> 207: ditto. > > ire_route_recursive_v4 will return the REJECT/BLACKHOLE ire when > no route is found, hence the assert.
Ah, ok. In any event, the assert doesn't do much because the very next line dereferences the pointer and thus has an MMU-enforced assertion "for free." (And that's an assertion that's a superset of the one in the code -- it checks not just NULL, but (char *)1, and all other bad pointers.) I prefer to reserve ASSERT for unobvious design constraints that the compiler and/or CPU can't determine on their own. Null pointer isn't one of those cases. (And, yeah, if you look back to some of the code I wrote when I was new at Sun, I littered it with foo!=NULL asserts. I feel bad about that now. ;-}) >> ip_arp.c: >> >> 83: Suggest not burying an extern amid a field of statics. > > Ok, > >> 103: I've never seen this before; does it work? Can module names >> really have embedded fs-special characters such as "/"? > > I didn't know it wouldn't work: I'm not sure I understand the implications. I suggest checking with meem. I've never seen a module name with special characters. >> 121-169: Probably not your code, but suggest using a single common >> macro to clean this up. Something like: >> >> #define ARP_HOOK_INOUT(_hook, _event, _ilp, _olp, _hdr, _fm, _m, ipst) >> [...] >> #define ARP_HOOK_IN(_hook, _event, _ilp, _hdr, _fm, _m, ipst) \ >> ARP_HOOK_INOUT(_hook, _event, _ilp, NULL, _fm, _m, ipst) >> #define ARP_HOOK_OUT(_hook, _event, _olp, _hdr, _fm, _m, ipst) \ >> ARP_HOOK_INOUT(_hook, _event, NULL, _olp, _fm, _m, ipst) > > given that its not directly related to the changes here, I'll file > an RFE on that one.. Seems like a trivial fix ... but ok. >> 294: doesn't do anything. > > this is the assignment: > > 294 *sncec = ncec; > > but that's passed later to > 986 arp_notify(src_paddr, mp1, AR_CN_BOGON, > 987 &iras, src_ncec); Right. But it doesn't do anything because of line 242. *sncec is already set, and only needs to be changed if it's being set back to NULL. >> 300-302: unclear why this is true. (because we never resolve probe >> targets on the IPMP member links?) > > because all ncec's (both probe targets and others) are created with > ncec_ill set to the ipmp_ill. OK ... so, was this true at line 248, right after returning from ncec_lookup_illgrp_v4? I'm not making the connecting between getting "thus far" and the IPMP ill. >> 333: where's the check for an unchanged lladdr in PROBE state? > > that happens here: > > 324 ll_changed = nce_cmp_ll_addr(ncec, src_haddr, hlen); > : > 337 if (op == ARP_RESPONSE && > 338 (ncec->ncec_state == ND_PROBE || > : > 340 new_state = ND_REACHABLE; > > : > 342 nce_update(ncec, new_state, (ll_changed ? src_haddr : NULL)) I don't see how the code matches the comment. The comment says that if we're in PROBE state and the lladdr is unchanged and it's a response, then we go to REACHABLE state. The code, though, says that if we are in PROBE state and it's a response, then we go to REACHABLE state -- regardless of whether the lladdr has changed. Which is right? >> 800: how does M_IOCTL end up on the rput side of the stream, and if >> it's there, why would sending M_IOCNAK downwards be the answer? > > This is a relic of the old unmerged version- strictly speaking we should > never end up over here. So returning ENOENT will at least let someone > notice that somethings not right. Unless it's used for some known operating mode, I suggest removing it and just letting the default case free the mblk. I don't think STREAMS modules should have special code to try to respond to things that are outside of the framework design, such as an M_IOCTL coming up unexpectedly on the rput side. There are just too many wild things that could be checked to have a hope of checking them all, and those "helpful" responses all too often lead to deathmatch tragedy. The safer thing to do is to discard. > should be removed. DTRACE_PROBE is probably more appropriate. > >> 890: interface is not what? > > the selected "cast_ill" for ipmp. The string is mostly there as a > diagnostic, but perhaps it can be more verbose. I have no clue what a "cast_ill" might be (an opportunity for an understudy? a shaped ailment?), so I wasn't sure how to read this. >> 1689: shouldn't this return ill_reachable_retrans_time instead? Zero >> would mean that we give up on the first sign of error. > > but arp_output itself will only fail if the ill is down, or if ENOMEM > was encountered.. I suppose it would be better to return > ill_reachable_retrans_time for the ENOMEM case, at least. If the ill is down, then either (a) it could come back momentarily, in which case retrying is better than not or (b) we should have flushed all of our unresolved ARP queries. Right? If it's (a) (or in the ENOMEM case, as you suggest), then the code isn't quite right. If it's (b), then a comment about this being a timing issue might be helpful. >> 1697,1737: suggest B_TRUE for success rather than failure. > > this would alos require reversing the return semantics of ndp_announce.. > let me investigate. OK. I don't feel strongly about it at all, but seeing boolean true for failure just looks a little odd. > Thanks again for the review.. I'll send out an updated webrev > later this week . Sounds good. I'll look it over when it's ready and I have a chance. -- James Carlson 42.703N 71.076W <[email protected]> _______________________________________________ networking-discuss mailing list [email protected]
