Hi
Anders,
As told, here are some review comments.
1)
in.ndpd/main.c :
L1514 -
L1689 -
Add comment block above new function definition.
you might consider doing a validity check for passed arguments, if
applicable.
L1531-
Do buffer overflow check for strncpy.
L1696-1703 -
len is of type size_t, later it is being compared with sizeof (int).
shouldn't len be declared of type int ?
2)
in.ndpd/tables.h :
just a thought -
Shouldn't this entire file be renamed as ndpd.h,
tables.h as a file name of in.ndpd daemon, seems inconsistent to me.
3)
in.routed/defs.h:
L167-
add one liner comment above extern declaration for struct dr.
You could choose self-explanatory names for variables.
4)
in.routed/input.c:
L1063 - change it to "Honor only those requests ... "
L1096-
Don't we need RIPCMD_DEFROUTERS_RESP implemented as well,
we declare the ripcmd (DEFROUTERS_RESP at L-58 in in.routed/trace.c)?
5)
in.routed/rdisc.c
L90
L91
Same, you can add comment or you may choose to be descriptive with
variable names.
5)
protocols/routed.h
L157:
Rename defr_t to def_rtr_t for better understanding
6)
inet/ip/ip6.c
General comment:
(a)
Please double check wherever we are failing to do adjmsg, pullupmsg
or doing freemsg() before doing return on some error condition.
Those are the places those might need increments for relevant MIB error
counters.
(b)
Do Null pointer dereference checks before updating mibs for ill, i.e.
ill->ill_ip_mib.
(c)
I see, declaration of "mib2_ipIfStatsEntry_t ip6_mib" in ip6.c
(similar to ip_mib in ip.c) but no usage.
Shouldn't mibs under ip6_mib be updated as well when you update
ill specific mibs for IPv6?
i.e.
BUMP_MIB(&ip6_mib, ipIfStatsInDiscards);
BUMP_MIB(ill->ill_ip_mib, ipIfStatsInDiscards);
(d)
L636
L914
L1259
L1266
L1273
L1312
L1724,
L1734,
L1899,
L1893
L3330
L3516
L3702
L7325
shouldn't we be incrementing ipIfStatsInDiscards above?
L1313
L2795
Shouldn't we be incrementing ipIfstatsOutNoroutes above?
L6531
L6582
IPv6 option handling code discards packets here what various error cases
shouldn't MIBs for iIfStatsInDiscards be incremented above, as well?
L1257
L1265
L1273
L1875
L1889
L1893
L1916
shouldn't MIBs for iIfStatsInDiscards be incremented above, as well?
L7755
L7772
L7788
Shouldn't here tcpIfStatsInErrs be updated as well?
L7911
This is UDP specific error, since pullup failed on UDP
header, so shouldn't the UDP specific error mibs be updated?
L9035
Shouldn't ipIfStatsReasmOks be incremented here?
L8646
L8936
L8986
L9010
Shouldn't ipIfStatsInReasmFails be incremented for above lines?
L433
L490
L1916
L2702, L2703
L6662
L10391
L10479
L10697
L10828
Like L7358, shouldn't ipIfStatsInMcastPkts etc. be updated above?
-Deepti
Anders Persson 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.
Many thanks,
Anders
_______________________________________________
networking-discuss mailing list
[email protected]
_______________________________________________
networking-discuss mailing list
[email protected]
