On 02/19/10 02:55 PM, Tom Whitten wrote: > Thanks for your feedback Antonello. I'm playing recording secretary today, > and below you'll find responses from the three of us. You're welcome ;-)
> > tom > > Antonello Cruz writes: ... >> 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. I've been there myself! :-) ... >> 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. OK, I wanted to be sure you had considered this case. > >> >> 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. OK > >> >> 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? I think the code would be less ambiguous that way. I was confused because I thought I had missed where setemilog could have changed its value since its initialization. You don't need to change the code, but maybe a comment could be of some help here... If you keep the code as it is, would you consider initializing setemilog to B_TRUE instead of one? ... >> >> 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. Or you can put a comment on the top of engine_import() stating that the return value of 1 indicates there is nothing to import. You don't have to change anything, I just wanted to be sure that a return value was not being overlooked > >> >> 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. I was somehow under the impression that if find_manifests() was called on an empty dir it would return -1 and not 0. Please make sure you test this case. ... >> 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. OK, that's what I had in mind... :-) ... >> >> 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. OK. >> >> 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. OK. ... >> 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. I'll wait for the comment :-) ... >> >> 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. That comment was a brain fart, sorry! > >> 15582 - 15589: Are you sure you don't need a default clause? > > Will add. I made this comment because I've noticed (according to the manpages) that scf_pg_delete(3SCF) can fail with an error that is not being handled in the switch statement. Antonello
