Hi Tony,
Thanks for your support! Please refer to inline comments below.
Truong.Q.Nguyen at Sun.COM <mailto:Truong.Q.Nguyen at Sun.COM> wrote:
> On 01/28/10 01:33 PM, Michael Corcoran wrote:
>> Hi All,
>>
>> I'm looking for a couple of reviewers to review the smf related
>> changes for the Intel CPU/Memory hot add project since we're not
>> experts in this area :)
>>
>> I'd like to get the following files reviewed:
>>
>> usr/src/cmd/acpihpd/acpihpd.xml
>> usr/src/cmd/acpihpd/svc-acpihpd
>> usr/src/cmd/svc/profile/platform_i86pc.xml
>>
>> It'd also be great if you could look at the implementation of the
>> daemon that it is trying to start to make sure it handles the daemon
>> exiting correctly. The internals of what the daemon does are
>> irrelevant in this case :)
>>
>> usr/src/cmd/acpihpd/acpihpd.c
>>
>> The daemon should be running on any system which supports this new
>> hot add capability and by default will run on all x86 platforms and
>> disable itself if the platform does not support this feature.
>>
>> I'd like to make sure that we're following secure by default and that
>> we have limited privileges correctly. We must run as root as we're
>> dependent upon syseventd which requires this.
>>
>> The webrev is at:
>>
>> http://cr.opensolaris.org/~mec/smf_review/
>>
>> I only included the acpihpd makefiles in case anyone is interested,
>> but don't need to have those reviewed/
>>
>> If you'd like to see this in the context of the larger changes, let
>> me know and I can point you at a webrev of that as well.
>>
>
> We discussed offline with respect to service's enabled state. The
> privileges definition looks sensible to me and I assume you used
> 'ppriv' to verify the daemon runs with the set privileges.
>
> acpihpd.xml - Set instance's enabled state to 'enabled as discussed.
>
> platform_i86pc.xml - No longer necessary
>
> svc-acpihpd - It's stylistic opinion but the code here can be
> simplified to:
>
> /usr/sbin/prtconf -D /devices/fw/acpidr at 0 > /dev/null 2>&1 if [ $?
> -ne 0 ]; then svcadm disable -t $SMF_FMRI
> sleep 5&
> exit $SMF_EXIT_OK
> fi
>
> $ACPIHPD && exit $SMF_EXIT_OK || exit $SMF_EXIT_ERR_FATAL
Will make above changes.
> acpihpd.c - I reviewed only the relevant area and the exit codes are
> appropriate. Just one quick nit:
>
> 85 - 102: Would you get ':' if optstring doesn't start with a ':'?
The format string "d:" states that user may pass an option "-d" with an
argument, say "acpihpd -d 1". If user only missed the argument for "-d"
option, say "acpihpd -d", getopt() function returns a ":" to signal that
the argument is missed.
Thanks!
>
> -tn
Liu Jiang (Gerry)
OpenSolaris, OTC, SSG, Intel
