Hi Matt,
One large change in target_selection.py is that we have a new zfs(1M) option,
'-B' which auto carves up the disk for GPT. That is, in the case of x86 it
makes either the Bios Boot or EFI System Partition as needed, and in the case
of SPARC and x86 creates the reserved partition ZFS needs and the rest is given
to one big partition to install on.
Responses inline below.
On Dec 20, 2011, at 7:46 AM, Matt Keenan wrote:
Niall,
Reviewed target_selection.py, In general looks pretty clean, nice work.
Will try and review the rest of the targets code as well.
target_selection.py:
1772 :
- You've removed the validation for determining if the disk is
contained in the root pool, what's your reasoning behind this ?
The thinking was that we no longer need to make sure root pool is on VTOC
labelled disk. But I believe we are missing the case of old SPARC here and
should put the test back in if gpt_firmware_check() returned False.
- For the children check e.g. VTOC sparc children must be Slices etc.
Can you add the text for these tests to the comments at the start of
the validate disk method.
- Also add an other specific GPT validation checks to comment summary
at the start of the method
will do
1805:
- check for disk has children in a whole disk scenarion is commented
out, if this is intentional should just remove the code completely,
but I can't think of a good reason to remove this,
I believe the commenting out of this code is a fine example of why code reviews
are so valuable :-)
This will be uncommented.
- Again the check for check_in_root_pool for the disk why are you
removing this here ?
Overzealous, the test does not make sense for GPT where zfs(1M) is partitioning. But
still makes perfect sense when the label is VTOC. This will be restored for when
disk.label == "VTOC".
1985, 1988, 2000:
- Check for root_slice_required and root_slice_found
Both of these are set to false at start of function, the only place
they are set then is at 1985 and 1988, where they are both always set
to true, effectively meaning that they will always both be either
False or both True, thus the check at 2000 can never actually happen.
When would a root_slice_required not be needed ? surely a root slice
is always required regardless of slice or gptslice ?
Yes something must be there to boot from. I suspect "root_slice_required" was forward thinking for
the before the GPT project. But it can be removed and "root_slice" can probably be changed to the
more festive "root_partyslice" to indicate a slice or GPT Partition has been found to boot from.
2851, 3375;
- Why is disk_copy.whole_disk being set to True for GPT slice ?
Ah, 2851 has totally been refactored last night to let the controller handle
more. So I'm going to defer till then and ask you to revisit in next webrev.
3375 needs same refactoring and will get it.
3523:
- Manual setting of in_zpool/in_vdev on Controller selected disks was
not required before, how come it's required now ?
Probably because I wasn't going through the controller above. When those are
both fixed I suspect they won't be needed. Again lets make sure they are gone
in next webrev (or I'll have an explanation).
cheers
Matt
Thank you!
-Dave
On 12/19/11 09:19, Niall Power wrote:
Hi!
Myself, Drew and Dave Marker would like to ask for code reviewers for
the Install UEFI, GRUB2 and large disk boot project.
I've broken down the webrev into component groupings and specified
people who we would particularly like to review particular components
(eg. leads for those compononents) but we welcome and appreciate any and
all review comments from others too of course.
Some things to be aware of:
This is about 95%, but not yet 100% complete.
The core is complete and functional but there are a few odds and ends
that need to be done still:
- usbgen and usbcopy need to be ported to be GRUB2 and UEFI compatible.
I am figuring out this with Seth. Will create a separate webrev.
- installadm changes for GRUB2 and UEFI. Sue is working on this and will
be handled in a separate review
- Installation of UEFI system into an MBR partitioned disk. This is a
minor amount of work that doesn't impact the existing code changes (it
will essentially add a small amount more code). I'll get this included
in time for round 2, otherwise create a separate webrev.
I'd like to set an initial timeout for collection of round 1 review
comments on Friday Dec 23rd.
Following that I propose to ring in the new year with a second round of
review comments from Monday Jan 2nd until Friday Jan 6th.
Please let me know if you intend to review any components, or if you are
unavailable to review specific requested components.
*Webrev URL:
----------------*
https://cr.opensolaris.org/action/browse/caiman/niall/slim_uefi_version_01/webrev-uefi-ver-1/
Most of the files below have only a very small number of lines of code
changes so it's not as big a task as it looks.
*
Makefile and Misc.: Volunteers
----------------------------------------*
usr/src/Makefile.master
usr/src/Targetdirs*
*usr/src/lib/Makefile.targ
usr/src/lib/install_common/__init__.py.src
*
*
*Targets: Dermot, Darren, Matt
---------------------------------------*
usr/src/pkg/manifests/system-library-install.mf
usr/src/lib/install_manifest/dtd/target.dtd
usr/src/lib/install_target/Makefile
usr/src/lib/install_target/__init__.py
usr/src/lib/install_target/controller.py
usr/src/lib/install_target/cuuid.py
usr/src/lib/install_target/discovery.py
usr/src/lib/install_target/instantiation.py
usr/src/lib/install_target/libdiskmgt/diskmgt.py
usr/src/lib/install_target/libefi/Makefile
usr/src/lib/install_target/libefi/__init__.py
usr/src/lib/install_target/libefi/cfunc.py
usr/src/lib/install_target/libefi/const.py
usr/src/lib/install_target/libefi/cstruct.py
usr/src/lib/install_target/libefi/efi.py
usr/src/lib/install_target/logical.py
usr/src/lib/install_target/physical.py
usr/src/lib/install_target/shadow/__init__.py
usr/src/lib/install_target/shadow/physical.py
usr/src/lib/install_target/test/test_shadow_list.py
*AI: Darren, Matt
--------------------*
usr/src/cmd/auto-install/checkpoints/target_selection.py
usr/src/cmd/auto-install/test/test_target_selection_gpt.py
*DC and Boot: Ethan, Seth, Drew
-----------------------------------------*
usr/src/cmd/distro_const/checkpoints/create_iso.py**
usr/src/lib/install_boot/boot.py
usr/src/lib/install_boot/test/test_boot.py
usr/src/tools/tests/tests.nose
*
GUI: Darren, Dermot
---------------------------*
*
*usr/src/cmd/gui-install/src/Makefile
usr/src/cmd/gui-install/src/confirm_screen.py
usr/src/cmd/gui-install/src/disk_panel.py
usr/src/cmd/gui-install/src/disk_screen.py
usr/src/cmd/gui-install/src/fdisk_panel.py
usr/src/cmd/gui-install/src/gpt_panel.py
usr/src/cmd/gui-install/src/screen_manager.py
usr/src/cmd/gui-install/xml/Makefile*
*usr/src/pkg/manifests/system-install-gui-install.mf *
*
*
I seriously wouldn't bother code reviewing these glade auto-generated
XML files but if you enjoy pulling your eyes out :)*
usr/src/cmd/gui-install/xml/fdisk-panel.xml
usr/src/cmd/gui-install/xml/gpt-panel.xml
usr/src/cmd/gui-install/xml/installationdisk.xml
*
**
Text Install: Karen + Volunteer
--------------------------------------*
usr/src/cmd/text-install/Makefile
usr/src/cmd/text-install/__init__.py
usr/src/cmd/text-install/disk_selection.py
usr/src/cmd/text-install/disk_window.py
usr/src/cmd/text-install/fdisk_partitions.py
usr/src/cmd/text-install/gpt_partitions.py
usr/src/cmd/text-install/helpfiles/Makefile
usr/src/cmd/text-install/helpfiles/gpt_partitions.txt
usr/src/cmd/text-install/partition_edit_screen.py
usr/src/cmd/text-install/progress.py
usr/src/cmd/text-install/summary.py
usr/src/cmd/text-install/ti_install.py
usr/src/cmd/text-install/ti_install_utils.py
usr/src/cmd/text-install/ti_target_utils.py
usr/src/cmd/text-install/welcome.py*
*usr/src/pkg/manifests/system-install-text-install.mf
*
*Thanks!!
Niall, Dave& Drew
*
**
*
