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

[Hari Bathini]

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@xxxxxxxxxxxxxxxx
> https://lists.ozlabs.org/listinfo/linuxppc-dev