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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090113/911ed253/attachment.html>
