Sundar Yamunachari wrote: > Susan Sohn wrote: >> Sundar, >> >> Thank you very much for the review. See below. >> >> On 03/06/09 09:25, Sundar Yamunachari wrote: >>> Susan Sohn wrote: >>>> Please review the changes for: >>>> >>>> 6128 installadm reuses /var/ai data and screw up manifest >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6128 >>>> >>>> and >>>> >>>> 7122 installadm stop kills /usr/bin/dns-sd for all services of >>>> similar name >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7122 >>>> >>>> which are posted at: >>>> >>>> http://cr.opensolaris.org/~sohn/6128_7122 >>>> >>>> Thanks, >>>> Sue >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> installadm.c: >>> >>> 468: Can you add a comment on why normalize_service() is needed for >>> service_name? >> >> done >> >>> 708, 1019, 1119: Printing the service name along with txt record is >>> better >> >> In all of those cases, the user has specified the service name on the >> command line, so it seemed redundant to also print the service name. I >> can put it in if you feel strongly. > I feel that the port number is something the user doesn't know. > txt_record is what we create internally. It doesn't make any sense for > the user. If you add the service there, there will be some correlation. > > - Sundar
Hi Sundar, I have made the changes and updated the webrev. Sue >>> 862-867: Should change the status only if we can successfully stopped >>> the service. Move these lines after 878 >> >> I originally had it there, but moved it up. My reasoning was that, >> looking at setup-service, remove_service returns non-zero if the >> service is not running. If the service is already not running, then it >> is appropriate to set the status to off anyway. So moving those lines >> up, to ensure the status was set to off, seemed better so that the >> file stayed consistent with the actual status. Otherwise, it would be >> possible to end up with the service not running but the file status >> still set to on. >> >>> installadm_util.c: >>> 39: get_a_free_tcp_port() declared in this file and installadm.c >>> (line 54). Can you move this declaration to installadm.h? >> >> done >> >>> 43: installadm_system() declared in this file and installadm.c (line >>> 55). Can you move this declaration to installadm.h? >> >> done >> >>> 141: Can You could ombine these lines to one return statement such as: >>> return(read_service_data_file(path, data)); >> >> After asking you privately, it turned out you meant line 380. >> I will combine them. >> >> Sue >> >>> - Sundar >>> >>> >> >
