Copyrights look good now.
Sue
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
>>
>
