Skip to content

Refactored mkdumprd and mkfadumprd into functions with relation to issue #140#156

Open
ajahagir-rh wants to merge 5 commits into
rhkdump:mainfrom
ajahagir-rh:akhilesh-kex515
Open

Refactored mkdumprd and mkfadumprd into functions with relation to issue #140#156
ajahagir-rh wants to merge 5 commits into
rhkdump:mainfrom
ajahagir-rh:akhilesh-kex515

Conversation

@ajahagir-rh

Copy link
Copy Markdown

Moves mkdumprd and mkfadumprd functionality into kdumpctl as internal functions. This eliminates duplicate kdump.conf parsing.

Resolves #140

Changes

  • kdumpctl: Added 11 top-level helper functions (avoiding nested function anti-pattern)
  • kdumpctl: Added `_mkdumprd()` and `_mkfadumprd()` internal functions that reuse the already-parsed OPT array
  • kdumpctl: Updated `rebuild_kdump_initrd()` and `rebuild_fadump_initrd()` to call internal functions
  • kdumpctl: Added hidden CLI commands `_mkdumprd` and `_mkfadumprd` for backward compatibility
  • mkdumprd & mkfadumprd: Converted to thin wrapper scripts with deprecation warnings
  • mkdumprd.8: Added deprecation documentation

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread kdumpctl Outdated
Comment thread kdumpctl Outdated
Comment thread mkdumprd Outdated
Comment thread mkfadumprd Outdated
Comment thread kdumpctl Outdated
Comment thread kdumpctl Outdated
@prudo1

prudo1 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

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.

  1. Having multiple commits helps reviewers massively as they only need to understand one change at a time.
  2. Similar it helps finding bugs introduced by your PR. You always should assume that you introduce new bugs with every change. To find them git bisect is very helpful. It allows you to identify the commit that introduced the bug/new behavior. However, if that commit is massive and contains multiple changes, knowing the commit doesn't help you with identifying the problem.
  3. Commit messages are an extremely important, if not even the most important, piece of documentation for developers. They allow us to understand why the code is the way it is. Knowing that is extremely important! It prevents us to make the same mistakes over and over again. That's also why commit messages should focus on describing why a commit is needed not what it does (ideally this should be obvious when you stick to the one-commit-per-change-rule).

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 Signed-off-by: to the commits. With this you show that you are the author of the code and that you have permission to submit it to upstream, e.g. that you didn't copy any code that is protected by a incompatible license. You can automatically add your Signed-off-by: by using the -s option with git commit.

Furthermore, my long term goal for kdump-utils is to make it more modular. To achieve that it is necessary to split up kdumpctl into multiple files. With each of these files handling one aspect of it. In that picture rebuilding the initrd is a prime candidate to be split out as it is pretty much self-contained. That's why I moved the code to a new kdump-lib-dracut.sh rather than including it in kdumpctl on the branch I gave you. I'd appreciate if you could do the same.

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. mkdumprd was always intended only to be used via kdumpctl and I cannot think about any reason why a user would want to do it differently. In fact using it directly can lead to horrible consequences, as the issue linked to in #140 shows. So IMHO breaking backward compatibility is not a bug but a feature in this case. Worst thing that can happen is that we have to re-introduce the backward compatibility later again in case some one complains.

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
Philipp

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Turn mkdumprd and mkfadumprd into functions

2 participants