lkml.org 
[lkml]   [2016]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [v2,1/2] refactor code parsing size based on memory range
From
Date


On 07/05/2016 10:48 AM, Michael Ellerman wrote:
>> On 06/24/2016 10:56 AM, Michael Ellerman wrote:
>>> On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote:
> ...
>> While the code is moved to kernel/params.c file, there is no change in logic
>> for crashkernel parameter parsing as the moved code is invoked with function
>> calls at appropriate places.

Hi Michael,

> Are you sure that's true?

Yes. I tested it.

>
> The old code would return -EINVAL from parse_crashkernel_mem() for any
> error, regardless of whether it had already parsed some of the string.
>
> eg:
>
>>>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>>>> index 56b3ed0..d43f5cc 100644
>>>> --- a/kernel/kexec_core.c
>>>> +++ b/kernel/kexec_core.c
>>>> @@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char *cmdline,
>>>> char *cur = cmdline, *tmp;
>>>>
>>>> /* for each entry of the comma-separated list */
>>>> - do {
>>>> - unsigned long long start, end = ULLONG_MAX, size;
>>>> -
>>>> - /* get the start of the range */
>>>> - start = memparse(cur, &tmp);
>>>> - if (cur == tmp) {
>>>> - pr_warn("crashkernel: Memory value expected\n");
>>>> - return -EINVAL;
>>>> - }
>>>> - cur = tmp;
>>>> - if (*cur != '-') {
>>>> - pr_warn("crashkernel: '-' expected\n");
>>>> - return -EINVAL;
>>>> - }
>>>> - cur++;
>>>> -
>>>> - /* if no ':' is here, than we read the end */
>>>> - if (*cur != ':') {
>>>> - end = memparse(cur, &tmp);
>>>> - if (cur == tmp) {
>>>> - pr_warn("crashkernel: Memory value expected\n");
>>>> - return -EINVAL;
>>>> - }
> So eg, if I give it "128M-foo" it will modify cur, and then error out here ^

It does modify cur (local variable) but that would have no bearing on
parsing logic
as we are returning immediately..

> You've changed that to:
>
>>>> + *crash_size = parse_mem_range_size("crashkernel", &cur, system_ram);
>>>> + if (cur == cmdline)
>>>> + return -EINVAL;
> Which only returns EINVAL if cur is not modified at all.

I think the confusion is with the same local variable cur in
parse_crashkernel_mem()
& parse_mem_range_size() functions.

We modified cur (local variable) in parse_mem_range_size() but the
output parameter (char **str)
remains unchanged unless we find a match.

Thanks
Hari

> And looking below:
>
>>>> diff --git a/kernel/params.c b/kernel/params.c
>>>> index a6d6149..84e40ae 100644
>>>> --- a/kernel/params.c
>>>> +++ b/kernel/params.c
> ...
>>>> +unsigned long long __init parse_mem_range_size(const char *param,
>>>> + char **str,
>>>> + unsigned long long system_ram)
>>>> +{
>>>> + char *cur = *str, *tmp;
>>>> + unsigned long long mem_size = 0;
>>>> +
>>>> + /* for each entry of the comma-separated list */
>>>> + do {
>>>> + unsigned long long start, end = ULLONG_MAX, size;
>>>> +
>>>> + /* get the start of the range */
>>>> + start = memparse(cur, &tmp);
>>>> + if (cur == tmp) {
>>>> + printk(KERN_INFO "%s: Memory value expected\n", param);
>>>> + return mem_size;
>>>> + }
>>>> + cur = tmp;
>>>> + if (*cur != '-') {
>>>> + printk(KERN_INFO "%s: '-' expected\n", param);
>>>> + return mem_size;
>>>> + }
>>>> + cur++;
>>>> +
>>>> + /* if no ':' is here, than we read the end */
>>>> + if (*cur != ':') {
>>>> + end = memparse(cur, &tmp);
>>>> + if (cur == tmp) {
>>>> + printk(KERN_INFO "%s: Memory value expected\n",
>>>> + param);
>>>> + return mem_size;
> If we error out here for example, we have modified cur, so the code above
> *won't* return EINVAL.

>
> Which looks like a behaviour change to me?
>
> cheers
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

\
 
 \ /
  Last update: 2016-07-05 09:41    [W:0.074 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site