On Tue, Jun 03, 2014 at 01:25:58PM +0530, Hari Bathini wrote:
Take a backup of original initrd when fadump is used first time or
when user has switched from kdump to fadump. This will allow us to
fall back to original initrd when kdump service fails to rebuild
the fadump ready default initrd.
Signed-off-by: Mahesh Salgaonkar <mahesh(a)linux.vnet.ibm.com>
Signed-off-by: Hari Bathini <hbathini(a)linux.vnet.ibm.com>
---
kdumpctl | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 70 insertions(+), 16 deletions(-)
diff --git a/kdumpctl b/kdumpctl
index 417494a..e57aeff 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -16,6 +16,7 @@ DUMP_MODE="kdump" # kdump shall be the default dump mode
. /lib/kdump/kdump-lib.sh
standard_kexec_args="-p"
+declare -i image_time
What's this? forcing the type of image_time to integer. What if we don't
do that. Does the time stamp comparision does not work fine?
If this is a bug please post a separate patch to fix it. Also put a
nice command explaining this. And possibly keep the declaration local
instead of global.
if [ -f /etc/sysconfig/kdump ]; then
. /etc/sysconfig/kdump
@@ -29,12 +30,25 @@ single_instance_lock()
determine_dump_mode()
{
+ if [ -z "$KDUMP_KERNELVER" ]; then
+ kdump_kver=`uname -r`
+ else
+ kdump_kver=$KDUMP_KERNELVER
+ fi
+
I see we doing same thing in check_rebuild(). I think this should be
part of initializations. How about writing a function say do_init() and
handle simple initiliazions there. That way we don't have to copy code
And do_init() we can call from main() early.
+ default_initrd="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}.img"
+ kdump_initrd="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}kdump.img"
Why do we need both kdump_initrd and default_initrd?
+ default_initrd_bak="$default_initrd.default.bak"
Again, some of the initrd initialization code overlaps with
check_rebuild(). Let us use global variables with capital letters
and do this initialization in do_init().
+
# Check if firmware-assisted dump is enabled
# if yes, set the dump mode as fadump
if is_fadump_capable; then
echo "Using dump mode fadump"
DUMP_MODE="fadump"
fi
+
+ # Handle dump mode switch from kdump to fadump OR fadump to kdump
+ handle_dump_mode_switch
What are we doing so many things in a function called
determine_dump_mode(). This function should just return the dump mode and
should not try to do anything else.
When does dump mode switch happen? How does one disable/enable fadump?
}
# remove_cmdline_param <kernel cmdline> <param1> [<param2>] ...
[<paramN>]
@@ -134,6 +148,10 @@ function save_core()
function rebuild_initrd()
{
+ if [ $DUMP_MODE == "fadump" ]; then
+ backup_default_initrd
+ fi
+
I would rather call it backup_fadump_initrd. This will make it much
clear that what initrd we are talking about.
$MKDUMPRD $kdump_initrd $kdump_kver
if [ $? != 0 ]; then
echo "mkdumprd: failed to make kdump initrd" >&2
@@ -163,6 +181,47 @@ function check_executable()
done
}
+backup_default_initrd()
+{
+ # Check if backup initrd is already present. If not, then
+ # this is the first time fadump is being used OR user
+ # has switched from kdump to fadump.
+ # Take a backup of the original default initrd before
+ # we rebuild default initrd for fadump support.
+ if [ ! -e $default_initrd_bak ];then
+ echo "Backing up default initrd"
Same, let us call it "Backing up fadump initrd".
+ cp $default_initrd $default_initrd_bak
+ sync
+ fi
+}
+
+handle_dump_mode_switch()
+{
+ if [ $DUMP_MODE == "fadump" ] && [ -e $kdump_initrd ];then
+ # This means user has switched from kdump to fadump.
+ # Remove kdump initrd which is no longer needed
+ rm -f $kdump_initrd
+ fi
Why should we remove kdump initrd? I think just leave it there. Dump
mode is determined from presence of /sys files and if somebody switched
from kdump to fadump over next reboot then you should check fadump
intird and see if kdump module is there or not and then do the rebuilding
accordingly.
Why does it matter whether kdump initrd was present or not.
+}
+
+find_initrd_image_time()
+{
+ image_time=0
+
+ # Check to see if dependent files have been modified
+ # since last build of the image file
+ if [ $DUMP_MODE == "fadump" ]; then
+ if [ -f $default_initrd ]; then
+ image_time=`stat -c "%Y" $default_initrd 2>/dev/null`
+ fi
+ else
+ if [ -f $kdump_initrd ]; then
+ image_time=`stat -c "%Y" $kdump_initrd 2>/dev/null`
+ return
+ fi
+ fi
I think we shorten it by storing file name in common variable
local file
if [ $DUMP_MODE == "fadump" ]
file = $fadump_initrd
else
file = $kdump_initrd
fi
image_time = `stat -c "%Y" $file 2>/dev/null`
+}
+
function check_config()
{
local nr
@@ -234,14 +293,7 @@ function check_rebuild()
local extra_modules modified_files=""
local _force_rebuild force_rebuild="0"
- if [ -z "$KDUMP_KERNELVER" ]; then
- kdump_kver=`uname -r`
- else
- kdump_kver=$KDUMP_KERNELVER
- fi
-
Hmm,, you removed it from here. That means you don't like initialization
in check_config() and wanted to move it earlier. But determine_dump_mode()
is not the place to do. In fact that function should just read /sys files.
Why do you need to set these variables that early. Until and unless
somebody is doing "start/restart" you should not have to worry about
what's the initrd?
And in that case check_config() is alsmost equivalent of do_init() I
suggested and we might not need do_init().
kdump_kernel="${KDUMP_BOOTDIR}/${KDUMP_IMG}-${kdump_kver}${KDUMP_IMG_EXT}"
- kdump_initrd="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}kdump.img"
_force_rebuild=`grep ^force_rebuild $KDUMP_CONFIG_FILE 2>/dev/null`
if [ $? -eq 0 ]; then
@@ -256,13 +308,9 @@ function check_rebuild()
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 $kdump_initrd ]; then
- image_time=`stat -c "%Y" $kdump_initrd 2>/dev/null`
- else
- image_time=0
- fi
+ # Find initrd image time based on whether dependent files have been
+ # modified since last build of the image file
+ find_initrd_image_time
#also rebuild when Pacemaker cluster conf is changed and fence kdump is enabled.
modified_files=$(get_pcs_cluster_modified_files $image_time)
@@ -285,9 +333,15 @@ function check_rebuild()
done
if [ $image_time -eq 0 ]; then
- echo -n "No kdump initial ramdisk found."; echo
+ if [ $DUMP_MODE == "kdump" ]; then
+ echo -n "No kdump initial ramdisk found."; echo
+ fi
Do we need to check for DUMP_MODE=kdump? Will there ever be case where
you will assign image_time = 0 for fadump initrd?
elif [ "$force_rebuild" != "0" ]; then
- echo -n "Force rebuild $kdump_initrd"; echo
+ if [ $DUMP_MODE == "kdump" ]; then
+ echo -n "Force rebuild $kdump_initrd"; echo
+ else
+ echo -n "Force rebuild $default_initrd with fadump support"; echo
+ fi
This nested if block looks horrible. This needs to be simplified.
Why do we need to give two different messages. Why not just say.
echo -n "Force rebuild $kdump_initrd".
And initialize kdump_initrd to eitehr fadump or kdump initrd based on
dump mode.
Thanks
Vivek
elif [ -n "$modified_files" ]; then
echo "Detected change(s) the following file(s):"
echo -n " "; echo "$modified_files" | sed 's/\s/\n /g'