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 >
