Below is the code review feedback on the remaining files, except
manifest-import. I'll send feedback on
usr/src/cmd/svc/milestone/manifest-import tomorrow morning.
Please, make sure you run hg nits and fix *all* copyright years.
Antonello
usr/src/cmd/svc/mfstscan/mfstscan.c
OK
usr/src/cmd/svc/configd/backend.c
flight_recorder_event()
The comments for flight_recorder_event() advises to take a gcore
of configd to look into the events with mdb. Why not extend
usr/src/cmd/mdb/common/modules/svc.configd/configd.c
to display that information?
It could be a follow up RFE if schedule is tight.
So flight_recorder_missed counts failed attempts to grab the
lock, not the number of events overwritten. What kind of errors
are you expecting from a pthread_mutex_t initialized with the
defaults?
The mutex synchronizes access to the counters only? What about
the be_flight_event_t data itself? Doesn't it need to be
synchronized?
You are obviously not expecting configd to have more threads
than MAX_FLIGHT_RECORDER_EVENTS. Are configd number of threads
bounded somehow?
71: either IS_VOLATILE(be) has its logic reversed or the test should
be ((be)->be_ppath == NULL). The comment on line 103 says
be_ppath is /* path to persistent db */. In this case the answer
to IS_VOLATILE(be) should be false. Maybe I am confused...
Please clarify.
1466-1467: It was easier to understand remove_src after I read the
old comment block. However in the new code, I think explaining
sw_back in backend_switch() would do a favor to someone reading the
code.
1758-1761: you use the variable pres only to hold the result of
backend_copy_repository() for the following if. You could just
compare the return value of the function instead.
2672: is FAST_REPOSITORY_DB persistent? if not, why be->be_ppath is
assigned to it? I guess this is because of my interpretation
of IS_VOLATILE(be) and be_ppath.
usr/src/cmd/svc/milestone/emi.xml
OK
usr/src/cmd/svc/shell/netservices.sh
OK
usr/src/lib/libscf/inc/libscf_priv.h
OK
On 03/ 9/10 06:30 PM, Tony Nguyen wrote:
> An updated webrev is generated to reflect the comments we've had thus far:
>
> http://cr.opensolaris.org/~tonyn/emi_mar_08_2010/
>
> The last webrev is still available at
>
> http://cr.opensolaris.org/~tonyn/EMI_webrev
>
> Thanks,
> tn
>
>> Subject: Early Manifest Import - code review request
>> Date: Fri, 05 Feb 2010 14:21:46 -0800
>> From: Tony Nguyen <Truong.Q.Nguyen at sun.com>
>> To: Antonello Cruz <Antonello.Cruz at Sun.COM>, Liane Praza
>> <Liane.Praza at Sun.COM>, Susan Kamm-Worrell
>> <Susan.Kamm-Worrell at Sun.COM>, Jean McCormack <Jean.McCormack at Sun.COM>,
>> smf-discuss <smf-discuss at opensolaris.org>
>>
>>
>>
>> We completed testing and fixed all found bugs. We'd appreciate feedback
>> by Feb 19th as that would allow us to turnaround comments and still make
>> the scheduled integration date.
>>
>> Webrev:
>> http://cr.opensolaris.org/~tonyn/EMI_webrev/
>>
>> Design Doc:
>> http://hub.opensolaris.org/bin/download/Community+Group+smf/smf_design_docs/emidesign.html
>>
>>
>>
>
>
