* [PATCH v2 1/2] refactor code parsing size based on memory range [not found] <20160622192213.11192.93613.stgit@hbathini.in.ibm.com> @ 2016-06-22 19:25 ` Hari Bathini 2016-06-24 5:26 ` [v2,1/2] " Michael Ellerman 0 siblings, 1 reply; 6+ messages in thread From: Hari Bathini @ 2016-06-22 19:25 UTC (permalink / raw) To: Michael Ellerman, lkml, linuxppc-dev Cc: Rusty Russell, kexec, Eric Biederman, Vivek Goyal Currently, crashkernel parameter supports the below syntax to parse size based on memory range: crashkernel=<range1>:<size1>[,<range2>:<size2>,...] While such parsing is implemented for crashkernel parameter, it applies to other parameters with similar syntax. So, move this code to a more generic place for code reuse. Cc: Eric Biederman <ebiederm@xmission.com> Cc: Vivek Goyal <vgoyal@redhat.com> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: kexec@lists.infradead.org Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> --- changes in v2: 1. Rebased to latest upstream. 2. Marked few more people on cc. include/linux/kernel.h | 5 +++ kernel/kexec_core.c | 63 +++----------------------------- kernel/params.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 58 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 94aa10f..72f55e5 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -436,6 +436,11 @@ extern char *get_options(const char *str, int nints, int *ints); extern unsigned long long memparse(const char *ptr, char **retptr); extern bool parse_option_str(const char *str, const char *option); +extern bool __init is_param_range_based(const char *cmdline); +extern unsigned long long __init parse_mem_range_size(const char *param, + char **str, + unsigned long long system_ram); + extern int core_kernel_text(unsigned long addr); extern int core_kernel_data(unsigned long addr); extern int __kernel_text_address(unsigned long addr); 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; - } - cur = tmp; - if (end <= start) { - pr_warn("crashkernel: end <= start\n"); - return -EINVAL; - } - } - - if (*cur != ':') { - pr_warn("crashkernel: ':' expected\n"); - return -EINVAL; - } - cur++; - - size = memparse(cur, &tmp); - if (cur == tmp) { - pr_warn("Memory value expected\n"); - return -EINVAL; - } - cur = tmp; - if (size >= system_ram) { - pr_warn("crashkernel: invalid size\n"); - return -EINVAL; - } - - /* match ? */ - if (system_ram >= start && system_ram < end) { - *crash_size = size; - break; - } - } while (*cur++ == ','); + *crash_size = parse_mem_range_size("crashkernel", &cur, system_ram); + if (cur == cmdline) + return -EINVAL; if (*crash_size > 0) { while (*cur && *cur != ' ' && *cur != '@') @@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char *cmdline, const char *name, const char *suffix) { - char *first_colon, *first_space; char *ck_cmdline; BUG_ON(!crash_size || !crash_base); @@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char *cmdline, return parse_crashkernel_suffix(ck_cmdline, crash_size, suffix); /* - * if the commandline contains a ':', then that's the extended + * if the parameter is range based, then that's the extended * syntax -- if not, it must be the classic syntax */ - first_colon = strchr(ck_cmdline, ':'); - first_space = strchr(ck_cmdline, ' '); - if (first_colon && (!first_space || first_colon < first_space)) + if (is_param_range_based(ck_cmdline)) return parse_crashkernel_mem(ck_cmdline, system_ram, crash_size, crash_base); diff --git a/kernel/params.c b/kernel/params.c index a6d6149..84e40ae 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -268,6 +268,102 @@ char *parse_args(const char *doing, return err; } +/* + * is_param_range_based - check if current parameter is range based + * @cmdline: points to the parameter to check + * + * Returns true when the current paramer is range based, false otherwise + */ +bool __init is_param_range_based(const char *cmdline) +{ + char *first_colon, *first_space; + + first_colon = strchr(cmdline, ':'); + first_space = strchr(cmdline, ' '); + if (first_colon && (!first_space || first_colon < first_space)) + return true; + + return false; +} + +/* + * parse_mem_range_size - parse size based on memory range + * @param: the thing being parsed + * @str: (input) where parse begins + * expected format - <range1>:<size1>[,<range2>:<size2>,...] + * (output) On success - next char after parse completes + * On failure - unchanged + * @system_ram: system ram size to check memory range against + * + * Returns the memory size on success and 0 on failure + */ +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; + } + cur = tmp; + if (end <= start) { + printk(KERN_INFO "%s: end <= start\n", param); + return mem_size; + } + } + + if (*cur != ':') { + printk(KERN_INFO "%s: ':' expected\n", param); + return mem_size; + } + cur++; + + size = memparse(cur, &tmp); + if (cur == tmp) { + printk(KERN_INFO "%s: Memory value expected\n", param); + return mem_size; + } + cur = tmp; + if (size >= system_ram) { + printk(KERN_INFO "%s: invalid size\n", param); + return mem_size; + } + + /* match ? */ + if (system_ram >= start && system_ram < end) { + mem_size = size; + *str = cur; + break; + } + } while (*cur++ == ','); + + return mem_size; +} + /* Lazy bastard, eh? */ #define STANDARD_PARAM_DEF(name, type, format, strtolfn) \ int param_set_##name(const char *val, const struct kernel_param *kp) \ _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v2,1/2] refactor code parsing size based on memory range 2016-06-22 19:25 ` [PATCH v2 1/2] refactor code parsing size based on memory range Hari Bathini @ 2016-06-24 5:26 ` Michael Ellerman 2016-06-24 17:15 ` Hari Bathini 0 siblings, 1 reply; 6+ messages in thread From: Michael Ellerman @ 2016-06-24 5:26 UTC (permalink / raw) To: Hari Bathini, lkml, linuxppc-dev Cc: Rusty Russell, kexec, Eric Biederman, Vivek Goyal On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote: > Currently, crashkernel parameter supports the below syntax to parse size > based on memory range: > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...] > > While such parsing is implemented for crashkernel parameter, it applies to > other parameters with similar syntax. So, move this code to a more generic > place for code reuse. > > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: kexec@lists.infradead.org > Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> Hari, it's not immediately clear that this makes no change to the logic in the kexec code. Can you reply with a longer change log explaining why the old & new logic is the same for kexec. cheers > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 94aa10f..72f55e5 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -436,6 +436,11 @@ extern char *get_options(const char *str, int nints, int *ints); > extern unsigned long long memparse(const char *ptr, char **retptr); > extern bool parse_option_str(const char *str, const char *option); > > +extern bool __init is_param_range_based(const char *cmdline); > +extern unsigned long long __init parse_mem_range_size(const char *param, > + char **str, > + unsigned long long system_ram); > + > extern int core_kernel_text(unsigned long addr); > extern int core_kernel_data(unsigned long addr); > extern int __kernel_text_address(unsigned long addr); > 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; > - } > - cur = tmp; > - if (end <= start) { > - pr_warn("crashkernel: end <= start\n"); > - return -EINVAL; > - } > - } > - > - if (*cur != ':') { > - pr_warn("crashkernel: ':' expected\n"); > - return -EINVAL; > - } > - cur++; > - > - size = memparse(cur, &tmp); > - if (cur == tmp) { > - pr_warn("Memory value expected\n"); > - return -EINVAL; > - } > - cur = tmp; > - if (size >= system_ram) { > - pr_warn("crashkernel: invalid size\n"); > - return -EINVAL; > - } > - > - /* match ? */ > - if (system_ram >= start && system_ram < end) { > - *crash_size = size; > - break; > - } > - } while (*cur++ == ','); > + *crash_size = parse_mem_range_size("crashkernel", &cur, system_ram); > + if (cur == cmdline) > + return -EINVAL; > > if (*crash_size > 0) { > while (*cur && *cur != ' ' && *cur != '@') > @@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char *cmdline, > const char *name, > const char *suffix) > { > - char *first_colon, *first_space; > char *ck_cmdline; > > BUG_ON(!crash_size || !crash_base); > @@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char *cmdline, > return parse_crashkernel_suffix(ck_cmdline, crash_size, > suffix); > /* > - * if the commandline contains a ':', then that's the extended > + * if the parameter is range based, then that's the extended > * syntax -- if not, it must be the classic syntax > */ > - first_colon = strchr(ck_cmdline, ':'); > - first_space = strchr(ck_cmdline, ' '); > - if (first_colon && (!first_space || first_colon < first_space)) > + if (is_param_range_based(ck_cmdline)) > return parse_crashkernel_mem(ck_cmdline, system_ram, > crash_size, crash_base); > > diff --git a/kernel/params.c b/kernel/params.c > index a6d6149..84e40ae 100644 > --- a/kernel/params.c > +++ b/kernel/params.c > @@ -268,6 +268,102 @@ char *parse_args(const char *doing, > return err; > } > > +/* > + * is_param_range_based - check if current parameter is range based > + * @cmdline: points to the parameter to check > + * > + * Returns true when the current paramer is range based, false otherwise > + */ > +bool __init is_param_range_based(const char *cmdline) > +{ > + char *first_colon, *first_space; > + > + first_colon = strchr(cmdline, ':'); > + first_space = strchr(cmdline, ' '); > + if (first_colon && (!first_space || first_colon < first_space)) > + return true; > + > + return false; > +} > + > +/* > + * parse_mem_range_size - parse size based on memory range > + * @param: the thing being parsed > + * @str: (input) where parse begins > + * expected format - <range1>:<size1>[,<range2>:<size2>,...] > + * (output) On success - next char after parse completes > + * On failure - unchanged > + * @system_ram: system ram size to check memory range against > + * > + * Returns the memory size on success and 0 on failure > + */ > +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; > + } > + cur = tmp; > + if (end <= start) { > + printk(KERN_INFO "%s: end <= start\n", param); > + return mem_size; > + } > + } > + > + if (*cur != ':') { > + printk(KERN_INFO "%s: ':' expected\n", param); > + return mem_size; > + } > + cur++; > + > + size = memparse(cur, &tmp); > + if (cur == tmp) { > + printk(KERN_INFO "%s: Memory value expected\n", param); > + return mem_size; > + } > + cur = tmp; > + if (size >= system_ram) { > + printk(KERN_INFO "%s: invalid size\n", param); > + return mem_size; > + } > + > + /* match ? */ > + if (system_ram >= start && system_ram < end) { > + mem_size = size; > + *str = cur; > + break; > + } > + } while (*cur++ == ','); > + > + return mem_size; > +} > + > /* Lazy bastard, eh? */ > #define STANDARD_PARAM_DEF(name, type, format, strtolfn) \ > int param_set_##name(const char *val, const struct kernel_param *kp) \ _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2,1/2] refactor code parsing size based on memory range 2016-06-24 5:26 ` [v2,1/2] " Michael Ellerman @ 2016-06-24 17:15 ` Hari Bathini 2016-07-05 5:18 ` Michael Ellerman 2016-07-20 4:22 ` Hari Bathini 0 siblings, 2 replies; 6+ messages in thread From: Hari Bathini @ 2016-06-24 17:15 UTC (permalink / raw) To: Michael Ellerman, lkml, linuxppc-dev Cc: Rusty Russell, kexec, Eric Biederman, Vivek Goyal On 06/24/2016 10:56 AM, Michael Ellerman wrote: > On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote: >> Currently, crashkernel parameter supports the below syntax to parse size >> based on memory range: >> >> crashkernel=<range1>:<size1>[,<range2>:<size2>,...] >> >> While such parsing is implemented for crashkernel parameter, it applies to >> other parameters with similar syntax. So, move this code to a more generic >> place for code reuse. >> >> Cc: Eric Biederman <ebiederm@xmission.com> >> Cc: Vivek Goyal <vgoyal@redhat.com> >> Cc: Rusty Russell <rusty@rustcorp.com.au> >> Cc: kexec@lists.infradead.org >> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> > Hari, it's not immediately clear that this makes no change to the logic in the > kexec code. Can you reply with a longer change log explaining why the old & new > logic is the same for kexec. > Hi Michael, Please consider this changelog for this patch: -- crashkernel parameter supports different syntaxes to specify the amount of memory to be reserved for kdump kernel. Below is one of the supported syntaxes that needs parsing to find the memory size to reserve, based on memory range: crashkernel=<range1>:<size1>[,<range2>:<size2>,...] While such parsing is implemented for crashkernel parameter, it applies to other parameters, like fadump_reserve_mem, which could use similar syntax. So, to reuse code, moving the code that checks if the parameter syntax is as above and also the code that parses memory size to reserve, for this syntax. 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. -- Thanks Hari > > >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >> index 94aa10f..72f55e5 100644 >> --- a/include/linux/kernel.h >> +++ b/include/linux/kernel.h >> @@ -436,6 +436,11 @@ extern char *get_options(const char *str, int nints, int *ints); >> extern unsigned long long memparse(const char *ptr, char **retptr); >> extern bool parse_option_str(const char *str, const char *option); >> >> +extern bool __init is_param_range_based(const char *cmdline); >> +extern unsigned long long __init parse_mem_range_size(const char *param, >> + char **str, >> + unsigned long long system_ram); >> + >> extern int core_kernel_text(unsigned long addr); >> extern int core_kernel_data(unsigned long addr); >> extern int __kernel_text_address(unsigned long addr); >> 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; >> - } >> - cur = tmp; >> - if (end <= start) { >> - pr_warn("crashkernel: end <= start\n"); >> - return -EINVAL; >> - } >> - } >> - >> - if (*cur != ':') { >> - pr_warn("crashkernel: ':' expected\n"); >> - return -EINVAL; >> - } >> - cur++; >> - >> - size = memparse(cur, &tmp); >> - if (cur == tmp) { >> - pr_warn("Memory value expected\n"); >> - return -EINVAL; >> - } >> - cur = tmp; >> - if (size >= system_ram) { >> - pr_warn("crashkernel: invalid size\n"); >> - return -EINVAL; >> - } >> - >> - /* match ? */ >> - if (system_ram >= start && system_ram < end) { >> - *crash_size = size; >> - break; >> - } >> - } while (*cur++ == ','); >> + *crash_size = parse_mem_range_size("crashkernel", &cur, system_ram); >> + if (cur == cmdline) >> + return -EINVAL; >> >> if (*crash_size > 0) { >> while (*cur && *cur != ' ' && *cur != '@') >> @@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char *cmdline, >> const char *name, >> const char *suffix) >> { >> - char *first_colon, *first_space; >> char *ck_cmdline; >> >> BUG_ON(!crash_size || !crash_base); >> @@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char *cmdline, >> return parse_crashkernel_suffix(ck_cmdline, crash_size, >> suffix); >> /* >> - * if the commandline contains a ':', then that's the extended >> + * if the parameter is range based, then that's the extended >> * syntax -- if not, it must be the classic syntax >> */ >> - first_colon = strchr(ck_cmdline, ':'); >> - first_space = strchr(ck_cmdline, ' '); >> - if (first_colon && (!first_space || first_colon < first_space)) >> + if (is_param_range_based(ck_cmdline)) >> return parse_crashkernel_mem(ck_cmdline, system_ram, >> crash_size, crash_base); >> >> diff --git a/kernel/params.c b/kernel/params.c >> index a6d6149..84e40ae 100644 >> --- a/kernel/params.c >> +++ b/kernel/params.c >> @@ -268,6 +268,102 @@ char *parse_args(const char *doing, >> return err; >> } >> >> +/* >> + * is_param_range_based - check if current parameter is range based >> + * @cmdline: points to the parameter to check >> + * >> + * Returns true when the current paramer is range based, false otherwise >> + */ >> +bool __init is_param_range_based(const char *cmdline) >> +{ >> + char *first_colon, *first_space; >> + >> + first_colon = strchr(cmdline, ':'); >> + first_space = strchr(cmdline, ' '); >> + if (first_colon && (!first_space || first_colon < first_space)) >> + return true; >> + >> + return false; >> +} >> + >> +/* >> + * parse_mem_range_size - parse size based on memory range >> + * @param: the thing being parsed >> + * @str: (input) where parse begins >> + * expected format - <range1>:<size1>[,<range2>:<size2>,...] >> + * (output) On success - next char after parse completes >> + * On failure - unchanged >> + * @system_ram: system ram size to check memory range against >> + * >> + * Returns the memory size on success and 0 on failure >> + */ >> +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; >> + } >> + cur = tmp; >> + if (end <= start) { >> + printk(KERN_INFO "%s: end <= start\n", param); >> + return mem_size; >> + } >> + } >> + >> + if (*cur != ':') { >> + printk(KERN_INFO "%s: ':' expected\n", param); >> + return mem_size; >> + } >> + cur++; >> + >> + size = memparse(cur, &tmp); >> + if (cur == tmp) { >> + printk(KERN_INFO "%s: Memory value expected\n", param); >> + return mem_size; >> + } >> + cur = tmp; >> + if (size >= system_ram) { >> + printk(KERN_INFO "%s: invalid size\n", param); >> + return mem_size; >> + } >> + >> + /* match ? */ >> + if (system_ram >= start && system_ram < end) { >> + mem_size = size; >> + *str = cur; >> + break; >> + } >> + } while (*cur++ == ','); >> + >> + return mem_size; >> +} >> + >> /* Lazy bastard, eh? */ >> #define STANDARD_PARAM_DEF(name, type, format, strtolfn) \ >> int param_set_##name(const char *val, const struct kernel_param *kp) \ > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2,1/2] refactor code parsing size based on memory range 2016-06-24 17:15 ` Hari Bathini @ 2016-07-05 5:18 ` Michael Ellerman 2016-07-05 7:10 ` Hari Bathini 2016-07-20 4:22 ` Hari Bathini 1 sibling, 1 reply; 6+ messages in thread From: Michael Ellerman @ 2016-07-05 5:18 UTC (permalink / raw) To: Hari Bathini, lkml, linuxppc-dev Cc: Rusty Russell, kexec, Eric Biederman, Vivek Goyal > 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 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2,1/2] refactor code parsing size based on memory range 2016-07-05 5:18 ` Michael Ellerman @ 2016-07-05 7:10 ` Hari Bathini 0 siblings, 0 replies; 6+ messages in thread From: Hari Bathini @ 2016-07-05 7:10 UTC (permalink / raw) To: Michael Ellerman, lkml, linuxppc-dev Cc: Rusty Russell, kexec, Eric Biederman, Vivek Goyal 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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2,1/2] refactor code parsing size based on memory range 2016-06-24 17:15 ` Hari Bathini 2016-07-05 5:18 ` Michael Ellerman @ 2016-07-20 4:22 ` Hari Bathini 1 sibling, 0 replies; 6+ messages in thread From: Hari Bathini @ 2016-07-20 4:22 UTC (permalink / raw) To: Eric Biederman, Vivek Goyal Cc: Michael Ellerman, Rusty Russell, kexec, lkml, linuxppc-dev Ping.. On Friday 24 June 2016 10:45 PM, Hari Bathini wrote: > > > On 06/24/2016 10:56 AM, Michael Ellerman wrote: >> On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote: >>> Currently, crashkernel parameter supports the below syntax to parse >>> size >>> based on memory range: >>> >>> crashkernel=<range1>:<size1>[,<range2>:<size2>,...] >>> >>> While such parsing is implemented for crashkernel parameter, it >>> applies to >>> other parameters with similar syntax. So, move this code to a more >>> generic >>> place for code reuse. >>> >>> Cc: Eric Biederman <ebiederm@xmission.com> >>> Cc: Vivek Goyal <vgoyal@redhat.com> >>> Cc: Rusty Russell <rusty@rustcorp.com.au> >>> Cc: kexec@lists.infradead.org >>> Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com> >> Hari, it's not immediately clear that this makes no change to the >> logic in the >> kexec code. Can you reply with a longer change log explaining why the >> old & new >> logic is the same for kexec. >> > > Hi Michael, > > Please consider this changelog for this patch: > > -- > crashkernel parameter supports different syntaxes to specify the amount > of memory to be reserved for kdump kernel. Below is one of the supported > syntaxes that needs parsing to find the memory size to reserve, based on > memory range: > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...] > > While such parsing is implemented for crashkernel parameter, it > applies to > other parameters, like fadump_reserve_mem, which could use similar > syntax. > So, to reuse code, moving the code that checks if the parameter syntax > is as > above and also the code that parses memory size to reserve, for this > syntax. > 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. > -- > > Thanks > Hari > >> >> >>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >>> index 94aa10f..72f55e5 100644 >>> --- a/include/linux/kernel.h >>> +++ b/include/linux/kernel.h >>> @@ -436,6 +436,11 @@ extern char *get_options(const char *str, int >>> nints, int *ints); >>> extern unsigned long long memparse(const char *ptr, char **retptr); >>> extern bool parse_option_str(const char *str, const char *option); >>> +extern bool __init is_param_range_based(const char *cmdline); >>> +extern unsigned long long __init parse_mem_range_size(const char >>> *param, >>> + char **str, >>> + unsigned long long system_ram); >>> + >>> extern int core_kernel_text(unsigned long addr); >>> extern int core_kernel_data(unsigned long addr); >>> extern int __kernel_text_address(unsigned long addr); >>> 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; >>> - } >>> - cur = tmp; >>> - if (end <= start) { >>> - pr_warn("crashkernel: end <= start\n"); >>> - return -EINVAL; >>> - } >>> - } >>> - >>> - if (*cur != ':') { >>> - pr_warn("crashkernel: ':' expected\n"); >>> - return -EINVAL; >>> - } >>> - cur++; >>> - >>> - size = memparse(cur, &tmp); >>> - if (cur == tmp) { >>> - pr_warn("Memory value expected\n"); >>> - return -EINVAL; >>> - } >>> - cur = tmp; >>> - if (size >= system_ram) { >>> - pr_warn("crashkernel: invalid size\n"); >>> - return -EINVAL; >>> - } >>> - >>> - /* match ? */ >>> - if (system_ram >= start && system_ram < end) { >>> - *crash_size = size; >>> - break; >>> - } >>> - } while (*cur++ == ','); >>> + *crash_size = parse_mem_range_size("crashkernel", &cur, >>> system_ram); >>> + if (cur == cmdline) >>> + return -EINVAL; >>> if (*crash_size > 0) { >>> while (*cur && *cur != ' ' && *cur != '@') >>> @@ -1272,7 +1222,6 @@ static int __init __parse_crashkernel(char >>> *cmdline, >>> const char *name, >>> const char *suffix) >>> { >>> - char *first_colon, *first_space; >>> char *ck_cmdline; >>> BUG_ON(!crash_size || !crash_base); >>> @@ -1290,12 +1239,10 @@ static int __init __parse_crashkernel(char >>> *cmdline, >>> return parse_crashkernel_suffix(ck_cmdline, crash_size, >>> suffix); >>> /* >>> - * if the commandline contains a ':', then that's the extended >>> + * if the parameter is range based, then that's the extended >>> * syntax -- if not, it must be the classic syntax >>> */ >>> - first_colon = strchr(ck_cmdline, ':'); >>> - first_space = strchr(ck_cmdline, ' '); >>> - if (first_colon && (!first_space || first_colon < first_space)) >>> + if (is_param_range_based(ck_cmdline)) >>> return parse_crashkernel_mem(ck_cmdline, system_ram, >>> crash_size, crash_base); >>> diff --git a/kernel/params.c b/kernel/params.c >>> index a6d6149..84e40ae 100644 >>> --- a/kernel/params.c >>> +++ b/kernel/params.c >>> @@ -268,6 +268,102 @@ char *parse_args(const char *doing, >>> return err; >>> } >>> +/* >>> + * is_param_range_based - check if current parameter is range based >>> + * @cmdline: points to the parameter to check >>> + * >>> + * Returns true when the current paramer is range based, false >>> otherwise >>> + */ >>> +bool __init is_param_range_based(const char *cmdline) >>> +{ >>> + char *first_colon, *first_space; >>> + >>> + first_colon = strchr(cmdline, ':'); >>> + first_space = strchr(cmdline, ' '); >>> + if (first_colon && (!first_space || first_colon < first_space)) >>> + return true; >>> + >>> + return false; >>> +} >>> + >>> +/* >>> + * parse_mem_range_size - parse size based on memory range >>> + * @param: the thing being parsed >>> + * @str: (input) where parse begins >>> + * expected format - >>> <range1>:<size1>[,<range2>:<size2>,...] >>> + * (output) On success - next char after parse completes >>> + * On failure - unchanged >>> + * @system_ram: system ram size to check memory range against >>> + * >>> + * Returns the memory size on success and 0 on failure >>> + */ >>> +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; >>> + } >>> + cur = tmp; >>> + if (end <= start) { >>> + printk(KERN_INFO "%s: end <= start\n", param); >>> + return mem_size; >>> + } >>> + } >>> + >>> + if (*cur != ':') { >>> + printk(KERN_INFO "%s: ':' expected\n", param); >>> + return mem_size; >>> + } >>> + cur++; >>> + >>> + size = memparse(cur, &tmp); >>> + if (cur == tmp) { >>> + printk(KERN_INFO "%s: Memory value expected\n", param); >>> + return mem_size; >>> + } >>> + cur = tmp; >>> + if (size >= system_ram) { >>> + printk(KERN_INFO "%s: invalid size\n", param); >>> + return mem_size; >>> + } >>> + >>> + /* match ? */ >>> + if (system_ram >= start && system_ram < end) { >>> + mem_size = size; >>> + *str = cur; >>> + break; >>> + } >>> + } while (*cur++ == ','); >>> + >>> + return mem_size; >>> +} >>> + >>> /* Lazy bastard, eh? */ >>> #define STANDARD_PARAM_DEF(name, type, format, >>> strtolfn) \ >>> int param_set_##name(const char *val, const struct >>> kernel_param *kp) \ >> _______________________________________________ >> Linuxppc-dev mailing list >> Linuxppc-dev@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/linuxppc-dev > > > _______________________________________________ > kexec mailing list > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-20 4:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20160622192213.11192.93613.stgit@hbathini.in.ibm.com>
2016-06-22 19:25 ` [PATCH v2 1/2] refactor code parsing size based on memory range Hari Bathini
2016-06-24 5:26 ` [v2,1/2] " Michael Ellerman
2016-06-24 17:15 ` Hari Bathini
2016-07-05 5:18 ` Michael Ellerman
2016-07-05 7:10 ` Hari Bathini
2016-07-20 4:22 ` Hari Bathini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).