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.

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

Handle default bootloader entry patterns correctly #188

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

benmaddison
Copy link
Contributor

@benmaddison benmaddison commented Jul 5, 2022

Resolves #183

This update implements matching of boot entry names against the pattern in the
default option from loader/loader.conf, intended to be compatible with the
behaviour of systemd-boot.

Outstanding work

@rbradford
Copy link
Member

@benmaddison Looks like a great start. I'm very happy with what you've done so far.

Some minor things:

  1. Could you split the comment updates/reformatting into a separate commit
  2. Try and follow the existing commit message style (with the starting "fat:" etc..)
  3. Don't forget your "-s"

On the "slice_take" thing. I don't know ... do you think it's on a path for staying around or might be dropped...it seems quite a stale nightly feature with not much activity.

I would also prefer to fix the broken OS support right now than perhaps addressing missing feature gaps unless they are in active use (that could be in a follow-up!)

@benmaddison
Copy link
Contributor Author

Some minor things:

1. Could you split the comment updates/reformatting into a separate commit

2. Try and follow the existing commit message style (with the starting "fat:" etc..)

3. Don't forget your "-s"

Yup - I plan to rebase and squash before asking for a final review

On the "slice_take" thing. I don't know ... do you think it's on a path for staying around or might be dropped...it seems quite a stale nightly feature with not much activity.

I'm not sure that I know the answer to that. I'll take another look at it to see if I can re-implement that loop without loosing too much readability.

I would also prefer to fix the broken OS support right now than perhaps addressing missing feature gaps unless they are in active use (that could be in a follow-up!)

Ack. Agreed.

Signed-off-by: Ben Maddison <benm@workonline.africa>
Implement matching of boot entry names against the pattern in the
default option from `loader/loader.conf`.

Signed-off-by: Ben Maddison <benm@workonline.africa>
@benmaddison
Copy link
Contributor Author

Open issues for review:

  • I have kept the use of slice_take: I'm not seeing a way of avoiding it without effectively re-implementing that feature here. I'm happy to commit to doing the re-factor in future if the feature is dropped or this project wants to move to stable before the feature is stabilized.
  • find_entry will currently bail out with an error if a call to compare_entry fails (due to an unterminated string), rather than falling back to a valid entry that was found earlier. Making that happen is not hard, but will be verbose, and I'm not sure it's worth the bother since that can only really happen if the FAT FS is broken. Guidance welcome.

@rbradford
Copy link
Member

I would love to test with this an integration test? Is there a cloud image I can use?

Open issues for review:

* I have kept the use of `slice_take`: I'm not seeing a way of avoiding it without effectively re-implementing that feature here. I'm happy to commit to doing the re-factor in future if the feature is dropped or this project wants to move to stable before the feature is stabilized.

👍

* `find_entry` will currently bail out with an error if a call to `compare_entry` fails (due to an unterminated string), rather than falling back to a valid entry that was found earlier. Making that happen is not hard, but will be verbose, and I'm not sure it's worth the bother since that can only really happen if the FAT FS is broken. Guidance welcome.

👍 I don't think we need to worry about dealing with a corrupt FAT.

Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Really happy with this. Just some very minor nits.

src/fat.rs Outdated Show resolved Hide resolved
src/fat.rs Outdated Show resolved Hide resolved
src/loader.rs Outdated Show resolved Hide resolved
Signed-off-by: Ben Maddison <benm@workonline.africa>
@benmaddison
Copy link
Contributor Author

I have group_imports = StdExternalCrate in my local fallback rustfmt.toml file, which was why that grouping was occurring.

To ensure that doesn't happen to future contributors (incl. me), I have dropped an empty rustfmt.toml into the repo root, so that the defaults get used.

@benmaddison
Copy link
Contributor Author

I would love to test with this an integration test? Is there a cloud image I can use?

The NixOS image that I was using when I found the issue is locally generated, and the downloadable ones ship with grub.

I've had a search around for other options. So far I've checked ubuntu, fedora and arch: they all use grub :-(

I think the best I can offer is to generate a minimal nixos one, and store it in the repo with LFS, but it will make cloning unpleasant.

@rbradford
Copy link
Member

I have group_imports = StdExternalCrate in my local fallback rustfmt.toml file, which was why that grouping was occurring.

To ensure that doesn't happen to future contributors (incl. me), I have dropped an empty rustfmt.toml into the repo root, so that the defaults get used.

Ah, cool.

@rbradford rbradford merged commit 396bc4e into cloud-hypervisor:main Aug 1, 2022
@benmaddison benmaddison deleted the default-entry-wip branch August 1, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NixOS with systemd-boot broken in v0.4
2 participants