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
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 ':'?
-tn
