[v2,1/2] refactor code parsing size based on memory range

[Michael Ellerman]

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

Are you sure that's true?

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 ^

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.

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