jan damborsky wrote: > Hi Ethan, > > I have looked at the updated webrev, I have only nits: > > ict.c > ----- > > 490 target, login ? login : "NULL"); > -> > 490 target, login != NULL ? login : "NULL");
Okay. > > 538 > - according to man page for tmpnam(3C), NULL could be returned > in case of failure. I think we should check for NULL before > tmp_ua is used and return with failure if it is NULL. Okay I'll check for this. > > 565 - might it be mv(1) used instead of cp(1) + rm(1) ? cp(1) preserves the stats of the destination file and mv does not. So I'd rather not have to readjust the permimssions after a mv(1). thanks, -ethan > > > > Other than that the changes look good. > > Thank you, > Jan > > > On 03/26/09 03:26, Ethan Quach wrote: >> Sundar, >> >> I've made the changes and the webrev is updated. >> >> If I can get one more reviewer for this, it would be great. >> >> >> thanks, >> -ethan >> >> >> Ethan Quach wrote: >>> >>> >>> Sundar Yamunachari wrote: >>>> Ethan,* >>>> >>>> usr/src/lib/libict/ict.c: >>>> >>>> *486-489 - The checking of target could be moved before line 481 -- >>>> the target could be null >>> >>> I'll move this up. >>> >>>> >>>> 547-559 - Does it make sense to update the file after completing >>>> both the tasks (changing root and user)? If first one is successful >>>> and second one is not successful, we may have partially updated file. >>> >>> If anything fails here, ICT is going to report a failure which >>> causes an overall install failure anyway, but I see your point. >>> I'll move lines 561-564 up above 547 (making sure it modifies the >>> tmp_ua file). >>> >>> >>>> >>>> *usr/src/lib/libict/ict_test.c: >>>> >>>> *55, 77, 118, 123-124: Are these changes part of this bug fix? >>> >>> Its cleanup from 5554. Somebody forgot to update those when the >>> set_host_node_name() interface was modified so I'm fixing it here. >>> Without these changes, 'make ict_test' fails to build. >>> (I think Joe's got these in his webrev as well, but we'll just >>> have to merge.) >>> >>> >>> thanks, >>> -ethan >>> >>>> >>>> Thanks, >>>> Sundar >>>> >>>> Ethan Quach wrote: >>>>> Can I get a review for this blocker... >>>>> >>>>> >>>>> Webrev: >>>>> ------ >>>>> http://cr.opensolaris.org/~equach/webrev.4215/ >>>>> >>>>> >>>>> Defect: >>>>> ------ >>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4215 >>>>> >>>>> >>>>> >>>>> Tested this fix by installing via the LiveCD and AI, >>>>> with and without a login name specified. All tests >>>>> succeeded as expected. >>>>> >>>>> >>>>> >>>>> thanks, >>>>> -ethan >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
