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"); 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. 565 - might it be mv(1) used instead of cp(1) + rm(1) ? 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
