Niall:
Please do the following as well:
1. Rerun any slim install unit tests and make sure there are no regression.
2. Please make sure the slim source are pep8 slim_code_cleanliness:
You can find the instuructions on how to do this in usr/src/README.
The last putback added additional 751 pep8 slim_code_cleanliness and
this is too much.
Thanks !!!
On 04/25/11 12:17 PM, Dave Miner wrote:
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
