Currently initramfs is rebuilt even when crash kernel memory is not available and then latter on kdump service is failed.
Its better to fail during feasibility itself when crash memory is not reserved.
Signed-off-by: Pratyush Anand panand@redhat.com --- kdumpctl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c7abaafdda27..38f29d2a305d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -545,13 +545,6 @@ need_64bit_headers() # as the currently running kernel. load_kdump() { - MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size) - if [ $MEM_RESERVED -eq 0 ] - then - echo "No memory reserved for crash kernel." >&2 - return 1 - fi - ARCH=`uname -m` if [ "$ARCH" == "i686" -o "$ARCH" == "i386" ] then @@ -876,8 +869,26 @@ check_fence_kdump_config() return 0 }
+is_crash_mem_reserved() +{ + MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size) + if [ $MEM_RESERVED -eq 0 ] + then + echo "No memory reserved for crash kernel." >&2 + return 0 + fi + + return 1 +} + + check_dump_feasibility() { + is_crash_mem_reserved + if [ $? -eq 0 ];then + return 1 + fi + if [ $DEFAULT_DUMP_MODE == "fadump" ]; then return 0 fi
On 2016/04/29 at 15:57, Pratyush Anand wrote:
Currently initramfs is rebuilt even when crash kernel memory is not available and then latter on kdump service is failed.
Its better to fail during feasibility itself when crash memory is not reserved.
Signed-off-by: Pratyush Anand panand@redhat.com
kdumpctl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c7abaafdda27..38f29d2a305d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -545,13 +545,6 @@ need_64bit_headers() # as the currently running kernel. load_kdump() {
- MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)
- if [ $MEM_RESERVED -eq 0 ]
- then
echo "No memory reserved for crash kernel." >&2return 1- fi
- ARCH=`uname -m` if [ "$ARCH" == "i686" -o "$ARCH" == "i386" ] then
@@ -876,8 +869,26 @@ check_fence_kdump_config() return 0 }
+is_crash_mem_reserved() +{
- MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)
- if [ $MEM_RESERVED -eq 0 ]
- then
echo "No memory reserved for crash kernel." >&2return 0- fi
- return 1
+}
check_dump_feasibility() {
- is_crash_mem_reserved
- if [ $? -eq 0 ];then
return 1- fi
I don't know if fadump needs reserved memory like kdump?
Regards, Xunlei
if [ $DEFAULT_DUMP_MODE == "fadump" ]; then return 0 fi
Hi Xunlei,
Thanks for the review
On Fri, Apr 29, 2016 at 2:04 PM, Xunlei Pang xpang@redhat.com wrote:
On 2016/04/29 at 15:57, Pratyush Anand wrote:
Currently initramfs is rebuilt even when crash kernel memory is not available and then latter on kdump service is failed.
Its better to fail during feasibility itself when crash memory is not reserved.
Signed-off-by: Pratyush Anand panand@redhat.com
kdumpctl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c7abaafdda27..38f29d2a305d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -545,13 +545,6 @@ need_64bit_headers() # as the currently running kernel. load_kdump() {
MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)if [ $MEM_RESERVED -eq 0 ]thenecho "No memory reserved for crash kernel." >&2return 1fiARCH=`uname -m` if [ "$ARCH" == "i686" -o "$ARCH" == "i386" ] then@@ -876,8 +869,26 @@ check_fence_kdump_config() return 0 }
+is_crash_mem_reserved() +{
MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)if [ $MEM_RESERVED -eq 0 ]thenecho "No memory reserved for crash kernel." >&2return 0fireturn 1+}
check_dump_feasibility() {
is_crash_mem_reservedif [ $? -eq 0 ];thenreturn 1fiI don't know if fadump needs reserved memory like kdump?
I am also not sure, but it looks like that fadump also uses same memory reservation model. Documentation says: "Fadump uses the same firmware interfaces and memory reservation model as phyp assisted dump."
~Pratyush
On 04/29/16 at 02:50pm, Pratyush Anand wrote:
Hi Xunlei,
Thanks for the review
On Fri, Apr 29, 2016 at 2:04 PM, Xunlei Pang xpang@redhat.com wrote:
On 2016/04/29 at 15:57, Pratyush Anand wrote:
Currently initramfs is rebuilt even when crash kernel memory is not available and then latter on kdump service is failed.
Its better to fail during feasibility itself when crash memory is not reserved.
Signed-off-by: Pratyush Anand panand@redhat.com
kdumpctl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c7abaafdda27..38f29d2a305d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -545,13 +545,6 @@ need_64bit_headers() # as the currently running kernel. load_kdump() {
MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)if [ $MEM_RESERVED -eq 0 ]thenecho "No memory reserved for crash kernel." >&2return 1fiARCH=`uname -m` if [ "$ARCH" == "i686" -o "$ARCH" == "i386" ] then@@ -876,8 +869,26 @@ check_fence_kdump_config() return 0 }
+is_crash_mem_reserved() +{
MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)if [ $MEM_RESERVED -eq 0 ]thenecho "No memory reserved for crash kernel." >&2return 0fireturn 1+}
check_dump_feasibility() {
is_crash_mem_reservedif [ $? -eq 0 ];thenreturn 1fiI don't know if fadump needs reserved memory like kdump?
I am also not sure, but it looks like that fadump also uses same memory reservation model. Documentation says: "Fadump uses the same firmware interfaces and memory reservation model as phyp assisted dump."
phyp assisten dump is not kdump, it is another firmware assisted dump in powerpc. I think fadump does not need check crash_mem.
We need it only in check_kdump_feasibility, Ccing Hari anyway.
Thanks Dave
On 05/11/2016 08:39 AM, Dave Young wrote:
On 04/29/16 at 02:50pm, Pratyush Anand wrote:
Hi Xunlei,
Thanks for the review
On Fri, Apr 29, 2016 at 2:04 PM, Xunlei Pang xpang@redhat.com wrote:
On 2016/04/29 at 15:57, Pratyush Anand wrote:
Currently initramfs is rebuilt even when crash kernel memory is not available and then latter on kdump service is failed.
Its better to fail during feasibility itself when crash memory is not reserved.
Signed-off-by: Pratyush Anand panand@redhat.com
kdumpctl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c7abaafdda27..38f29d2a305d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -545,13 +545,6 @@ need_64bit_headers() # as the currently running kernel. load_kdump() {
MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)if [ $MEM_RESERVED -eq 0 ]thenecho "No memory reserved for crash kernel." >&2return 1fiARCH=`uname -m` if [ "$ARCH" == "i686" -o "$ARCH" == "i386" ] then@@ -876,8 +869,26 @@ check_fence_kdump_config() return 0 }
+is_crash_mem_reserved() +{
MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)if [ $MEM_RESERVED -eq 0 ]thenecho "No memory reserved for crash kernel." >&2return 0fireturn 1+}
- check_dump_feasibility() {
is_crash_mem_reservedif [ $? -eq 0 ];thenreturn 1fiI don't know if fadump needs reserved memory like kdump?
I am also not sure, but it looks like that fadump also uses same memory reservation model. Documentation says: "Fadump uses the same firmware interfaces and memory reservation model as phyp assisted dump."
phyp assisten dump is not kdump, it is another firmware assisted dump in powerpc. I think fadump does not need check crash_mem.
Pratyush, fadump does not need this check.. As Dave mentioned, this check should go in check_kdump_feasibility..
We need it only in check_kdump_feasibility, Ccing Hari anyway.
Thanks for cc'ing me, Dave.
-Hari
Thanks Dave _______________________________________________ kexec mailing list kexec@lists.fedoraproject.org http://lists.fedoraproject.org/admin/lists/kexec@lists.fedoraproject.org
On 2016/04/29 at 15:57, Pratyush Anand wrote:
Currently initramfs is rebuilt even when crash kernel memory is not available and then latter on kdump service is failed.
Its better to fail during feasibility itself when crash memory is not reserved.
Signed-off-by: Pratyush Anand panand@redhat.com
kdumpctl | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c7abaafdda27..38f29d2a305d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -545,13 +545,6 @@ need_64bit_headers() # as the currently running kernel. load_kdump() {
- MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)
- if [ $MEM_RESERVED -eq 0 ]
- then
echo "No memory reserved for crash kernel." >&2return 1- fi
- ARCH=`uname -m` if [ "$ARCH" == "i686" -o "$ARCH" == "i386" ] then
@@ -876,8 +869,26 @@ check_fence_kdump_config() return 0 }
+is_crash_mem_reserved() +{
- MEM_RESERVED=$(cat /sys/kernel/kexec_crash_size)
- if [ $MEM_RESERVED -eq 0 ]
- then
echo "No memory reserved for crash kernel." >&2return 0
Besides, I would recommend "return 0" for successful cases, and "return 1" for failed cases, it's semantically better.
Then we can have:
if ! is_crash_mem_reserved; then return 1 fi
Regards, Xunlei
- fi
- return 1
+}
check_dump_feasibility() {
- is_crash_mem_reserved
- if [ $? -eq 0 ];then
return 1- fi
- if [ $DEFAULT_DUMP_MODE == "fadump" ]; then return 0 fi
Hi Xunlei,
On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang xpang@redhat.com wrote:
Besides, I would recommend "return 0" for successful cases, and "return 1" for failed cases, it's semantically better.
I think all the function which starts with is_ has same semantic what has been used in this patch. For example, when is_dump_target_configured() returns 1, it means dump target has been used in kdump.conf. Moreover, it seems reasonable to return true(1) when answer of question is_xxxxxxx is "yes".
Then we can have:
if ! is_crash_mem_reserved; then return 1 fi
~Pratyush
On 2016/04/29 at 17:29, Pratyush Anand wrote:
Hi Xunlei,
On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang xpang@redhat.com wrote:
Besides, I would recommend "return 0" for successful cases, and "return 1" for failed cases, it's semantically better.
I think all the function which starts with is_ has same semantic what has been used in this patch. For example, when is_dump_target_configured() returns 1, it means dump target has been used in kdump.conf. Moreover, it seems reasonable to return true(1) when answer of question is_xxxxxxx is "yes".
Yes, I understood that way at the beginning, but in shell world, seems return 0 is true.
is_dump_target_configured() actually returns 0 for true cases(target found): [ -n "$_target" ]
if $_target is NULL, $? is 1, otherwise $? is 0(true).
Please correct me if I'm wrong :-)
Regards, Xunlei
Then we can have:
if ! is_crash_mem_reserved; then return 1 fi
~Pratyush
On Fri, Apr 29, 2016 at 3:17 PM, Xunlei Pang xpang@redhat.com wrote:
On 2016/04/29 at 17:29, Pratyush Anand wrote:
Hi Xunlei,
On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang xpang@redhat.com wrote:
Besides, I would recommend "return 0" for successful cases, and "return 1" for failed cases, it's semantically better.
I think all the function which starts with is_ has same semantic what has been used in this patch. For example, when is_dump_target_configured() returns 1, it means dump target has been used in kdump.conf. Moreover, it seems reasonable to return true(1) when answer of question is_xxxxxxx is "yes".
Yes, I understood that way at the beginning, but in shell world, seems return 0 is true.
is_dump_target_configured() actually returns 0 for true cases(target found): [ -n "$_target" ]
if $_target is NULL, $? is 1, otherwise $? is 0(true).
$_target is NULL or non-empty? I think, -n represent non-empty. I am also a beginner for shell, so others can confirm.
~Pratyush
On 04/29/16 at 03:36pm, Pratyush Anand wrote:
On Fri, Apr 29, 2016 at 3:17 PM, Xunlei Pang xpang@redhat.com wrote:
On 2016/04/29 at 17:29, Pratyush Anand wrote:
Hi Xunlei,
On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang xpang@redhat.com wrote:
Besides, I would recommend "return 0" for successful cases, and "return 1" for failed cases, it's semantically better.
I think all the function which starts with is_ has same semantic what has been used in this patch. For example, when is_dump_target_configured() returns 1, it means dump target has been used in kdump.conf. Moreover, it seems reasonable to return true(1) when answer of question is_xxxxxxx is "yes".
Yes, I understood that way at the beginning, but in shell world, seems return 0 is true.
is_dump_target_configured() actually returns 0 for true cases(target found): [ -n "$_target" ]
if $_target is NULL, $? is 1, otherwise $? is 0(true).
$_target is NULL or non-empty? I think, -n represent non-empty. I am also a beginner for shell, so others can confirm.
I think xunlei means non-empty, you have same understanding :)
Just the return code difference, in bash return 0 should be true otherwise it is false.
Thanks Dave
-
Dave Young -
Hari Bathini -
Pratyush Anand -
Xunlei Pang