Hi Jack,
Thank you very much for the review. Responses below...
> 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.
It is needed because without the import, the BootMods object won't get
registered with the DOC, and therefore won't be able to find an object
to parse and import the <boot_mods> section from the XML manifest.
I spent several hours pulling my hair out over this one :)
>
> 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
> rror condition?
dc_pers_dict (the persistent dictionary) does not get automatically created by
DC
unlike dc_dict (the volatile dictionary) which has to be initialised with
pkg_img_path from
the get go. dc_pers_dict being present depends on one of the checkpoints
creating it so
there is no guarantee for it to be there. We do expect it to be there on X86
however since
we expect the boot-setup checkpoint to store the bios-eltorito-img key/val
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?
Factoring in Drew's suggestions, how about the following:
if self.arch == 'i386':
self.dc_pers_dict = self.doc.persistent.get_children(
name=DC_PERS_LABEL,
class_type=DataObjectDict,
not_found_is_err=True)[0].data_dict
self.bios_eltorito = self.dc_pers_dict["bios-eltorito-img"]
This way, it is only treated as an error on X86.
>
> 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?
I think these are different beasts. insert_at is an attribute of the
<boot_entry> element
whereas <kernel_args> is an element in its own right. Do you feel I should
specify the
"insert_at" attribute? eg.
<!-- Uncomment before using
<boot_entry insert_at="end">
<title_suffix>MyTitle</title_suffix>
<kernel_args></kernel_args>
</boot_entry>
-->
>
> 76-78: There is no magnifier entry in this file, so
> this comment can be
> removed.
Fixed.
>
> 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?)
Fixed.
>
> 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.
Fixed.
>
> 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?
We thought about this and decided on last in the door approach to handling
multiple defaults. If multiple entries are defined in the manifest with the
default
attribute as True, only the last one will actually be treated as the default
overwriting
the previous "default" entry. This behaviour is enforced in boot.py at line 854
(despite
my innacurate comment :)
>
> 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.
Yeah the comment is actually bogus. Sorry. I have replace it with something
more appropriate:
# The last boot entry in the list tagged as the default boot entry
# overrides the previous default if more than one is tagged as
# the default.
>
> 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?
I think it might be appropriate. It's not just kernel_args that
are not supported however. timeout isn't either nor are
boot_entry objects as a whole. At the moment they are just
silently ignored (and not included in the DC sparc manifests).
As per Dave's comments I have updated the boot.dtd comments
to clarify matters. I had intended to catch BootmgmtUnsupportedPropertyError
via pybootmgmt's SPARC backend when setting these properties but since we
don't have SPARC support I had to bolt on a SPARC solution in a bit of a hurry
and
bypass the methods that process boot_mods and boot_entries. Processing them
is going to further entangle the currently bolted on SPARC support so I'd
rather not
do that unless you feel strongly on the matter?
Thanks again!
Niall
>
> 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-di
> scuss
>
--
This message posted from opensolaris.org
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
