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?
>>
>> * 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?
>>
>> *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.
>>
>> 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).
>> -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.
>>
>> - 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.
>
thanks,
sarah
*****
>> That's it so far. I will review the rest soon.
>>
>> sarah
>>
>>
>>
>
> Thanks!
> -evan
