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.
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
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?
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.
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.
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
