On Thu, 10 Sep 2009 16:29:12 -0400
Anurag Maskey <Anurag.Maskey at Sun.COM> wrote:
> Michael Hunter wrote:
> > /net/coupe.eng/builds/mph/nwam1_work/webrev
> >
> apologies if comments overlap with those of Renee.
>
>
> libnwam_event.c
>
> these are the only place in the library that write to syslog(), right?
> is this ok, done in other libraries?
I'm not crazy about syslogs coming from any library. But this function
is strange. It just keeps going after errors and I've seen strange
things happen. If we were using dtrace as our main dynamic debugging
tool I'd leave these out and say just use dtrace. But we are not. So
I put them in.
>
> ncu.c
>
> 211, 212: why don't you wrap these two lines around "if
> (ncu->ncu_node.u_if.ipv{4,6})"? and then add
> "ncu->ncu_node.u_if.{dhcp,stateful,stateless}_configured = B_FALSE" to
> the appropriate clauses.
>
> 224-227, 233-239: these lines are redundant like your comment says.
> might be better to remove it completely so that we aren't wondering
> what's going on in the future
>
> 211-240 could be written as:
>
> if (ncu->ncu_node.u_if.ipv4) {
> unplumb;
> dhcp=false;
> plumb;
> }
> if (ncu->ncu_node.u_if.ipv6) {
> unplumb;
> stateless=false;
> stateful=false;
> plumb;
> }
>
> 458-463: can be rewritten as above to avoid unnecessary calls to unplumb.
This is all rewritten.
>
> ncu_phys.c
>
> 1841-1849: doesn't nwamd_ncp_activate_priority_group() bring the start
> the state machine for this NCU?
I don't understand the question.
>
> ncu_ip.c
>
> 387-383: We shouldn't log error if smf_restore_instance() fails. If the
> service isn't it maintenance, smf_restore_instance returns non-success.
> I've seen noise from this in the current builds.
These lines are at ncu_ip.c:898-903? But I agree.
>
> 866-868: I think we should check to make sure that the NCU is not
> already online. Can cause multiple online/up state event being sent to
> listeners.
That shouldn't be done here. If we want state changes to be idempotent
that should be done in nwamd_object_set_state() or supporting routines.
>
> 860, 955: XXX ?
ACCEPT
>
> I am getting some ideas for 11103 going through these changes.
Cool. New webrev in place but I also need to do some more testing.
mph
>
> Anurag
>
