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:
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.
ip_arp.h:
31: I think more was expected here. :-/
ip_arp.c:
83: Suggest not burying an extern amid a field of statics.
103: I've never seen this before; does it work? Can module names
really have embedded fs-special characters such as "/"?
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)
179: some compilers complain on the dangling "," in the initializer,
don't they?
186: what if the arl is condemned while waiting for the mutex?
294: doesn't do anything.
300-302: unclear why this is true. (because we never resolve probe
targets on the IPMP member links?)
311: nit: s/addtion/addition/
314: nit: s/dependant/dependent/
313: comment seems to go with line 340 ... ?
333: where's the check for an unchanged lladdr in PROBE state?
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: !
489: 'err' can be used uninitialized here. (lint?)
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?
839: printf rather than ip0dbg?
890: interface is not what?
980: don't assert values that came from the wire.
1195,1199: suggest losing these; they don't add much to readability.
1255: a blocking memory allocation while holding an ill lock might not
be the best thing here.
1262: strcpy?
1558-1559: comments don't seem to match code; how can 'mp' be
provided?
1689: shouldn't this return ill_reachable_retrans_time instead? Zero
would mean that we give up on the first sign of error.
1697,1737: suggest B_TRUE for success rather than failure.
1829: what is this?
1898,1904: there are cases here where detach_mp is leaked depending on
the unbind mp.
2340,2342: not needed.
--
James Carlson 42.703N 71.076W <[email protected]>
_______________________________________________
networking-discuss mailing list
[email protected]
