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 5 commits into
Conversation
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 |
Move initrd building functionality from standalone mkdumprd and mkfadumprd scripts into a new shared library kdump-lib-dracut.sh. This is the first step in eliminating duplicate kdump.conf parsing. The library contains: - Helper functions for checking dump targets, mounting, size verification - mkdumprd() function for building kdump initramfs - mkfadumprd() function for building fadump initramfs These functions will be sourced by kdumpctl, allowing it to reuse its already-parsed configuration (OPT array) instead of re-parsing kdump.conf. This also includes a fix for shellcheck warning SC2086 by converting _timeout_cmd from a string to a proper bash array to prevent word-splitting issues with the timeout command. Related: rhkdump#140 Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Update kdumpctl to source the new kdump-lib-dracut.sh library and call its mkdumprd() and mkfadumprd() functions instead of invoking external scripts. Changes: - Source kdump-lib-dracut.sh after kdump-lib.sh - Update rebuild_kdump_initrd() to call mkdumprd() function - Update rebuild_fadump_initrd() to call mkfadumprd() function - Export KDUMP_TMPDIR and KDUMP_TMPMNT for use by library functions - Add check to ensure KDUMP_TMPDIR was created successfully This allows the library functions to access the already-parsed OPT array from kdumpctl, avoiding the need to re-parse kdump.conf. Related: rhkdump#140 Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Remove the external mkdumprd and mkfadumprd scripts as their functionality has been moved into kdump-lib-dracut.sh and is now called directly from kdumpctl. These scripts were always intended to be used only via kdumpctl and not directly by users. Removing them enforces this design and eliminates the maintenance burden of keeping wrapper scripts for backward compatibility. The new approach has several advantages: - Eliminates duplicate parsing of kdump.conf - Reduces code duplication - Improves performance by reusing already-parsed configuration - Makes the call path clearer (kdumpctl -> library function) If users need to rebuild the initramfs, they should use 'kdumpctl rebuild' as documented. Related: rhkdump#140 Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Remove the mkdumprd man page as the mkdumprd script has been removed. Users should refer to kdumpctl(8) for information on rebuilding the kdump initramfs. The recommended command is now: kdumpctl rebuild Related: rhkdump#140 Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
Update Makefile to reflect the removal of mkdumprd and mkfadumprd scripts and addition of kdump-lib-dracut.sh library: - Remove mkdumprd.8 from manpages target - Remove mkdumprd installation from sbindir - Remove mkfadumprd installation (ppc64/ppc64le) - Add kdump-lib-dracut.sh to library installation The new kdump-lib-dracut.sh library is installed to pkglibdir alongside other kdump libraries and will be sourced by kdumpctl. Related: rhkdump#140 Signed-off-by: Akhilesh Jahagirdar <ajahagir@redhat.com>
fec70fe to
9be21bc
Compare
Moves mkdumprd and mkfadumprd functionality into kdumpctl as internal functions. This eliminates duplicate kdump.conf parsing.
Resolves #140
Changes