| Summary: | Add 'update' action to kernel module | ||
|---|---|---|---|
| Product: | Gentoo Hosted Projects | Reporter: | Florian Schmaus <flow> |
| Component: | eselect | Assignee: | Gentoo eselect Team <eselect> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | Keywords: | InVCS |
| Priority: | Normal | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Attachments: |
0001-Add-update-action-to-kernel-module.patch
0001-Add-update-action-to-kernel-module.patch 0001-Add-update-action-to-kernel-module.patch 0001-Add-update-action-to-kernel-module.patch 0001-Add-update-action-to-kernel-module.patch Couldn't update the kernel symlink" Couldn't update the kernel symlink" 0001-Add-update-action-to-kernel-module.patch 0001-Add-update-action-to-kernel-module.patch 0001-Add-update-action-to-kernel-module.patch |
||
|
Description
Florian Schmaus
2023-03-14 12:03:22 UTC
Looks good. One tiny comment: uname --kernel-release is a GNUism. I suggest to use -r instead which is the POSIX option. Created attachment 857673 [details, diff]
0001-Add-update-action-to-kernel-module.patch
Use -r instead of --kernel-release when invoking uname.
Documentation in man/kernel.eselect.5 has to be updated as well. Created attachment 857685 [details, diff]
0001-Add-update-action-to-kernel-module.patch
(In reply to Florian Schmaus from comment #4) > Created attachment 857685 [details, diff] [details, diff] > 0001-Add-update-action-to-kernel-module.patch Sorry, I have more comments: - The exit status of uname should be caught. - In the for loop, "${targets[@]}" should be quoted (to be whitespace-safe). - Variable current_kernel_symlink_target is neither declared nor assigned anywhere. Typo for running_kernel_symlink_target? - At the end of do_update(), collecting and showing target_list is not how other modules do things. Also, the user will typically execute "eselect kernel list" anyway, in order to get the numbers. So, I'd much prefer something simple like this: write_error_msg \ "No sources for running kernel ${running_kernel_release} found." write_error_msg "\"eselect kernel list\" will show available sources." die -q "Couldn't update the kernel symlink" - In the man page, please order actions alphabetically. That is, "update" should go last. Created attachment 857715 [details, diff]
0001-Add-update-action-to-kernel-module.patch
Sorry, today is not the best day for me to write code. I fixed all, but one of your concerns, plus additional things reported by shellcheck.
I really would like to display the list of available sources if the 'update' action fails to find a suitable one. It is a good service to the user to display some more context when an operation fails and hence provides a better UX. That other actions do to behave similarly is more an argument that those should be changed and not that this action should follow that undesirable pattern.
Created attachment 857717 [details, diff]
0001-Add-update-action-to-kernel-module.patch
Fixed trailing newline when listing available sources.
(In reply to Florian Schmaus from comment #6) > I really would like to display the list of available sources if the 'update' > action fails to find a suitable one. It is a good service to the user to > display some more context when an operation fails and hence provides a > better UX. That other actions do to behave similarly is more an argument > that those should be changed and not that this action should follow that > undesirable pattern. Sorry, but that's not going to happen. Error messages are concise in all modules that are part of eselect itself and in most external modules. If you really insist on this, the "eselect way" of doing it would be along the lines of this: write_error_msg \ "No sources for running kernel ${running_kernel_release} found." is_output_mode brief || do_list >&2 die -q "Couldn't update the kernel symlink" do_list shows the list of targets, with the target numbers and with proper coloring. This should be suppressed though if the --brief option was specified. (And yes, it will call find_targets() again. That's acceptable in the error case, which should be rare.) OK, I did some tests. Things seem to work well, except for one minor problem: If the sources of the running kernel have been removed, but the symlink still points to them (i.e. the link is dead), then the action will succeed.
This will fix it:
--- a/modules/kernel.eselect
+++ b/modules/kernel.eselect
@@ -146,9 +146,7 @@ do_update() {
die -q "${EROOT}/usr/src/linux exists but is not a symlink"
fi
- if [[ $1 == ifunset \
- && -L ${EROOT}/usr/src/linux \
- && -e ${EROOT}/usr/src/linux ]]; then
+ if [[ $1 == ifunset && -e ${EROOT}/usr/src/linux ]]; then
# The /usr/src/linux symlink exists, points to a path that
# exists, and 'ifunset' is provided. Nothing to do.
return
@@ -159,7 +157,7 @@ do_update() {
running_kernel_release=$(uname -r) || die -q "uname failed with $?"
local running_kernel_symlink_target="linux-${running_kernel_release}"
- if [[ -L ${EROOT}/usr/src/linux ]]; then
+ if [[ -e ${EROOT}/usr/src/linux ]]; then
local current_target
current_target=$(basename "$(canonicalise "${EROOT}/usr/src/linux")")
if [[ ${current_target} == "${running_kernel_symlink_target}" ]]; then
(The case that -e is true but -L is false cannot happen, because of the sanity check at the begin of the function.)
Created attachment 857807 [details, diff]
Couldn't update the kernel symlink"
No need to be sorry. It is just a technical disagreement. But in the end it appears we could reach consensus using the approach you have shown. And thanks for testing the code.
Attached is the latest patch will your latest suggestions incorporated.
Created attachment 857809 [details, diff]
Couldn't update the kernel symlink"
Fixing the patch description…
Created attachment 857811 [details, diff]
0001-Add-update-action-to-kernel-module.patch
Created attachment 857813 [details, diff]
0001-Add-update-action-to-kernel-module.patch
Moved the the "! -L" and "ifunset" checks under the "-e /usr/src/linux" condition, avoiding an additional "-e" and making the code more readable.
Created attachment 857815 [details, diff]
0001-Add-update-action-to-kernel-module.patch
Remove unused 'target_list' variable (leftover from previous versions).
(In reply to Florian Schmaus from comment #14) > Created attachment 857815 [details, diff] [details, diff] > 0001-Add-update-action-to-kernel-module.patch > + if [[ -e ${EROOT}/usr/src/linux && ]]; then Spurious && here. I have fixed it locally. The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/eselect.git/commit/?id=d5bd4e5f8d28c3fc6e7d15d639538ac9b6459e7a commit d5bd4e5f8d28c3fc6e7d15d639538ac9b6459e7a Author: Florian Schmaus <flow@gentoo.org> AuthorDate: 2023-03-14 10:02:59 +0000 Commit: Ulrich Müller <ulm@gentoo.org> CommitDate: 2023-03-15 11:04:22 +0000 New "update" action in kernel module * modules/kernel.eselect (do_update, describe_update) (describe_update_options): New action, attempts to update the /usr/src/linux symlink to point to the sources of the running kernel. Bug 901209. * man/kernel.eselect.5: Document it. Thanks to ulm for helpful suggestions when working on this functionality. Bug: https://bugs.gentoo.org/901209 Signed-off-by: Florian Schmaus <flow@gentoo.org> [Tweaked bash syntax. Fixed highlighting in man page.] Signed-off-by: Ulrich Müller <ulm@gentoo.org> ChangeLog | 6 +++++ man/kernel.eselect.5 | 16 ++++++++++-- modules/kernel.eselect | 71 +++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 3 deletions(-) AFAICS everything works, but can you double-check with eselect-9999 before I make a new release? I also did some testing with -9999 and "eselect kernel update" behaves as expected in the tested cases. The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=015d7e1648ba32072eed926ee3009a3adf9037a1 commit 015d7e1648ba32072eed926ee3009a3adf9037a1 Author: Ulrich Müller <ulm@gentoo.org> AuthorDate: 2023-03-20 17:27:09 +0000 Commit: Ulrich Müller <ulm@gentoo.org> CommitDate: 2023-03-20 17:27:09 +0000 app-admin/eselect: add 1.4.22 Closes: https://bugs.gentoo.org/901205 Closes: https://bugs.gentoo.org/901209 Signed-off-by: Ulrich Müller <ulm@gentoo.org> app-admin/eselect/Manifest | 1 + app-admin/eselect/eselect-1.4.22.ebuild | 58 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) |