Hi Baoquan,
On 06/05/2016:11:10:33 AM, Baoquan He wrote:
On 05/05/16 at 05:20pm, Pratyush Anand wrote:
> Relevant kdump files are also part of system. Therefore, moving logic of
> file modification checking in is_system_modified() function now.
>
> Signed-off-by: Pratyush Anand <panand(a)redhat.com>
> ---
> kdumpctl | 69 +++++++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 42 insertions(+), 27 deletions(-)
This looks a really good clean up. I like it.
Thanks :-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 6bd2608343c0..793774d79a61 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -14,6 +14,7 @@ FADUMP_ENABLED_SYS_NODE="/sys/kernel/fadump_enabled"
> FADUMP_REGISTER_SYS_NODE="/sys/kernel/fadump_registered"
> #kdump shall be the default dump mode
> DEFAULT_DUMP_MODE="kdump"
> +image_time=0
>
> . /lib/kdump/kdump-lib.sh
>
> @@ -275,7 +276,6 @@ check_config()
> # return list of modified file for fence_kdump modified in Pacemaker cluster
> get_pcs_cluster_modified_files()
> {
> - local image_time=$1
> local time_stamp
> local modified_files
>
> @@ -327,19 +327,59 @@ setup_target_initrd()
> fi
> }
>
> +is_files_modified()
> +{
> + local modified_files=""
> +
> + #also rebuild when Pacemaker cluster conf is changed and fence kdump is enabled.
> + modified_files=$(get_pcs_cluster_modified_files)
> +
> + EXTRA_BINS=`grep ^kdump_post $KDUMP_CONFIG_FILE | cut -d\ -f2`
> + CHECK_FILES=`grep ^kdump_pre $KDUMP_CONFIG_FILE | cut -d\ -f2`
> + EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
> + CHECK_FILES=`grep ^extra_bins $KDUMP_CONFIG_FILE | cut -d\ -f2-`
> + EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
> + files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS /etc/fstab"
> +
> + check_exist "$files" && check_executable
"$EXTRA_BINS"
> + [ $? -ne 0 ] && return 2
If return value checking is following the function invocation, using "$?"
direclty in checking is fine to me, Just like it is doing in this place,
I say so because there's a different place in below block.
OK..so here we are returning 2 specifically to let check_rebuild() know that
kdump service should be failed.
> +
> + for file in $files; do
> + time_stamp=`stat -c "%Y" $file`
> + if [ "$time_stamp" -gt "$image_time" ]; then
> + modified_files="$modified_files $file"
> + fi
> + done
> + if [ -n "$modified_files" ]; then
> + echo "Detected change(s) in the following file(s):"
> + echo -n " "; echo "$modified_files" | sed 's/\s/\n
/g'
> + return 1
> + fi
> +
> + return 0
> +}
> +
> # returns 0 if system is not modified
> # returns 1 if system is modified
> # returns 2 if system modification is invalid
> is_system_modified()
> {
> + local ret
> +
> [[ -f $TARGET_INITRD ]] || return 1
>
> + is_files_modified
> + ret=$?
Here, I would like to not use temporary variable "ret" here. Really
unnecessary.
Since we can have 3 states so we need to use "ret". We need to return when
either we find system is modified or we find the kdump service need to be
aborted.
> + if [ $ret -ne 0 ]; then
> + return $ret
> + fi
> +
> return 0
> }
>
> check_rebuild()
> {
> - local extra_modules modified_files=""
> + local extra_modules
> local _force_rebuild force_rebuild="0"
> local ret system_modified="0"
> local initramfs_has_fadump
> @@ -375,30 +415,8 @@ check_rebuild()
> #since last build of the image file
> if [ -f $TARGET_INITRD ]; then
> image_time=`stat -c "%Y" $TARGET_INITRD 2>/dev/null`
> - else
> - image_time=0
> fi
>
> - #also rebuild when Pacemaker cluster conf is changed and fence kdump is enabled.
> - modified_files=$(get_pcs_cluster_modified_files $image_time)
> -
> - EXTRA_BINS=`grep ^kdump_post $KDUMP_CONFIG_FILE | cut -d\ -f2`
> - CHECK_FILES=`grep ^kdump_pre $KDUMP_CONFIG_FILE | cut -d\ -f2`
> - EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
> - CHECK_FILES=`grep ^extra_bins $KDUMP_CONFIG_FILE | cut -d\ -f2-`
> - EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
> - files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS /etc/fstab"
> -
> - check_exist "$files" && check_executable
"$EXTRA_BINS"
> - [ $? -ne 0 ] && return 1
> -
> - for file in $files; do
> - time_stamp=`stat -c "%Y" $file`
> - if [ "$time_stamp" -gt "$image_time" ]; then
> - modified_files="$modified_files $file"
> - fi
> - done
> -
> is_system_modified
> ret=$?
> if [ $ret -eq 2 ]; then
> @@ -420,9 +438,6 @@ check_rebuild()
> echo -n "Force rebuild $TARGET_INITRD"; echo
> elif [ "$system_modified" != "0" ]; then
> :
> - elif [ -n "$modified_files" ]; then
> - echo "Detected change(s) in the following file(s):"
> - echo -n " "; echo "$modified_files" | sed 's/\s/\n
/g'
> else
> return 0
> fi
> --
> 2.5.5
>
~Pratyush