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. > 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 > >
