Update from V2: - Improve the error message - Also check module dependencies - Clean up code
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 | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-)
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.
Also give a proper error message if the file doesn't exist.
Signed-off-by: Kairui Song kasong@redhat.com --- kdumpctl | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 75eebac..739f162 100755 --- a/kdumpctl +++ b/kdumpctl @@ -338,11 +338,23 @@ 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" + if [ -e "$file" ]; then + 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) + time_stamp=`stat -c "%Y" $file` + if [ "$time_stamp" -gt "$image_time" ]; then + modified_files="$modified_files $file" + fi + fi + else + echo "$file doesn't exist" 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'
On 05/13/19 at 02:20pm, 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.
Also give a proper error message if the file doesn't exist.
Signed-off-by: Kairui Song kasong@redhat.com
kdumpctl | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 75eebac..739f162 100755 --- a/kdumpctl +++ b/kdumpctl @@ -338,11 +338,23 @@ check_files_modified() [ $? -ne 0 ] && return 2
for file in $files; do
time_stamp=`stat -c "%Y" $file`if [ "$time_stamp" -gt "$image_time" ]; thenmodified_files="$modified_files $file"
if [ -e "$file" ]; thentime_stamp=`stat -c "%Y" $file`if [ "$time_stamp" -gt "$image_time" ]; thenmodified_files="$modified_files $file"fiif [ -L "$file" ]; thenfile=$(readlink -m $file)time_stamp=`stat -c "%Y" $file`if [ "$time_stamp" -gt "$image_time" ]; thenmodified_files="$modified_files $file"fifielseecho "$file doesn't exist"- 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
Good catch!
Acked-by: Dave Young dyoung@redhat.com
Thanks Dave
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 kasong@redhat.com --- kdumpctl | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 739f162..e77877a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -334,6 +334,28 @@ 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*//')" + if [ -n "$EXTRA_MODULES" ]; then + if [ -e /lib/modules/$kdump_kver/modules.dep ]; then + files="$files /lib/modules/$kdump_kver/modules.dep" + fi + for _module in $EXTRA_MODULES; do + _module_file="$(modinfo --set-version "$kdump_kver" --filename "$_module" 2>/dev/null)" + if [[ $? -eq 0 ]]; then + files="$files $_module_file" + for _dep_modules in $(modinfo -F depends $_module | tr ',' ' '); do + files="$files $(modinfo --set-version "$kdump_kver" --filename $_dep_modules 2>/dev/null)" + done + else + # If it's not a module nor builtin, give an error + if ( modprobe --set-version "$kdump_kver" --dry-run "$_module" &>/dev/null ); then + echo "Module $_module not found" + fi + fi + done + fi + check_exist "$files" && check_executable "$EXTRA_BINS" [ $? -ne 0 ] && return 2
@@ -563,7 +585,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" @@ -603,10 +624,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
On 05/13/19 at 02:20pm, 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 kasong@redhat.com
kdumpctl | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 739f162..e77877a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -334,6 +334,28 @@ 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*//')"
- if [ -n "$EXTRA_MODULES" ]; then
if [ -e /lib/modules/$kdump_kver/modules.dep ]; thenfiles="$files /lib/modules/$kdump_kver/modules.dep"fifor _module in $EXTRA_MODULES; do_module_file="$(modinfo --set-version "$kdump_kver" --filename "$_module" 2>/dev/null)"if [[ $? -eq 0 ]]; thenfiles="$files $_module_file"for _dep_modules in $(modinfo -F depends $_module | tr ',' ' '); dofiles="$files $(modinfo --set-version "$kdump_kver" --filename $_dep_modules 2>/dev/null)"doneelse# If it's not a module nor builtin, give an errorif ( modprobe --set-version "$kdump_kver" --dry-run "$_module" &>/dev/null ); thenecho "Module $_module not found"fifidone- fi
- check_exist "$files" && check_executable "$EXTRA_BINS" [ $? -ne 0 ] && return 2
@@ -563,7 +585,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"
@@ -603,10 +624,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
The force rebuild in case extra_modules was inherited from RHEL6 code, because there was not easy way to detect extra module directories correctly probably the modprobe tools limitatioins.
Since this has been tested, and covered weak modules and other cases, I think it is fine to drop the limitation. But it would be good to add above history info, also add some test info in patch log.
Otherwise:
Acked-by: Dave Young dyoung@redhat.com
Thanks Dave
-
Dave Young -
Kairui Song