On 02/ 1/10 11:54 PM, Liu, Jiang wrote: > 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. >
Liu Jiang, I thought getopt() returns ":" only if the optstring starts with ":", and in this case the optstring should be ":d:". With the current optstring, I believe getopt() would return "?" for lack of optarg. -tn
