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