[PATCH V3 0/4] kdumpctl: force rebuild when dump target is modified

[Pratyush Anand]

When dump target is re-formatted then its UUID changes. These patches add
support to recognize such changes and then force initramfs rebuild, if
dracut argument has an UUID based root location.

Changes since v2:
-- now is_system_modified returns 2 in case of invalid modification, 1 in
case of valid modification and 0 in case of no modification.
-- file modification check has been moved to is_system_modified now
-- variables have been put under "" in if condition check

Changes since v1:
-- Local variable is defined in shell function  now
-- raw target does not take persistent names "by-uuid" now
-- dracut arguments grep is more robust
-- When no UUID then warn and return

Pratyush Anand (4):
  mkdumprd: do not lookup in by-uuid dirs for raw device's persistent
    name
  kdumpctl: force rebuild in case of dynamic system modification
  kdumpctl: Move file modification check logic in is_system_modified()
  kdumpctl: force rebuild in case of dump target modification

kdumpctl | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++------------
 mkdumprd |  14 +++++---
 2 files changed, 110 insertions(+), 27 deletions(-)

[PATCH V3 1/4] mkdumprd: do not lookup in by-uuid dirs for raw device's persistent name

[Pratyush Anand]

raw devices are not mounted and also does not need to contain any
filesystem. So they may have UUIDs(when formatted) and may not have UUIDs
when raw. Therefore, do not look for persistent names by-uuid for raw
devices.

Signed-off-by: Pratyush Anand 
Suggested-by: Dave Young 
---
 mkdumprd | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/mkdumprd b/mkdumprd
index 6e3d9757f3d8..ad40d2872f98 100644
--- a/mkdumprd
+++ b/mkdumprd
@@ -30,14 +30,20 @@ perror() {
 }

