On (10/05/09 21:33), James Carlson wrote:
> Erik Nordmark wrote:
> > The webrev for usr/src is at
> > http://cr.opensolaris.org/~nordmark/refactor-review/
>
> Sowmini asked me to look over some ARP-related bits this morning. I
> didn't look at all of it, and I didn't find any issues that were
> specifically DAD-related, but I do have some comments:
Jim,
thanks for taking the time to do this review.
> ibcm_arp_link.c:
>
> 154: this ASSERT doesn't do much (the reference on line 155 will do
> fine), but it's unclear to me why it's correct to assume that
> the lookup will never fail. Is that really true? Even with
> the edge cases that cause "in transition" routes? Even when
> there's no configured default route?
>
> 207: ditto.
ire_route_recursive_v4 will return the REJECT/BLACKHOLE ire when
no route is found, hence the assert.
> ip_arp.h:
>
> 31: I think more was expected here. :-/
Oops, will fix and send out new webrev.
> ip_arp.c:
>
> 83: Suggest not burying an extern amid a field of statics.
Ok,
>
> 103: I've never seen this before; does it work? Can module names
> really have embedded fs-special characters such as "/"?
I didn't know it wouldn't work:
> arp_mod_info::print struct module_info
{
mi_idnum = 0x1645
mi_idname = 0xf94122a0 "ip/arp"
mi_minpsz = 0x1
mi_maxpsz = 0xffffffff
mi_hiwat = 0x10000
mi_lowat = 0x400
}
> 121-169: Probably not your code, but suggest using a single common
> macro to clean this up. Something like:
>
> #define ARP_HOOK_INOUT(_hook, _event, _ilp, _olp, _hdr, _fm, _m, ipst)
> [...]
> #define ARP_HOOK_IN(_hook, _event, _ilp, _hdr, _fm, _m, ipst) \
> ARP_HOOK_INOUT(_hook, _event, _ilp, NULL, _fm, _m, ipst)
> #define ARP_HOOK_OUT(_hook, _event, _olp, _hdr, _fm, _m, ipst) \
> ARP_HOOK_INOUT(_hook, _event, NULL, _olp, _fm, _m, ipst)
given that its not directly related to the changes here, I'll file
an RFE on that one..
> 179: some compilers complain on the dangling "," in the initializer,
> don't they?
I didn't get any complaints but I do recall seeing these- it's worth
cleaning up, since it's accidental here.
> 186: what if the arl is condemned while waiting for the mutex?
actually this turned out to be a dead function, that will be removed.
> 294: doesn't do anything.
this is the assignment:
294 *sncec = ncec;
but that's passed later to
986 arp_notify(src_paddr, mp1, AR_CN_BOGON,
987 &iras, src_ncec);
> 300-302: unclear why this is true. (because we never resolve probe
> targets on the IPMP member links?)
because all ncec's (both probe targets and others) are created with
ncec_ill set to the ipmp_ill.
> 311: nit: s/addtion/addition/
> 314: nit: s/dependant/dependent/
Ok, will fix
> 313: comment seems to go with line 340 ... ?
true, I'll re-arrange.
> 333: where's the check for an unchanged lladdr in PROBE state?
that happens here:
324 ll_changed = nce_cmp_ll_addr(ncec, src_haddr, hlen);
:
337 if (op == ARP_RESPONSE &&
338 (ncec->ncec_state == ND_PROBE ||
:
340 new_state = ND_REACHABLE;
:
342 nce_update(ncec, new_state, (ll_changed ? src_haddr : NULL))
>
> 387: char * with a constant string as a dtrace probe argument? ew.
> That should be part of the probe name instead. (Also, entry/
> exit dtrace probes shouldn't be necessary given fbt. It's
> more important to expose hidden state with them -- for example,
> nested select and if tests.)
> 478: !
true, I'll clean these up, since they are not very useful as they
exist today.
> 489: 'err' can be used uninitialized here. (lint?)
yes, good catch.
> 800: how does M_IOCTL end up on the rput side of the stream, and if
> it's there, why would sending M_IOCNAK downwards be the answer?
This is a relic of the old unmerged version- strictly speaking we should
never end up over here. So returning ENOENT will at least let someone
notice that somethings not right.
> 839: printf rather than ip0dbg?
should be removed. DTRACE_PROBE is probably more appropriate.
>
> 890: interface is not what?
the selected "cast_ill" for ipmp. The string is mostly there as a
diagnostic, but perhaps it can be more verbose.
> 980: don't assert values that came from the wire.
good point.. this should be a 'if (plen != 4) {/* bail */}'
> 1195,1199: suggest losing these; they don't add much to readability.
Ok.
> 1255: a blocking memory allocation while holding an ill lock might not
> be the best thing here.
true, I'll re-arrange this.
> 1262: strcpy?
yes, that would be better
> 1558-1559: comments don't seem to match code; how can 'mp' be
> provided?
comments got left behind as code was changed. They shall catch up.
> 1689: shouldn't this return ill_reachable_retrans_time instead? Zero
> would mean that we give up on the first sign of error.
but arp_output itself will only fail if the ill is down, or if ENOMEM
was encountered.. I suppose it would be better to return
ill_reachable_retrans_time for the ENOMEM case, at least.
> 1697,1737: suggest B_TRUE for success rather than failure.
this would alos require reversing the return semantics of ndp_announce..
let me investigate.
> 1829: what is this?
good question. it is unused (also in onnv). I'll remove it from *arp.c
at least.
> 1898,1904: there are cases here where detach_mp is leaked depending on
> the unbind mp.
yes, you are right.. I'll restructure and get back.
> 2340,2342: not needed.
shall be removed.
Thanks again for the review.. I'll send out an updated webrev
later this week .
--Sowmini
_______________________________________________
networking-discuss mailing list
[email protected]
