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.
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?
ncu_ip.c, 356-57: Code organization nit: do these lines really need
to be split?
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)?
ncu_ip.c, 455-56: This statement isn't strictly correct; we might
have marked it configured = false, right?
ncu_ip.c, 592: My favorite cstyle nit, need spaces around the '+'
ncu_ip.c, 778-782: This comment has me stumped. Which boolean does
it refer to? Which second argument?
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.
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.
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.
ncu_ip.c, 837-38: we just want to update this particular address, not
all of them.
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.
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.
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.
-renee
