On 04/20/11 09: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.
...
Webrev location:
http://cr.opensolaris.org/~niall/cud-boot/
Reviewed all non-test files.
Please run "hg nits" before sending out for review; you have a number of
incorrect copyright dates and such that would have been caught. I won't
comment on those items directly.
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.
boot.dtd:
* I'm unclear what can usefully be done on SPARC with the boot_mods
section. It seems all you can supply is a title, but I'm totally
unclear how that would be used. I don't recall this sort of change
being proposed in the UEFI design. Even more confusing, the SPARC
manifests don't have any changes.
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.
boot.py:
58ff: Why not just import these from distro_const?
312: Why mount under /var/tmp rather than /tmp or /system/volatile
(probably preferred)?
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.
519: metadevice? We are disinterested in supporting ZFS on SVM. Or is
something else meant by this term in the comment?
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.
654: It seems like something else should catch this unacceptable state
before you...
787: /boot/grub is packaged in the grub package, so why do we need to
create it?
833: this comment seems to conflict with the comment & code that follow
at 835-837.
854: comment seems incorrect
923-939: "El Torito"
1107: this stanza appeared in all of the subclasses; perhaps a
superclass method would be appropriate
Dave
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
