Erik Nordmark wrote:
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.
A few comments.
ip.h:
1882 mib2_ipIfStatsEntry_t *ill_ip_mib; /* ver indep.
interface mib */
1883 mib2_ipv6IfIcmpEntry_t *ill_icmp6_mib; /* Per interface
mib */
3159 extern void ip_mib2_add_ipstats(mib2_ipIfStatsEntry_t *,
3160 mib2_ipIfStatsEntry_t *);
Does this follow the same pattern as for IPv6 in terms of names as
well as where the mib data structures are allocated and freed?
The ill_ip_mib is allocated/freed where it was previously done for
ill_ip6_mib. I am not sure I follow the comment about naming, could you
elaborate?
ip.c (and elsewhere)
6103 BUMP_MIB(&ip_mib,
ipIfStatsInUnknownProtos);
6104 BUMP_MIB(ill->ill_ip_mib,
6105 ipIfStatsInUnknownProtos);
I think you must switch to only incrementing one MIB in each place; in
the vast majority of cases this will be the ill mib. Thus there should
never be a need to update the ill mib and the global mb in the same
place; use the IPv6 code paths as the example.
Then when handling a mib_get (and kstat update if there are kstats for
these) you can add the per-ill mib and the global mib together into a
separate sum which is what is returned as the global stats.
If there is code that sometimes have an ill and others not, instead of
doing this:
7536 BUMP_MIB(&ip_mib, ipIfStatsOutDiscards);
7537 if (ill != NULL)
7538 BUMP_MIB(ill->ill_ip_mib,
ipIfStatsOutDiscards);
you should do
if (ill != NULL)
BUMP_MIB(ill->ill_ip_mib, ...);
else
BUMP_MIB(&ip_mib, ...);
if there are functions that do lots of those, it might make sense to
have a local 'mibptr' variable set up front based on ill != NULL.
I believe that Rao have some concerns on taking this path.
ip.c:
Add an ill argument so that this code can use the per-ill stats:
2461 if (!pullupmsg(mp, -1)) {
2462 BUMP_MIB(&ip_mib, ipIfStatsInDiscards);
2463 return (NULL);
2464 }
based on earlier feedback, I have changed the code such the the caller
(icmp_inbound_error_fanout) takes care of bumping the counter.
ip.c:
Doesn't the caller of ip_proto_not_sup() know the ill so it can pass it
in as an additional argument?
6095 /* Get ill from index in ipsec_in_t. */
6096 ill = ill_lookup_on_ifindex(ii->ipsec_in_ill_index,
6097 (IPH_HDR_VERSION(ipha) == IPV6_VERSION), NULL, NULL,
NULL, NULL);
I am not sure if the ILL that the caller knows of will always be the
same as the one obtain by the lookup (although it might not matter). I
will look into it more and make the change if possible.
ip.c:
13207 static void
13208 ip_sctp_input(mblk_t *mp, ipha_t *ipha, ill_t *recv_ill,
boolean_t mctl_present,
13209 ire_t *ire, mblk_t *first_mp, uint_t flags, queue_t *q,
ipaddr_t dst)
...
13222 ill_t *ill = (ill_t *)q->q_ptr;
I fail to see how this works when the ill_t should be lo0, since there
is no q_ptr which points to the lo0 ill.
It might be that the original code is broken for lo0 traffic; you need
to verify this before proceeding.
OK, I will look into it.
ip.c:
13372 static boolean_t
13373 ip_rput_multimblk_ipoptions(queue_t *q, mblk_t *mp, ipha_t
**iphapp,
13374 ipaddr_t *dstp)
13375 {
13376 ill_t *ill = (ill_t *)q->q_ptr;
I'd much prefer adding an explicit ill_t argument than assuming that
the queue maps to an ill_t; it is very hard to ensure that with all
the code paths.
OK.
ip.c: I don't see why you need these lines; you've already added
pkt_len to the MIB earlier and the fact that the packet was padded by
Ethernet isn't important.
14705 UPDATE_MIB(&ip_mib,
ipIfStatsHCInOctets, len);
14706 UPDATE_MIB(ill->ill_ip_mib,
ipIfStatsHCInOctets,
14707 len);
and
14719 UPDATE_MIB(&ip_mib,
ipIfStatsHCInOctets, len);
14720 UPDATE_MIB(ill->ill_ip_mib,
ipIfStatsHCInOctets,
14721 len);
I was thinking about the case when the length reported in the header is
wrong for some reason.
ip.c: add an ill_t argument to ip_rput_forward_options() so that this
can use the ill mib:
16592 BUMP_MIB(&ip_mib,
ipIfStatsForwProhibits);
I will pull the counter out and let ip_rput_forward handler the update.
In general I think you should only need to use the global ip_mib in
ip_output/ip_newroute when you don't find an ire_t hence have to ill_t
to associate the packet with,
OK.
ip6.c and elsewhere: why does adding a second ill_t to ip_fanout_sctp
change things? Don't all the places pass in the same thing for both as
in this example:
10658 ip_fanout_sctp(mp, ill, ill, (ipha_t
*)ip6h, ports,
10659 fanout_flags|IP_FF_SEND_ICMP|IP_FF_IP6INFO,
10660 mctl_present, IP6_NO_IPPOLICY,
ipif_seqid,
10661 ire->ire_zoneid);
Right. That will be changed.
Thank you very much for your time,
Anders
_______________________________________________
networking-discuss mailing list
[email protected]
