Hi Dave
Thanks for the review. Responses inline below.
> Reviewed all non-test files.
>
> Please run "hg nits" before sending out for review;
Fixed
>
> create_iso.py:
>
> 116, 150: It would be better to be consistent and
> either place commented
> out future changes in both places, or neither (I'd
> vote for neither and
> put the suggested changes in a bug that'll be used to
> add this support
> later). As it stands right now this is just
> confusing.
Fixed. Opted for neither.
>
>
> boot.dtd:
>
> * I'm unclear what can usefully be done on SPARC with
> the boot_mods
> section.
I've tried to clarify this by adjusting the comments to be more explicit about
what does and doesn't work on SPARC in the boot.dtd and remove SPARC reference
in dc_*_x86.xml manifests. I'll push it in the next webrev.
> It seems all you can supply is a title, but
> I'm totally
> unclear how that would be used.
Basically, the only thing that works now on SPARC is 'title' within the AI
client, and that gets applied to /rpool/boot/menu.lst which will be picked up
by beadm(1M) and the openprom bootlst command.
I don't recall this
> sort of change
> being proposed in the UEFI design.
Yeah, apologies for that. This one fell through the cracks in my initial
designs. I identified that grub_mods was inadequate for GRUB2 because it had a
number of assumptions baked into it about the configuration file format of
legacy GRUB's menu.lst such as the "<line>" elements. Following discussion with
Drew and Alok about requirements and in keeping with the goal of abstracting
out specific boot loader knowledge and making things generic boot.dtd was
proposed. Myself, Seth, Alok, Drew and Darren reviewed and agreed on the
current boot.dtd definition as a replacement.
Even more
> confusing, the SPARC
> manifests don't have any changes.
Because boot customisation is not applicable to DC on SPARC OBP, the SPARC DC
manifests do not include the boot_mods section (if they are they will be
ignored). If GRUB2 becomes the boot loader on SPARC in future, then boot_mods
would be relevant.
>
> dc_*_x86.xml:
>
> * The "with magnifier" entry only appears in
> dc_livecd.xml; the NOTE
> that mentions it should not appear in the other two.
Fixed
>
> boot.py:
> 58ff: Why not just import these from distro_const?
Fixed.
>
> 312: Why mount under /var/tmp rather than /tmp or
> /system/volatile
> (probably preferred)?
Fixed. Using '/system/volatile' instead.
>
> 458: The makedirs here seems problematic in
> non-dry-run mode; it creates
> all intermediate directories as 777 the way it's
> called here, which is
> not a good result. I would suggest there's an error
> in the image
> generation if you need to create that directory in
> any non-testing mode.
The directory is under '/rpool/platform/'uname -m'/ so it won't be present as a
result
of pkg transfer checkpoint since that only populates the file hierarchy under
the BE mountpoint
(eg. rpool/boot/solaris). I've added a mode argument of 0755 to makedirs()
which matches what
I see on existing SPARC S11 systems.
>
> 519: metadevice? We are disinterested in supporting
> ZFS on SVM. Or is
> something else meant by this term in the comment?
This was what I imported from lib/libict_pymod/ict.py (line1661). I wasn't
crystal clear what
was going on either so opted for the 'if it ain't broke' approach since this is
what is used by
the current SPARC text installer.
>
> 578: It seems like we're setting eeprom's boot device
> in a loop here.
> Perhaps I don't get what result we're after here, but
> this doesn't seem
> right.
As with the above, this was what I ported over from ict.py as used by the
current SPARC text installer.
I decided to play it safe with this one also for the same reasons.
>
> 654: It seems like something else should catch this
> unacceptable state
> before you...
Yeah. I'll be amazed if TI or transfer doesn't choke on it first. Removed.
>
> 787: /boot/grub is packaged in the grub package, so
> why do we need to
> create it?
Because this is /rpool/boot/grub so the grub package will not create this
beacuse
it exists outside of the BE that pkg installs to.
I initially didn't create this directory and it cause the boot checkpoint to
fail due
to the absence of this directory.
>
> 833: this comment seems to conflict with the comment
> & code that follow
> at 835-837.
Fixed. I removed the conditional and simplified the comment:
# UEFI Note that we will have to specify additional firmware
# targets (uefi64 & SPARC OBP) when pybootmgmt supports them.
self.config.boot_loader.setprop('boot-targets', 'bios')
>
> 854: comment seems incorrect
Yeah. It's inaccurate. I've replace it with something more accurate:
# 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.
if entry.default_entry:
instance.default = True
else:
instance.default = False
>
> 923-939: "El Torito"
Fixed
>
> 1107: this stanza appeared in all of the subclasses;
> perhaps a
> superclass method would be appropriate
Fixed. I've added a method to ISOImageBootMenu:_add_chainloader_entry()
Once I've addressed Jacks and Drew's comments I'll push a new webrev out.
Thanks,
Niall.
>
> Dave
>
> _______________________________________________
> 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
