Hi Karen,
Thanks so much for reviewing all this stuff.
Please find my responses below:
On 12/24/11 05:26 AM, Karen Tung wrote:
I reviewed all the files in Makefiles and misc, DC and boot and Text
Install sections.
Here are my comments:
>
> Makefile and Misc.: Volunteers
> ----------------------------------------
> usr/src/Makefile.master
OK
> usr/src/Targetdirs
OK
> usr/src/lib/Makefile.targ
OK
> usr/src/lib/install_common/__init__.py.src
Lines 482-500: We already have a function that determines
the amount of memory a system have in lib/install_target/controller.py.
The function is called _get_system_memory(). Looks like it is only
used in the controller.py file. Can we either remove the
other one and use this? Just don't want to have duplicate code.
Actually, this is the function that was in controller.py. We saw it
there and it was felt that since it was a generic function not
specifically related to target controller at it belonged better in a
general location so that it could be reused. So it has been moved to
this location. Dave also ported it from forking prtconf to using a C
typed system library (it appears that could be replaced by os.sysconf
according to Darren's comments.
>
> DC and Boot: Ethan, Seth, Drew
> -----------------------------------------
> usr/src/cmd/distro_const/checkpoints/create_iso.py
OK
> usr/src/lib/install_boot/boot.py
line 676-678: there's no handling of the case where you can't find
any nodes from 673-675. Is that OK?
If that occurs, it means that target instantiation and transfer
checkpoints must have failed also because this checkpoint is executed
after those. So I can't see how it could occur because the previous
checkpoints would have to fail before we get here. It will blow shortly
after when we try to install the boot loader anyway :-)
line 856: commented out line
yeah. I left that there to be uncommented when ident file issues is
fixed by Seth
line 855-859: Looks like the "ident" file is created at the root of
the image. Is this file only used when we boot from media?
As far as I know yes.
Is this file also necessary for the installed system?
If it is not necessary for the installed system, it needs to
be cleaned up by the cleanup_cpio_install checkpoint.
I don't think it is. Good catch.
Seth: The installed system doesn't need this ident file, right?
If it is used by both ISO and installed system, would it be better to
deliver
the file as part of a package instead of creating it dynamically here?
It's supposed to be unique based on a timestamp or similar, so if we
have to deliver it
I don't think pkg would be a suitable vehicle. But I am almost certain
we can delete it
in cleanup_cpio_install
line 855-859: can we move these few lines inside the try block, after
line 865. That way, you don't need to do the unlink if the setprop fails
because of Legacy Grub.
I believe we could move also lines from 850 inside and avoid an
unnecessary open() and read() if the
boot loader doesn't support the property we are trying set, if I first
check the property is supported
in the boot loader rather than doing a try: except: to set the property. eg.
eg:
860 if hasattr(self.config.boot_loader, "PROP_IDENT_FILE"):
# Read volset ID and setup boot ident file here
....
Would that sound like a good solution?
line 997: fix this comment line? :-)
Maybe ;-)
> usr/src/lib/install_boot/test/test_boot.py
OK
> usr/src/tools/tests/tests.nose
OK
>
>
> Text Install: Karen + Volunteer
> --------------------------------------
> usr/src/cmd/text-install/Makefile
ok
> usr/src/cmd/text-install/__init__.py
ok
> usr/src/cmd/text-install/disk_selection.py
ok
> usr/src/cmd/text-install/disk_window.py
Drew made the code changes to Text Install, so I will comment on the
ones I think I can answer. And leave the nasty ones for him ;)
line 227: why not have this line immediately after line 223, in the else
block? Since it's obvious that we do not have gpt data at that point.
Actually I think it can be removed altogether since the constructor sets
it to False at line 147. Do you agree?
line 355: I noticed that the check to make sure
that get_parts_in_use() always return a list with length bigger
than 0 is removed. (eg: lines 322-323 in the original code).
I'm not sure what the reason there is so I'll ask Drew to comment on that.
> usr/src/cmd/text-install/fdisk_partitions.py
OK
> usr/src/cmd/text-install/gpt_partitions.py
OK
> usr/src/cmd/text-install/helpfiles/Makefile
OK
> usr/src/cmd/text-install/helpfiles/gpt_partitions.txt
OK
> usr/src/cmd/text-install/partition_edit_screen.py
OK
> usr/src/cmd/text-install/progress.py
OK
> usr/src/cmd/text-install/summary.py
OK
> usr/src/cmd/text-install/ti_install.py
OK
> usr/src/cmd/text-install/ti_install_utils.py
OK
> usr/src/cmd/text-install/ti_target_utils.py
Some of the comments in the newly added UIGPTPartition() class
still talks about slices, I guess left over from copy-and-paste. :-)
Yeah - that doesn't sound surprising - thanks for catching it :-)
Thanks again Karen!!
Niall
> usr/src/cmd/text-install/welcome.py
OK
> usr/src/pkg/manifests/system-install-text-install.mf
ok
Thanks,
--Karen
On 12/23/11 03:44 AM, Niall Power wrote:
Hi,
Just a reminder that the timeout for round 1 review comments is this
Friday 23rd.
Thanks to everyone who has already provided comments or volunteered
to review.
I'll still check e-mails for review feedback over the holiday break,
but expect responses to be somewhat spotty :-)
I'd also really like to limit the scope of round 2 comments to final
tweaks and polish. So if there is anything you are deeply
uncomfortable with in the webrev but haven't mentioned yet, best to
get it out in the air now :)
I'm getting good feedback on Targets, AI and the GUI.
I haven't heard anything back regarding the following sections:
- Text Install
- Makefiles and Misc.
- DC and Boot
Karen has kindly offered to review the Text and Makefile & misc.
sections.
If anyone else feels like having a stab at any of these let me know.
Thanks,
Niall
On 12/19/11 08:19 PM, 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
