Sundar Yamunachari wrote: > Evan, > > *usr/src/cmd/installadm/installadm-common.sh*: > > 35: SED is defined but unused while sed command is used in 247 and381.
I had originally removed line 35 based on other comments bug I've added it back in and changed lines 247 and 381 to use it. > > *usr/src/cmd/installadm/installadm.c: > > *174-191: Why the actions in if and else case are same? If these actions > (179-181) need to be taken for all cases, save the status from the line > 174 and take these actions before returning based on status. fixed. > > 327-329: Can you move the message to installadm.h in the same format of > the other installadm messages? I wasn't going to since this is only used in one place but since there have been several people with the same comment I've moved it. Sue mentioned another message we print out that isn't in the header, I'll move that one as well. > > 337: The comment indicates that an error is returned where as the > function is a void function. Yes this will be corrected. > > *usr/src/cmd/installadm/installadm_util.c: > > *130: Change the comment from service data files to SMF properties group fixed > > 209: pg_name is not checked for NULL. Why? It's probably best if we do this here. However the call to ai_read_all_props_in_pg() will catch this as well. I'll add the check in get_service_props as well. > > 217: This comment still refers to the service data file. Fixed > > *usr/src/lib/libaiscf/Makefile:* > > 42: This line is commented out. If it is not needed, remove it. removed. > > - Just curious: The file ai_create.c contains only function > ai_delete_install_service(). So why it was not called ai_delete.c? Originally it had an ai_create_install_service() however we found that wasn't needed so it was removed but the file was not renamed. Renaming it isn't a problem if that would make more sense. > > *usr/src/lib/libaiscf/ai_utils.c:* > > 79: Should be AI_SUCCESS (for success) > 80: should be ai_errno_t (for failure) both fixed. > 126: If the property group already exists, can an error like "property > group exists already" be returned instead of AI_CONFIG_ERR if the property group exists already that is a config error. ;-) I've added a new error and error string for this instance. > 295: Check whether strdup is successful fixed > 372-375: In some of the error cases (352, 357 and 365), the > start_transaction was successful (341). Do you have to do a > end_transaction() here? > 422: Same comment as the previous one. You're correct this is a problem. We're missing an about transaction function. I've added that and call it here if there is an error. void ai_abort_transaction(scfutilhandle_t *handle) { if (handle->trans != NULL) { scf_transaction_reset_all(handle->trans); scf_transaction_destroy(handle->trans); handle->trans = NULL; } } > 468: It is better to check whether name and valuestr are NULL here > (rather than at 486) since you can return sooner with error. fixed > 504, 506, 519, 520: Check return value for strdup. fixed > 501-525: If there is failure after allocating space for some of the > properties, is it worth while to free up partially filled prop_list? Yes it should be freed on error. I've fixed this along with the check for the return value from strdup mentioned above. > 598, 612: Check return value for strdup Done > 593-615: if there is failure after allocating space for some of the > property groups, is it worth while to free up partially filled pg_list? Same fix as comment from lines 501 - 525. > 634-635: The comment indicates that an error is returned where as the > function is a void function. Fixed. > 790: change to gettext("unknown Error") fixed. Thanks for the comments! -evan > > > Thanks, > Sundar > > **Sundar Yamunachari wrote: >> Evan, >> >> I will look at installadm changes. If it goes faster then I will >> look at the rest of the code changes. >> >> - Sundar >> >> Evan Layton wrote: >>> My apologies for the delay. I have fixed the problems with the >>> mis-merge, sanity tested the changes and updated the webrev. I've >>> listed out the files and split things up between the library and >>> installadm. Just pick a subset of files and let me know what you're >>> looking at som we make sure everyting gets covered. >>> >>> And thanks again to Sue for catching the mis-merge!!! >>> >>> Thanks! >>> -evan >>> >>> installadm files: >>> usr/src/cmd/installadm/Makefile >>> usr/src/cmd/installadm/installadm-common.sh >>> usr/src/cmd/installadm/installadm.c >>> usr/src/cmd/installadm/installadm.h >>> usr/src/cmd/installadm/installadm_util.c >>> usr/src/cmd/installadm/server.xml >>> usr/src/cmd/installadm/setup-service.sh >>> usr/src/cmd/installadm/svc-install-server >>> usr/src/pkgdefs/SUNWinstall-libs/prototype_com >>> >>> libaiscf Library files: >>> usr/src/lib/Makefile >>> usr/src/lib/libaiscf/Makefile >>> usr/src/lib/libaiscf/ai_create.c >>> usr/src/lib/libaiscf/ai_trans.c >>> usr/src/lib/libaiscf/ai_utils.c >>> usr/src/lib/libaiscf/libaiscf.h >>> >>> >>> And if anybody is REALLY board here's the test files... >>> >>> optional files: >>> usr/src/cmd/installadm/test/installadm_test.c >>> usr/src/cmd/installadm/test/manual_tests >>> >>> >>> Evan Layton wrote: >>>> Sue pointed out that the webrev shows that I have a mis-merge so if >>>> you're looking at the changes please stop. I'll get the mis-merge >>>> fixed and send out a new webrev as soon as possible. >>>> >>>> Thanks, >>>> -evan >>>> >>>> >>>> Evan Layton wrote: >>>>> >>>>> We need to get the fix for 7218 code reviewed. There are around >>>>> 2000 lines of changes and new code so we'll need volunteers that >>>>> can take sections of the code to look at. I'm sending this out now >>>>> so the link is out there but I'll want to get things divided up >>>>> tomorrow morning and get folks looking at this tomorrow. >>>>> >>>>> The bug is 7218 >>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7218 >>>>> >>>>> Webrev: >>>>> http://cr.opensolaris.org/~evanl/7218/ >>>>> >>>>> Thanks! >>>>> -evan >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > >