Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement bootloader interface for GRUB #193

Closed
wants to merge 6 commits into from
Closed

Implement bootloader interface for GRUB #193

wants to merge 6 commits into from

Conversation

anpep
Copy link
Collaborator

@anpep anpep commented Feb 21, 2023 •
edited
Loading

This commit implements the new bootloader interface for GRUB (doc).

The aim is to support bootloader operations directly on Pebble, like snapd does.
Note that the grubenv package is taken directly from snapd and is meant to be merged in x-go in the near future.

@anpep anpep requested review from flotter, niemeyer, benhoyt, atesburak and mvo5 and removed request for niemeyer February 21, 2023 12:28
@benhoyt
Copy link
Contributor

benhoyt commented Feb 21, 2023

@anpep A couple of things:

  1. I don't really have enough context on this at the moment to review this. If you'd like me to review, can you please point me to specs/discussions/info to give background? The PR itself doesn't show how it'll be used in Pebble, as it's just the interface and new code, not wired up.
  2. This seems quite low-level and specific. I wonder if this kind of code would be better in a "bootloader" or "rocks" repo, or even x-go, and then imported here via Go modules? (Similar with the FAT/FS PR.) I know that's a trade-off, but depending on who needs to review/approve/etc, that approach may help you work more quickly? Let me know if this has already been discussed/decided.

@anpep
Copy link
Collaborator Author

anpep commented Feb 22, 2023

@anpep A couple of things:

  1. I don't really have enough context on this at the moment to review this. If you'd like me to review, can you please point me to specs/discussions/info to give background? The PR itself doesn't show how it'll be used in Pebble, as it's just the interface and new code, not wired up.

Sorry about that, should have included a link to the document in the first place. I've added it now.

  1. This seems quite low-level and specific. I wonder if this kind of code would be better in a "bootloader" or "rocks" repo, or even x-go, and then imported here via Go modules? (Similar with the FAT/FS PR.) I know that's a trade-off, but depending on who needs to review/approve/etc, that approach may help you work more quickly? Let me know if this has already been discussed/decided.

It is, and in a similar fashion with the partinfo PR, I intend it to be included in x-go instead of Pebble. However, since the x-go patch is not being merged very soon, I wanted to include these changes in Pebble (specially because these PRs need to be merged so that we can implement critical functionality for Termus), and move them out to x-go when it's ready (which will be trivial since I'm not adding any dependency to other Pebble packages).

Let me know what you think, I greatly appreciate your feedback!

@anpep anpep marked this pull request as draft February 22, 2023 17:32
benhoyt
benhoyt reviewed Feb 27, 2023
View reviewed changes
internal/bootloader/bootloader.go Show resolved Hide resolved
internal/bootloader/bootloader.go Outdated Show resolved Hide resolved
internal/bootloader/bootloader.go Outdated Show resolved Hide resolved
internal/bootloader/bootloader.go Outdated Show resolved Hide resolved
internal/bootloader/bootloader.go Outdated Show resolved Hide resolved
internal/bootloader/grubenv/grubenv.go Show resolved Hide resolved
internal/bootloader/grubenv/grubenv.go Show resolved Hide resolved
internal/bootloader/grubenv/grubenv.go Show resolved Hide resolved
internal/bootloader/grubenv/grubenv.go Show resolved Hide resolved
internal/bootloader/grubenv/grubenv.go Show resolved Hide resolved
@anpep
Remove access to boot variables from Bootloader
d5e8bad 
 * Remove `GetBootVars()`/`SetBootVars()` from the interface.
 * Update interface methods with more consistent Go naming conventions.
 * Unexport internal symbols.
 * Add an `export_test.go` file to the `bootloader` package so internal
   symbols remain accessible from within the tests for mocking.
 * Fix permissions bug leading to dangling files in tests.
 * `Status()` no longer returns an error.
 * `ActiveSlot()` no longer returns an error.
@anpep anpep requested a review from benhoyt March 1, 2023 11:39
benhoyt
benhoyt approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment about changing Find to return Bootloader (an interface type) instead of *Bootloader.

My comments on grubenv.go remain, but yeah, if we're copying that verbatim from snapd, might be better to leave as is for now.

internal/bootloader/bootloader.go Outdated Show resolved Hide resolved
@anpep anpep marked this pull request as ready for review March 2, 2023 07:45
@anpep anpep requested review from niemeyer and removed request for mvo5 March 2, 2023 08:36
flotter
flotter reviewed Mar 6, 2023
View reviewed changes
internal/bootloader/bootloader.go Outdated Show resolved Hide resolved
internal/bootloader/bootloader.go Show resolved Hide resolved
internal/bootloader/grub.go Outdated Show resolved Hide resolved
internal/bootloader/grub.go Outdated Show resolved Hide resolved
@anpep
Minor changes
7e0733f 
 * Remove `Status()` from interface for now.
 * Use underscores instead of dots for GRUB variables.
@anpep
Replace Present() with Find() in bootloader code
59d0ea6
@anpep anpep requested a review from flotter April 17, 2023 08:17
@anpep anpep self-assigned this Apr 18, 2023
@anpep anpep added the Priority Look at me first label Apr 18, 2023
@anpep anpep added Blocked Waiting for something external and removed Priority Look at me first labels May 22, 2023
@niemeyer
Copy link
Contributor

Per our agreements, closing as this is moving elsewhere now.

@niemeyer niemeyer closed this May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benhoyt benhoyt benhoyt approved these changes

@atesburak atesburak Awaiting requested review from atesburak

@niemeyer niemeyer Awaiting requested review from niemeyer

@flotter flotter Awaiting requested review from flotter

Assignees

@anpep anpep

Labels
Blocked Waiting for something external
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@anpep @benhoyt @niemeyer @flotter