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.
>
> ncu.c, 251-270: Code organization nit: Can you pull the identical
> calls to nwamd_configure_interface_addresses() out of the
> if-else, and just do that first, in all cases?
ACCEPT
>
> ncu_ip.c, 356-57: Code organization nit: do these lines really need
> to be split?
ACCEPT
>
> ncu_ip.c, 450: Do we really want to print a boolean with %d (yeah,
> I know it gets the job done, just seems like poor form)?
ACCEPT
>
> ncu_ip.c, 455-56: This statement isn't strictly correct; we might
> have marked it configured = false, right?
ACCEPT
>
> ncu_ip.c, 592: My favorite cstyle nit, need spaces around the '+'
ACCEPT
>
> ncu_ip.c, 778-782: This comment has me stumped. Which boolean does
> it refer to? Which second argument?
A piece of code I'd removed. The comment is now also removed.
>
> ncu_ip.c, 796-98: I still think this part of the comment needs to
> be more clear. This describes the problem; that problem is solved
> in the code by checking the dhcp_requested booleans in addition to
> whether or not the dhcp flag is set on the interface.
You are right. I don't outline the algorithm in the overall comment,
but I do walk through it in the comments interspersed in the code. Do
I need more overview? I'd rather have details closer to the actual
code.
>
> Really, we could just skip the latter check, right? It should
> pretty much *always* be true: either dhcp is an addrsrc and we
> asked for a dhcp lease, or dhcp is not an addrsrc and we did a
> dhcp inform. Either way, the dhcp flag is set.
yes
>
> ncu_ip.c, 824-26: s/state stateless/not stateless/; and need a close
> paren after DHCP. Might also want to note here that we remove the
> unwanted address by unplumbing its particular lif--otherwise that
> call to nwamd_unplumb_interface() is a little startling.
You parsed that. Wow, my first read was that it was complete gibberish ;)
>
> ncu_ip.c, 837-38: we just want to update this particular address, not
> all of them.
ACCEPT
>
> ncu_ip.c, 839-847: whoa, what's going on here? This seems much more
> drastic than what I was expecting; I thought we were just removing
> one address.
ACCEPT
>
> ncu_ip.c, 860-62: I understand why you put this in, but I don't think
> we should integrate with XXXs in the code; and we're not going to
> change this logic before integration. At minimum, need to remove
> the XXXs. This applies in other places, as well.
ACCEPT
>
> ncu_ip.c, 914-18: "SEP"?? And in the previous sentence, you mean
> you're not sure how to figure out if the lost address was
> stateless or stateful? Probably should say that explicitly.
"Somebody Elses Problem" from Hitchhiker's Guide to the Galaxy,
required reading for any geek ;)
ACCEPT
Michael
>
> -renee
