Refactored mkdumprd and mkfadumprd into functions with relation to issue #140 - #156
Refactored mkdumprd and mkfadumprd into functions with relation to issue #140#156ajahagir-rh wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates the standalone mkdumprd and mkfadumprd scripts, refactoring their core logic into internal functions within kdumpctl to eliminate duplicate configuration parsing and improve efficiency. The original scripts are converted into thin compatibility wrappers, and the man page is updated accordingly. The code review identified several critical shell scripting issues in the refactored code, including a pipeline masking issue in _get_fs_size, a silenced exit status in add_mount, incorrect --debug flag forwarding in the wrappers, potential integer comparison errors in _check_size, and risky eval usage in _check_user_configured_target.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
78f926f to
fec70fe
Compare
|
Hi @ajahagir-rh, thanks for submitting the PR. I'm not fully done reviewing the changes in detail. Nevertheless there are a few high-level issues I want to discuss. First of all, please split up the PR in multiple commits with each one only doing 'one change' and write proper commit messages for them. There are multiple reasons for that.
You can have a look at #33, If you want same inspiration on how to split up the PR. While at it, please also add your Furthermore, my long term goal for Additionally, while I really appreciate that you are thinking about backward compatibility, I don't think it makes sense to keep it in this case. Finally, when you fix issues while the PR is in discussion, like the issues reported by Gemini. You only have to add a new commit to the PR, if that problem already existed before. If the problem was introduced by one of your commits it is better to update the commit rather than adding a new one. This helps to keep the git history shorter and easier to traverse. Thanks |
fec70fe to
9be21bc
Compare
|
I have run a local test against it, no issue found and all tests passed. But I haven't look into the code yet. PS. to run a local test: |
Hi Tao, thanks for testing! |
prudo1
left a comment
prudo1
left a comment
There was a problem hiding this comment.
Hi Akhilesh,
thanks for the update. All in all it looks much better. But unfortunately I have to nag again. Part of it is on me...
When I said that a commit should only contain 'one change' I didn't meant that it should only contain changes to one file. I rather meant that it should only contain one change in the programs logic, which might require to touch multiple files. For example when you remove/rename a file the commit needs to contain the actual removal/renaming plus all changes related to it, e.g. Makefile, spec file, files that source the other file, etc..
This is because it's important that not only the PR as whole works but every single commit in it. This is required for git bisect I mentioned earlier. With git bisect you perform a binary search on the git log to find a commit that introduced a bug. That requires to install and run the code on multiple commits from the log. If one of them doesn't 'work' (e.g. doesn't compile or install, exits with an error early, etc.). You cannot continue with git bisect but have to manually work around it. Which makes life for the person triaging the bug much harder.
In addition having multiple 'logical changes' in one commit makes reviewing a lot harder. For example in your first commit you basically moved the code from mkdumprd/mkfadumprd into a new file and at the same time made some changes to it, i.e. made use of the OPT array, converted _timeout_cmd to an array. The problem is that the diff from the git commit only shows a big block of lines that have been added. For me as a reviewer this means that I have to go through every line of the final code to find out which line has changed and if it has been changed correctly. If you had one commit that does the move (and no functional change) and one (or more) commit(s) that contain the changes. It would make life for reviewer much easier. Then the diff in the commit(s) that add the change will clearly show which lines have been updated and what has changed. Plus for the commit that moved the code I could focus on all the other changes that come with it, e.g. Makefile, spec file, etc.. In the end this will not only make my life easier but will also contribute to less bugs as they are caught before they even get merged.
BTW, having code that is easy to review is not only important to get code merged. It also helps with investigating bugs. At least in my workflow I use git blame quite a lot when I spot code that looks 'fishy' to me. It shows for every line in the file the last commit that has changed it. This helps me a lot to understand the history of the code and why certain changes have been made in the past. But for that the commits need to be easy to understand. So having a commit that is hard to review doesn't only hurt once but over and over again.
That's why I'm so keen on having not only the code but also the commits in good shape.
Thanks
Philipp
| if [[ $_fstype == "nfs"* ]]; then | ||
| _timeout_cmd=(timeout --preserve-status 10m) | ||
| fi |
There was a problem hiding this comment.
Here is an example on why moving and changing code should be two separate commits. How did the old code look like? What has changed?
Of course I could look it up by applying this commit and look at mkdumprd. But there it's also only one line in a huge block of code. Plus it makes it hard to spot the other related lines that need to be updated to make this change and check if they have been updated correctly.
For me as a reviewer it would be much easier when such a change is in a separate commit. Then the diff will tell me exactly what has changed.
|
|
||
| have_compression_in_dracut_args() | ||
| { | ||
| [[ "$(kdump_get_conf_val dracut_args)" =~ (^|[[:space:]])--(gzip|bzip2|lzma|xz|lzo|lz4|zstd|no-compress|compress|squash-compressor)([[:space:]]|$) ]] | ||
| } | ||
|
|
||
| # If "dracut_args" contains "--mount" information, use it | ||
| # directly without any check(users are expected to ensure |
There was a problem hiding this comment.
Here is an example on what would break git bisect. In case someone would try to run kdumpcrl rebuild after only applying this commit it would fail. Because mkdumprd tries to call this function which is now undefined.
The same problem happens by only updating the Makefile/spec file in the last commit. If someone tries to run make install or build the rpm without applying the last commit it will fail.
Move all helper functions and main logic from the standalone mkdumprd and mkfadumprd scripts into a new kdump-lib-dracut.sh library file. This is a pure structural move with no functional changes: - All 13 helper functions copied verbatim from mkdumprd - mkdumprd() wraps the config parsing loop and dracut invocation - mkfadumprd() wraps the fadump initrd build/repack logic - Original variable names preserved ($SAVE_PATH, $SSH_KEY_LOCATION, $MKDUMPRD_TMPMNT, perror_exit) The standalone scripts become thin wrappers that source the library and call the corresponding function. Resolves: rhkdump#140 Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Since the code now runs as library functions that may be sourced by kdumpctl, calling perror_exit (which does exit 1) would terminate the entire kdumpctl process. Replace with derror + return 1 so errors are reported but control returns to the caller. Also add || return 1 to callers that previously relied on perror_exit to stop execution (e.g. in the while-read config loop). Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Call mkdumprd() and mkfadumprd() as library functions instead of invoking the standalone scripts as subprocesses. - Source kdump-lib-dracut.sh alongside the other kdump libraries - Remove MKDUMPRD and MKFADUMPRD variables - Update rebuild_kdump_initrd() and rebuild_fadump_initrd() to call the library functions directly - Export KDUMP_TMPDIR for library use - Rename TMPMNT to KDUMP_TMPMNT and set MKDUMPRD_TMPMNT as a bridge variable until the library is updated to use KDUMP_TMPMNT directly Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
These scripts are now library functions in kdump-lib-dracut.sh, called directly by kdumpctl. Remove the standalone scripts, the mkdumprd man page, and all related install/packaging references. Update SEE ALSO sections in kdumpctl.8 and kdump.conf.5 to reflect the removal. Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
kdumpctl already parses kdump.conf into the OPT associative array.
Use it directly instead of reparsing the config file in mkdumprd().
Replace the kdump_read_conf while-loop with direct OPT lookups for
each config option. Replace $SSH_KEY_LOCATION with ${OPT[sshkey]},
$SAVE_PATH with ${OPT[path]}, and $MKDUMPRD_TMPMNT with $KDUMP_TMPMNT
throughout the helper functions.
Move have_compression_in_dracut_args from kdump-lib.sh into
kdump-lib-dracut.sh and switch it to use ${OPT[dracut_args]}.
Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Using an unquoted string variable as a command prefix is fragile and triggers shellcheck warnings. Convert _timeout_cmd to a proper bash array in check_user_configured_target and kdumpctl's fetch_status. Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
9be21bc to
83504fa
Compare
3 participants
Moves mkdumprd and mkfadumprd functionality into kdumpctl as internal functions. This eliminates duplicate kdump.conf parsing.
Resolves #140
Changes