venugopal iyer wrote:
Rao & Anders:
Attached is my partial review comments. I have only looked at the files
listed in the attachment.
-venu
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.
------------------------------------------------------------------------
From : http://cr.grommit.com/~apersson/mib_gate_webrev
(file:///net/nptbld.sfbay/disk1/mib_gate/webrev)
usr/src/uts/common/inet/mib2.h
L150
Is there an IPv6 equivalent for this? Looking at ip.c, it assigns
0 to name for IPv6?
DeviceIndex is used for both v4 and v6. In the case of
mib2_ipIfStatsEntry_t, there is a field (ipIfStatsIPVersion) indicating
which version it is.
L386
The comment need rephrasing.
OK.
L473-480
/* # of IPv4 packets received by IPv6 and dropped */
Counter ipIfStatsInIPv4;
/* # of IPv6 packets received by IPv4 and dropped */
#define ipIfStatsInIPv6 ipIfStatsInIPv4
The name ipIfStatsInIPv4 doesn't seem to imply dropped packets.
Is this name something that we have come up with, if so, I think
we should look at renaming it (I know previously it was called
ipv6InIPv4, so I'll leave it up to you). Neither does ipIfStatsInIPv6.
Similarly for ipIfStatsOutIPv4 & ipIfStatsOutIPv6. Do these mean
anything other than dropped packets?
Yes, it is a counter defined by Sun, and I agree with you that the name
has become very non-descriptive. The name will definitely be changed
(maybe to ipIfStatsInBadIPVersion). Similarly, the name for OutIP[v4|v6]
should be changed, but those counters do not indicate dropped packets,
just a call from ip_output to ip_output_v6, or vice versa.
L409,492
What is the diff between ipIfStatsInReceives & ipIfStatsHCInReceives?
32-64 bit?
Yes. All fields with HC in them are 64-bit counters.
L495
dg -> datagrams? If possible avoid using this abbreviation (I know
it is already used in other places). What is the diff between this
and L491? Incorrect comments?
The comment for L495 is incorrect and will be fixed.
L426,498
Again 32-64 bit?
Yes.
BTW, the comments L377-378 don't mention much. Could
you add some details why the first sizeof (mib2_ipv6IfStatsEntry_t)
is identical to mib2_ipv6IfStatsEntry_t? And also about the 32
and 64 bit counters for some.
OK.
L449-451, L486-488,L521-523
Indent these.
OK.
L506-520
Some have packets & octets, others only packets. Why?
That is defined by RFC 4293. But I assume that not all events are
thought to be important enough to warrant dual counters.
L521-523
These are not moved from mib2_ip_t, right? From the comments it seems
so.
Right, they were not moved, just included because they are part
mib2_ip_t. The comment will be updated.
L586
What does this mean? Looks like it is set to ARP query retransmit
interval? If so why is it part of this struct? Also, L635.
It's the time between retransmissions of ARP requests. To us it seemed
like the most reasonable place to put it.
L1243
Seems to be similar to L1145. And L1245 seems to be similar to
L1147. (Similar comments for mib2_udp_t).
They differ in that one is 32-bit, the other 64-bit. Similarly for
mib2_udp_t.
L1309,1362,1440,1480
What is the expected format of this? Looks like it is set to
lbolt64 (tcp_open_time).
Right, it's set to lbolt64. You think it should be stated explicitly in
the comment?
L1438,1476
Is uint32_t good enough? It is udp_t pointer right?
The MIB is limited to a 32-bit identifier, which is why I used uint32_t.
Also, the identifier is only used to distinguish endpoints that use the
same addresses and ports, so the chance of there being a conflict
should be very small.
usr/src/uts/common/inet/ip.h
L1882
Is this fine if you want to get this into an update?
I am not sure I understand the comment. Could you elaborate?
usr/src/uts/common/inet/ip/ip.c
L768
To keep it consistent with the others consider:
... ip_snmp_get_mib2_ip_traffic_stats(queue_t *, mblk_t *);
OK.
L2462,2811
Does this get reflected in ill_ip_mib? icmp_inbound_error_fanout()
calls this and does a go to drop_pkt, but doesn't increment
ill_ip_mib?
The target label will be changed to discard_pkt.
L2679
What changed here?
Both ill and recv_ill are being passed it.
L6272,6317
It looks right to me, but I haven't traced all the paths to
ip_fanout_proto() to check if ill is always non-null.
I will go over it again, and add a conditional if there is any uncertainty.
L6646
Why ill? Why not recv_ill?
According to the RFC, we should update the counters that are associated
with the interface to which the packets were addressed.
L6484,6610,6531,6536,6541,6549,6589
Do we need to update stats here?
L6672,6694
.. and here?
For most of those cases it would be reasonable to update the stats. I am
not sure about the cases where ip_process() is called.
L6964,7022
L12554,12614,12639,12643
L12821,
Which is the right one, ill or recv_ill?
We want to update the stats on the interface to which the packets are
addressed, so we update ill.
L7107,
The caller should update stats if suitable.
7275,7312,7318,7371,7376
Should we update stats here?
Most cases seem reasonable, but again, I am not sure about ip_process().
L7531
Why is this in else?
For v6, we only want to bump the global MIB (ip6_mib) if there is no ill
available. For v4 we need to provide aggregate statistics for IP, which
is why we bump both ip_mib, and ill_ip_mib (when available).
L7529,8755...
What does this exactly mean? i.e. ipIfstatsOutDiscards for an ILL.
If the processing of a packet has gotten so far that the system knows
which ill it should be going out on, but an internal error is
encountered before transmitting the packet, then OutDiscards should be
bumped.
L12495
Do we need to update stats here? Also, (in L12486 too) do we need
to update the ILL's checksum stats here? Or are these updated
in the caller?
The ILL's checksum stats should be updated, other statistics should be
handled by the caller.
L12652,12972,12990,13030,13049,13063,13067,13077,13099
Do we need to update stats here? (ipIfStatsHCInDelivers)
L13558,13568,13576
Pull this out.
OK.
L13701-13702
Would we also increment these in ip_rput_process_forward() @
L13753 (i.e. L13907-13908).
Oops, that should not happen, and will be fixed.
L13848
Do we need to update stats here?
Yes, it needs to be updated.
L14692-14695,L14705-L14707,L14718-14721
It is not exactly clear what the intent is. Do we want to
update the stats even if we end up dropping at L14714 or
L14728 or L14745 etc.?
Yes, we want to account for all incoming packets. I will add a comment
to make things clear.
L14882-14886
Does it make sense to have these in ip_rput_process_multicast()
instead? (Similar comment for L15016-15018 w.r.t
ip_rput_process_broadcast)
It makes perfect sense :) That way it will be consistent with
ip_rput_process_forward()
L16414
Do we need ipIfStatsForwProhibits here?
Yes.
L17230
Do we need to update InDiscards here (and in other places in this
function where the packet is dropped)?
Yes.
L18051-18063
Is casting these to uint32_t OK? Well, I suppose it should be OK, but
just checking.
It should be fine.
L18126,L18939
Perphaps this is mentioned in the docs, what does '0' here mean?
Could you define it as a marco in mib2.h?
The 0 is used to indicate the global IP MIB, i.e., not associated with
an ILL. I will define a macro to make it clearer.
L18138-18140,L18169-18172,L18969-18970,L19014-19016
If this fails, do we still want to keep going with the interface stats?
I think it is fine to keep going.
L18146-18165,L18976-18994
Could some of these be done in ill_allocate_mibs() itself or if
that is too early, when sufficient details are available for an ILL?
Yes, and I will do that.
L18327-18330
Define it as macro and use it (also change existing places to
use the macro), else if ip_areq_template is changed to a
diff. value, this might be left out. (Similar comments for
L18426).
OK. What would be a reasonable place to stuff that marcro? ip_if.h or arp.h?
L18936
Define this as a macro.
OK.
L20074-
Do we need to update stats here (and perhaps other places where we
bail out)
Yes, it needs to be updated.
L22708
Don't think the stq should have changed here, right? i.e. out_ill
is already set @ L22620. Or did I miss it?
stq should not change, but out_ill might not have been set due to a goto
(i.e. from L22205).
L23080
Do we need to reassign out_ill here? and L23142?
We might end up here coming from a goto, in which case we would miss the
initial assignment.
L23592
Shouldn't OutDiscards be updated too?
OutDiscards should only be used if there is no other, more specific,
counter.
L23743
cstype won't be happy is operators aren't surrounded by spaces (unless
they are at the end of the line).
OK
L23764
If it needs to be, do so, else remove these changes.
OK.
L24184
Remove this extra line.
OK.
L26353-26354- (and other places in this function)
Do we have to update the ill's mib too?
Yes, when the ill is available it's mib should be bumped.
L28821
Are there any new counters that need to be addede here?
I do not think so.
L29191-29197
Move these before the block comments for ip_xmit_v4().
OK.
L29279,29280
cstyle won't be happy with this, indent by 4 spaces.
But then the line will be longer than 80 characters, and it will still
complain :)
L29274-29275
when will this be the case?
Never, as far as I can see. I will remove it.
L29253
ipIfStatsInDiscards on the out_ill??
I do not understand why it was done that way, but it seems wrong. I will
remove the if-else and only use OutDiscards.
usr/src/uts/common/inet/ip/ip6.c
(just glanced a the changes and they seem fine).
L12326
Either add the change or remove this comment if it isn't needed.
OK.
usr/src/uts/common/inet/ip/ip_ftable.c
no comments
usr/src/uts/common/inet/ip/ip_if.c
L603
What does "at this point mean" is there a follow up work for
this?
No, but I could file a RFE. I will change to comment to be less confusing.
L962-964
Just curious why this discrepency between IPv4 & IPv6?
For v4 we need to provide aggregate MIB statistics, in addition to the
information specific to each ILL. So we update ip_mib whenever
ill_ip_mib gets bumped.
L960
So, is it OK to lose icmp6_mib?
Definitely not. Thank you for catching it.
usr/src/uts/common/inet/ip/ip_ndp.c
no comments
usr/src/uts/common/inet/ip6.h
no comments
usr/src/uts/common/inet/tcp.h
L437
Are these changes going into an update?
Yes, S10U4.
usr/src/uts/common/inet/tcp/tcp.c
L15979,16043
Does a ConnCreationProcess mean unknown?
A value of 0 means unknown process. This is in the RFC, but I can add a
macro to make it explicit.
L160640-16065
Since the synchronization is done in multiple places (in ip.c
and udp.c) consider using a marco
OK.
usr/src/uts/common/inet/udp/udp.c
Similar comments as in tcp.c for creationprocess and 32-64
bit synchronization.
OK.
L5856
cstyle won't be happy, add space around '?' and ':'
OK.
L5850-5852,L5883-5886
How valid is this assumption and if the assumption does not
hold, what results?
I am not sure how valid it is. However, the identifier is only used to
distinguish endpoints that use the same addresses and ports, so the
chance of there being a conflict should be small. The worst thing that
could happen is that the snmp agent only reports one of the endpoints.
usr/src/uts/common/inet/sctp/sctp_input.c
L3463,3481,3489
While we seem to be changing the fanout functions to take an
additional ill, we don't seem to care about it here?
That should be corrected.
General sctp comment, we seem to be adding an additional ill_t * to
the fanout/input functions just to update the ill_mib. Is that so?
Yes.
If so..
This does not seem to be done in some places (like sctp_input.c),
so, firstly, do we need this update? Secondly, the only place
(that I can see) where ip_fanout_sctp is called with different ill
and recv_ill is icmp_inbound_error_fanout(), so do we want to add
another ill_t just for that? Is it the case that the ills (ill and
recv_ill) are always different in this path? Why can't the ill_recv's
ill be updated since it is for ipIfStatsInDiscards, right? Basically,
the addition to the functions doesn't seem necessary to me, let me
know if I am reading it wrong.
The scenario I wanted to avoid was were the InDelivers counter was
bumped on one ill, and then the packet was discarded in sctp, bumping
another ill's InDiscards.
As a side note, it would have be ideal if sctp_input() would indicate
upon return whether or not the packet was deliver, then we could factor
out all the InDiscards from sctp.
Thank you :)
Anders
_______________________________________________
networking-discuss mailing list
[email protected]
