[PATCH 0/2] Check if extra_modules is modified rather then always rebuild

[Kairui Song]

First commit make check_files_modified follows symlink, as weak-modules
could be symlink and second commit append the extra_modules to the file
list to be checked.

Update from V1:
  - Update check_files_modified instead of introduce a new function
  - Give proper message when invalid/builtin module name is given
  - Behave properly with weak-modules

Kairui Song (2):
  kdumpctl: follow symlink when checking for modified files
  kdumpctl: don't always rebuild when extra_modules is set

kdumpctl | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

[PATCH v2 1/2] kdumpctl: follow symlink when checking for modified files

[Kairui Song]

Previously only the symlink's timestamp is used for checking if file are
modified, this will not trigger a rebuild if the symlink target it
modified.

So check both symlink timestamp and symlink target timestamp, rebuild
the initramfs on both symlink changed and target changed.

Signed-off-by: Kairui Song 
---
 kdumpctl | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 3f80ba4..3f36903 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -338,11 +338,19 @@ check_files_modified()
    [ $? -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
+		while [ -e "$file" ]; do
+			time_stamp=`stat -c "%Y" $file`
+			if [ "$time_stamp" -gt "$image_time" ]; then
+				modified_files="$modified_files $file"
+			fi
+			if [ -L "$file" ]; then
+				file=$(readlink -m $file)
+			else
+				break
+			fi
+		done
    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'

[PATCH v2 1/2] kdumpctl: follow symlink when checking for modified files

[Dave Young]

Hi Kairui,
On 05/06/19 at 03:17pm, Kairui Song wrote:
> Previously only the symlink's timestamp is used for checking if file are
> modified, this will not trigger a rebuild if the symlink target it
> modified.
>
> So check both symlink timestamp and symlink target timestamp, rebuild
> the initramfs on both symlink changed and target changed.

Do we have to check both symlink and the target file?  I can not think
of why we need check the timestamp of the symlink.  If useless then we
just check the target file then the code will be simpler.
> Signed-off-by: Kairui Song
>
> kdumpctl | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 3f80ba4..3f36903 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -338,11 +338,19 @@ check_files_modified()
>    [ $? -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
> while [ -e "$file" ]; do
> 	time_stamp=`stat -c "%Y" $file`
> 	if [ "$time_stamp" -gt "$image_time" ]; then
> 		modified_files="$modified_files $file"
> 	fi
> 	if [ -L "$file" ]; then
> 		file=$(readlink -m $file)
> 	else
> 		break
> 	fi
> done
> 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'
>
> -- 
> 2.20.1

[PATCH v2 1/2] kdumpctl: follow symlink when checking for modified files

[Kairui Song]

On Mon, May 6, 2019 at 4:21 PM Dave Young  wrote:
> Hi Kairui,
> On 05/06/19 at 03:17pm, Kairui Song wrote:
> > Previously only the symlink's timestamp is used for checking if file are
> > modified, this will not trigger a rebuild if the symlink target it
> > modified.
> >
> > So check both symlink timestamp and symlink target timestamp, rebuild
> > the initramfs on both symlink changed and target changed.
>
> Do we have to check both symlink and the target file?  I can not think
> of why we need check the timestamp of the symlink.  If useless then we
> just check the target file then the code will be simpler.

Just try to cover the case that user changed the symlink itself, that
could be possible.
And in this case the target could have an older timestamp and
initramfs will not rebuild.

--
Best Regards,
Kairui Song

[PATCH v2 2/2] kdumpctl: don't always rebuild when extra_modules is set

[Kairui Song]

We don't necessarily have to always rebuild the initramfs when
extra_modules is set, instead just detect if any module is updated
and only rebuild initramfs if found any updated kernel module.

Signed-off-by: Kairui Song 
---
 kdumpctl | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 3f36903..d3d1325 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -334,6 +334,21 @@ check_files_modified()
    files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS $CORE_COLLECTOR"
    [[ -e /etc/fstab ]] && files="$files /etc/fstab"

+	# Check for any updated extra module
+	EXTRA_MODULES="$(grep ^extra_modules $KDUMP_CONFIG_FILE | sed 's/^extra_modules\s*//')"
+	for _module in $EXTRA_MODULES; do
+		_module_file="$(modinfo -k "$kdump_kver" -n "$_module" 2>/dev/null)"
+		if [ $? -eq 0 ]; then
+			files="$files $_module_file"
+		else
+			# If it's not a module nor builtin, give an error
+			modprobe --set-version "$kdump_kver" --dry-run "$_module" &>/dev/null
+			if [ $? -ne 0 ]; then
+				echo "Invalid kernel module name: $_module"
+			fi
+		fi
+	done
+
    check_exist "$files" && check_executable "$EXTRA_BINS"
    [ $? -ne 0 ] && return 2

@@ -526,7 +541,6 @@ check_system_modified()

check_rebuild()
 {
-	local extra_modules
    local capture_capable_initrd="1"
    local _force_rebuild force_rebuild="0"
    local _force_no_rebuild force_no_rebuild="0"
@@ -566,10 +580,6 @@ check_rebuild()
    	return 0
    fi

-	#will rebuild every time if extra_modules are specified
-	extra_modules=`grep ^extra_modules $KDUMP_CONFIG_FILE`
-	[ -n "$extra_modules" ] && force_rebuild="1"
-
    #check to see if dependent files has been modified
    #since last build of the image file
    if [ -f $TARGET_INITRD ]; then

[PATCH v2 2/2] kdumpctl: don't always rebuild when extra_modules is set

[Dave Young]

On 05/06/19 at 03:17pm, Kairui Song wrote:
> We don't necessarily have to always rebuild the initramfs when
> extra_modules is set, instead just detect if any module is updated
> and only rebuild initramfs if found any updated kernel module.
> Signed-off-by: Kairui Song
>
> kdumpctl | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 3f36903..d3d1325 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -334,6 +334,21 @@ check_files_modified()
>    files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS $CORE_COLLECTOR"
>    [[ -e /etc/fstab ]] && files="$files /etc/fstab"
> # Check for any updated extra module
> EXTRA_MODULES="$(grep ^extra_modules $KDUMP_CONFIG_FILE | sed 's/^extra_modules\s*//')"
> for _module in $EXTRA_MODULES; do
> _module_file="$(modinfo -k "$kdump_kver" -n "$_module" 2>/dev/null)"
> if [ $? -eq 0 ]; then
> 	files="$files $_module_file"
> else
> 	# If it's not a module nor builtin, give an error
> 	modprobe --set-version "$kdump_kver" --dry-run "$_module" &>/dev/null
> 	if [ $? -ne 0 ]; then
> 		echo "Invalid kernel module name: $_module"

Maybe "Module $_module not found" is better
> 	fi
> fi
> done
> check_exist "$files" && check_executable "$EXTRA_BINS"
>  [ $? -ne 0 ] && return 2
>
> @@ -526,7 +541,6 @@ check_system_modified()
>
> check_rebuild()
>  {
> local extra_modules
> local capture_capable_initrd="1"
> local _force_rebuild force_rebuild="0"
> local _force_no_rebuild force_no_rebuild="0"
>
> @@ -566,10 +580,6 @@ check_rebuild()
>    	return 0
>    fi
> #will rebuild every time if extra_modules are specified
> extra_modules=`grep ^extra_modules $KDUMP_CONFIG_FILE`
> [ -n "$extra_modules" ] && force_rebuild="1"
> #check to see if dependent files has been modified
>  #since last build of the image file
>  if [ -f $TARGET_INITRD ]; then
>
> -- 
> 2.20.1

Thanks
Dave

[PATCH v2 2/2] kdumpctl: don't always rebuild when extra_modules is set

[Kairui Song]

On Mon, May 6, 2019 at 4:23 PM Dave Young  wrote:
> On 05/06/19 at 03:17pm, Kairui Song wrote:
> > We don't necessarily have to always rebuild the initramfs when
> > extra_modules is set, instead just detect if any module is updated
> > and only rebuild initramfs if found any updated kernel module.
> > Signed-off-by: Kairui Song
> >
> > kdumpctl | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index 3f36903..d3d1325 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -334,6 +334,21 @@ check_files_modified()
> >       files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS $CORE_COLLECTOR"
> >       [[ -e /etc/fstab ]] && files="$files /etc/fstab"
> > # Check for any updated extra module
> > EXTRA_MODULES="$(grep ^extra_modules $KDUMP_CONFIG_FILE | sed 's/^extra_modules\s*//')"
> > for _module in $EXTRA_MODULES; do
> >         _module_file="$(modinfo -k "$kdump_kver" -n "$_module" 2>/dev/null)"
> >         if [ $? -eq 0 ]; then
> >                 files="$files $_module_file"
> >         else
> >                 # If it's not a module nor builtin, give an error
> >                 modprobe --set-version "$kdump_kver" --dry-run "$_module" &>/dev/null
> >                 if [ $? -ne 0 ]; then
> >                         echo "Invalid kernel module name: $_module"
>
> Maybe "Module $_module not found" is better

Good idea, will update.
> >                 fi
> >         fi
> > done
> > check_exist "$files" && check_executable "$EXTRA_BINS"
> > [ $? -ne 0 ] && return 2
> >
> > @@ -526,7 +541,6 @@ check_system_modified()
> >
> > check_rebuild()
> >  {
> > local extra_modules
> > local capture_capable_initrd="1"
> > local _force_rebuild force_rebuild="0"
> > local _force_no_rebuild force_no_rebuild="0"
> >
> > @@ -566,10 +580,6 @@ check_rebuild()
> >               return 0
> >       fi
> > #will rebuild every time if extra_modules are specified
> > extra_modules=`grep ^extra_modules $KDUMP_CONFIG_FILE`
> > [ -n "$extra_modules" ] && force_rebuild="1"
> > #check to see if dependent files has been modified
> > #since last build of the image file
> > if [ -f $TARGET_INITRD ]; then
> >
> > --
> > 2.20.1
>
> Thanks
> Dave