On Mon, 30 Oct 2006 12:08:13 -0800 Anders Persson <[EMAIL PROTECTED]> wrote:
> Hi Everyone, > > The Networking MIBs project, which adds support for the latest > TCP/UDP/IP MIBs, is now open for code review. > > You can find the webrev at: > http://cr.grommit.com/~apersson/mib_gate_webrev/ > > or, if you are within SWAN you can find it at: > file:///net/nptbld.sfbay/disk1/mib_gate/webrev > > Please send in your comments to [EMAIL PROTECTED], or > directly to me by 11/13/06. netstat.c: in.ndpd/main.c: 1547: *ick* I'm not a big fan of a library support routine logging'ng an error. Sure, its an unlikely error. Also poll_add() could be simplified by doing the check for -1 and fd in the same loop and realizing that after the realloc you know where the next free slot is. It would be nice to fix while you are there... 1710-1714: Why not just keep track of these in phyint_create()/phyint_delete()? 1716,1754,1779, 1793 You send this chunk of related information out in 4 pieces ignoring errors. What happens if one of them fails? How does the receiver interpret the data? Imagine the sequence: request, reponse, response, response, drop, rerequest, drop, drop, drop, response. Is it old data? It is related? tables.h defs.h in.routed/input.c in.routed/rdisc.c: This is just a style change. This just makes diff'ng against any changes outside of SMI all that much more difficult. Although for this sourcebase I don't think that is common. in.routed/trace.c: protocols/ndpd.h: protocols/routed.h: prototype_com: inet/ip.h: ip/ip.c: 8782,8791: I'd probably do the common code on the common code path more for readability then anything else. When I see code that could be common repeated like this it takes me longer to read because I try to figure out why the coder would have done such a thing. 2373: Why is the statistic here a simple multiple of packets mixed in with this info about one packet? 23863, 24184: extra line 23898-23900: after having seen this code fragment some bazillion times I wonder if it would have been worthwhile to modify or make a new BUMP_MIB instead? 23961: Why do this? I think having macro "functions" defined in the middle of a file for a while and then undef'd hurts readability. ip/ip6.c: 7505: The MIB var called out in the comment isn't the same string as used in the code. 12326: XXX So what about them? ip_ftable.c: ip_if.c: ip_ndp.c: sdp.c: ip6.h: mib2.h: 158: Was there a reason not to pick 26? I suspect you were leaving room for growth but you might want to document the pattern you thought might want to be followed. Although given the already almost random nature of the allocations maybe thats a waste of time. 388,1123,1272,1323,1379,1420,1456: Whats the need for these uses of pragma pack? Under what circumstances have you shown they are needed? Its esp. concerning given the comment above and the first structure leading off with a bunch of bare 'int's. sctp_hash.c: sctp_input.c: sctp_ip.c: tcp.h: tcp.c: udp.c: udp_impl.h: mph _______________________________________________ networking-discuss mailing list [email protected]
