Hi Sundar, Thanks for your review. Comments below.
- Keith sundar Yamunachari wrote: > 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)? I should be able to remove the commented lines, as well as the 'python' make target. > > *usr/src/cmd/text-install/osol_install/profile/__init__.py:* > This file is empty That's correct. I don't need to define __all__ for this module, so this file's only purpose is to indicate to Python that the files under osol_install/profile are python modules. > > *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.* > * I do have plans to try and streamline the classes in this file. A common 'parent' object (which both DiskInfo and PartitionInfo could inherit) and a common 'child' object (which both PartitionInfo and SliceInfo would inherit) is something I've considered. Changes in the back-end require updating of __init__ and to_tgt() methods only. Changes in the UI are isolated from the backend (these structures are tied to the UI as is). > Line 85: Does %s needs an argument here (unit_str)? Yes it does. > 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. The data is usually sorted in disk_order (in general), not by number (and there's no guarantee that the list has been sorted that way until sort_disk_order() has been called). Although, when you bring it up that way, it would actually be convenient to maintain 2 lists - one sorted by offset/disk order, one sorted by number. Managing the addition/removal of partitions from such a pair of lists so that they don't get out of sync could be difficult, but there might be a few Python tricks I could utilize here (__setitem__ and so forth). Alternatively and possibly simpler would be to cache the index of the Extended (and Solaris) Partitions, as there can only be one of each. > Lines 295, 309: Is the function add_unused_parts() handles both parts > and slices? If so, update the comments with slices information. I'll do that. > Line 325, 331: The BACKUP_SLICE information is removed (from part) in > 325. Why you are checking for BACKUP_SLICE in 331? That line is obsolete. I'll remove it. > 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? start_pt needs a default value if parts is an empty list. > 382: You could replace '5' with a constant that indicate > 'FIRST_LOGICAL_PARTITION" for clarity. I can and should. > 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. The tgt module converts 0x82 to 0x182 if the partition is linux swap, so this conversion is safe. > 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.* > * Several of the functions have the same name for interface compatibility; e.g., you can call something.add_unused_parts() without caring if "something" is a DiskInfo or a PartitionInfo. > * > **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" I agree that's a much better error message. > > *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 This function has been updated to read the LANG environment variable instead of parsing the sysconfig/language file. > > 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 > >
