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

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

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