Hi Niall.
I reviewed group 3 and a few extra files:
Group 3:
========
usr/src/cmd/distro_const/__init__.py
------------------------------------
51: Not sure this is needed. No references to BootMods other than this
line exist in this file. If not needed, remove this file from the webrev.
usr/src/cmd/distro_const/checkpoints/create_iso.py
--------------------------------------------------
118: The lack of symmetry between SPARC and X86 makes me curious: When
would be the case where dc_pers_dict wouldn't exist? Would it be an
error condition? If it would be an error condition, could you:
1) Move lines 114 and 119 before the if on 111 and remove the else on 118
2) Let the lack of dc_pers_dict raise an exception when it is not found?
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
-----------------------------------------------
kernel_args is optional, and is specified empty on line 83. Does it
make sense to explicitly specify
<insert_at>end</insert_at>
since kernel_args is specified?
76-78: There is no magnifier entry in this file, so this comment can be
removed.
usr/src/cmd/distro_const/manifest/dc_livecd.xml
-----------------------------------------------
55, 57: Seems confusing to me to have two <boot_mods> items in this
file. It would clarify to me, if line 54 were changed to say something
like:
<!--
Sample boot_mods with title and timeout attributes.
Uncomment and replace un-attributed boot_mods entry
below before using.
-->
... if that's what it's for. (Else, what is it for?)
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
-------------------------------------------------
77-79: There is no magnifier entry in this file, so this comment can be
removed.
additional files:
=================
boot_spec.py
------------
BootEntry/from_xml():
Just curious: I think there is an implicit assumption that there will be
only one default, and that multiple defaults doesn't have to be checked
for. Should it be checked?
boot.py
-------
Is line 854 accurate? Is the first entry always the default? Looks
like when the entry is marked as the default, then it is the default,
but it doesn't have to be the first entry that is marked.
Not sure if this is the right place for this, but would it be a good
idea to emit a warning of kernel_args are provided in a SPARC manifest?
Thanks,
Jack
On 04/20/11 06:51 PM, Niall Power wrote:
Hi all,
I'm looking for a few brave souls to review my implementation of a boot
checkpoint based on pybootmgmgt. If you'd like to volunteer but get around to
it later, please let me know when
you could be available to review.
The changes aren't huge mostly tweaks here and there, with the bulk of the code
contained in
usr/src/lib/install_boot/boot.py (about 1100 LOC) and boot_spec.py (about 250
LOC) so I'd be
very grateful for any comments received by April 27/28th. I'll even buy you a
pint :-)
The changes can be logically grouped together as follows if you want to look at
a subset of the changes:
1) Boot schema definitions and Data Object Cache support
usr/src/lib/install_manifest/dtd/Makefile
usr/src/lib/install_manifest/dtd/boot.dtd
usr/src/lib/install_manifest/dtd/dc.dtd
usr/src/lib/install_boot/boot_spec.py
usr/src/lib/install_boot/test/test_boot_spec.py (Tests)
2) Boot checkpoint implementation
usr/src/lib/install_boot/__init__.py
usr/src/lib/install_boot/boot.py
usr/src/lib/install_boot/test/test_boot.py (Tests)
3) Distro Constructor changes to use new boot-setup checkpoint instead of
grub-setup for X86
usr/src/cmd/distro_const/__init__.py
usr/src/cmd/distro_const/checkpoints/create_iso.py
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
usr/src/cmd/distro_const/manifest/dc_livecd.xml
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
4) Packaging and makefile changes to slim_source repo
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_boot/Makefile
usr/src/lib/install_manifest/dtd/Makefile
usr/src/pkg/manifests/system-library-install.mf
Webrev location:
http://cr.opensolaris.org/~niall/cud-boot/
Thanks,
Niall
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
