-
Notifications
You must be signed in to change notification settings - Fork 3
dkms prerm fix #5
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
Conversation
|
TAG Bot TAG: 3.1.0-2 |
|
Hi @shy129. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Pull Request Overview
This PR fixes a bug in DKMS kernel removal scripts where parsing of dkms status output would fail when the output contains additional text like (original_module exists). The fix replaces IFS-based field splitting with explicit field extraction using shell utilities.
Key Changes
- Replaced IFS-based parsing with line-by-line processing and explicit field extraction using
cutandawk - Applied the fix to both
kernel_prerm.d_dkmsandkernel_prerm.d_dkms.intemplate files - Added changelog entries documenting the bug fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| kernel_prerm.d_dkms.in | Updated parsing logic to handle status output with extra annotations like "(original_module exists)" |
| kernel_prerm.d_dkms | Same parsing fix applied to the non-template version |
| debian/changelog | Added two new changelog entries documenting the modifications and bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
debian/changelog
Outdated
| * | ||
|
|
Copilot
AI
Nov 7, 2025
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.
There is an empty bullet point (line 4) in the changelog entry. This should either be removed or filled with a meaningful description of the changes.
| * |
kernel_prerm.d_dkms.in
Outdated
| name=$(echo "$line" | cut -d'/' -f1) | ||
| vers=$(echo "$line" | cut -d'/' -f2 | cut -d',' -f1) | ||
| arch=$(echo "$line" | awk -F', ' '{print $3}' | cut -d':' -f1) | ||
| status=$(echo "$line" | awk -F': ' '{print $2}' | awk '{print $1}') |
Copilot
AI
Nov 7, 2025
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 parsing logic creates multiple subshells with echo commands and pipes for each variable extraction. This approach is inefficient as it spawns multiple processes (echo, cut, awk) for each line in the loop. Consider using a single awk command or bash parameter expansion to extract all fields at once, which would be more performant and maintainable.
For example:
dkms status -k "$inst_kern" 2>/dev/null | while IFS= read -r line; do
# Extract fields using awk in a single call
eval $(echo "$line" | awk -F'[/,:]' '{
gsub(/^[ \t]+|[ \t]+$/, "", $1); gsub(/^[ \t]+|[ \t]+$/, "", $2);
gsub(/^[ \t]+|[ \t]+$/, "", $3); gsub(/^[ \t]+|[ \t]+$/, "", $4);
printf "name=\"%s\" vers=\"%s\" arch=\"%s\" status=\"%s\"", $1, $2, $3, $4
}')
status=$(echo "$status" | awk '{print $1}')
# ... rest of logic
done
Or using bash parameter expansion:
dkms status -k "$inst_kern" 2>/dev/null | while IFS= read -r line; do
# Extract module/version part
mv="${line%%,*}"
name="${mv%/*}"
vers="${mv#*/}"
# Extract remaining parts
rest="${line#*, }"
arch="${rest%%,*}"
status="${rest#*, }"
status="${status#*: }"
status="${status%% *}"
# ... rest of logic
done
| name=$(echo "$line" | cut -d'/' -f1) | |
| vers=$(echo "$line" | cut -d'/' -f2 | cut -d',' -f1) | |
| arch=$(echo "$line" | awk -F', ' '{print $3}' | cut -d':' -f1) | |
| status=$(echo "$line" | awk -F': ' '{print $2}' | awk '{print $1}') | |
| # Efficient field extraction using Bash parameter expansion | |
| mv="${line%%,*}" | |
| name="${mv%/*}" | |
| vers="${mv#*/}" | |
| rest="${line#*, }" | |
| arch="${rest%%,*}" | |
| status="${rest#*: }" | |
| status="${status%% *}" |
kernel_prerm.d_dkms
Outdated
| name=$(echo "$line" | cut -d'/' -f1) | ||
| vers=$(echo "$line" | cut -d'/' -f2 | cut -d',' -f1) | ||
| arch=$(echo "$line" | awk -F', ' '{print $3}' | cut -d':' -f1) | ||
| status=$(echo "$line" | awk -F': ' '{print $2}' | awk '{print $1}') |
Copilot
AI
Nov 7, 2025
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 parsing logic creates multiple subshells with echo commands and pipes for each variable extraction. This approach is inefficient as it spawns multiple processes (echo, cut, awk) for each line in the loop. Consider using a single awk command or bash parameter expansion to extract all fields at once, which would be more performant and maintainable.
For example:
dkms status -k "$inst_kern" 2>/dev/null | while IFS= read -r line; do
# Extract fields using awk in a single call
eval $(echo "$line" | awk -F'[/,:]' '{
gsub(/^[ \t]+|[ \t]+$/, "", $1); gsub(/^[ \t]+|[ \t]+$/, "", $2);
gsub(/^[ \t]+|[ \t]+$/, "", $3); gsub(/^[ \t]+|[ \t]+$/, "", $4);
printf "name=\"%s\" vers=\"%s\" arch=\"%s\" status=\"%s\"", $1, $2, $3, $4
}')
status=$(echo "$status" | awk '{print $1}')
# ... rest of logic
done
Or using bash parameter expansion:
dkms status -k "$inst_kern" 2>/dev/null | while IFS= read -r line; do
# Extract module/version part
mv="${line%%,*}"
name="${mv%/*}"
vers="${mv#*/}"
# Extract remaining parts
rest="${line#*, }"
arch="${rest%%,*}"
status="${rest#*, }"
status="${status#*: }"
status="${status%% *}"
# ... rest of logic
done
| name=$(echo "$line" | cut -d'/' -f1) | |
| vers=$(echo "$line" | cut -d'/' -f2 | cut -d',' -f1) | |
| arch=$(echo "$line" | awk -F', ' '{print $3}' | cut -d':' -f1) | |
| status=$(echo "$line" | awk -F': ' '{print $2}' | awk '{print $1}') | |
| mv="${line%%,*}" | |
| name="${mv%/*}" | |
| vers="${mv#*/}" | |
| rest="${line#*, }" | |
| arch="${rest%%,*}" | |
| status="${rest#*, }" | |
| status="${status#*: }" | |
| status="${status%% *}" |
64ed847 to
4a69a90
Compare
|
change to use patches. |
This reverts commit 3336b5c.
Signed-off-by: shaoyang <shaoyang@uniontech.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: opsiff The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
No description provided.