Hi Dave,
Thanks for your feedback.
On 20/04/2016:10:11:51 AM, Dave Young wrote:
On 04/18/16 at 03:23pm, Pratyush Anand wrote:
> There could be some dynamic system modification, which may affect kdump
> kernel boot process. In such situation initramfs must be rebulit on the
> basis of changes.
> Since most of these checking methods will use information from
> TARGET_INITRD, therefore check for its existence in common code.
>
> Signed-off-by: Pratyush Anand <panand(a)redhat.com>
> ---
> kdumpctl | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index ef18a2d4f6ce..64dc92da605b 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -327,10 +327,21 @@ setup_target_initrd()
> fi
> }
>
> +# returns 0 if system is not modified
> +# returns 1 if system is modified
> +# returns 2 if system modification is invalid
> +is_system_modified()
> +{
> + [[ -f $TARGET_INITRD ]] || return 1
Since earlier code will check image_time -eq 0 so it will never return 1
How about add replace it with a comment about we do not worry about
initrd existance since we will check image_time -eq 0 in other place?
is_system_modified() is called before checking of "image_time -eq 0". Since,
most of the sub-functions called from is_system_modified() needs $TARGET_INITRD,
so I thought to check it in common path.
> +
> + return 0
> +}
> +
> check_rebuild()
> {
> local extra_modules modified_files=""
> local _force_rebuild force_rebuild="0"
> + local ret system_modified="0"
> local initramfs_has_fadump
>
> check_boot_dir
> @@ -388,6 +399,14 @@ check_rebuild()
> fi
> done
>
> + is_system_modified
> + ret=$?
> + if [ "$ret" -eq 2 ]; then
> + return 1
> + elif [ "$ret" -eq 1 ];then
Nitpick: not necessary to quote "$ret"
OK, will correct.
> + system_modified="1"
> + fi
> +
> #check if target initrd has fadump support
> if [ "$DEFAULT_DUMP_MODE" = "fadump" ] && [ -f
"$TARGET_INITRD" ]; then
> initramfs_has_fadump=`lsinitrd -m $TARGET_INITRD | grep ^kdumpbase$ | wc -l`
> @@ -399,6 +418,8 @@ check_rebuild()
> echo "$TARGET_INITRD has no fadump support"
> elif [ "$force_rebuild" != "0" ]; then
> echo -n "Force rebuild $TARGET_INITRD"; echo
> + elif [ "$system_modified" != "0" ]; then
> + echo -n "System has been modified"; echo
A concern about the message is if we move modified files to is_system_modified
in later patch for those cases there will print more messages than before.
Ideally only below "Dected change(s) in the following file(s):" should be
better.
Maybe for system_modified we can differenciate the cases by more values?
ie. -1 means error, 1 means files being modified, 1 means fs uuid being modified
etc. then print different messages?
I think, we already have relevant debug messages coming from the different
functions called by is_system_modified(), so probably we can skip any message
printing here.
So may in stead of last "else return 0" we can have "elif [
"$system_modified"
= "0" ]; then return 0". What do you say?
~Pratyush