Jack,
Thanks for the code review. Comments are inline. I'm currently testing these
changes.
Jean
Jack Schwartz wrote:
> Hi Jean.
>
> Here are my comments:
>
> live-fs-root:
>
> 165: "differently" is misspelled
Fixed
>
> 209: My guess is that this is not correct. Line 216 says if no usb
> stick with a UFS file system on it was found (./liveusb is not a file
> -- see lines 192-207) then check for a CD. Lines 209-214 basically
> disable lines 216-223.
>
> IMO, there is no harm in checking for a CD; right now, AI does this
> for X86 to no harm, even though the AI image isn't a live CD. Also,
> eventually we may support a live CD for SPARC, and when we do, no
> changes will be needed here to enable it.
I tried removing lines 209-213. Appears to work.
>
> create_iso: OK
>
> bootroot_initialize.py: Thanks for cleaning up after me...
You're welcom.
>
> ai_sparc_image.xml:
>
> 36, 39: Please add a comment or XXX that this repo needs to be changed
> before release. Better yet, file a bug to track it so it doesn't get
> dropped.
Done. Bug 5780
>
> I suggest basing ai_sparc_image.xml on the existing ai_x86_image.xml
> since it has all of the comments present and current, and then
> changing the XML contents to be what you want. Doing this will also
> bring the comment at line 45 current, etc. If you don't do this now,
> you'll just end up having to do it later anyway...
Originally I had done that. I just forgot to keep them in sync I
believe. Done.
>
> Package list is probably OK.
>
> Regarding bootroot file list:
> Doing a diff between ai_x86_image.xml and ai_sparc_image.xml...
> - 398-9: Get rid of var/sadm/README and var/adm/spellhist
> - Add var/sadm/install/contents
I believe you're right. I'll make the changes.
> Not sure about the following:
> - 385: I'm wondering if you can delete usr/bin/sparcv7/ksh93. You may
> want to check...
Can't. The absence of this is what caused one of our major problems.
> - 422: ai_x86_image.xml doesn't include the opt dir. Do you need it?
I took it out.
> - add excluding of etc/notices directory
Done.
>
> Not sure about adding ai_post_bootroot_archive finalizer script:
> Karen asked why the bootroot didn't have the permissions set the same
> way; the reason is that this script wasn't in the SPARC manifest;
> however, things still seemed to work... So I'm wondering why it was
> needed in the first place, or if maybe it's needed for X86 but not for
> SPARC...
Well it's very x86 specific. I believe it shouldn't be there.
>
> DC_defs.py: OK
>
> bootroot_archive.py:
>
> Please honor 80-character line maximums.
Done.
>
> 62: compress() needs a header
Done.
>
> Nit: 67, 79, 88: if (status != 0) seems less circuitious than if not
> status == 0
Done
>
> 87: just use stat_out of the previous line: mode = stat.out.st_mode
Done.
>
> Error handling in compress() can be optimized. replace 79-83 and
> 92-96 with:
> if (status != 0):
> break
> and then at 103, raise an exception if status != 0.
I like that. done.
>
> Will files in the uncompress_flist be copied and compressed in the
> first loop, and then get copied again in the loop at 114? This may be
> worth optimizing of these files are large. Otherwise, it's OK.
Yes. If it appears to be an issue we can look into optimizing.
>
> 263: Changes to files should be done in bootroot_configure, not
> bootroot_archive.
Karen and I decided to do this here because it needs the bootroot size
which is computed in this file.
>
> 282, 296, other places?: an optimization would be to do
> is_sparc = (platform.platform().find('sparc') >= 0)
> and then use is_sparc instead of repetatively doing platform.platform.
Definitely. Done.
>
> Thanks,
> Jack
>
> On 12/10/08 12:56, Jean McCormack wrote:
>> Karen and anyone else please review:
>>
>> http://cr.opensolaris.org/~jeanm/slim_sparc/
>>
>> I believe the appropriate defect is:
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4230
>>
>> Yes, I'll need to commit with this for the comments.
>>
>> Jean
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>
