Here are my initial set of comments. I'll hopefully have more on Monday:
usr/src/uts/common/inet/ip.h 326-329: What's the point of putting this under the protection of ifndef? 327: While I'm no STREAMS expert, this practice of hijacking obsolete STREAMS message types for ip's internal use seems dangerous and broken. Maybe someone with more expertise in this area explain why we can't just add message types in <sys/stream.h> (if a new message type is really the way to go)? 1545-1554: From the descriptios, it seems like lots of these could be simultaneously true. For example SEND_FAILED because LLHDR_RESLV_FAILED. Or LOOKUP_PASSED and LLHDR_RESLV_PASSED are almost always true. Could the block comment be expanded to explain which states are valid when? 1545-1554: SEND_IN_PROGRESS, LOOKUP_PASSED, LOOKUP_FAILED, and LLHDR_RESLV_PASSED are never used anywhere. 1554: status_t sounds too generic a name to me. How about ipxmit_state_t? 2084: I'd remove the "(ill_1) == (ill_2)" short circuit, as this is an optimization for a condition that almost never happens. Removing this check makes the comparison in the more typical case faster by not having to compare values that are almost always different. usr/src/uts/common/inet/ip/ip.c 6548: This debug statement generates useless noise. dtrace gives you this information if you want to know it... 6555: Printing a debug statement for every packet successfully sent is a bad idea. Lots of noise, little signal. Again, people can easily use dtrace to get this information (and more). 6984: forwarding a packet 7333: might as well rename that local variable to res_mp and fix the comment above. 7336: fix comment 7651: should be NULL 12661: Has this been addressed? 12665: I'm not sure this implementation is complete. It seems to be missing rewind, play, stop, and pause buttons. ;-) Oh, and remember to put a tape cleaner in there once every six months or so. 13534-13548: I don't think I agree with moving this out of ip_input(). Is the idea that Nemo polling can't be enabled today if CGTP is in use, and therefore CGTP can be optimized out of ip_input? To me, that's an implementation flaw with TCP fast-path. When that flaw is fixed, someone is going to have to yank that code back into ip_input()... Plus, overloading the hdrlen argument to ip_input() in this way is just plain wrong. When CGTP is not enabled, you're only saving a couple of instructions by moving this code here, and that's not a significant saving. 13752: Has this been addressed yet? Clearview is planning on removing Mobile IP from Solaris, but if you want to just do it now, that's fine too. :-) usr/src/uts/common/inet/ip/ip6.c 4858: I don't understand this comment. The IPV6_ADDR_LEN check in itself doesn't tell the driver anything. What did you mean to say (and please re-format the comment block)? 4881: What if ill_phys_addr_length is 0, will this do the right thing? This code path will also be used for Ethernet, or any other media that has a MAC address size != 16 bytes... Passing in NULL as a next-hop address doesn't seem right at all. It seems like this change would make it impossible for IPv6 to work with a driver that doesn't support IP fast-path. 5799-5819: Same two comments as above. _______________________________________________ networking-discuss mailing list [email protected]
