Michael Hunter wrote:
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:
I will let Rao address the above comments.
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.
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.
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.
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?
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.
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.
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.
Many thanks!
Anders
_______________________________________________
networking-discuss mailing list
[email protected]
