Page MenuHomeFreeBSD

Change the way efibootmgr works by specifying bootnum via -b parameter
ClosedPublic

Authored by bcran on Dec 24 2018, 5:01 AM.

Details

Summary

Instead of passing the bootnum to each different parameter, require users
to specify -b when running operations that need a bootnum.

This allows activation of a new boot entry at same same time it's created
by adding -a onto the command line.

This is obviously a breaking change.

Sponsored by: Netflix

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bcran created this revision.Dec 24 2018, 5:01 AM
bcran updated this revision to Diff 52281.Dec 24 2018, 6:48 AM

Update bsdinstall/bootconfig to use the -a parameter when creating the boot entry.
Fix some typos.
Try and improve the error message when EFI variables aren't supported: use err() to
print the underlying error.

imp added a comment.Dec 24 2018, 8:15 AM

Be sure to commit these separately.

imp added a comment.Dec 24 2018, 8:28 AM

I went back and forth on this when I was writing the code. I thought it was a more natural interface to say --activate #, --deactivate #, etc, rather that --activate --bootnum #. I see the use-case, though. It could be handled by a second invocation, but the original code was already confused over what set_active means (in some places using it as a boolean, other places using it and the boot number). So I think on the whole this is good, modulo my comments.

I'm also not that I'm super stoked about this interface. It's too old school. We should have something more like nvmecontrol, et al, that does efibootmgr create <create-args>, efibootmgr activate -b XXXXX <args>, etc. But that will have to wait for another day (and likely need the current setup around for backwards compat for a release or so).

usr.sbin/efibootmgr/efibootmgr.c
98 ↗(On Diff #52281)

ditto?

103 ↗(On Diff #52281)

does this need no_argument too?

105–106 ↗(On Diff #52281)

ditto?

356 ↗(On Diff #52281)

I'd move this to parse_args, with the other sanity checks like this.
I'd also trigger usage there as well to be consistent.

496 ↗(On Diff #52281)

ditto

910 ↗(On Diff #52281)

I don't like this change. It's not at all related, and it makes the error message worse, not better.

917 ↗(On Diff #52281)

gratuitous {} added.

bcran marked 7 inline comments as done.Dec 24 2018, 3:24 PM
bcran added inline comments.
usr.sbin/efibootmgr/efibootmgr.c
356 ↗(On Diff #52281)

I realized that after I've submitted the review.

910 ↗(On Diff #52281)

Agreed, I'll revert it.

bcran updated this revision to Diff 52296.Dec 24 2018, 3:32 PM
bcran marked 2 inline comments as done.

Remove unrelated changes, move error handling.
I'll fix the typos in a separate commit

imp added inline comments.Dec 24 2018, 5:13 PM
usr.sbin/efibootmgr/efibootmgr.c
178 ↗(On Diff #52296)

add [-a] here too.

bcran updated this revision to Diff 52299.Dec 24 2018, 5:18 PM

Add [-a] to create usage text

bcran added a comment.Dec 24 2018, 5:22 PM
In D18648#397712, @imp wrote:

I went back and forth on this when I was writing the code. I thought it was a more natural interface to say --activate #, --deactivate #, etc, rather that --activate --bootnum #. I see the use-case, though. It could be handled by a second invocation, but the original code was already confused over what set_active means (in some places using it as a boolean, other places using it and the boot number). So I think on the whole this is good, modulo my comments.

Yeah, I agree that it's a more natural interface the way it is, but given the Linux efibootmgr requires the -b parameter and activating the entry with a second invocation is rather clunky (especially in scripts) I think this change makes sense.

I'm also not that I'm super stoked about this interface. It's too old school. We should have something more like nvmecontrol, et al, that does efibootmgr create <create-args>, efibootmgr activate -b XXXXX <args>, etc. But that will have to wait for another day (and likely need the current setup around for backwards compat for a release or so).

At least it's mostly compatible with the Linux efibootmgr parameters, which I presume isn't accidental.

bcran marked an inline comment as done.Dec 26 2018, 3:14 PM
In D18648#397711, @imp wrote:

Be sure to commit these separately.

I will do.

imp accepted this revision.Dec 30 2018, 5:14 PM

Works for me. Again, I think it was a mistake to have flags for both verbs and options, but that's what linux did and will need to be fixed in a separate commit.

This revision is now accepted and ready to land.Dec 30 2018, 5:14 PM
This revision was automatically updated to reflect the committed changes.