I trust you will run hg nits before pushing, so I'll not comment about
and copyright year update ;-)
I haven't finished all files I am planning to review, but I thought it
would be useful to send feedback of the files I have finished.
usr/src/cmd/svc/common/manifest_find.c
95: why did you choose to grow the arry arithmetically instead of
geometrically?
313-315: why you test inof->mi_prop for NULL but don't test
info->mi_path?
usr/src/cmd/svc/common/manifest_find.h
OK
usr/src/cmd/svc/milestone/manifest-import
39: uses SMF_FMRI to check if it is EMI or LMI. The design doc is
obviously out-of-sync. Please update the design doc.
usr/src/cmd/svc/startd/fork.c
689: All cookie definitions are together in startd.h It would be
easier to maintain code if we keep it this way. I suggest
moving the macro for EMI_COOKIE to startd.h
You could also move the other #defines if you think it is
appropriate.
725: I know it is almost impossible to hit SCF_ERROR_DELETED here,
but shouldn't the same action for SCF_ERROR_NOT_FOUND be taken?
859:
870: Why are you using uu_warn() instead of log_framework()?
These messages don't need to be localized (gettext)?
953: Isn't setemilog always true here?
usr/src/cmd/svc/startd/startd.c
784: It may be worth to update the EMI comment to point out that it
starts *after* the fork_configd_thread
usr/src/cmd/svc/startd/startd.h
OK
usr/src/cmd/svc/svccfg/Makefile
usr/src/cmd/svc/svccfg/svccfg.h
270: if service_manifest_t is used only in svccfg_libscf.c why did
you split the typedef and structure definition? Couldn't both
go in svccfg_libscf.c?
usr/src/cmd/svc/svccfg/svccfg.l
OK
usr/src/cmd/svc/svccfg/svccfg.y
OK
usr/src/cmd/svc/svccfg/svccfg_engine.c
736: why we return 1 if there were no manifests to import? Is that
an error? from svccfg.y only -2 generates error.
757: What is the case where you get manifests == NULL here?
Shouldn't this be an assert()?
engine_cleanup(): It seems you could have used scf_walk_fmri() in
here. Have you considered it? If so, why have you chose not to use
it?
usr/src/cmd/svc/svccfg/svccfg_help.c
no help for cleanup?
usr/src/cmd/svc/svccfg/svccfg_libscf.c
in create_instance_list():
14425: Perhaps the error message if scf_iter_next_instance() fails
should give a bit more of info, so that it is easier to track
where the error occurred.
14453,14475: Why is SCF_ERROR_DELETED Not treated the same as
SCF_ERROR_NOT_FOUND for this purpose?
disable_instance()
leaks fmribuf.
It will also consider a service that failed the stop method
(and placing it into maintenance as in 6401708) as successfuly
operation. Is this the desired behavior?
create_manifest_tree():
14650: for (c = 0; dirs[c]; c++) seem a better idiom here...
14655: The error msg use LIBSVC_DIR, but should use dirs[c]
check_manifest_history()
14735: if *(mfsthist_start + st.st_size) != '\0' is true but the
mmap succeeded, don't we need to munmap()?
14748,14750: mfsthist_ptr is used only in the test. Would it
make sense to get rid of it an place the strstr() in the if
statement?
teardown_service()
14769: where is the output of this going to?
check_instance_support()
The comment block states that the funciton calls return 0 for no
instances *or* errors. However, the code suggests -1 is returned
on error. Please update the comment block.
lscf_service_cleanup():
It seems that if IGNORE_VAR is true (i.e. emi), we bail at
lscf_service_cleanup() unconditionally. Would it make sense not
even doing service iteration in engine_cleanup() at all if
IGNORE_VAR is set, or have I missed something?
The array 'mpvarry' grows arithmetically, is there a reason why
it does not grow geometrically?
15379 - 15381: Please sync the code and the comment.
Comment refers to 'list of service instances is NULL'
but tests the parameter activity
15339 - 15366:
15396 - 15409:
15453 - 15468:
All these loops initialize an index to 0, test a condition
and increment the index. It seems that a for loop would be a
better idiom here...
lscf_hash_cleanup():
15580: since scf_pg_delete() may fail with SCF_ERROR_NOT_SET,
shouldn't we call scf_pg_update() first?
15582 - 15589: Are you sure you don't need a default clause?
usr/src/cmd/svc/svccfg/svccfg_xml.c
2973: if scf_handle_bind() fails, you'll leak the handler.
I would suggest to write this function a bit differently:
initialize all scf_*_t to NULL at declaration and
rc = SCF_FAILED. Use a label (out:) before the scf_*_destroy()
calls, set rc = SCF_SUCCESS befor the label (out:) and use
goto out in case of any failures you're testing in the function.
scf_*_destroy() are practically no-ops for NULL arguments.
usr/src/lib/libscf/inc/libscf.h
Standard services definitions
SCF_SERVICE_EMI and SCF_SERVICE_FS_MINIMAL are instances
definitions. I've seen enough misunderstanding between service
and instance that I think we should minimize abusing the
terminology. The precedent set by SCF_SERVICE_CONFIGD and
SCF_SERVICE_STARTD should not serve as an excuse to keep abusing
terminology.
What are the arguments for not using SCF_INSTANCE_EMI and
SCF_INSTANCE_FS_MINIMAL ?
I will send feedback of the files below later this week or early next
week
Antonello
usr/src/cmd/svc/configd/backend.c
usr/src/cmd/svc/mfstscan/mfstscan.c
usr/src/cmd/svc/milestone/emi.xml
usr/src/cmd/svc/profile/generic_limited_net.xml
usr/src/cmd/svc/profile/generic_open.xml
usr/src/cmd/svc/prophist/prophist.SUNWcsr (deleted)
usr/src/cmd/svc/prophist/prophist.c (deleted)
usr/src/cmd/svc/shell/manifest_cleanup.ksh
usr/src/cmd/svc/shell/netservices.sh
usr/src/lib/libscf/inc/libscf_priv.h
On 02/ 5/10 02:21 PM, Tony Nguyen wrote:
> 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
>
>
> The number of modified files dramatically decreased since changes to
> SVR4 packages are obsoleted. Susan and Jean, would you mind review the
> IPS packaging changes and the Makefiles?
>
> Antonello and Liane, you guys still have the SMF changes :)
>
> Thanks in advance for your help,
> -tn
