Hi Joe. 2nd iteration. I'll double check when the revised webrev comes out.
On 01/13/09 06:53, Joseph J VLcek wrote: > Jack, > > Thank you for the review feedback. My comments are in line below. > > I will give others a chance to comment and after testing I will post > an updated webrev. > > Thanks, Joe > x23116 ~= 781.442.3116 > > Jack Schwartz wrote: > >> Joseph J VLcek <Joseph.Vlcek at Sun.COM> >> caiman-discuss <caiman-discuss at opensolaris.org> >> <sundar.yamunachari at Sun.COM> >> Tycho Nightingale <Tycho.Nightingale at Sun.COM> >> William Schumann <William.Schumann at Sun.COM> >> Jan Damborsky <Jan.Damborsky at Sun.COM> >> >> Hi Joe. >> >> Please update the bugs with more info on your changes. (This would help >> us with code review too.) Currently 6074 talks only of a buffer passed >> to KIOCLAYOUT, which sounds like a small change to one file (line 701, >> ict.py), but there are many files in this code review and many other >> things affected. 4163 has only info that it was made not a blocker and >> then a blocker again. (This may be OK as it is a blocker bug, not sure.) >> > > I have updated the bug descriptions. > Thanks for updating the bugs. Looks like set_Solaris_partition_active needs to be added to the bug in the not-needed-for-SPARC list. >> ict.c: >> >> 3: put inside #ifdef __sparc >> >> I would like to see ict_installboot() split out into >> ict_installboot_sparc and ict_installboot_x86, and ict_installboot >> itself as a wrapper routine. The multiple #ifdef __sparc declarations >> really make it hard to follow the flow. Separating the two routines >> would make them more clear and more maintainable. >> > > I disagree with having the 2 routines. It would result in both having > some common code. Having common code in multiple places can lead to > changes to that code not getting done in both. > Yeah... I guess one can argue either way; OK... > >> Alternatively, confirm input args (740-746) before calling uname. Then >> you can have one less #ifdef __sparc. Besides, it makes sense to check >> args before doing stuff in the routine. >> Was this done then? >> ict_test.c: >> >> 87: I know there is a bug filed to use proper eeprom device names for >> SPARC, but if I understand correctly, this message is inconsistent with >> the way things are currently. I would like to see the example device >> listed in usage() to be consistent with the actual device names that >> need to be specified. Use an #ifdef __sparc here. >> > > The way it is is correct. > > After the bug to use eeprom is fixed ICT ict_installboot will continue > to accept a Solaris style device name on both SPARC and non-SPARC > systems. On SPARC it will convert it to an OBP format to be pass that to > eeprom. > I know what is there will be correct eventually, but it is inconsistent with the device name format recognized currently. What I was suggesting was that, until the device is fixed to use cwtxdysz names, change the usage to reflect what device format is currently accepted. I guess this is OK since this is for a development build, not a polished product; and since this will be very temporary... I'm protecting the end user who may look to usage() for the proper format of calling the command. As is, usage will steer that user in the wrong direction. > >> ict.py: >> >> I know you didn't put the comments in between 650 - 652, but can you >> please see if a bug has been filed to resolve these issues. Please make >> sure that the bug states to remove remove_bootpath() as part of that bugfix. >> > > I am not sure. I'm researching this. > OK, thanks. > >> 748: It seems more appropriate to me to return failure rather than >> success. What if the caller expected something and got nothing? It is >> clearer to modify any SPARC would-be-callers to not call this function >> if it is inappropriate to do so. The call to this function can be put >> inside #ifdef __sparc. >> > > Fixed to issue a message and return error: ICT_INVALID_PLATFORM > Beautiful. Thanks for this one and the rest too. Thanks, Jack > >> Thanks, >> Jack >> >> On 01/12/09 14:22, Joseph J VLcek wrote: >> >>> * Please review the changes for SPARC support for ICT and a fix for a >>> bug which was found while testing on SPARC. >>> >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4163 >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6074 >>> >>> * webrev >>> >>> http://cr.opensolaris.org/~joev/sparc_ict/ >>> >>> * The modules affected and tested: >>> >>> Orchestrator >>> ICT, Library and Python >>> >>> * Testing done on SPARC: >>> >>> On SPARC, with help from Jan a full AI install was performed. >>> >>> * SPARC Results: >>> All ICT completed successfully and system booted >>> >>> * Testing done on x86 (to confirm no regressions:) >>> >>> [1] Booted LiveCD image on live hardware HP Pavilion dv5000 >>> [2] using ld_preload to load updated liborchestrator >>> [3] Installer run >>> >>> * Results: >>> All ICT completed successfully and system booted >>> >>> * Description: >>> >>> This code change is to provide SPARC support in ICT. >>> >>> * Outstanding issue: >>> >>> ICT set_Solaris_partition_active needs to be updated to convert a >>> Solaris device name to an OBP format. >>> >>> Bug 6080 has been filed to track this. >>> >>> >>> >>> Huge thanks for help to Tycho Nightingale, Jan Damborsky and William >>> Schumann. >>> >>> >>> >>> Thank you. >>> Joe >>> _______________________________________________ >>> 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 > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090113/76a5c232/attachment.html>
