On 03/16/09 11:47, Joseph J VLcek wrote: > Susan Sohn wrote: >> Hi Joe, >> >> On 03/14/09 03:53, Joseph J. VLcek wrote: >>> Sue Sohn wrote: >>>> Please review the changes for: >>>> >>>> 7388 create-service not reusing port number on a disabled service >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7388 >>>> >>>> and >>>> >>>> 7275 /etc/netboot/wanboot.conf has wrong permissions >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7275 >>>> >>>> which are posted at: >>>> >>>> http://cr.opensolaris.org/~sohn/7388_7275 >>>> >>>> Thanks, >>>> Sue >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> usr/src/cmd/installadm/installadm_util.c >>> ---------------------------------------------------------------- >>> >>> Suggestion: >>> >>> 386 if (access(path, F_OK) != 0) { >>> 387 return (B_FALSE); >>> 388 } >>> >>> >>> I would rather see the call to access be done in function >>> read_service_data_file() >> >> Any particular reason? I would actually prefer to leave it where it >> is, since why try to read the file with read_service_data_file unless >> you know it exists? > > I think this would make read_service_data_file() more able to stand on > it's own. Early in read_service_data_file() it should validate it's > arguments. Even if this includes validating if "path" is accessible. > > If the issue is that you simply don't want the error issued at line > 390/391 then you could check access(path) only if remove_service_data() > fails and add a comment indicating why the error message is not displayed > > e.g.: > 390 if (read_service_data_file(path, data) != B_TRUE) { > 391 (void) fprintf(stderr, > MSG_READ_SERVICE_DATA_FILE_FAIL, > 392 path); > 393 return (B_FALSE); > 394 } > 395 return (B_TRUE); > 396 } > 397 > > To: > > if (read_service_data_file(path, data) != B_TRUE) { > if (access(path, F_OK) == 0) { > (void) fprintf(stderr, MSG_READ_SERVICE_DATA_FILE_FAIL, > path); > return (B_FALSE); > } > } > return (B_TRUE); > > > Or update read_service_data_file() so it returns an enum indicating why > it failed. > > Joe
Joe and Sanjay, I am going to withdraw this code review for now. When Jean and Evan's SMF changes are done, I will revisit fixing this problem. Sue >>> usr/src/cmd/installadm/installadm.c >>> --------------------------------------------------------- >>> Looks OK. >>> I did see Sanjay's comment about do_create_service() being large and >>> deeply nested. >>> I agree addressing would be good and would have made reviewing the >>> code easier. >> >> I agree, however as I said in my response to Sanjay, I would like to >> open a new bug to track that work. >> >> thanks, >> Sue >
