[PATCH] kdumpctrl: kdump feasibility should fail if no crash memory

[Pratyush Anand]

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

[Xunlei Pang]

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

I don't know if fadump needs reserved memory like kdump?

Regards,
Xunlei
> if [ $DEFAULT_DUMP_MODE == "fadump" ]; then
>    	return 0
>    fi

[Pratyush Anand]

Hi Xunlei,

Thanks for the review

On Fri, Apr 29, 2016 at 2:04 PM, Xunlei Pang  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
> >
> > 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
>
> I 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

[Dave Young]

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

[Hari Bathini]

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  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
> > > >
> > > > 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
> > >
> > > I 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
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

[Xunlei Pang]

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

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

[Pratyush Anand]

Hi Xunlei,

On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang  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

[Xunlei Pang]

On 2016/04/29 at 17:29, Pratyush Anand wrote:
> Hi Xunlei,
>
> On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang  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

[Pratyush Anand]

On Fri, Apr 29, 2016 at 3:17 PM, Xunlei Pang  wrote:
> On 2016/04/29 at 17:29, Pratyush Anand wrote:
> > Hi Xunlei,
> >
> > On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang  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

[Dave Young]

On 04/29/16 at 03:36pm, Pratyush Anand wrote:
> On Fri, Apr 29, 2016 at 3:17 PM, Xunlei Pang  wrote:
> > On 2016/04/29 at 17:29, Pratyush Anand wrote:
> > > Hi Xunlei,
> > >
> > > On Fri, Apr 29, 2016 at 2:15 PM, Xunlei Pang  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