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.
Actually, why do we try to enable the service if it is an apparent
failure scenario?
* 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).
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?
*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?
*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?
source file: installadm-common.sh
-Why is SED defined and then not used?
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.
-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?
- lines:
519 prop_list->name = strdup(name);
520 prop_list->valstr = strdup(valuestr);
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
That's it so far. I will review the rest soon.
sarah
