On Fri, 28 Aug 2009 15:55:27 -0400
"Anurag S. Maskey" <Anurag.Maskey at Sun.COM> wrote:
> The README definitely needs to be expanded/added with the QUEUE_QUIET
> stuff and the event handling logic. I think I know how it works, but
> not entirely sure.
DEFER (I'll probably get it written before I push but I don't want it
to hold up the functionality).
>
>
> ncp.c
>
> Why can't nwamd_ncp_deactivate_priority_group_all() call
> nwamd_ncp_deactivate_priority_group() with lower priority group
> numbers. same logic in two places.
ACCEPT
>
> bzero()ing the nwamd_ncu_check_walk_arg and then filling in the values
> is more elegant.
I'm not sure I agree that it is more elevant and it isn't correct if
the structure contained pointers (although that is mostly a historical
fact now). Just 'cuz something is 0 doesn't mean that isn't relevant.
Also you might want to show which ones you are filling in and why. But
in this case I agree.
BTW, it isn't bzero(). That is old speak. We like to churn
identifiers every few decades.
ACCEPT
>
> nwamd_ncp_check_priority_group() doesn't find priority-group higher than
> the one passed in. Is it supposed to always start the search at 0?
ACCEPT (I've changed the comment)
>
>
> events.c
>
> shouldn't the timed condition checking also go though the queue
> quieting? It and the triggered condition checking can stomp on top of
> each other.
ACCEPT
>
>
> ncu.c
>
> 1187, 1191, 1637: unnecessary { ... }
This isn't a big deal to me. I often put them in if I think there
might be reason to add code in those blocks later. In this case I've
removed them from the simple conditionals and left them in the if/else
as I think it looks better that way.
>
> ncu_ip.c:697 smf_restore_instance() before smf_refresh_instance() in
> case network/location is in maintenance.
ACCEPT
>
> do_dhcp_release() can be static.
ACCEPT
>
> 688-702 duplicates what follows in 722-738.
Those lines don't even look close to me. On the next webrev give me
the line numbers and starting/ending line contents so I can match them
up.
>
> at 714, is a "return;" missing? otherwise the state changes to online/up
> in 722.
This one also.
>
>
>
> ncu_phys.c
>
> comment 1744-1748 seems appropriate at 1759
ACCEPT
>
> why is 1774-1776 moved out from 1798? We don't have NCUs with lower
> priority (ie bigger numbers) going to up state.
>
> also, why 1830-1832 outside of priority group checking?
I need to investigate and write more on these (and almost certainly
comment). FWIR the issue was that we would lose state for something
that we later needed state for when we were transitioning to it. But I
have a dinner date so I'll have to write that later :)
mph
>
>
> Anirag
>
