Keith,
I reviewed installation profile code.
*usr/src/cmd/text-install/osol_install/profile/Makefile*:
Line 33: This line is commented. If it is not needed, remove the line.
Otherwise remove the comment.
Lines 71, 76-81: See above comment
Line 57: PROGS is defined as empty and why we need python $(PROGS)?
*usr/src/cmd/text-install/osol_install/profile/__init__.py:*
This file is empty
*usr/src/cmd/text-install/osol_install/profile/disk_info.py:
*General comment: In the DiskInfo class, you are trying to treat
partitions and slices in the same way since only one can exist at any
time (parts for X86 and slices for sparc). I am concerned that any
deviation between partitions and slices either in the GUI or in the back
end may cause lot of changes in the common area.*
*Line 85: Does %s needs an argument here (unit_str)?
Lines 248, 266: You need to check only the partitions 1-4 for standard
and extended partitions. If you data is stored based on the partition
numbers, that will be more efficient.
Lines 295, 309: Is the function add_unused_parts() handles both parts
and slices? If so, update the comments with slices information.
Line 325, 331: The BACKUP_SLICE information is removed (from part) in
325. Why you are checking for BACKUP_SLICE in 331?
Lines 308, 315, 335: The start_pt is initialized in lines 308, 315. It
is again set to a different value in 335. What is the reason for
initializing start_pt in 308 and 315?
382: You could replace '5' with a constant that indicate
'FIRST_LOGICAL_PARTITION" for clarity.
536-541: Why the partition of type 0x82 is converted in to SOLARIS
(0xBF)? It could be a genuine linux swap partition and if that partition
is chosen to install opensolaris, Linux may fail to work. You should
check whether it is a LINUX SWAP before changing it to SOLARIS. The
target discovery tells you whether it is Linux swap or not.
809: add_unused_parts() generally talks about partitions but deals only
with slices. It is little bit confusing since the function is called
add_unused_parts(). In general, I prefer the functions for partitions
and slices are separate. If you chose to keep them together, please add
comments to indicate what is done for partitions and what is done for
slices.*
**usr/src/cmd/text-install/osol_install/profile/ip_address.py*
87: The string in exception could be meaningful such as "The values
should be between 0 and 255"
*usr/src/cmd/text-install/osol_install/profile/system_info.py:*
41: Is there any specific reason to choose "/etc/sysconfig/language" to
store language? Is the language is set in this file during boot?
68: Can you use LANG_FILE here instead of /etc/sysconfig/language
Thanks,
Sundar
Keith Mitchell wrote:
> Hi Glenn and all other reviewers,
>
> I have posted an updated webrev at:
> http://cr.opensolaris.org/~kemitche/text_v2/
>
> Additionally, the opensolaris.org project gate has been updated to
> match those changes.
> hg clone ssh://anon at hg.opensolaris.org/hg/caiman/text_installer
>
> Please disregard the initial webrev. The updated webrev contains the
> most recent changes project files, and some of those files have
> changed significantly since the first webrev. An updated breakdown of
> which files fall into which sections is below.
>
> Thanks,
> Keith
>
> Breakdown of file groupings:
>
> Installation Profile:
> cmd/text-install/osol_install/profile/__init__.py
> cmd/text-install/osol_install/profile/disk_info.py*
> cmd/text-install/osol_install/profile/install_profile.py
> cmd/text-install/osol_install/profile/ip_address.py
> cmd/text-install/osol_install/profile/network_info.py
> cmd/text-install/osol_install/profile/system_info.py
> cmd/text-install/osol_install/profile/user_info.py
>
> * The changes mentioned in previous emails are present in the updated
> webrev; please DO review this file
>
> UI Components:
> cmd/text-install/osol_install/text_install/action.py
> cmd/text-install/osol_install/text_install/base_screen.py*
> cmd/text-install/osol_install/text_install/color_theme.py
> cmd/text-install/osol_install/text_install/edit_field.py
> cmd/text-install/osol_install/text_install/error_window.py
> cmd/text-install/osol_install/text_install/inner_window.py
> cmd/text-install/osol_install/text_install/list_item.py
> cmd/text-install/osol_install/text_install/main_window.py
> cmd/text-install/osol_install/text_install/scroll_window.py
> cmd/text-install/osol_install/text_install/window_area.py
>
> * This file is also relevant to the "Screens" group
>
> Screens:
> cmd/text-install/osol_install/text_install/date_time.py
> cmd/text-install/osol_install/text_install/disk_selection.py
> cmd/text-install/osol_install/text_install/fdisk_partitions.py
> cmd/text-install/osol_install/text_install/help_screen.py
> cmd/text-install/osol_install/text_install/install_progress.py
> cmd/text-install/osol_install/text_install/install_status.py
> cmd/text-install/osol_install/text_install/log_viewer.py
> cmd/text-install/osol_install/text_install/network_nic_configure.py
> cmd/text-install/osol_install/text_install/network_nic_select.py
> cmd/text-install/osol_install/text_install/network_type.py
> cmd/text-install/osol_install/text_install/partition_edit_screen.py
> cmd/text-install/osol_install/text_install/summary.py
> cmd/text-install/osol_install/text_install/timezone.py
> cmd/text-install/osol_install/text_install/timezone_locations.py
> cmd/text-install/osol_install/text_install/timezone_regions.py
> cmd/text-install/osol_install/text_install/users.py
> cmd/text-install/osol_install/text_install/welcome.py
>
> Text Install python core:
> cmd/text-install/osol_install/text_install/__init__.py
> cmd/text-install/osol_install/text_install/disk_window.py*
> cmd/text-install/osol_install/text_install/screen_list.py
> cmd/text-install/osol_install/text_install/text-install.py**
> cmd/text-install/osol_install/text_install/ti_install_utils.py
> cmd/text-install/osol_install/text_install/ti_install.py
>
> * This subclass of "InnerWindow" has enough knowledge of Text
> Installer specifics that it can't be classified as a 'generic' UI
> component
> ** This is the 'main' program
>
> Image/Distro-const:
> cmd/distro_const/auto_install/ai_plat_setup.py
> cmd/distro_const/auto_install/ai_post_boot_archive_pkg_image_mod
> cmd/distro_const/auto_install/ai_sparc_image.xml
> cmd/distro_const/auto_install/ai_x86_image.xml
> cmd/distro_const/text_install/text_mode_sparc.xml
> cmd/distro_const/text_install/text_mode_x86.xml
> cmd/distro_const/text_install/tm_gen_cd_content
> cmd/distro_const/text_install/tm_generic_live.xml
> cmd/distro_const/text_install/tm_pre_boot_archive_pkg_image_mod
> cmd/distro_const/utils/grub_setup.py
> cmd/distro_const/utils/Makefile
> cmd/distro_const/utils/plat_setup.py
> cmd/distro_const/utils/post_boot_archive_pkg_image_mod_custom
> cmd/slim-install/finish/install-finish
> cmd/slim-install/svc/media-fs-root
> cmd/text-install/svc/text-mode-menu.xml
> cmd/text-install/text-mode-menu/text-mode-menu.ksh
>
> Library Changes:
> lib/libtarget_pymod/discover.c
> lib/libtarget_pymod/disk.c
> lib/libtarget_pymod/disk.h
> lib/libtarget_pymod/geometry.c
> lib/libtarget_pymod/geometry.h
> lib/libtarget_pymod/instantiate.c
> lib/libtarget_pymod/instantiate.h
> lib/libtarget_pymod/mapfile-vers
> lib/libtarget_pymod/partition.c
> lib/libtarget_pymod/partition.h
> lib/libtarget_pymod/slice.c
> lib/libtarget_pymod/slice.h
> lib/libtarget_pymod/tgt.c
> lib/libtarget_pymod/tgt.h
> lib/libtarget_pymod/zpool.c
> lib/libtarget_pymod/zpool.h
> lib/libtd/td_api.h
> lib/libtd/td_dd.c
> lib/libtd/td_dd.h
> lib/libtd/test_td.c
> lib/libti/ti_api.h
> lib/libzoneinfo_pymod/libzoneinfo.c
>
> Packaging/Makefiles: Package definitions, dependencies, makefiles, and
> so forth
> Makefile.master
> README
> Targetdirs
> cmd/Makefile
> cmd/Makefile.cmd
> cmd/Makefile.targ
> cmd/distro_const/Makefile
> cmd/distro_const/auto_install/Makefile
> cmd/distro_const/text_install/Makefile
> cmd/text-install/Makefile
> cmd/text-install/helpfiles/Makefile
> cmd/text-install/osol_install/profile/Makefile
> cmd/text-install/osol_install/text_install/Makefile
> cmd/text-install/svc/Makefile
> cmd/text-install/text-mode-menu/Makefile
> lib/Makefile
> lib/libict/Makefile
> lib/libtarget_pymod/Makefile
> lib/libtd/Makefile
> lib/libzoneinfo_pymod/Makefile
>
> Other: (Help Files, miscellaneous)*
> cmd/text-install/helpfiles/date_time.txt
> cmd/text-install/helpfiles/disks.txt
> cmd/text-install/helpfiles/network.txt
> cmd/text-install/helpfiles/network_manual.txt
> cmd/text-install/helpfiles/sparc_solaris_slices.txt
> cmd/text-install/helpfiles/sparc_solaris_slices_select.txt
> cmd/text-install/helpfiles/summary.txt
> cmd/text-install/helpfiles/timezone.txt
> cmd/text-install/helpfiles/users.txt
> cmd/text-install/helpfiles/welcome.txt
> cmd/text-install/helpfiles/x86_fdisk_partitions.txt
> cmd/text-install/helpfiles/x86_fdisk_partitions_select.txt
> cmd/text-install/helpfiles/x86_fdisk_slices.txt
> cmd/text-install/helpfiles/x86_fdisk_slices_select.txt
> cmd/text-install/osol_install/__init__.py**
>
> * The help text files are under separate review.
> ** This file is not packaged, as osol_install/__init__.py is delivered
> by SUNWinstall. It exists so that the UI can be run directly from the
> workspace.
>
> Glenn Lagasse wrote:
>> Hi Keith,
>>
>> * Keith Mitchell (Keith.Mitchell at Sun.COM) wrote:
>>
>>> All,
>>>
>>> As of yet, we've not received any code review comments for the first
>>> round of review. I realize that there is a lot of code, so to
>>> facilitate reviewing, I've broken down the files into the
>>> subsections (see below for which files fall into which subsections).
>>> Reviewers may find it easier to select one or two subsections for
>>> review.
>>>
>>
>> Is this [1] link still the link to use for reviewing? I ask because the
>> DC xml files for building the TM image don't appear to be updated for
>> the post-VMC integration (specifically they don't include the im-pop
>> finalizer script).
>>
>> Cheers,
>>
>>
> _______________________________________________
> 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/20091218/a34a0e77/attachment.html>
