Hi Baoquan,
Thanks for the review.
On 06/05/2016:11:06:08 AM, Baoquan He wrote:
On 05/05/16 at 05:20pm, 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 8ec6b2de914b..6bd2608343c0 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
Well, we might not need to implement functions like C programming
language. Seems shell has its own conventions. Like 0 means true, 1
means false. And for is_system_modified its return value should reflect
its naming, 0 means it is modified, 1 means it is not modified. If
invalid, we can warning or error and return directly in the detect
function. It may not be necessary to use return value 2 here.
Actually this is coming from next patch. is_files_modified() needs 3 states. So
if minimum required files does not exist, or may be some required binary does
not exist then we need to abort rebuild process and kdump service should fail.
So, basically when it returns 2, then we should land into "Starting kdump:
[FAILED]".
~Pratyush
>
> > +is_system_modified()
> > +{
> > + [[ -f $TARGET_INITRD ]] || return 1
> > +
> > + 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
> > + 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
> > + :
> > elif [ -n "$modified_files" ]; then
> > echo "Detected change(s) in the following file(s):"
> > echo -n " "; echo "$modified_files" | sed 's/\s/\n
/g'
> > --
> > 2.5.5
> >