get_persistent_dev() {
-    local i _tmp _dev
+    local i _tmp _dev _lookup_dirs

_dev=$(udevadm info --query=name --name="$1" 2>/dev/null)
     [ -z "$_dev" ] && {
         perror_exit "Kernel dev name of $1 is not found."
     }

-    for i in /dev/mapper/* /dev/disk/by-uuid/* /dev/disk/by-id/*; do
+    if [[ $2 = "raw" ]];then
+	_lookup_dirs="/dev/mapper/* /dev/disk/by-id/*"
+    else
+	_lookup_dirs="/dev/mapper/* /dev/disk/by-uuid/* /dev/disk/by-id/*"
+    fi
+
+    for i in $_lookup_dirs; do
         _tmp=$(udevadm info --query=name --name="$i" 2>/dev/null)
         if [ "$_tmp" = "$_dev" ]; then
             echo $i
@@ -138,7 +144,7 @@ to_mount() {
     _mntopts="$_target $_fstype $_options"
     #for non-nfs _dev converting to use udev persistent name
     if [ -b "$_source" ]; then
-        _pdev="$(get_persistent_dev $_source)"
+        _pdev="$(get_persistent_dev $_source $_fstype)"
         if [ $? -ne 0 ]; then
             return 1
         fi
@@ -565,7 +571,7 @@ do
         dd if=$config_val count=1 of=/dev/null > /dev/null 2>&1 || {
             perror_exit "Bad raw disk $config_val"
         }
-        _praw=$(get_persistent_dev $config_val)
+        _praw=$(get_persistent_dev $config_val "raw")
         if [ $? -ne 0 ]; then
             exit 1
         fi

[PATCH V3 1/4] mkdumprd: do not lookup in by-uuid dirs for raw device's persistent name

[Dave Young]

On 04/18/16 at 03:23pm, Pratyush Anand wrote:
> raw devices are not mounted and also does not need to contain any
> filesystem. So they may have UUIDs(when formatted) and may not have UUIDs
> when raw. Therefore, do not look for persistent names by-uuid for raw
> devices.
>
> Signed-off-by: Pratyush Anand 
> Suggested-by: Dave Young
>
> mkdumprd | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/mkdumprd b/mkdumprd
> index 6e3d9757f3d8..ad40d2872f98 100644
> --- a/mkdumprd
> +++ b/mkdumprd
> @@ -30,14 +30,20 @@ perror() {
>  }
>
> get_persistent_dev() {
> local i _tmp _dev
>
> local i _tmp _dev _lookup_dirs
>
> _dev=$(udevadm info --query=name --name="$1" 2>/dev/null)
> [ -z "$_dev" ] && {
>     perror_exit "Kernel dev name of $1 is not found."
> }
> for i in /dev/mapper/* /dev/disk/by-uuid/* /dev/disk/by-id/*; do
> if [[ $2 = "raw" ]];then
> _lookup_dirs="/dev/mapper/* /dev/disk/by-id/*"
> else
> _lookup_dirs="/dev/mapper/* /dev/disk/by-uuid/* /dev/disk/by-id/*"
> fi
> for i in $_lookup_dirs; do
>     _tmp=$(udevadm info --query=name --name="$i" 2>/dev/null)
>     if [ "$_tmp" = "$_dev" ]; then
>         echo $i
>
> @@ -138,7 +144,7 @@ to_mount() {
>      _mntopts="$_target $_fstype $_options"
>      #for non-nfs _dev converting to use udev persistent name
>      if [ -b "$_source" ]; then
>    _pdev="$(get_persistent_dev $_source)"
>    _pdev="$(get_persistent_dev $_source $_fstype)"
>    if [ $? -ne 0 ]; then
>        return 1
>    fi
>
> @@ -565,7 +571,7 @@ do
>          dd if=$config_val count=1 of=/dev/null > /dev/null 2>&1 || {
>              perror_exit "Bad raw disk $config_val"
>          }
>    _praw=$(get_persistent_dev $config_val)
>    _praw=$(get_persistent_dev $config_val "raw")
>    if [ $? -ne 0 ]; then
>        exit 1
>    fi
>
> -- 
> 2.5.0
> _______________________________________________
> kexec mailing list
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

Pratyush,

Thanks for the update!, a few more comments for patch 2,3,4, replied inline...

For this one:
Reviewed-by: Dave Young

Thanks
Dave

[PATCH V3 2/4] kdumpctl: force rebuild in case of dynamic system modification

[Pratyush Anand]

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 
---
 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
+
+	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
+		echo -n "System has been modified"; echo
    elif [ -n "$modified_files" ]; then
    	echo "Detected change(s) in the following file(s):"
    	echo -n "  "; echo "$modified_files" | sed 's/\s/\n  /g'

[PATCH V3 2/4] kdumpctl: force rebuild in case of dynamic system modification

[Dave Young]

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
>
> 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?
> 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"
> 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?
> 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.0
> _______________________________________________
> kexec mailing list
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

[PATCH V3 2/4] kdumpctl: force rebuild in case of dynamic system modification

[Pratyush Anand]

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
> >
> > 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

[PATCH V3 2/4] kdumpctl: force rebuild in case of dynamic system modification

[Dave Young]

Hi, Pratyush

On 04/20/16 at 08:52am, Pratyush Anand wrote:
> 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
> > >
> > > 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.

Ok, I do not have strong opinion about this.
> > > 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?

It is fine to print in different functions to me. how about keep the
original else return 0, just add a nul command:

elif [ "$system_modified" != "0" ]; then
                :
        else
                return 0
        fi

Thanks
Dave

[PATCH V3 2/4] kdumpctl: force rebuild in case of dynamic system modification

[Pratyush Anand]

On 20/04/2016:04:08:10 PM, Dave Young wrote:
> It is fine to print in different functions to me. how about keep the
> original else return 0, just add a nul command:
>     elif [ "$system_modified" != "0" ]; then
>             :
>     else
>             return 0
>     fi

Agreed.

Thanks.

~Pratyush

[PATCH V3 3/4] kdumpctl: Move file modification check logic in is_system_modified()

[Pratyush Anand]

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 
---
 kdumpctl | 65 ++++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 64dc92da605b..8ef11501342b 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -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 $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 2
+
+	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=$?
+	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
@@ -379,26 +419,6 @@ check_rebuild()
    	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 +440,6 @@ check_rebuild()
    	echo -n "Force rebuild $TARGET_INITRD"; echo
    elif [ "$system_modified" != "0" ]; then
    	echo -n "System has been modified"; echo
-	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

[PATCH V3 3/4] kdumpctl: Move file modification check logic in is_system_modified()

[Dave Young]

On 04/18/16 at 03:23pm, 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
>
> kdumpctl | 65 ++++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 64dc92da605b..8ef11501342b 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -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 $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 2
> for file in $files; do
> time_stamp=`stat -c "%Y" $file`
> if [ "$time_stamp" -gt "$image_time" ]; then

Since we use image_time in both this function and check_rebuild, how about
add a global var in the head of the file or passing it as an argument?
> 	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=$?
>
> if [ "$ret" -ne 0 ]; then

Ditto about a nitpick "$ret" quotation..
> 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
>
> @@ -379,26 +419,6 @@ check_rebuild()
>    	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 +440,6 @@ check_rebuild()
>    	echo -n "Force rebuild $TARGET_INITRD"; echo
>    elif [ "$system_modified" != "0" ]; then
>    	echo -n "System has been modified"; echo
> 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.0
> _______________________________________________
> kexec mailing list
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

[PATCH V3 3/4] kdumpctl: Move file modification check logic in is_system_modified()

[Pratyush Anand]

Hi Dave,

On 20/04/2016:10:14:35 AM, Dave Young wrote:
> On 04/18/16 at 03:23pm, 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
> >
> > kdumpctl | 65 ++++++++++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 41 insertions(+), 24 deletions(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index 64dc92da605b..8ef11501342b 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -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 $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 2
> > for file in $files; do
> > time_stamp=`stat -c "%Y" $file`
> > if [ "$time_stamp" -gt "$image_time" ]; then
>
> Since we use image_time in both this function and check_rebuild, how about
> add a global var in the head of the file or passing it as an argument?

Oh..I introduced a bug in my patch and did not notice.
Sure, will correct it. Will use it as a global variable.

Thanks for pointing it out.

~Pratyush

[PATCH V3 4/4] kdumpctl: force rebuild in case of dump target modification

[Pratyush Anand]

kdump also passes persistent device mapping as --mount or --device argument
of dracut. However this persistent id (UUID) is changed if dump target is
re-formated.
kdumpctl must have a mechanism to recognise this modification, so that its
service restart is able to rebuild initramfs.

Testing:
a) Attach an IDE device, lets say it is /dev/sdb1
b) Format it as ext4
    # mkfs.ext4 /dev/sdb1
    # blkid /dev/sdb1
    /dev/sdb1: UUID="21c7baff-e35d-49c7-aa08-0fba4513f5bf" TYPE="ext4"
c) Mount it into /mnt and create a var/crash directory in it.
    # mount /dev/sdb1 /mnt;mkdir /mnt/var;mkdir /mnt/var/crash
d) Add following line in /etc/kdump.conf
    ext4 /dev/sdb1
e) Restart kdumpctl
    # kdumpctl restart
f) crash
    # echo c > /proc/sysrq-trigger
g) Here you will be able save vmcore with or without this patch.
h) repeat step (b), (c), (e) and (f)
i) Now you will be able to save vmcore only when you have this patch in
your kexec-tools. Your initramfs will be rebuilt when you repeat step (e)
after reformatting.

Signed-off-by: Pratyush Anand 
---
 kdumpctl | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index 8ef11501342b..5b8b8c82be4d 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -359,6 +359,39 @@ is_files_modified()
    return 0
 }

+is_dump_target_modified()
+{
+	local _target _dracut_args _uuid
+
+	_target=$(egrep "^ext[234]|^xfs|^btrfs" /etc/kdump.conf)
+
+	#if dump target does not exist then do not rebuild
+	[[ -n "$_target" ]] || return 0
+
+	_dracut_args=$(lsinitrd $TARGET_INITRD | grep "^Arguments:" | head -1)
+	if [[ -z "$_dracut_args" ]];then
+		echo "Warning: No dracut arguments found in initrd"
+		return 0
+	fi
+	#if --mount or --device is not by UUID, then also do not rebuild
+	echo $_dracut_args | grep "by-uuid" &> /dev/null
+	[[ $? -eq 0 ]] || return 0
+
+	_target=$(echo $_target | cut -d ' ' -f 2)
+	_uuid=$(blkid $_target | awk -F"UUID=" '{print $2}' | cut -d '"' -f 2)
+	if [[ -z "$_uuid" ]];then
+		echo "Warning: UUID is not present"
+		return 1
+	fi
+
+	echo $_dracut_args  | grep $_uuid &> /dev/null
+	#if dracut argument and _target have same uuid, then also do not rebuild
+	[[ $? -ne 0 ]] || return 0
+
+	echo "Detected change in dump target"
+	return 1
+}
+
 # returns 0 if system is not modified
 # returns 1 if system is modified
 # returns 2 if system modification is invalid
@@ -374,6 +407,12 @@ is_system_modified()
    	return $ret
    fi

+	is_dump_target_modified
+	ret=$?
+	if [ "$ret" -ne 0 ]; then
+		return $ret
+	fi
+
    return 0
 }

[PATCH V3 4/4] kdumpctl: force rebuild in case of dump target modification

[Dave Young]

On 04/18/16 at 03:23pm, Pratyush Anand wrote:
> kdump also passes persistent device mapping as --mount or --device argument
> of dracut. However this persistent id (UUID) is changed if dump target is
> re-formated.
> kdumpctl must have a mechanism to recognise this modification, so that its
> service restart is able to rebuild initramfs.
>
> Testing:
> a) Attach an IDE device, lets say it is /dev/sdb1
> b) Format it as ext4
>    # mkfs.ext4 /dev/sdb1
>    # blkid /dev/sdb1
>    /dev/sdb1: UUID="21c7baff-e35d-49c7-aa08-0fba4513f5bf" TYPE="ext4"
> c) Mount it into /mnt and create a var/crash directory in it.
>    # mount /dev/sdb1 /mnt;mkdir /mnt/var;mkdir /mnt/var/crash
> d) Add following line in /etc/kdump.conf
>    ext4 /dev/sdb1
> e) Restart kdumpctl
>    # kdumpctl restart
> f) crash
>    # echo c > /proc/sysrq-trigger
> g) Here you will be able save vmcore with or without this patch.
> h) repeat step (b), (c), (e) and (f)
> i) Now you will be able to save vmcore only when you have this patch in
> your kexec-tools. Your initramfs will be rebuilt when you repeat step (e)
> after reformatting.
> Signed-off-by: Pratyush Anand
>
> kdumpctl | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index 8ef11501342b..5b8b8c82be4d 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -359,6 +359,39 @@ is_files_modified()
>    return 0
>  }
>
> +is_dump_target_modified()

Nitpick: how about change the function name to is_fs_uuid_changed so that
it is more straiforward, we can have other case in the future for dump target
modification..
> +{
> local _target _dracut_args _uuid
> _target=$(egrep "^ext[234]|^xfs|^btrfs" /etc/kdump.conf)
> #if dump target does not exist then do not rebuild
> [[ -n "$_target" ]] || return 0
> _dracut_args=$(lsinitrd $TARGET_INITRD | grep "^Arguments:" | head -1)
> if [[ -z "$_dracut_args" ]];then
> echo "Warning: No dracut arguments found in initrd"
> return 0
> fi
> #if --mount or --device is not by UUID, then also do not rebuild
> echo $_dracut_args | grep "by-uuid" &> /dev/null
> [[ $? -eq 0 ]] || return 0
> _target=$(echo $_target | cut -d ' ' -f 2)
> _uuid=$(blkid $_target | awk -F"UUID=" '{print $2}' | cut -d '"' -f 2)
> if [[ -z "$_uuid" ]];then
> echo "Warning: UUID is not present"
> return 1
> fi
> echo $_dracut_args  | grep $_uuid &> /dev/null
> #if dracut argument and _target have same uuid, then also do not rebuild
> [[ $? -ne 0 ]] || return 0
> echo "Detected change in dump target"
> return 1
>
> +}
>
> # returns 0 if system is not modified
>  # returns 1 if system is modified
>  # returns 2 if system modification is invalid
> @@ -374,6 +407,12 @@ is_system_modified()
>    	return $ret
>    fi
> is_dump_target_modified
> ret=$?
> if [ "$ret" -ne 0 ]; then
> return $ret
> fi
> return 0
>
> }
>
> -- 
> 2.5.0
> _______________________________________________
> kexec mailing list
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

[PATCH V3 4/4] kdumpctl: force rebuild in case of dump target modification

[Pratyush Anand]

On 20/04/2016:10:15:54 AM, Dave Young wrote:
> On 04/18/16 at 03:23pm, Pratyush Anand wrote:
> > kdump also passes persistent device mapping as --mount or --device argument
> > of dracut. However this persistent id (UUID) is changed if dump target is
> > re-formated.
> > kdumpctl must have a mechanism to recognise this modification, so that its
> > service restart is able to rebuild initramfs.
> >
> > Testing:
> > a) Attach an IDE device, lets say it is /dev/sdb1
> > b) Format it as ext4
> >    # mkfs.ext4 /dev/sdb1
> >    # blkid /dev/sdb1
> >    /dev/sdb1: UUID="21c7baff-e35d-49c7-aa08-0fba4513f5bf" TYPE="ext4"
> > c) Mount it into /mnt and create a var/crash directory in it.
> >    # mount /dev/sdb1 /mnt;mkdir /mnt/var;mkdir /mnt/var/crash
> > d) Add following line in /etc/kdump.conf
> >    ext4 /dev/sdb1
> > e) Restart kdumpctl
> >    # kdumpctl restart
> > f) crash
> >    # echo c > /proc/sysrq-trigger
> > g) Here you will be able save vmcore with or without this patch.
> > h) repeat step (b), (c), (e) and (f)
> > i) Now you will be able to save vmcore only when you have this patch in
> > your kexec-tools. Your initramfs will be rebuilt when you repeat step (e)
> > after reformatting.
> > Signed-off-by: Pratyush Anand
> >
> > kdumpctl | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index 8ef11501342b..5b8b8c82be4d 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -359,6 +359,39 @@ is_files_modified()
> >    return 0
> >  }
> >
> > +is_dump_target_modified()
>
> Nitpick: how about change the function name to is_fs_uuid_changed so that
> it is more straiforward, we can have other case in the future for dump target
> modification..

OK.

~Pratyush