- Download Diff

Summary:

6086 add install bootblock option for bootadm

Review Request #79 — Created July 26, 2015 and submitted Sept. 16, 2015, 9:40 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	6086
Depends On:	78, 80
Blocks:	85, 89, 90
Commit:	b5cd118...
Reviewers
Groups:	general
People:

Description

6086 add install bootblock option for bootadm

Testing Done

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 sector
mount 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 sector
specify 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 sector
ok, 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

Issues

0
0
12
0
All Issues: 12

Description	From	Last Updated
cstyle... != NULL	jeffpc	Aug. 31, 2015, 5:41 p.m.
Remove superfluous {}. (Or add them to the else - just be consistent) Actually, these few lines could be written ...	jeffpc	Aug. 31, 2015, 5:41 p.m.
Why not: return (f()); You could even get rid of ret completely, and make the if: if (check_subcmd_and_options(...) == BAM_ERROR)	jeffpc	Aug. 31, 2015, 5:41 p.m.
This seems unrelated to what's in the bug report.	jeffpc	Aug. 31, 2015, 5:41 p.m.
This seems unrelated to what's in the bug report.	jeffpc	Aug. 31, 2015, 5:41 p.m.
What is defaults in this context? How is this similar to beadm activate? Is running this command actually going to ...	rm	Sept. 11, 2015, 8:39 p.m.
This is the first time in the manual page that boot blocks are mentioned. They need to be defined and ...	rm	Sept. 11, 2015, 8:39 p.m.
Isn't bootblock really a SPARCism at the moment? Regardless, feels like it should be couched in terms of what the ...	rm	Sept. 11, 2015, 8:39 p.m.
Presumably there aren't 'bootlock's. I think this would be better by describing what always happens on x86 and SPARC up ...	rm	Sept. 11, 2015, 8:39 p.m.
I'd change the wording and fix up the grammar here. I'd write this as: 'In an install-bootloader operation, the bootloader ...	rm	Sept. 11, 2015, 8:39 p.m.
I'd again not refer to boot block files here, but I'd instead phrase this as: 'In an install-bootloader operation, the ...	rm	Sept. 11, 2015, 8:39 p.m.
Minor nit: these can be const	jeffpc	Sept. 13, 2015, 12:59 p.m.

Change Summary:

update dependency on 80 (#6087)

Depends On:

+	~~80 - 6087 bootadm message.h is hiding errors from lint~~

Diff:

Revision 2 (+368 -15)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/man/man1m/bootadm.1m

Change Summary:

options string was missing i: for install_bootloader

Diff:

Revision 3 (+368 -15)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/man/man1m/bootadm.1m

Ship it!

```
Ship It!
```

tsoome Aug. 31, 2015, 5:40 p.m.

also, after this update, dmake was clean, dmake lint was clean and git-pbchk did not complain.

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)
Show all issues
```
cstyle... != NULL
```
1. tsoome Aug. 31, 2015, 5:37 p.m.
  yep, and not only on this line, there were few more instances.

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)

Show all issues

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;

tsoome Aug. 31, 2015, 5:36 p.m.
```
good suggestion, updated.
```

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)

Show all issues

Why not: return (f());
You could even get rid of ret completely, and make the if: if (check_subcmd_and_options(...) == BAM_ERROR)

tsoome Aug. 31, 2015, 5:36 p.m.
```
updated, yes its more clean this way.
```

usr/src/man/man1m/bootadm.1m (Diff revision 3)

Show all issues

This seems unrelated to what's in the bug report.

tsoome Aug. 31, 2015, 5:35 p.m.

fixed. I'll open separate bug for those 2 options.

usr/src/man/man1m/bootadm.1m (Diff revision 3)
Show all issues
```
This seems unrelated to what's in the bug report.
```
1. tsoome Aug. 31, 2015, 5:34 p.m.
  fixed.

Change Summary:

updates regarding to comments from Josef Sipek

Diff:

Revision 4 (+361 -13)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/man/man1m/bootadm.1m

Ship it!

```
Ship It!
```

rm Sept. 2, 2015, 11:17 p.m.

Just a general note on all this, while it generally looks good, I think the manual page can be cleaned up quite a lot to actually reflect what's going on and tightened up. At least, as someone who nominally administers some systems, I was confused by a lot of what it was referring to.

usr/src/man/man1m/bootadm.1m (Diff revision 4)

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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.

Change Summary:

man page update. To ease the review, check also http://cr.illumos.org/~webrev/tsoome/6086/

Diff:

Revision 5 (+374 -13)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/man/man1m/bootadm.1m

Change Summary:

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)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/man/man1m/bootadm.1m

Change Summary:

minor updates to man and removed unneeded note.

Diff:

Revision 7 (+398 -13)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/man/man1m/bootadm.1m

Change Summary:

squashed 6085 and 6086

Commit:

+	abb5334d305a66a9eb66ebb72ad73ed206c69474

Diff:

Revision 8 (+544 -34)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/lib/libbe/common/be_activate.c
	usr/src/lib/libbe/common/libbe.h
	usr/src/lib/libbe/common/mapfile-vers
	usr/src/man/man1m/bootadm.1m

Ship it!

```
Ship It!
```

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 8)
Show all issues
```
Minor nit: these can be const
```

Change Summary:

added "const" qualifier for option string.

Commit:

-	abb5334d305a66a9eb66ebb72ad73ed206c69474
+	b5cd118726478622226df928e4afba00f66c3232

Diff:

Revision 9 (+544 -34)

Show changes

	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/lib/libbe/common/be_activate.c
	usr/src/lib/libbe/common/libbe.h
	usr/src/lib/libbe/common/mapfile-vers
	usr/src/man/man1m/bootadm.1m

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

6086 add install bootblock option for bootadm

Change Summary:

Depends On:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)