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