Thanks for your feedback Antonello. I'm playing recording secretary today, and below you'll find responses from the three of us.
tom Antonello Cruz writes: > > 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? Good idea. We'll make the change. > 313-315: why you test inof->mi_prop for NULL but don't test > info->mi_path? Tom went for coffee between writing those too frees. We'll make them consistent. > ... > 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. Yes. The design doc needs to be updated. > > 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. Agreed. EMI_COOKIE and EMI_LOG will be moved to startd.h > > 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? In emi_set_state(), we set the service's state if it exists. The deleted scenario is treated as an error as nothing should be deleting EMI service this early in boot. > > 859: > 870: Why are you using uu_warn() instead of log_framework()? > These messages don't need to be localized (gettext)? I believe the intent is to explicitly tell users that EMI is disabled or improperly deleted by printing to stderr. log_framework() only logs to stdout if svc.startd runs in debug mode. Most uu_warn messages in this file aren't localized so we were trying to be consistent. > > 953: Isn't setemilog always true here? setemilog is set to signal whether to configure method log. Would you prefer int setemilog; .. .. setemilog = emi_set_state(RESTARTER_STATE_OFFLINE, B_TRUE); which is simpler but doesn't show that we're using 'setemilog' to determine whether to set the log? > > > 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 Yes, that would be helpful information. > > ... > > 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? At one point this was used in multiple files, but now is not. Will make the recommended change. > ... > > 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. This falls into the category of preserving the behavior of the existing code. Take a look at lines 575-582 of the old code. mhash_test_file() is called to check the hashes for the manifest file. If the hashes are OK and there is no work to be done, mhash_test_file() returns MHASH_RECONCILED and this value is returned to the caller of engine_import(). MHASH_RECONCILED == 1, so the EMI code also returns 1 if there is no work to be done. It is not an error if there is no work to do. That's the history. As you point out, though, nobody looks to see if 1 is returned, so we could just as easily return 0 to indicate there is no error and be more consistent with the rest of the function. > > 757: What is the case where you get manifests == NULL here? > Shouldn't this be an assert()? It happens if find_manifests() is called on line 716 with a directory that contains no manifests that need to be imported. See the comments for the find_manifests() function. An assert would not be appropriate. > > 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? > Will update to use... There was really no other reason for coding the way it was other than I wasn't aware of the scf_walk_fmri, and wanted to work with the scf_service_t which the iterations seemed the most direct way to get that. > usr/src/cmd/svc/svccfg/svccfg_help.c > no help for cleanup? We'll treat it like delhash. You'll be able to do svccfg help cleanup, but it won't show up in svccfg help. > > 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. Will update the message > > 14453,14475: Why is SCF_ERROR_DELETED Not treated the same as > SCF_ERROR_NOT_FOUND for this purpose? A last-import snapshot should never be "deleted" if it is something really odd is going on. But with that said I don't have a problem switching the code as the instance will still be skipped and left in tact which is ultimately what we want to do. > > 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? Yes, in fact the maintenance scenario is a very likely scenario in the case where binaries do not exist. But in this case we are circumventing the 6401708 issue as we are using a timer to watch for the service to go out of enabled state. If it does not then we will just give up and destroy the instance. Really all we are attempting to do here, is give a service/instance a chance to cleanly exit before we destroy it. If it does not in a reasonable amount of time then we will just destroy the service/instance. > > 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] Will change each of these. > > check_manifest_history() > 14735: if *(mfsthist_start + st.st_size) != '\0' is true but the > mmap succeeded, don't we need to munmap()? Yes... will fix. > 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? Was using for printing development/debug information, but will change as this is no longer needed. > > teardown_service() > 14769: where is the output of this going to? error messages are going through uu_warn, but the status of deleting the service goes to standard out to be logged or consumed by the caller as the caller sees fit. > > 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. Will update the comment. > > 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 IGNORE_VAR is pared with the second var check so that if that check is false (in other words the manifest is in /var) then if the IGNORE_VAR variable is set the piece is ignored. The grouping can be a bit confusing so will add a comment. > > The array 'mpvarry' grows arithmetically, is there a reason why > it does not grow geometrically? Will update... > > 15379 - 15381: Please sync the code and the comment. > Comment refers to 'list of service instances is NULL' > but tests the parameter activity Move the comment to the instance check. > > 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... Will change to for loops. > > lscf_hash_cleanup(): > 15580: since scf_pg_delete() may fail with SCF_ERROR_NOT_SET, > shouldn't we call scf_pg_update() first? Not exactly sure what this buys, or not really sure how this affects things. scf_pg_update() can return the same error. > 15582 - 15589: Are you sure you don't need a default clause? Will add. > > 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. Will rewrite this function - simply put, the code logic isn't changing (much) more the order in which things are created. > > 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 ? Change is good, will update. ... > > 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 > _______________________________________________ > smf-discuss mailing list > smf-discuss at opensolaris.org
