Hi Tony, >>> 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. >> > 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.
I have read the man page for getopt() again and you are right. I will change the optstring to ":d:" to get the right behavior. Thanks! Liu Jiang (Gerry) OpenSolaris, OTC, SSG, Intel Truong.Q.Nguyen at Sun.COM <mailto:Truong.Q.Nguyen at Sun.COM> wrote: > 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 Liu Jiang (Gerry) OpenSolaris, OTC, SSG, Intel
