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. > > Here are my per-file comments: > > Makefile: > > OK, but I'm curious what file is missing that the object search path > needed to be extended? be bits. ict_test.c failed to compile because it wasn't picking up some be bits. > > 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. > 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. > > ict_api.h: OK > > ict_private.h: OK > > ict_test.c: > > 43: nit: use INSTALLBOOT not INSTALLGRUB as the routine is now > ict_installboot, not ict_installgrub. Fixed > 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. > 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. > 653: Is this comment needed? Please delete if not. done > > 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 > > 813-816: ditto Fixed > > 898-901: ditto. If this routine isn't going to do anything, it > shouldn't return success that it did something. Fixed > > 957-960: ditto. SPARC shouldn't call this routine in the first place... Fixed > > 1153-1156: ditto. If I were to guess, I would say that the routines > where I have made this comment are all being called from the same > place. If this is true, just put them all inside an #ifdef __sparc. Fixed > > 1429: typo: proplerty -> property Fixed > > 1433-1437: ditto about returning failure instead of success Fixed > > 1515-1518: ditto Fixed > > 1542-1545: ditto Fixed > > 1567-1570: ditto Fixed > > If there isn't time now for changing the returns to failures and just > not calling the routines from SPARC, please file a bug so this doesn't > get dropped and I'll be OK with the code as is for now. > > perform_slim_install.c: OK > > 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
