-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
dracut: add EFI kernel hook #22484
dracut: add EFI kernel hook #22484
Conversation
298ab48
to
51d137a
Compare
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 good to me
@sgn could you take a look at this one? |
fi | ||
|
||
if [ -z "${KERNEL_CMDLINE}" ] && [ "x${SKIP_KERNEL_CMDLINE}" != x1 ]; then | ||
echo "ERROR: You need to set the kernel command line in /etc/default/dracut-uefi-hook" |
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.
Is this logic necessary?
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.
It's solving the issue that if dracut doesn't find a kernel_cmdline
entry in a dracut.conf
file and isn't passed a cmdline in the command line argumets, it will prompt the user for a 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 couldn't think of another way of solving this particular issue.
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.
The other way of solving this would be always passing at least an empty string to the command line parameter. But it doesn't protect the user from dumb mistakes.
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 seems like a good option, provided it has no unanticipated side effects.
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 would still prefer erroring out, but if you think it's fine.
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.
Hmm there's also the use case without secure boot that allows the bundle to use the cmdline from refind/grub, so the command line could be empty.
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.
Just write a warning in the configuration file, and let the user see the boot failure 🤷
Don't bother with it, I would say.
exit 1 | ||
fi | ||
|
||
OPTIONS="${OPTIONS} --force --uefi ${UEFI_BUNDLE_DIR:=boot/efi/EFI/Void}/linux-${VERSION}.efi ${VERSION}" |
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.
mkdir -p "$UEFI_BUNDLE_DIR"
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.
Maybe void
lowercase v
is better.
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.
And I'm thinking about supporting this in sbsigntool
and refind
hooks.
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.
And I'm thinking about supporting this in sbsigntool and refind hooks.
How would this look?
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.
Force them read this hooks if exists, maybe.
Rather messy but I don't like different parts of system refusing co-operating with each others. Just make sure variable introduced by this change doesn't overlap with them, I can make it works.
I like this idea, (so I can sign the whole efi, my initrd is unsigned, now) but I don't use gummiboot, for example.
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.
Something like this to prepare for it
sgn@9bbcb81
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.
Cool! I like the idea.
DRACUT_OPTIONS="${DRACUT_OPTIONS} --force --uefi ${UEFI_BUNDLE_DIR:=boot/efi/EFI/void}/linux-${VERSION}.efi ${VERSION}" | ||
mkdir -p "${UEFI_BUNDLE_DIR}" | ||
|
||
dracut -q --kernel-cmdline="${KERNEL_CMDLINE}" ${DRACUT_OPTIONS} |
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.
dracut -q ${KERNEL_CMDLINE:+--kernel-cmdline="${KERNEL_CMDLINE}"} ${DRACUT_OPTIONS}
looks better?
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 think the issue is that, if you don't set kernel_cmdline
in the config file or the dracut
command line, the user will be prompted for a value, which will stall the hook. @ericonr, is this correct?
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.
It does!
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.
Gah I hadn't seen @ahesford's comment.
So, that one does look better, and if I add the -q
argument to dracut it simply plows ahead and gets an empty cmdline. So it should be fine.
fi | ||
|
||
if [ -z "${KERNEL_CMDLINE}" ] && [ "x${SKIP_KERNEL_CMDLINE}" != x1 ]; then | ||
echo "ERROR: You need to set the kernel command line in /etc/default/dracut-uefi-hook" |
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.
Just write a warning in the configuration file, and let the user see the boot failure 🤷
Don't bother with it, I would say.
Also made some small formatting changes. Close: void-linux#22484
Updated again. |
Also made some small formatting changes. Close: void-linux#22484
Also made some small formatting changes. Close: void-linux#22484
Also made some small formatting changes. Close: void-linux#22484
Also made some small formatting changes. Close: void-linux#22484
Is this ready? Or are we better to split the uefi bundle hook and gummiboot? If this is ready, I'll merge this in, without worrying about other option. |
I don't think it's fully ready, the post remove hook isn't using
? Should it exit with 1 and show an error, since having the hook installed but not dracut is an issue? |
exit 0 | ||
fi | ||
|
||
: "${UEFI_BUNDLE_DIR:=boot/efi/EFI/Void}" |
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.
Lowercase V
in Void
.
Since grub will create this very same directory with lowercase void
.
I'm not sure what they will do if they're conflicting now.
And I couldn't say about their future.
53da9d8
to
fb548b7
Compare
If this hook is included with But, this hook depends on both
I, myself, don't care about the broken system if their owners choose to break it |
I agree. It is mostly following the style of the other hook, that's why it has the check.
Hmm that might be a better way of doing it. We mention the dependency in the config file, at least. Would it be okay to have a subpackage only for the hook and the conf file? |
On 2020-07-27 05:59:09-0700, Érico Nogueira Rolim ***@***.***> wrote:
> If this hook is included with dracut, this check is likely unnecessary.
I agree. It is mostly following the style of the other hook, that's why it has the check.
> I'm not sure if it's better to split it out and explicitly depends on gummiboot?
Hmm that might be a better way of doing it. We mention the
dependency in the config file, at least. Would it be okay to have
a subpackage only for the hook and the conf file?
I support for subpackage, don't know about other.
We have a ton of meta package already,
and despite this package is a single POSIX script and a config,
it must be installed in system level.
Still better than `sx` which is single 4x line bash script,
which I want to remove badly.
…--
Danh
|
Heh, ok! Will split it off. |
Done. |
Looks good. I hope you don't mind to wait for few days to let other people complain :D |
No worries, I'm not even using it any more :p Thanks! Edit: I am not using on my main device, I am indeed using it on my old one. |
Added binutils to dracut-uefi, it needs it for the |
srcpkgs/dracut/template
Outdated
i686*|x86_64*|aarch64*) # EFI bundle config and hooks | ||
_efi_bundle=1 | ||
subpackages+=" dracut-uefi" |
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.
uefi-bundle
is not supported in aarch64. (/usr/bin/dracut:1140
)
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.
Oh, good find. Wonder if they forgot to add aarch64 or if it doesn't work.
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 guess it's ready, I'll pull this patch in dracutdevs/dracut@6184c7a
It seems like it's already there? But, somehow wasn't pulled in
Restricted to i686* and x86_64* for now. Other changes: - small formatting changes - use relative path in kernel-hook-postrm
vmove usr/lib/dracut/modules.d/${f} | ||
done | ||
} | ||
} | ||
|
||
dracut-uefi_package() { | ||
depends="binutils gummiboot dracut-${version}_${revision}" |
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.
The dracut UEFI bundle requires a separate UEFI stub provided (in our case) by gummiboot.
Also made some small formatting changes.
@ahesford