On Mon, Sep 14, 2009 at 02:54:40AM -0700, Michael Hunter wrote:
> On Thu, 10 Sep 2009 13:06:40 -0700
> Renee Danson Sommerfeld <renee.sommerfeld at sun.com> wrote:
>
> > On Thu, Sep 10, 2009 at 07:11:57AM -0700, Michael Hunter wrote:
> > > /net/coupe.eng/builds/mph/nwam1_work/webrev
> >
> > ncu.c, 224-27,233-39: these are redundant since you kept the unplumbs
> > at the top of the function.
>
> I reworked this. Please review.
The re-work looks good.
I have one remaining comment that's not related to the ncu_ip.c changes:
object.c, line 335: haven't heard any concerns here, so I say the assert
and the parameter both need to go.
And then there are still several for ncu_ip.c. I think we're getting
close here, most of these are pretty picky comments-about-comments. But
I still have some questions about that address-up logic (the comment about
line 835 is the main point).
line 249: can remove the colon at the end of the msg string now.
line 311, addresses_match() function, and other places: looks like you had to
add these /*LINTED*/ lines because you started passing things around as
sockaddrs rather than sockaddr_storage. I don't remember what contortions
you had to do when they were sockaddrs; but was it really worse than this?
The combos at 379, 381 and 385, 387 are especially icky!
line 596: Should be on two lines (open-curly-brace on second line).
line 809: Wouldn't 'stateless_running' be more appropriate, given the way
you've named (and defined) v4dhcp_running and static_addr?
line 825: s/dicide/decide/
line 831: the comment here seems wrong, or at least confusing. I think the
block comment that follows the if expression is sufficient. (I really
hope I haven't contradicted myself here...I know we've been back and
forth about the comments and logic in this section quite a lot!)
line 835: (yeah, I know I just said this comment was sufficient; but I think
it could be phrases a little more clearly) I think something like "If
the address set was not a static addr we requested, and we did request
DHCP, we'll assume this was a DHCP-assigned address. Set dhcp state
configured variable for appropriate IP version." would be more clear.
But wait! What if this was a v6 stateless address? The flags we're
checking to set v4dhcp_running and stateless_running are not specific
to the logical interface, they're specific to the index, right? So we
don't know definitively what the source of this particular address was,
unless it was on our configured static list.
line 851: s/ then remove it by unplumbing/; remove it by unplumbing the
logical interface/
line 909: s/attending/attention/
line 910: s/disappearning/disappearing/
lines 920-23: what's the status on this issue?
-renee
