Susan Sohn wrote: > 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. Thanks Sue. The read_service_data_file issue will go away with our work. I had visions of merge nightmares if you refactored do_create_service.
Jean > > 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 >> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
