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
