On 2023/9/1 17:49, Baoquan He wrote:
>>> +
>>> + *high = true;
>>> + } else if (ret || !*crash_size) {
>> This check can be moved outside of #ifdef. Because even '!high', it's
>> completely
>> applicable. The overall adjustment is as follows:
> Hmm, the current logic is much easier to understand. However, I may not
> 100% get your suggestion. Can you paste the complete code in your
> suggested way? Do not need 100% correct code, just the skeleton of code logic
> so that I can better understand it and add inline comment.
int __init parse_crashkernel(...)
{
int ret;
/* crashkernel=X[@offset] */
ret = __parse_crashkernel(cmdline, system_ram, crash_size,
crash_base, NULL);
#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
if (high && ret == -ENOENT) {
... ... //The code for your original branch "else if
(ret == -ENOENT) {"
ret = 0; //Added based on the next discussion
}
+#endif
if (!*crash_size)
ret = -EINVAL;
return ret;
}
>
>> - if (!high)
>> - return ret;
>>
>> #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
>> if (high && ret == -ENOENT) {
>> ... ...
>> if (ret || !*crash_size) //parse HIGH
>> ... ...
>> }
>>
>> //At this point, *crash_size is not 0 and ret is 0.
>> //We can also delete if (!*crash_size) above because it will be checked
>> later.
>> #endif
>>
>> if (!*crash_size)
>> ret = -EINVAL;
>>
>> return ret;
> When crashkernel=,high is specified while crashkernel=,low is omitted,
> the ret==-ENOENT, but we can't return ret directly. That is still an
> acceptable way.
Oh, yes. Sorry, I didn't notice branch "ret==-ENOENT" didn't return. So "ret =
0;"
needs to be added.
if (high && ret == -ENOENT) {
... ...
*high = true;
+ ret = 0;
}
>
>> - return 0;
>>
>>> + /* The specified value is invalid */
>>> + return -1;
>>> + }
>>> +#endif
>>> return 0;
>>> }
>>>
>>>
--
Regards,
Zhen Lei
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
