a bit later than promised, but a webrev with fixes is available
at
http:://cr.opensolaris.org/~sowmini/jim_review
Some additional comments (items not flagged here have been accepted
as suggested, with clarifying comments/strings as appropriate):
> >> 103: I've never seen this before; does it work? Can module names
> >> really have embedded fs-special characters such as "/"?
> >
:
> I'm not sure I understand the implications. I suggest checking with
> meem. I've never seen a module name with special characters.
In the interest of caution, I've changed this to "arpip"
> >> 294: doesn't do anything.
>
> Right. But it doesn't do anything because of line 242. *sncec is
> already set, and only needs to be changed if it's being set back to NULL.
agreed. So I've removed the useless line.
> >> 333: where's the check for an unchanged lladdr in PROBE state?
>
> The code, though, says that if we are in PROBE state and it's a
> response, then we go to REACHABLE state -- regardless of whether the
> lladdr has changed.
>
> Which is right?
good catch.. see updated fix (which marks ll_changed responses as STALE
if we are in probe)
>
> >> 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.
>
> If the ill is down, then either (a) it could come back momentarily, in
> which case retrying is better than not or (b) we should have flushed all
> of our unresolved ARP queries. Right?
>
> If it's (a) (or in the ENOMEM case, as you suggest), then the code isn't
> quite right. If it's (b), then a comment about this being a timing
> issue might be helpful.
As you point out, the ENOMEM and !ill_dl_up case are likely transient
errors, and if they are not, then something else should be cleaning
up the data structures that triggered this call anyway, so it makes
sense to return ill_reachable_retrans_time. The only other case is
ILLF_NOARP, which can be handled simply at the top of arp_request itself.
There was one other comment was about the possible leakage of
detach_mp. Meem subsequently pointed out that detach is not needed at
all for any case, so I've removed it as part of this round of changes. .
--Sowmini
_______________________________________________
networking-discuss mailing list
[email protected]
