2022-04-25 22:24:30

by Lizhe

Subject: [PATCH] kernel/crash_core.c : Remove redundant checks for ck_cmdline is NULL

When ck_cmdline is NULL, the only caller of get_last_crashkernel()
has already done non-NULL check(see __parse_crashkernel()),
so it doesn't make any sense to make a check here

Signed-off-by: lizhe <[email protected]>
---
kernel/crash_core.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 256cf6db573c..c232f01a2c54 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -222,9 +222,6 @@ static __init char *get_last_crashkernel(char *cmdline,
p = strstr(p+1, name);
}

- if (!ck_cmdline)
- return NULL;
-
return ck_cmdline;
}

--
2.25.1

2022-04-26 11:28:33

by Philipp Rudo

Subject: Re: [PATCH] kernel/crash_core.c : Remove redundant checks for ck_cmdline is NULL

Hi lizhe,

On Mon, 25 Apr 2022 08:38:57 -0700
lizhe <[email protected]> wrote:

> When ck_cmdline is NULL, the only caller of get_last_crashkernel()
> has already done non-NULL check(see __parse_crashkernel()),
> so it doesn't make any sense to make a check here

sorry, but I still don't like the description. What I don't understand
in particular is why you are mentioning the caller (__parse_crashkernel)
here. ck_cmdline is a local variable to get_last_crashkernel. So the
caller cannot perform any check on the variable but only the return
value of the function. So the patch description should describe why we
can remove the additional return NULL without changing the behavior of
the function.

Thanks
Philipp

> Signed-off-by: lizhe <[email protected]>
> ---
> kernel/crash_core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 256cf6db573c..c232f01a2c54 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -222,9 +222,6 @@ static __init char *get_last_crashkernel(char *cmdline,
> p = strstr(p+1, name);
> }
>
> - if (!ck_cmdline)
> - return NULL;
> -
> return ck_cmdline;
> }
>

2022-05-04 04:17:18

by Philipp Rudo

Subject: Re: [PATCH] kernel/crash_core.c : Remove redundant checks for ck_cmdline is NULL

Hi lizhe,

On Mon, 2 May 2022 18:11:20 +0800 (CST)
lizhe <[email protected]> wrote:

> HI Philipp Rudo.
>
>
> When ck_cmdline is NULL. The last three lines of this function are equivalent to :
> if ( ! NULL)
> return NULL;
> return NULL;
> This is obviously a redundant check.
>
>
> I will use the above description to describe the patch,

the explanation looks good to me.

Thanks!
Philipp


>
> thanks.
> lizhe
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> At 2022-04-26 16:39:52, "Philipp Rudo" <[email protected]> wrote:
> >Hi lizhe,
> >
> >On Mon, 25 Apr 2022 08:38:57 -0700
> >lizhe <[email protected]> wrote:
> >
> >> When ck_cmdline is NULL, the only caller of get_last_crashkernel()
> >> has already done non-NULL check(see __parse_crashkernel()),
> >> so it doesn't make any sense to make a check here
> >
> >sorry, but I still don't like the description. What I don't understand
> >in particular is why you are mentioning the caller (__parse_crashkernel)
> >here. ck_cmdline is a local variable to get_last_crashkernel. So the
> >caller cannot perform any check on the variable but only the return
> >value of the function. So the patch description should describe why we
> >can remove the additional return NULL without changing the behavior of
> >the function.
> >
> >Thanks
> >Philipp
> >
> >> Signed-off-by: lizhe <[email protected]>
> >> ---
> >> kernel/crash_core.c | 3 ---
> >> 1 file changed, 3 deletions(-)
> >>
> >> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> >> index 256cf6db573c..c232f01a2c54 100644
> >> --- a/kernel/crash_core.c
> >> +++ b/kernel/crash_core.c
> >> @@ -222,9 +222,6 @@ static __init char *get_last_crashkernel(char *cmdline,
> >> p = strstr(p+1, name);
> >> }
> >>
> >> - if (!ck_cmdline)
> >> - return NULL;
> >> -
> >> return ck_cmdline;
> >> }
> >>