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
