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.)
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?
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.
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.
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.
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.
653: Is this comment needed? Please delete if not.
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.
813-816: ditto
898-901: ditto. If this routine isn't going to do anything, it
shouldn't return success that it did something.
957-960: ditto. SPARC shouldn't call this routine in the first place...
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.
1429: typo: proplerty -> property
1433-1437: ditto about returning failure instead of success
1515-1518: ditto
1542-1545: ditto
1567-1570: ditto
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
>
