On Tue, 14 Nov 2006 01:04:40 -0800 Anders Persson <[EMAIL PROTECTED]> wrote:
[...] > > 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. > > > What common code are you referring to? There are two different counters > being updated on those two lines, InNoRoutes and OutNoRoutes. Misread. After two misreads this is the point that I warn you I cracked some ribs the other day and am taking painkillers. > > 2373: Why is the statistic here a simple multiple of packets mixed in > > with this info about one packet? > > > Do you mean line 23743? Here I use the length of the original packet > (header and payload), and add the overhead caused by the fragmentation, > that is (pkt-1)*header. I spent some time making sure the equivalent ipv6 code was correct as it looks nastier but didn't here. > > 23863, 24184: extra line > > > > > OK. > > 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? > > > Maybe a new BUMP_MIB could be declared in ip.c. However, I do not think > the extra conditional really reduce the readability, so I do not think > it is worthwhile adding an additional BUMP_MIB. ok > > 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. > > > > > I was trying to make it a bit more readable, by not "drowning" the code > with MIB updates. Would you prefer the macros to be expanded? Or is > there be a better way to handle a "local" macro? Expanded would be worse I think. All macros are local. What value do you get by undef'ng it? > > ip/ip6.c: > > 7505: The MIB var called out in the comment isn't the same string as > > used in the code. > > > OK. > > 12326: XXX So what about them? > > > I am not sure if the "else" case is taken only when packets are > forwarded. If that is the case, then MIB counter should be bumped. This needs to be resolved before putback. > > 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. > > > The value (31) is the OID of the MIB branch as defined in RFC 4293, and > was chosen in order to have some reason behind the value. OK. Spot checking that appears to true of the rest. That list had previously been ordered numerically. Why did you choose to break that order and insert 31 in the middle? > > 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. > > > > > Running a 32-bit userland application (e.g., snmp agent) on an amd64 > system. The size of the structure returned by the kernel would then > possibly be larger than what the user application is expecting, which > could cause it to break. ANSI doesn't guarantee that #pragma means anything. Can't you used types defined by size to achieve this goal? I think the following thread lays out why I don't think you should use pragma pack. https://www.opensolaris.org/jive/thread.jspa?messageID=64062慨 mph > > Many thanks! > Anders _______________________________________________ networking-discuss mailing list [email protected]
