Sarah Jelinek wrote: > Hi Evan, >> Sarah Jelinek wrote: >>> Hi Evan, >>> >>> Here are some of my review comments for the source files I listed... >>> >>> source file: installadm.c: >>> >>> -starting at line 174.. it appears we always try to enable the smf >>> service and the return value is simply the difference. why not just >>> make the attempt, then check for the usage issues and exit accordingly? >>> >>> So.. something like: >>> /* >>> * Make an attempt to enable the smf service. >>> */ >>> (void) check_for_enabled_services(handle); >>> service_enable_attempt(instance); >>> ai_scf_fini(handle); >>> >>> then the if (...) statement with exit values. >> >> Ok yes that is better, done. >> >>> >>> Actually, why do we try to enable the service if it is an apparent >>> failure scenario? >> >> This may not be the only AI service and if there are others that >> should be on we need to at least attempt to get the smf service up and >> running. >> > Why? If the installadm command fails, the user will have to run it > again, right? So, what purpose does it serve to bring any other services > online at this point?
I was thinking of the case where there are AI servers already configured. Having one AI service down should not stop all of them from being available. This is there to insure that those that are capable of running (have a status property of on) are available. I think the confusing part here is that the term service is severely overloaded such that it can be had to tell when we're talking about an smf service or in AI service. >>> >>> * service_enable_attempt >>> 334 * Description: >>> 335 * Attempt to enable the designated smf service. >>> 336 * If the service goes into maintenance mode, >>> 337 * return an error to the caller. >>> >>> This is a void function, so you don't return an error. This comment >>> should be updated. Also, calls to this function should use (void). >> >> I need to talk to Jean about this a bit more. >> >>> >>> In the case of enabling an instance or restoring it, you don't wait >>> or check for any confirmation of success. What is the affect of a >>> service not being enabled from this call? >>> >>> -line 327: shouldn't this be an fprintf to stderr? >> >> I don't think it's necessary since this is coming from the installadm >> command but it definitley won't hurt anything to change that to >> fprintf. done... >> >>> >>> *check_for_enabled_services() >>> -line 324, while the command is waiting to ensure that the service >>> went in to maintenance, does installadm appear to just hang? >> >> Yes it can. >> > That's not a good user experience. Is there a way we can add text that > will appear that says configuring smf service or something like that. > Frank L, do you have any suggestions on this? The only time I've seen this was when smf was behaving badly and it took awhile to go into maintenance. However I agree that we should at least print something out saying what's going on... >>> >>> *service_enable_attempt() >>> -Why do we not wait or verify the service has been enabled here, but >>> we do in check_for_enabled_services when we transition it to >>> maintenance? >> >> I'll need to confirm why with Jean on this one. >> > Ok. Seems to me that we need to be consistent and in the event of a > failure of transitioning a service to a different state we should check > and handle appropriately. agreed. >>> >>> source file: installadm-common.sh >>> -Why is SED defined and then not used? >> >> Something else I missed, removed... >> >>> >>> source file: ai_utils.c >>> -free() will do the right thing if value is null. no need to check >>> for null prior to calling free. >> >> The call to free does the right thing internally if what we're >> attempting to free is NULL. >> > Right, that's my point. So, why are we checking for a NULL value before > calling free? Is there any case in the code that the pointer would have > been free'd but not nulled out? If it always is NULL or a valid memory > value then we don't need to check for if (XXX != NULL). Sorry I should have said that I've removed these checks for NULL. They got in there through a due to a bad habit of doing this... >>> -I assume the scf_xxx_destroy() functions need a non-null value to be >>> passed in? That is they don't do the right thing if the value is null? >> >> According to the man pages they should do the right thing but... >> Am I missing checks for things I should be checking before these calls? >> > No, I am suggesting you don't need to check for the if (XXX != NULL) if > the scf_xxx_destroy functions will handle NULL values appropriately. I don't trust the scf commands enough not to have this check in here. I've seen some that don't do things correctly and we end up core dumping. Thanks! -evan
