Thanks Jack. I took you up on:
> .. or if you don't because you > are too short of time, I'm a happy camper. I'll rework this later as I have a different solution which would require changes in more places. Joe Jack Schwartz wrote: > Hi Joe. > > Looks really good. > > One optimization would be to declare the two "invalid platform" messages > somewhere in the class, and just refer to them in the many places where > they are used, to save some memory. > > iv_plat1 = "This ICT is not supported on this hardware platform." > iv_plat2 = "Failure. Returning: ICT_INVALID_PLATFORM" > > then > > prerror(iv_plat1) > prerror(iv_plat2) > return ICT_INVALID_PLATFORM > > Don't worry about it if it is too much trouble. > > Whether you do this and retest (preferred), or if you don't because you > are too short of time, I'm a happy camper. > > Thanks, > Jack > > > On 01/13/09 12:53, Joseph J VLcek wrote: >> Did I say "_rev2" postfix? Oops I meant to say "_rev3" postfix. ;) >> >> >> The new webrev name includes a "_rev3" postfix and can be found at: >> http://cr.opensolaris.org/~joev/sparc_ict_rev3/ >> >> >> Thanks Sue! >> >> >> >> Susan Sohn wrote: >> >>> Hi Joe, >>> >>> Don't forget to update all of the copyrights to 2009. >>> >>> Thanks, >>> Sue >>> >>> Joseph J VLcek wrote: >>> >>>> Thanks Jack! >>>> >>>> I have posted an updated webrev which includes the fixes that address >>>> Jack's comments. >>>> >>>> The new webrev name include the "_rev2" postfix and can be found at: >>>> http://cr.opensolaris.org/~joev/sparc_ict_rev2/ >>>> >>>> Some tests have not yet been completed. Jan is helping run them in his >>>> test lab in CZ. I expect them to be done tomorrow morning. I will not >>>> push until they have completed successfully. >>>> >>>> Jack please confirm these changes address all of the points you raised. >>>> >>>> Thanks, Joe >>>> >>>> >>>> >>>> >>>> >>>> Jack Schwartz wrote: >>>> >>>>> Hi everyone. >>>>> >>>>> I talked with Joe on the phone. This email publicizes our discussion. >>>>> >>>>> See inline below. >>>>> >>>>> On 01/13/09 08:56, Jack Schwartz wrote: >>>>> >>>>>> 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. >>>>>> >>>>> No. That set_Solaris_partition_active is not being called by ict.py >>>>> for SPARC is temporary. When the bug 6080 gets fixed and this >>>>> routine can convert cwtxdysz namaes to eeprom-style names, it will be >>>>> called. No need to add it to the 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? >>>>>> >>>>> We have agreed that it will be done. >>>>> >>>>>>>> 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. >>>>>> >>>>> I was mixed up. ict_test tests the code in ict.c, nothing to do with >>>>> ict.py. The line in question pertains to a command which ends up >>>>> calling installboot. Installboot already takes cwtxdysz-style >>>>> names. The usage statement is correct. >>>>> >>>>> Thanks, >>>>> Jack >>>>> >>>>>>> >>>>>>> >>>>>>>> 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 >>>>>>> >>>>>>> >>>>> ------------------------------------------------------------------------ >>>>> >>>>> _______________________________________________ >>>>> 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 >> >
