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. > > * 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. > > *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. > > 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. > -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? > > - lines: > 519 prop_list->name = strdup(name); > 520 prop_list->valstr = strdup(valuestr); Checks for NULL added... > > Since you are checking for null values in memory allocation situations > in other parts of the code, it would be good to check here as well. I > assume it matters if we cannot strdup the values for this list members > data? > > Ditto line 611 > Check added. > That's it so far. I will review the rest soon. > > sarah > > > Thanks! -evan
