Hi Darren,
Thanks for reviewing. I've responded to some of your points below - the
AI target selection comments I will defer to Dave :)
On 12/23/11 01:38 AM, Darren Kenny wrote:
Hi Niall,
Working my way through some files, here are some comments:
usr/src/Makefile.master
- I would think that the DC and AI DTD versions should really be updated
here to reflect the fact that the targets version being imported in them
has changed too. The main reason being this would be the best way for a
user/consumer of the DTD (writing an XML manifest) to be specific about
the expected versions of things.
Yeah, I think you have a point there alright. There would be no visible
changes to DC manifests in practice because we only changed the physical
section, and DC only uses the logical section I believe. But there
certainly will be changes to the AI manifest for use of GPT (we will
also need to update docs, man pages and sample manifests for this). I am
not sure what is the appropriate action to take here, but I will
certainly adjust the AI and DC DTD versions if you and others feel this
is the appropriate thing to do.
usr/src/cmd/auto-install/checkpoints/target_selection.py;
- lines 1150-1154 and 1156-1160
I'm fairly sure we could remove the need to do this condition check
twice. Possibly using a temporary variable to store the size.
- lines 1805 to 1811
Are these lines commented out intentionally? If so, why?
- lines 2000-2003
The error refers to slices - what terminology should we be using here?
Given that we have gpt_partitions in the DTD, seems wrong to to refer to
them as such in the errors too...
usr/src/lib/install_common/__init__.py:
- lines 478-490:
Why aren't we using the built in os.sysconf() method to get the
pagesize, and number of pages, e.g.:
$ python
>>> import os
>>> print os.sysconf('SC_PAGESIZE')
4096
>>> print os.sysconf('SC_PHYS_PAGES')
1957677
Dave wrote this so I will ask for his take......
Dave? :)
usr/src/lib/install_target/discovery.py:
- lines 1104-1116:
Why is it necessary to mount the ESP partition here, would fstyp not
give you the fs format?
You probably saw Seth's response already. fstyp is unreliable.
Have a look at 6913949 for further tails of woe.
usr/src/lib/install_target/logical.py:
- lines 93-97, 100-104, 193-195, 511-513:
This shouldn't be necessary and should be enforced by the DTD.
But if you wish to have it here, it looks like this would be best done
as a common method rather than repeating the code like this.
Dave did this however he didn't change any actual functionality but
rather improved the logic of the implementation.
I also see similar else where in other files like physical.py.
usr/src/lib/install_target/shadow/__init__.py:
- lines 124-126:
These should probably be removed.
Yep. Dave has removed these in the lastest slime_uefi source.
Thanks!!
Niall
Thanks,
Darren.
On 19/12/2011 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
*
**
*
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
