- Download Diff

Summary:

6702 libbe should support x86 installboot command

Review Request #162 — Created March 1, 2016 and submitted Sept. 23, 2016, 5:18 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	6702
Depends On:
Reviewers
Groups:	general
People:

Description

Testing Done

root@uefi-oi:/home/tsoome# beadm activate -v uefi-139
be_do_installboot: device mirror-0
be_do_installboot: device c3t0d0
Command: "/usr/sbin/installboot -m -f //boot/pmbr //boot/gptzfsboot /dev/rdsk/c3t0d0s1"
Errors:
bootblock version installed on /dev/rdsk/c3t0d0s1 is more recent or identical
Use -F to override or install without the -u option
be_do_installboot: device c3t1d0
Command: "/usr/sbin/installboot -m -f //boot/pmbr //boot/gptzfsboot /dev/rdsk/c3t1d0s1"
Errors:
bootblock version installed on /dev/rdsk/c3t1d0s1 is more recent or identical
Use -F to override or install without the -u option
Activated successfully
root@uefi-oi:/home/tsoome#

Issues

0
0
1
2
All Issues: 3

Description	From	Last Updated
Why not make it } else if ... and skip re-indentation? Or make it look like the block starting at ...	yuripv	March 25, 2016, 12:02 p.m.
I keep thinking I'd prefer this be else if (be_has_loader()) or whatever, to make things clearer, but I'm not really ...	richlowe	Aug. 9, 2016, 9:26 p.m.
Is there really no better way to do that?	richlowe	Aug. 9, 2016, 9:26 p.m.

usr/src/lib/libbe/common/be_activate.c (Diff revision 1)

Not your code, but wonder why is it needed? We are leaking the original memory pointed to by path.

tsoome March 25, 2016, 11:54 a.m.

actually there is different bug. strdup() itself is needed as the code later will modify the path by inserting '\0', so that part is ok.
it is also ok to assign like its done on line 910, because the path is initialized by nvlist_lookup_string() and that space is released by nvlist_free(), but the bug is at line 904, that free() must not be there.

yuripv March 25, 2016, 6:25 p.m.
```
Oh ok, I'm always forgetting that.
```

usr/src/lib/libbe/common/be_activate.c (Diff revision 1)

Show all issues

Why not make it } else if ... and skip re-indentation?
Or make it look like the block starting at line 934:
if (be_is_isa("i386")) {

  if (be_has_grub()) {

    / grub /

  } else {

    / new loader /

  }

} else if (be_is_isa("sparc")) {

    / sparc /

} else {

    / unsupported /

}

tsoome March 25, 2016, 11:54 a.m.

I like the second option better, will keep the similar logic as used before and is reasonably clean approach.

Change Summary:

changes suggested by Yuri.

Diff:

Revision 2 (+35 -14)

Show changes

	usr/src/lib/libbe/common/be_activate.c
	usr/src/lib/libbe/common/libbe_priv.h

Ship it!

```
Ship It!
```

Change Summary:

rebase on 6314 buffer overflow in dsl_dataset_name

Diff:

Revision 3 (+35 -14)

Show changes

	usr/src/lib/libbe/common/be_activate.c
	usr/src/lib/libbe/common/libbe_priv.h

usr/src/lib/libbe/common/be_activate.c (Diff revision 3)

Show all issues

I keep thinking I'd prefer this be else if (be_has_loader()) or whatever, to make things clearer, but I'm not really sure why

tsoome July 28, 2016, 9:42 p.m.

I don't really have any brilliant ideas about it. Tbh, I would leave it as is right now, as it does the work to distinguish between two known case, if its not good enough, can always fix later:)

usr/src/lib/libbe/common/be_activate.c (Diff revision 3)
Show all issues
```
Is there really no better way to do that?
```

Change Summary:

Rewrite of pool component loop.

Testing Done:

	+	root@uefi-oi:/home/tsoome# beadm activate -v uefi-139
	+	be_do_installboot: device mirror-0
	+	be_do_installboot: device c3t0d0
	+	Command: "/usr/sbin/installboot -m -f //boot/pmbr //boot/gptzfsboot /dev/rdsk/c3t0d0s1"
	+	Errors:
	+	bootblock version installed on /dev/rdsk/c3t0d0s1 is more recent or identical
	+	Use -F to override or install without the -u option
	+	be_do_installboot: device c3t1d0
	+	Command: "/usr/sbin/installboot -m -f //boot/pmbr //boot/gptzfsboot /dev/rdsk/c3t1d0s1"
	+	Errors:
	+	bootblock version installed on /dev/rdsk/c3t1d0s1 is more recent or identical
	+	Use -F to override or install without the -u option
	+	Activated successfully
	+	root@uefi-oi:/home/tsoome#

Diff:

Revision 4 (+64 -20)

Show changes

	usr/src/lib/libbe/common/be_activate.c
	usr/src/lib/libbe/common/libbe_priv.h

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

6702 libbe should support x86 installboot command

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Testing Done:

Diff:

Status: Closed (submitted)