Change Summary:
update dependency on 80 (#6087)
Depends On: |
|
||
---|---|---|---|
Diff: |
Revision 2 (+368 -15) |
Review Request #79 — Created July 26, 2015 and submitted
Information | |
---|---|
tsoome | |
illumos-gate | |
6086 | |
|
|
85, 89, 90 | |
b5cd118... | |
Reviewers | |
general | |
6086 add install bootblock option for bootadm
- no output from defaults. this is because the grub capfiles check does not provide any verbose messages - perhaps should add some?
root@testoi:~# bootadm install-bootloader -v
root@testoi:~#
- this is normal installgrub output with version check disabled. the default locations are used for stage files. note: this is whole disk setup, so libbe is enforcing MBR update. note: -F in installgrub is in fact useless as our grub does not use embedded versioning, but included anyhow for sake of completeness.
root@testoi:~# bootadm install-bootloader -vf
Command: "/sbin/installgrub -F -m -f //boot/grub/stage1 //boot/grub/stage2 /dev/rdsk/c3t3d0s0"
Output:
stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
stage1 written to partition 0 sector 0 (abs 256)
stage1 written to master boot sector
install to pool "tank" using current root as source for stage files
root@testoi:/# bootadm install-bootloader -v -P tank -R /
Command: "/sbin/installgrub -m -f //boot/grub/stage1 //boot/grub/stage2 /dev/rdsk/c3t5d0s0"
Output:
stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
stage1 written to partition 0 sector 0 (abs 256)
stage1 written to master boot sectormount previous BE to /a and use it as altroot
root@testoi:/# beadm mount openindiana-1 /a
Mounted successfully on: '/a'
root@testoi:/# bootadm install-bootloader -v -P tank -R /a
Command: "/sbin/installgrub -m -f /a/boot/grub/stage1 /a/boot/grub/stage2 /dev/rdsk/c3t5d0s0"
Output:
stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
stage1 written to partition 0 sector 0 (abs 256)
stage1 written to master boot sectorspecify only pool
root@testoi:~# bootadm install-bootloader -v -P tank
Command: "/sbin/installgrub -m -f /tmp/.be.ZfaGnc/boot/grub/stage1 /tmp/.be.ZfaGnc/boot/grub/stage2 /dev/rdsk/c3t5d0s0"
Output:
stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
stage1 written to partition 0 sector 0 (abs 256)
stage1 written to master boot sectorok, those tests show tempnames for mountpoints, is it actually correbt mountpoint? one way to make sure would to run for example with truss, but for this test I incremented version in BE capability file
root@testoi:~# beadm mount tank-1 /a
Mounted successfully on: '/a'
root@testoi:~# vi /a/boot/grub/capability
root@testoi:~# grep VERSION /a/boot/grub/capability
VERSION=26
root@testoi:~# grep VERSION /tank/boot/grub/capability
VERSION=25
root@testoi:~# beadm umount /a
Unmounted successfully
root@testoi:~# bootadm install-bootloader -v -P tank
Command: "/sbin/installgrub -m -f /tmp/.be.kjaqqc/boot/grub/stage1 /tmp/.be.kjaqqc/boot/grub/stage2 /dev/rdsk/c3t5d0s0"
Output:
stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
stage1 written to partition 0 sector 0 (abs 256)
stage1 written to master boot sector
root@testoi:~# grep VERSION /tank/boot/grub/capability
VERSION=26
update dependency on 80 (#6087)
Depends On: |
|
||
---|---|---|---|
Diff: |
Revision 2 (+368 -15) |
usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3) |
---|
Remove superfluous {}. (Or add them to the else - just be consistent)
Actually, these few lines could be written more clearly. E.g.,
if (ret) ret = BAM_ERROR; else ret = be_installboot(nvl) ? BAM_ERROR : 0;
usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3) |
---|
Why not: return (f());
You could even get rid of ret completely, and make the if: if (check_subcmd_and_options(...) == BAM_ERROR)
usr/src/man/man1m/bootadm.1m (Diff revision 4) |
---|
What is defaults in this context? How is this similar to beadm activate? Is running this command actually going to cause my current boot environment to change? If so, why is that?
In general, this could use a bit more information discussing when you would want to run this.
usr/src/man/man1m/bootadm.1m (Diff revision 4) |
---|
This is the first time in the manual page that boot blocks are mentioned. They need to be defined and explained in a way that an administrator can relate it to the act of installing the boot loader.
I'd instead initially just describe this in the already defined and generally known terms, that of installing the boot loader.
Afterwards, I'd go into the specific details of what this is going to do on each platform. What parts of what disks are actually going to be touched. Is it the MBR, something related to the EFI/GPT, etc.
usr/src/man/man1m/bootadm.1m (Diff revision 4) |
---|
Isn't bootblock really a SPARCism at the moment? Regardless, feels like it should be couched in terms of what the administrators know and if we're going to use bootblock, it should be discussed in detail and explained in the DESCRIPTION.
usr/src/man/man1m/bootadm.1m (Diff revision 4) |
---|
Presumably there aren't 'bootlock's. I think this would be better by describing what always happens on x86 and SPARC up in the initial discussion of the command and then only indicating the side effects that -M has here, rather than hiding a lot of the x86 semantics of this in the -M option.
usr/src/man/man1m/bootadm.1m (Diff revision 4) |
---|
I'd change the wording and fix up the grammar here. I'd write this as:
'In an install-bootloader operation, the bootloader is installed on the devices of the specified ZFS pool. If the -P option is not specified, then the current ZFS system boot pool is used instead.'
This is also the kind of information (the default) that I'd put up in the initial discussion of the command.
usr/src/man/man1m/bootadm.1m (Diff revision 4) |
---|
I'd again not refer to boot block files here, but I'd instead phrase this as:
'In an install-bootloader operation, the boot loader is still installed on the specified pool; however, the bootloader itself will come from the alternate root.
man page update. To ease the review, check also http://cr.illumos.org/~webrev/tsoome/6086/
Diff: |
Revision 5 (+374 -13) |
---|
man page updates regarding to Robert's suggestions and help, thanks a lot!
added more error messages about BE/pool availability. Current version was too silent when bootadm install-bootloader is used in environment where rootfs is not located on zfs pool (such as cd boot), and/or no BE is available.
Diff: |
Revision 6 (+405 -13) |
---|
squashed 6085 and 6086
Commit: |
|
||
---|---|---|---|
Diff: |
Revision 8 (+544 -34) |
added "const" qualifier for option string.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+544 -34) |