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