On Mon, 9 Jan 2023 16:39:44 +0800 Coiby Xu coxu@redhat.com wrote:
Hi Philipp,
The patch looks good to me except for a small concern.
On Wed, Dec 21, 2022 at 04:27:25PM +0100, Philipp Rudo wrote:
At the beginning of do_estimate it currently checks whether the TARGET_INITRD exists and if not fails with an error message. This not only requires the user to manually trigger the build of the initrd but also ignores all cases where the TARGET_INITRD exists but need to be rebuild. For example when there were changes to kdump.conf or when the system switches from kdump to fadump. All these changes will impact the outcome of do_estimate. Thus properly check whether the initrd needs to be rebuild and if it does trigger the rebuild automatically.
To do so move the check whether the TARGET_INITRD has fadump enabled to is_system_modified and call this function. With this force_(no_)rebuild options in kdump.conf are ignored to avoid unnecessary rebuilds.
While at it cleanup check_system_modified and rename it to is_system_modified. Furthermore move printing the info that the initrd gets rebuild to rebuild_initrd to avoid every caller has the same line.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 62 ++++++++++++++++++++++++-------------------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 5954527..d5bf408 100755 --- a/kdumpctl +++ b/kdumpctl @@ -117,6 +117,8 @@ rebuild_kdump_initrd()
rebuild_initrd() {
- dinfo "Rebuilding $TARGET_INITRD"
- if [[ ! -w $(dirname "$TARGET_INITRD") ]]; then derror "$(dirname "$TARGET_INITRD") does not have write permission. Cannot rebuild $TARGET_INITRD" return 1
@@ -532,32 +534,27 @@ check_fs_modified()
# returns 0 if system is not modified # returns 1 if system is modified -# returns 2 if system modification is invalid -check_system_modified() +# returns 2 if an error occurred +is_system_modified() { local ret
[[ -f $TARGET_INITRD ]] || return 1
- check_files_modified
- ret=$?
- if [[ $ret -ne 0 ]]; then
return $ret
- # in case of fadump mode, check whether the default/target initrd is
- # already built with dump capture capability
- if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
if ! lsinitrd -f $DRACUT_MODULES_FILE "$TARGET_INITRD" | grep -q -e ^kdumpbase$ -e ^zz-fadumpinit$; then
dinfo "Rebuild $TARGET_INITRD with dump capture support"
return 1
fifi
- check_fs_modified
- ret=$?
- if [[ $ret -ne 0 ]]; then
return $ret
- fi
- image_time=$(stat -c "%Y" "$TARGET_INITRD" 2> /dev/null)
Maybe it's better define/assign image_time in check_files_modified so we get rid of this global variable and make it easier for code reasoning? I just checked image_file is only used by check_files_modified and get_pcs_cluster_modified_files which is only called by check_files_modified.
right, this will make it even better.
I'll prepare a v2 with the two changes you proposed.
Thanks Philipp
- check_files_modified || return
- check_fs_modified || return check_drivers_modified
- ret=$?
- if [[ $ret -ne 0 ]]; then
return $ret
- fi
- return 0
}
# need_initrd_rebuild - check whether the initrd needs to be rebuild @@ -600,18 +597,7 @@ need_initrd_rebuild() return 1 fi
- # in case of fadump mode, check whether the default/target initrd is
- # already built with dump capture capability
- if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
if ! lsinitrd -f $DRACUT_MODULES_FILE "$TARGET_INITRD" | grep -q -e ^kdumpbase$ -e ^zz-fadumpinit$; then
dinfo "Rebuild $TARGET_INITRD with dump capture support"
return 1
fi
- fi
- image_time=$(stat -c "%Y" "$TARGET_INITRD" 2> /dev/null)
- check_system_modified
- is_system_modified
}
# On ppc64le LPARs, the keys trusted by firmware do not end up in @@ -994,7 +980,6 @@ start() # Nothing to do ;; 1)
*)dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd || return ;;
@@ -1105,7 +1090,6 @@ rebuild()
setup_initrd || return 1
- dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd
}
@@ -1118,10 +1102,18 @@ do_estimate() local size_mb=$((1024 * 1024))
setup_initrd
- if [[ ! -f $TARGET_INITRD ]]; then
derror "kdumpctl estimate: kdump initramfs is not built yet."
exit 1
- fi
is_system_modified
case "$?" in
0)
# Nothing to do
;;
1)
rebuild_initrd || return
;;
*)
return
;;
esac
kdump_mods="$(lsinitrd "$TARGET_INITRD" -f /usr/lib/dracut/hostonly-kernel-modules.txt | tr '\n' ' ')" baseline=$(kdump_get_arch_recommend_size)
-- 2.38.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue