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(a)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
+ fi
fi
- 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.
+ 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(a)lists.fedoraproject.org
To unsubscribe send an email to kexec-leave(a)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
--
Best regards,
Coiby