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

units: introduce systemd-hibernate-clear.service that clears stale HibernateLocation EFI variable #32043

Merged
merged 7 commits into from Apr 3, 2024
Merged

units: introduce systemd-hibernate-clear.service that clears stale HibernateLocation EFI variable #32043

merged 7 commits into from Apr 3, 2024

Conversation

YHNdnzj
Copy link
Member

@YHNdnzj YHNdnzj commented Apr 1, 2024 •
edited

Fixes #32021

@YHNdnzj YHNdnzj added this to the v256 milestone Apr 1, 2024
@YHNdnzj YHNdnzj force-pushed the resume-clear-efi branch 2 times, most recently from b6dc97f to 8e71ea3 Compare April 1, 2024 12:16
@YHNdnzj YHNdnzj mentioned this pull request Apr 1, 2024
Closed
yuwata
yuwata reviewed Apr 2, 2024
View reviewed changes
src/basic/efivars.c Outdated Show resolved Hide resolved
src/hibernate-resume/hibernate-resume.c Show resolved Hide resolved
src/hibernate-resume/hibernate-resume.c Outdated Show resolved Hide resolved
units/systemd-hibernate-resume-clear-efi.service.in Outdated Show resolved Hide resolved
@YHNdnzj YHNdnzj force-pushed the resume-clear-efi branch 3 times, most recently from d3c2969 to 3438c78 Compare April 2, 2024 21:42
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 3, 2024
@YHNdnzj YHNdnzj added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 3, 2024
@YHNdnzj YHNdnzj mentioned this pull request Apr 3, 2024
Merged
@YHNdnzj YHNdnzj removed the please-review PR is ready for (re-)review by a maintainer label Apr 3, 2024
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Apr 3, 2024
yuwata
yuwata approved these changes Apr 3, 2024
View reviewed changes
src/hibernate-resume/hibernate-resume.c

/* Let's insist that the system identifier is verified still. After all if things don't match,
* the resume wouldn't get triggered in the first place. We should not erase the var if booted
* from LiveCD/portable systems/... */
Copy link
Member

Choose a reason for hiding this comment

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

After re-reading your reply in the above and this comment, yeah, that sounds reasonable.

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Apr 3, 2024
poettering
poettering reviewed Apr 3, 2024
View reviewed changes
src/hibernate-resume/hibernate-resume.c Outdated
@@ -32,6 +33,7 @@ static int help(void) {
"\n%sInitiate resume from hibernation.%s\n\n"
" -h --help Show this help\n"
" --version Show package version\n"
" --clear-efi Clear stale HibernateLocation EFI variable and exit\n"
Copy link
Member

Choose a reason for hiding this comment

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

Well, the "and exit" is redundant. Of course we exit after executing the command.

And I'd drop the reference to the name of the EFI variable. That smells a bit too much of an implementation detail. i.e. sounds great for the man page, but I would not stuff it into the --help text.

And this doesn't just clear stale info, it clears any info afaics, stale or not. Hence, don#t say "stale". I mean, it's hopefully typically used to drop stale info, but that's not really strictly the only case.

Hence maybe "Clear hibernation stroage information from EFI variables" or so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the "and exit" is redundant. Of course we exit after executing the command.

Hmm, I see an ambiguity here: people might assume that if and only if this option is specified, the efivar gets cleared before resuming. But it actually just clears it and quits.

@poettering
Copy link
Member

looks good, just some comments on naming things pretty much

@poettering poettering added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Apr 3, 2024
@YHNdnzj
hibernate-util: make clear_efi_hibernate_location_and_warn return 1 if
fbc8882 
actually cleared the variable
@YHNdnzj
hibernate-resume: EINVAL -> ENOTBLK where appropriate
667b0c4
@YHNdnzj
hibernate-resume-config: expose get_efi_hibernate_location
9fa0f58
@YHNdnzj
hibernate-resume: add options handling
89c727e
@YHNdnzj
hibernate-resume: introduce --clear for clearing hibernate storage var
8f8e481
@YHNdnzj
units: introduce systemd-hibernate-clear.service that clears
dfad86b 
stale HibernateLocation EFI variable

Currently, if the HibernateLocation EFI variable exists,
but we failed to resume from it, the boot carries on
without clearing the stale variable. Therefore, the subsequent
boots would still be waiting for the device timeout,
unless the variable is purged manually.

There's no point to keep trying to resume after a successful
switch-root, because the hibernation image state
would have been invalidated by then. OTOH, we don't
want to clear the variable prematurely either,
i.e. in initrd, since if the resume device is the same
as root one, the boot won't succeed and the user might
be able to try resuming again. So, let's introduce a
unit that only runs after switch-root and clears the var.

Fixes systemd#32021
@YHNdnzj
man/kernel-command-line: be clear that resumeflags= is about timeout …
5f0cd57 
…opts only
@YHNdnzj
Copy link
Member Author

YHNdnzj commented Apr 3, 2024

Addressed all comments, except for the "and exit" part.

@YHNdnzj YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed build-system meson good-to-merge/with-minor-suggestions labels Apr 3, 2024
@YHNdnzj YHNdnzj changed the title units: introduce systemd-hibernate-resume-clear-efi.service that clears stale HibernateLocation EFI variable units: introduce systemd-hibernate-clear.service that clears stale HibernateLocation EFI variable Apr 3, 2024
@yuwata yuwata merged commit 3a6bee0 into systemd:main Apr 3, 2024
42 of 48 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Apr 3, 2024
@YHNdnzj YHNdnzj deleted the resume-clear-efi branch April 3, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poettering poettering poettering left review comments

@yuwata yuwata yuwata approved these changes

Assignees
No one assigned
Labels
documentation hibernate-resume sleep units util-lib
Milestone
v256
Development

Successfully merging this pull request may close these issues.

Failed resume does not clear HibernateLocation EFI variable
3 participants
@YHNdnzj @poettering @yuwata