Messages in this thread | | | Subject | Re: [PATCH v2 3/8] crash_core: change parse_crashkernel() to support crashkernel=,high|low parsing | From | "Leizhen (ThunderTown)" <> | Date | Mon, 4 Sep 2023 10:47:58 +0800 |
| |
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
| |