Here's my partial review (haven't looked at files not listed below).
-venu
___________________________________________________________________________
usr/src/uts/common/inet/arp.h
vi-0 L62 (old) : I think AR_ENTRY_LLAQUERY was added by 4246024.
And, reading 4069191, it seems the ATM code may have
references to this as well, but given that we never
implemented anything, I suppose no one could be
*using* it. However, assuming an ATM application
refers to this, will it break (compiling, i.e)?
Also, can we link bugs 4246024 and 4069191?
I looked around for the usage of AR_XMIT_REQUEST/
AR_XMIT_RESPONSE, didn't find any, hopefully no one
(SunCluster etc.) uses these (came across an
old contract for AR_XMIT_REQUEST - PSARC/1998/504 -
not sure if it still holds, just checking).
usr/src/uts/common/inet/arp/arp.c
vi-1 L75 : Rename to have ACE prefix. Also, variables to
art_arl, art_naces, art_aces, if possible
(keeps it consistent with others).
vi-2 L241,245 : I am bit unclear about the disabling stuff. The
design doc mentions that "probing and defense"
can be disabled by setting these to 0 (what is
the behavior if one is 0, and the other isn't?).
The doc also mentions that "conflict detection
and resolution cannot and should not be disabled"
(I am assuming it is not possible to do so?). I
am not sure if "defense" and "conflict resolution"
in the above sentences are related. Also, would it
be more clear (un-ambiguous) if we also had a
arp_pnd_disable (prob-n-defense), or equiv., here?
vi-3 L289-296 : (not related to your changes) while we are here,
can we just define the sap length and the hw address
length as macros in some (possibly) netinet file
and use them?
vi-4 L404-406 : (nit) when conditions span a line, I think, it is
L747-749 usually enclosed by {}
L772-775
L3992-3993
vi-5 L405 : Can we define something like IS_IPV4_LL_SPACE (or
equiv) and use?
vi-6 L661,681-682 : The comments mention we return true when the h/w
address changes, but we seem to do it conditionally.
Can we update the comment to explain this?
vi-7 L752-753 : Can be moved to L758.
vi-9 L914 : Just curious, how is this arh_operation value
L3962 used?
vi-10 L1339-1342 : I suppose (the design doc also mentions this)
in.routed also does something similar to avoid
collision. Is this something that can be generalized
so that it can be re-used?
vi-11 L1410,1418,1421,: Seems to me that L1425-1426 can be moved to L1411.
1426 What is the relationship between L1421 and L1425-
1426, i.e if ace is not found, and aflags has
ACE_F_PUBLISH, but doesn't have ACE_F_UNVERIFIED
set. (havent' looked around to check if that is
possible). Do we still do DAD?
vi-12 L1502,1513, : It will be helpful if these also printed out some
1518 details like the arl, address etc. Remove those not
L3128,3133,3138 required.
L3167, 3225, 3235
L3974, 4079, 4125,
L4133
vi-13 L2113 : What does link_state == B_TRUE mean? If it means
whether the link is up or down then can we rename
this to arl_link_up, else can we assign
ARL_LINK_UP/ARL_LINK_DOWN to this instead?
vi-14 L2351 : (cosmetic) Delete blank line.
vi-15 L2524 : ar_below_is_ip is a bit confusing to me, and
MODULE_BELOW_IS_IP too (so it is not specifically
your change). I guess it may be better to rename
to indicate that it is ARP -> IP -> Driver as
opposed to ARP -> IP (since below_is_ip is
true for both, but we are interested only in the
former).
(nit) Anyway, I suppose the '()' around on the
RHS can be removed. Or L2524 & 2525 can be
combined.
(..don't feel strongly about this).
vi-16 L2926-2935 : Not sure why this is a function; it is used
only at one place. If needed, please rename.
vi-17 L2954 : Let's avoid using variable names such as 'i'.
L4019
vi-18 L3078-3079 : I suppose (uint32_t)..arh_hlen & 0xFF was because
hlen is uint32_t and arh_hlen is uchar_t.
Hopefully, this cast is fine.
vi-19 L3033-3036(old) : Do we still send a broadcast response to IP?
vi-20 L3305-3307 : If it is just "ce" then should we check for the
arl_name along with the check for DL_NOTIFY_ACK?
vi-21 L3859 : (not your change) while you are here, can you
change this to NULL?
vi-22 L3967,3971 : arl->arl_hw_addr_length ->
ace->ace_proto_addr_length?
vi-23 L3985 : Not sure what exactly the code std. says, but I
prefer a line per variable.
vi-24 L4000-4004 : Does this get the most delayed RESCHED_LIST_LEN
aces? Since we fill up RESCHED_LIST_LEN aces, then
insert remaining aces within this array according
to the last_bcast value, is it possible that the
list might not actually contain the most delayed
ones, i.e assuming RESCHED_LIST_LEN to be 4 and
we have 6 aces with last_bcast as 20 30 40 50 12 10
and the defend_rate is 4, then will we have the list
as 12 10 40 50? Or am I just reading the algo.
incorrectly?
vi-25 L4178-4179 : We use ire_cache_lookup(), but elsewhere we ask
IP to do this via ar_client_notify() can't we just
do it directly in other places too?
usr/src/uts/common/inet/arp_impl.h
vi-26 L75 : Same as comments for arp.c (vi-13)
usr/src/uts/common/inet/ip.h
vi-27 L1668 : (just curious) Are these changes planned for an
update release as well?
vi-28 L2764 : seems like a generic function can we have this
someplace else? If you want to retain, have an ip_
prefix to keep it consistent.
usr/src/uts/common/inet/ip/ip.c
vi-29 L3399-3598 : General comment. ip.c keeps growing, can we please
put these (and possibly other DAD stuff) in, say,
ip[6]_dad.c?
vi-30 L3415 : Same as vi-23.
L3466
L3681,3683
vi-31 L3422 : Will we get here if ipif->ipif_lcl_addr ==
INADDR_ANY?
vi-32 L3446 : (nit) I think when a condition spans a line we
L3525 use {} (henceforth I'll just put this as {})
L3529, 3552
L3564, 3591
L3710
vi-33 L3463 : Is the 10 for the ipif_id?
vi-34 L3464,3465 : define/use macros instead of 128/16.
vi-35 L3470-3482 : So, AR_CN_BOGON is the default case and "claimed"
is by someone else? also if it is not arp_extended,
will we come this path (i.e. default)?
vi-36 L3488-3491 : This isn't clear to me. Is this case for a probe
packet, do we get them here?
vi-37 L3562-3563 : Can we just do this? Can't ipif_down() ever return
EINPROGRESS? If it cannot can we have an assert?
If it can, then given ipif_down() passes NULL for
queue and mblk, I suppose you would need to provide
some deferred processing logic here.
vi-38 L3666-3670
L3720-3723 : These two seem to be a bit different. We will end
up tearing down the interface now (i.e with your
changes), right? We don't do it currently (i.e.
existing stuff)?? Also, if it is not ill_arp_extend,
can we skip L3681-3718?
vi-39 L5462-5494 : Same as ip.h, vi-28.
vi-40 L5490 : (nit) NUL -> NULL.
vi-41 L14512 : I am OK with this, but Res_act_move seems to suggest
something else, though I don't have a better name
for it.
vi-42 L14671-14674 : This is probably mentioned in the design doc. So,
when a link comes up we do a DAD, because it might
contain some unverified/duplicate address(es)?
vi-43 L24815 : Set as int and used as boolean.
usr/src/uts/common/inet/ip/ip6.c
vi-44 L6439 : separate line / variable, if possible.
vi-45 L6467 : (nit) could have been done @ L6445.
vi-46 L6573,6593, : are these freemsg instead of freeb just to avoid
L6610,6618, checking for non-null dl_mp?
L6624,6635
usr/src/uts/common/inet/ip/ip6_if.c
vi-47 L1263 : In this case should we return err (EINPROGRESS)
instead of 0?
usr/src/uts/common/inet/ip/ip_if.c
vi-48 L669-682 : I am not sure why these are functions, can only see
one place in ip.c where these are being used.
vi-49 L10551 : separate line / variable, if possible.
vi-50 L10589 : (nit) {}
L10817
L17857
L13157-13158
vi-51 L10582-10583 : Is this the DL down (unbind) event? or does this
L10810-10811 relate to 'need_arp_down'?
vi-52 L10589-10590 : Can we also check for ill_dl_up here since it is
L10817-10818 not going to change after this (as we check for
!need_up)?
vi-53 L13121-13130 : Is this required in case of Res_act_defend too?
vi-54 L13245 : Should we also check for ill_ipif_dup_count != 0
here?
vi-55 L13259-13374 : Consider moving this to ip_dad.c.
vi-56 L13327 : Is this possible? Should it be an assert?
vi-57 L19935-19936 : Should !ipif->ipif_addr_ready suffice?
usr/src/uts/common/inet/ip/ip_ire.c
vi-58 L4241-4252 : (nit) I suppose some of these can just be combined.
vi-59 L4250 : (nit) {}
L4263
vi-60 L4263-4264 : I haven't checked the assignments to hwm_hwlen or
dl_dest_addr_length. Does dl_dest_addr_length
contain the mac addrlen + abs(saplen)? or mac
addrlen + saplen?
usr/src/uts/common/inet/ip/ip_ndp.c
General Needs merging with onnv-gate.
vi-61 L244 : (nit) I suppose err could just be initialized @
L141.
vi-62 L563-600 : Consider moving to ip[6]_dad.c
L1360-1509
vi-63 L564,596 : I suppose this corresponds to the caller doing
a ndp_lookup(), I'd prefer the caller doing the
NCE_REFRELE as well, any specific reason this
is being done here?
vi-64 L1381-1384 : (nit) {}
vi-65 L1390 : I haven't exactly traced the flow, but do we expect
the check for DHCP/TEMP to be done before coming
here, say in ip_ndp_excl() or do it somewhere
later?
vi-66 L1376-1401 : I am probably missing something (given L1361),
anyway, we call ip_ndp_recvover via
ipif6_dup_recovery, which is scheduled by
ip_ndp_excl for an ipif flag that is IPIF_DUPLICATE.
We seem to walk the list if ipifs for the ILL and
schedule the timer. Yet, in ip_ndp_recover we
walk the list of ipifs for the ILL?
vi-67 L1439 : Consider prefixing extract_ether_address() with
ip_ndp if this belongs to this file, if it is
more of a generic utility, move it elsewhere.
vi-68 L1464 : Should this check be for "==" instead of ">="?
vi-69 L1476 : (nit) can alen be < 0?
vi-70 L1521 : Consider defining/using a macro.
vi-71 L1381-1391 : Consider defining a macro.
L1538-1548
vi-72 L1556-1558 : (nit) {}
L1634-1635
L2517-2521
vi-73 L1568-1569 : Same as vi-37.
vi-74 L1614 : line / variable.
vi-75 L1650 : Consider defining/using macro.
vi-76 L1741 : At least in nce_xmit() I see that the plen contains
L1933 sizeof (nd_opt_hdr_t). I am curious if this is a
bug that's being addressed or is this due to changes
by this project itself.
vi-77 L1830 : Use macro/variable instead of hard-coding 15000.
No Comments:
usr/src/uts/common/inet/ip/ip_squeue.c
usr/src/uts/common/inet/ip6.h
usr/src/uts/common/inet/ip_if.h
usr/src/uts/common/inet/ip_ire.h
usr/src/uts/common/inet/ip_ndp.h
usr/src/uts/common/net/if.h
usr/src/uts/common/net/if_arp.h
usr/src/uts/common/netinet/arp.h
General comments/etc.:
______________________
The interoperability tests in the design doc looks great. What kind of
regression tests have been done (including ATM)? Doesn't look like this
might come in the way of any performance tests (unless they include
adding/removing addresses in the course of the test), but have you done
any to check? Also, looks like we might increase the address bring-up
time when the system comes up, have you checked for any applications that
might be affected by this? I remember a SunCluster issue (6371284) when
it take some time for the deault router to be set up (due to switch set up),
please check this. Just looking at the source I don't see any issues with
the processsing in tcp_multisend_data(), I suppose you have looked at
this. BTW, has someone familiar with ND protocol reviewed the IPv6 changes?
and are folks working on any ARP-related projects in sync with these changes?
_______________________________________________
networking-discuss mailing list
[email protected]
