On Mon, 14 Sep 2009 12:42:17 -0700
Renee Danson Sommerfeld <renee.sommerfeld at sun.com> wrote:
> 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.
The assert was never hit in my testing (which includes a fair bit of
wifi). OTOH it is used in the wifi codepath (~ncu.c:1707). I'll take
the assert out although I really find the idea behind this function
wrong.
>
> 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.
ACCEPT
>
> 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!
The mistake is using sockaddr_stoarge declared in <sys/socket_impl.h>.
The only reason I don't rip it out is 'cuz it is in the external
interface. I didn't want to go that far in this change.
The /*LINTED*/ stuff is minor to that.
>
> line 596: Should be on two lines (open-curly-brace on second line).
ACCEPT
>
> line 809: Wouldn't 'stateless_running' be more appropriate, given the way
> you've named (and defined) v4dhcp_running and static_addr?
ACCEPT
>
> line 825: s/dicide/decide/
One of my favorite death metal bands is deicide. This comment is
talking about killing an address ;)
ACCEPT
>
> 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!)
ACCEPT
>
> 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.
ACCEPT
>
> 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.
As I talked about in email this should really written to find the
lifnum that this addr is on and then do the appropriate things with
that lifnum. Since I first rewrote this code I've done that in another
place and will just reuse that code. Its needs a little refactoring
FWIR anyways.
>
> line 851: s/ then remove it by unplumbing/; remove it by unplumbing the
> logical interface/
ACCEPT
>
> line 909: s/attending/attention/
ACCEPT
>
> line 910: s/disappearning/disappearing/
ACCEPT
>
> lines 920-23: what's the status on this issue?
Hmmm, I thought I'd worked this out. Investigating.
It'll be fixed in my next webrev which will include 11103.
mph
>
> -renee
