> 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
