On Mon, 9 Jan 2023 16:39:44 +0800
Coiby Xu <coxu(a)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(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.
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(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