-
Notifications
You must be signed in to change notification settings - Fork 31
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
Support kexec reboot for grub2-bls and sdboot #126
Conversation
dc614ed
to
ff8b0ee
Compare
c74dad1
to
ed3116f
Compare
I can confirm that this PR works now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good now, just a few nits.
lib/BlsEntry.cpp
Outdated
std::string kernel{}; | ||
std::string initrd{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialization with braces is not needed here / doesn't make any difference...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
lib/BlsEntry.cpp
Outdated
std::string initrd{}; | ||
while(getline(is, str)) | ||
{ | ||
TransactionalUpdate::Util::trim(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in the TransactionalUpdate namespace already (same for the other occurrences in the patch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
lib/Reboot.cpp
Outdated
kernel = efi / std::filesystem::path(kernel).relative_path(); | ||
initrd = efi / std::filesystem::path(initrd).relative_path(); | ||
} | ||
command = "kexec --kexec-syscall-auto -l " + kernel + " --initrd=" + initrd + " --reuse-cmdline;"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that now, with the BLS, in theory the kernel / initrd entries could also contain things like spaces, e.g. a pathname such as My System/6.9.9-1-default/initrd? Or can we be sure that the subdirectory will always be without whitespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is correct, FAT32 can have spaces in the directory and file names. sdbootutil does not generate them tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it is better to add quotes Just in case this might happen. Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now the user could add a '
to the file or directory name ;-) If we wanted to protect ourselves, then every occurrence of '
would have to be replaced with '\''
.
Otherwise I'll just merge tomorrow.
8c157ff
to
20cb4ae
Compare
20cb4ae
to
019ac90
Compare
No description provided.