2007-09-25 18:23:25

by Bernhard Walle

[permalink] [raw]
Subject: [patch 1/7] Extended crashkernel command line

This is the generic part of the patch. It adds a parse_crashkernel() function
in kernel/kexec.c that is called by the architecture specific code that
actually reserves the memory. That function takes the whole command line and
looks itself for "crashkernel=" in it.

If there are multiple occurrences, then the last one is taken. The advantage
is that if you have a bootloader like lilo or elilo which allows you to append
a command line parameter but not to remove one (like in GRUB), then you can add
another crashkernel value for testing at the boot command line and this one
overwrites the command line in the configuration then.


Signed-off-by: Bernhard Walle <[email protected]>

---
include/linux/kexec.h | 2
kernel/kexec.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 144 insertions(+)

--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -187,6 +187,8 @@ extern u32 vmcoreinfo_note[VMCOREINFO_NO
extern size_t vmcoreinfo_size;
extern size_t vmcoreinfo_max_size;

+int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
+ unsigned long long *crash_size, unsigned long long *crash_base);

#else /* !CONFIG_KEXEC */
struct pt_regs;
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1146,6 +1146,148 @@ static int __init crash_notes_memory_ini
}
module_init(crash_notes_memory_init)

+
+/*
+ * parsing the "crashkernel" commandline
+ *
+ * this code is intended to be called from architecture specific code
+ */
+
+
+/*
+ * This function parses command lines in the format
+ *
+ * crashkernel=<ramsize-range>:<size>[,...][@<base>]
+ *
+ * The function returns 0 on success and -EINVAL on failure.
+ */
+static int __init parse_crashkernel_mem(char *cmdline,
+ unsigned long long system_ram,
+ unsigned long long *crash_size,
+ unsigned long long *crash_base)
+{
+ char *cur = cmdline;
+
+ /* for each entry of the comma-separated list */
+ do {
+ unsigned long long start = 0, end = ULLONG_MAX;
+ unsigned long long size = -1;
+
+ /* get the start of the range */
+ start = memparse(cur, &cur);
+ if (*cur != '-') {
+ printk(KERN_WARNING "crashkernel: '-' expected\n");
+ return -EINVAL;
+ }
+ cur++;
+
+ /* if no ':' is here, than we read the end */
+ if (*cur != ':') {
+ end = memparse(cur, &cur);
+ if (end <= start) {
+ printk(KERN_WARNING "crashkernel: end <= start\n");
+ return -EINVAL;
+ }
+ }
+
+ if (*cur != ':') {
+ printk(KERN_WARNING "crashkernel: ':' expected\n");
+ return -EINVAL;
+ }
+ cur++;
+
+ size = memparse(cur, &cur);
+ if (size < 0) {
+ printk(KERN_WARNING "crashkernel: invalid size\n");
+ return -EINVAL;
+ }
+
+ /* match ? */
+ if (system_ram >= start && system_ram <= end) {
+ *crash_size = size;
+ break;
+ }
+ } while (*cur++ == ',');
+
+ if (*crash_size > 0) {
+ while (*cur != ' ' && *cur != '@')
+ cur++;
+ if (*cur == '@')
+ *crash_base = memparse(cur+1, &cur);
+ }
+
+ return 0;
+}
+
+/*
+ * That function parses "simple" (old) crashkernel command lines like
+ *
+ * crashkernel=size[@base]
+ *
+ * It returns 0 on success and -EINVAL on failure.
+ */
+static int __init parse_crashkernel_simple(char *cmdline,
+ unsigned long long *crash_size,
+ unsigned long long *crash_base)
+{
+ char *cur = cmdline;
+
+ *crash_size = memparse(cmdline, &cur);
+ if (cmdline == cur)
+ return -EINVAL;
+
+ if (*cur == '@')
+ *crash_base = memparse(cur+1, &cur);
+
+ return 0;
+}
+
+/*
+ * That function is the entry point for command line parsing and should be
+ * called from the arch-specific code.
+ */
+int __init parse_crashkernel(char *cmdline,
+ unsigned long long system_ram,
+ unsigned long long *crash_size,
+ unsigned long long *crash_base)
+{
+ char *p = cmdline, *ck_cmdline = NULL;
+ char *first_colon, *first_space;
+
+ BUG_ON(!crash_size || !crash_base);
+ *crash_size = 0;
+ *crash_base = 0;
+
+ /* find crashkernel and use the last one if there are more */
+ p = strstr(p, "crashkernel=");
+ while (p) {
+ ck_cmdline = p;
+ p = strstr(p+1, "crashkernel=");
+ }
+
+ if (!ck_cmdline)
+ return -EINVAL;
+
+ ck_cmdline += 12; /* strlen("crashkernel=") */
+
+ /*
+ * if the commandline contains a ':', 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))
+ return parse_crashkernel_mem(ck_cmdline, system_ram,
+ crash_size, crash_base);
+ else
+ return parse_crashkernel_simple(ck_cmdline, crash_size,
+ crash_base);
+
+ return 0;
+}
+
+
+
void crash_save_vmcoreinfo(void)
{
u32 *buf;

--


2007-09-25 20:38:06

by Oleg Verych

[permalink] [raw]
Subject: Re: [patch 1/7] Extended crashkernel command line

* Tue, 25 Sep 2007 20:22:58 +0200
>
> This is the generic part of the patch. It adds a parse_crashkernel() function
> in kernel/kexec.c that is called by the architecture specific code that
> actually reserves the memory. That function takes the whole command line and
> looks itself for "crashkernel=" in it.
[]
> +++ b/include/linux/kexec.h
> @@ -187,6 +187,8 @@ extern u32 vmcoreinfo_note[VMCOREINFO_NO
> extern size_t vmcoreinfo_size;
> extern size_t vmcoreinfo_max_size;
>
> +int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
> + unsigned long long *crash_size, unsigned long long *crash_base);

I still want to point out my consernes about `unsigned long long long'
bloat.

> #else /* !CONFIG_KEXEC */
> struct pt_regs;
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
[]
> +/*
> + * This function parses command lines in the format
> + *
> + * crashkernel=<ramsize-range>:<size>[,...][@<base>]

#v+
olecom@flower:/tmp$ grep '@offset' <extended-crashkernel-cmdline.mbox
crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
- /* crashkernel=size@offset specifies the size to reserve for a crash
+While the "crashkernel=size[@offset]" syntax is sufficient for most
+ crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
+ crashkernel=range1:size1[,range2:size2,...][@offset]
olecom@flower:/tmp$ grep '@base' <extended-crashkernel-cmdline.mbox
+ * crashkernel=size[@base]
olecom@flower:/tmp$ grep '@<base' <extended-crashkernel-cmdline.mbox
+ * crashkernel=<ramsize-range>:<size>[,...][@<base>]
olecom@flower:/tmp$ grep '@<offset' <extended-crashkernel-cmdline.mbox
olecom@flower:/tmp$
#v-

> + *
> + * The function returns 0 on success and -EINVAL on failure.
> + */
> +static int __init parse_crashkernel_mem(char *cmdline,
> + unsigned long long system_ram,
> + unsigned long long *crash_size,
> + unsigned long long *crash_base)
> +{
> + char *cur = cmdline;
> +
> + /* for each entry of the comma-separated list */
> + do {
> + unsigned long long start = 0, end = ULLONG_MAX;
> + unsigned long long size = -1;

memparse(), as a wrapper for somple_strtoll(), always have a return value
(zero by default).
<http://article.gmane.org/gmane.linux.kernel/583426>

Consider coded error path now, please.

And why not to make overall result reliable? This is kernel after all.

I.e. if there's valid `crashkernel=' option, but some parsing errors, why
not to apply some heuristics with warning in syslog, if user have some
conf, bootloader (random) errors, but feature still works?

> +
> + /* get the start of the range */
> + start = memparse(cur, &cur);
> + if (*cur != '-') {
> + printk(KERN_WARNING "crashkernel: '-' expected\n");
> + return -EINVAL;
> + }
> + cur++;
> +
> + /* if no ':' is here, than we read the end */
> + if (*cur != ':') {
> + end = memparse(cur, &cur);
> + if (end <= start) {
> + printk(KERN_WARNING "crashkernel: end <= start\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (*cur != ':') {
> + printk(KERN_WARNING "crashkernel: ':' expected\n");
> + return -EINVAL;
> + }
> + cur++;
> +
> + size = memparse(cur, &cur);
> + if (size < 0) {
> + printk(KERN_WARNING "crashkernel: invalid size\n");
> + return -EINVAL;
> + }
> +
> + /* match ? */
> + if (system_ram >= start && system_ram <= end) {
> + *crash_size = size;
> + break;
> + }
> + } while (*cur++ == ',');
> +
> + if (*crash_size > 0) {
> + while (*cur != ' ' && *cur != '@')
> + cur++;
> + if (*cur == '@')
> + *crash_base = memparse(cur+1, &cur);
> + }
> +
> + return 0;
> +}
--
-o--=O`C
#oo'L O
<___=E M

2007-09-26 08:35:12

by Bernhard Walle

[permalink] [raw]
Subject: Re: [patch 1/7] Extended crashkernel command line

* Oleg Verych <[email protected]> [2007-09-25 22:53]:
> * Tue, 25 Sep 2007 20:22:58 +0200
> >
> > +++ b/include/linux/kexec.h
> > @@ -187,6 +187,8 @@ extern u32 vmcoreinfo_note[VMCOREINFO_NO
> > extern size_t vmcoreinfo_size;
> > extern size_t vmcoreinfo_max_size;
> >
> > +int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
> > + unsigned long long *crash_size, unsigned long long *crash_base);
>
> I still want to point out my consernes about `unsigned long long long'
> bloat.

What "concerns" (it's unsigned long long and not unsigned long long
long)? Is is common coding style in the Linux kernel to *not* use
unsigned long long? This type *is* used e.g. in
arch/i386/kernel/e820.c also for pysical memory values.

> #v+
> olecom@flower:/tmp$ grep '@offset' <extended-crashkernel-cmdline.mbox
> crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> - /* crashkernel=size@offset specifies the size to reserve for a crash
> +While the "crashkernel=size[@offset]" syntax is sufficient for most
> + crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> + crashkernel=range1:size1[,range2:size2,...][@offset]
> olecom@flower:/tmp$ grep '@base' <extended-crashkernel-cmdline.mbox
> + * crashkernel=size[@base]
> olecom@flower:/tmp$ grep '@<base' <extended-crashkernel-cmdline.mbox
> + * crashkernel=<ramsize-range>:<size>[,...][@<base>]
> olecom@flower:/tmp$ grep '@<offset' <extended-crashkernel-cmdline.mbox
> olecom@flower:/tmp$
> #v-

The patch below fixes this.

> > + *
> > + * The function returns 0 on success and -EINVAL on failure.
> > + */
> > +static int __init parse_crashkernel_mem(char *cmdline,
> > + unsigned long long system_ram,
> > + unsigned long long *crash_size,
> > + unsigned long long *crash_base)
> > +{
> > + char *cur = cmdline;
> > +
> > + /* for each entry of the comma-separated list */
> > + do {
> > + unsigned long long start = 0, end = ULLONG_MAX;
> > + unsigned long long size = -1;
>
> memparse(), as a wrapper for somple_strtoll(), always have a return value
> (zero by default).
> <http://article.gmane.org/gmane.linux.kernel/583426>

Next reply (because of a different patch).


---

Only use 'offset' and not 'base' for the address where the reserved area
starts.


Signed-off-by: Bernhard Walle <[email protected]>

---
kernel/kexec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1157,7 +1157,7 @@ module_init(crash_notes_memory_init)
/*
* This function parses command lines in the format
*
- * crashkernel=<ramsize-range>:<size>[,...][@<base>]
+ * crashkernel=ramsize-range:size[,...][@offset]
*
* The function returns 0 on success and -EINVAL on failure.
*/
@@ -1222,7 +1222,7 @@ static int __init parse_crashkernel_mem(
/*
* That function parses "simple" (old) crashkernel command lines like
*
- * crashkernel=size[@base]
+ * crashkernel=size[@offset]
*
* It returns 0 on success and -EINVAL on failure.
*/

2007-09-26 16:16:18

by Bernhard Walle

[permalink] [raw]
Subject: Re: [patch 1/7] Extended crashkernel command line

* Oleg Verych <[email protected]> [2007-09-25 22:53]:
>
> > + *
> > + * The function returns 0 on success and -EINVAL on failure.
> > + */
> > +static int __init parse_crashkernel_mem(char *cmdline,
> > + unsigned long long system_ram,
> > + unsigned long long *crash_size,
> > + unsigned long long *crash_base)
> > +{
> > + char *cur = cmdline;
> > +
> > + /* for each entry of the comma-separated list */
> > + do {
> > + unsigned long long start = 0, end = ULLONG_MAX;
> > + unsigned long long size = -1;
>
> memparse(), as a wrapper for somple_strtoll(), always have a return value
> (zero by default).
> <http://article.gmane.org/gmane.linux.kernel/583426>

Ok, that's fixed now, see patch below.

> And why not to make overall result reliable? This is kernel after all.
>
> I.e. if there's valid `crashkernel=' option, but some parsing errors, why
> not to apply some heuristics with warning in syslog, if user have some
> conf, bootloader (random) errors, but feature still works?

I'm against guessing. The user has to specify a parameter which is
right according to syntax.

However, I plan to make a patch that the kernel can detect a sensible
offset automatically for i386 and x86_64 as it's done in ia64. Since
both architectures have a relocatable kernel now, that makes perfectly
sense. But that's another patch.

---

This patches improves error handling in parse_crashkernel_mem() by comparing
the return pointer of memparse() with the input pointer and also replaces
all printk(KERN_WARNING msg) with pr_warning(msg).


Signed-off-by: Bernhard Walle <[email protected]>

---
kernel/kexec.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)

--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1172,33 +1172,50 @@ static int __init parse_crashkernel_mem(
do {
unsigned long long start = 0, end = ULLONG_MAX;
unsigned long long size = -1;
+ char *tmp;

/* get the start of the range */
- start = memparse(cur, &cur);
+ start = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("crashkernel: Memory value expected\n");
+ return -EINVAL;
+ }
+ cur = tmp;
if (*cur != '-') {
- printk(KERN_WARNING "crashkernel: '-' expected\n");
+ pr_warning("crashkernel: '-' expected\n");
return -EINVAL;
}
cur++;

/* if no ':' is here, than we read the end */
if (*cur != ':') {
- end = memparse(cur, &cur);
+ end = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("crashkernel: Memory "
+ "value expected\n");
+ return -EINVAL;
+ }
+ cur = tmp;
if (end <= start) {
- printk(KERN_WARNING "crashkernel: end <= start\n");
+ pr_warning("crashkernel: end <= start\n");
return -EINVAL;
}
}

if (*cur != ':') {
- printk(KERN_WARNING "crashkernel: ':' expected\n");
+ pr_warning("crashkernel: ':' expected\n");
return -EINVAL;
}
cur++;

- size = memparse(cur, &cur);
+ size = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("Memory value expected\n");
+ return -EINVAL;
+ }
+ cur = tmp;
if (size < 0) {
- printk(KERN_WARNING "crashkernel: invalid size\n");
+ pr_warning("crashkernel: invalid size\n");
return -EINVAL;
}

2007-09-26 18:03:17

by Oleg Verych

[permalink] [raw]
Subject: Re: [patch 1/7] Extended crashkernel command line

Wed, Sep 26, 2007 at 06:16:02PM +0200, Bernhard Walle (part two, see bottom):
> > memparse(), as a wrapper for somple_strtoll(), always have a return value
> > (zero by default).
> > <http://article.gmane.org/gmane.linux.kernel/583426>

Sorry for my typos, i should write `simple_strtoull()'. This function
(ULL from str) have always return value grater or equal to zero.

Thus,

>
> Signed-off-by: Bernhard Walle <[email protected]>
>
> ---
> kernel/kexec.c | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -1172,33 +1172,50 @@ static int __init parse_crashkernel_mem(
> do {
> unsigned long long start = 0, end = ULLONG_MAX;
> unsigned long long size = -1;

no need in assigning values here, unless you plan to use them in case
of `return -EINVAL', but i can not see that,

> + char *tmp;
>
> /* get the start of the range */
> - start = memparse(cur, &cur);
> + start = memparse(cur, &tmp);
> + if (cur == tmp) {
> + pr_warning("crashkernel: Memory value expected\n");
> + return -EINVAL;
> + }
> + cur = tmp;
> if (*cur != '-') {
> - printk(KERN_WARNING "crashkernel: '-' expected\n");
> + pr_warning("crashkernel: '-' expected\n");
> return -EINVAL;
> }
> cur++;
>
> /* if no ':' is here, than we read the end */
> if (*cur != ':') {
> - end = memparse(cur, &cur);
> + end = memparse(cur, &tmp);
> + if (cur == tmp) {
> + pr_warning("crashkernel: Memory "
> + "value expected\n");
> + return -EINVAL;
> + }
> + cur = tmp;
> if (end <= start) {
> - printk(KERN_WARNING "crashkernel: end <= start\n");
> + pr_warning("crashkernel: end <= start\n");
> return -EINVAL;
> }
> }
>
> if (*cur != ':') {
> - printk(KERN_WARNING "crashkernel: ':' expected\n");
> + pr_warning("crashkernel: ':' expected\n");
> return -EINVAL;
> }
> cur++;
>
> - size = memparse(cur, &cur);
> + size = memparse(cur, &tmp);
> + if (cur == tmp) {
> + pr_warning("Memory value expected\n");
> + return -EINVAL;
> + }
> + cur = tmp;
> if (size < 0) {

`size' cannot be less that zero here (wonder, if it matters to have
userspace model of this parser and actually feed it with garbage input).

> - printk(KERN_WARNING "crashkernel: invalid size\n");
> + pr_warning("crashkernel: invalid size\n");
> return -EINVAL;
> }
>


Wed, Sep 26, 2007 at 06:16:02PM +0200, Bernhard Walle (part one):
> Ok, that's fixed now, see patch below.
>
> > And why not to make overall result reliable? This is kernel after all.
> >
> > I.e. if there's valid `crashkernel=' option, but some parsing errors, why
> > not to apply some heuristics with warning in syslog, if user have some
> > conf, bootloader (random) errors, but feature still works?
>
> I'm against guessing. The user has to specify a parameter which is
> right according to syntax.
>
> However, I plan to make a patch that the kernel can detect a sensible
> offset automatically for i386 and x86_64 as it's done in ia64. Since
> both architectures have a relocatable kernel now, that makes perfectly
> sense. But that's another patch.

I was thinking about errors in YaST or typos in bootloader config, that
may appear sometimes. And kernel must tolerate this kind of userspace
input to be more reliable. But you know better, i just am waving hands.
____
("Mail-Followup-To:" respected)

2007-09-26 18:19:08

by Bernhard Walle

[permalink] [raw]
Subject: Re: [patch 1/7] Extended crashkernel command line

* Oleg Verych <[email protected]> [2007-09-26 20:18]:
>
> I was thinking about errors in YaST or typos in bootloader config, that
> may appear sometimes. And kernel must tolerate this kind of userspace
> input to be more reliable. But you know better, i just am waving hands.

Of course the kernel must be able to handle them -- outputting an
error message that can be read by the user and not crashing. But I
don't expect that the kernel then reservate crash memory by guessing
of values.



Thanks,
Bernhard

2007-09-26 21:06:23

by Bernhard Walle

[permalink] [raw]
Subject: Re: [patch 1/7] Extended crashkernel command line

* Oleg Verych <[email protected]> [2007-09-26 20:18]:
> >
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -1172,33 +1172,50 @@ static int __init parse_crashkernel_mem(
> > do {
> > unsigned long long start = 0, end = ULLONG_MAX;
> > unsigned long long size = -1;
>
> no need in assigning values here, unless you plan to use them in case
> of `return -EINVAL', but i can not see that,

What about this (and yes, I tested with some wrong strings with Qemu):

----

This patch improves error handling in parse_crashkernel_mem() by comparing
the return pointer of memparse() with the input pointer and also replaces
all printk(KERN_WARNING msg) with pr_warning(msg).


Signed-off-by: Bernhard Walle <[email protected]>

---
kernel/kexec.c | 54 +++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 39 insertions(+), 15 deletions(-)

--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1167,44 +1167,59 @@ static int __init parse_crashkernel_mem(
unsigned long long *crash_size,
unsigned long long *crash_base)
{
- char *cur = cmdline;
+ char *cur = cmdline, *tmp;

/* for each entry of the comma-separated list */
do {
- unsigned long long start = 0, end = ULLONG_MAX;
- unsigned long long size = -1;
+ unsigned long long start, end = ULLONG_MAX, size;

/* get the start of the range */
- start = memparse(cur, &cur);
+ start = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("crashkernel: Memory value expected\n");
+ return -EINVAL;
+ }
+ cur = tmp;
if (*cur != '-') {
- printk(KERN_WARNING "crashkernel: '-' expected\n");
+ pr_warning("crashkernel: '-' expected\n");
return -EINVAL;
}
cur++;

/* if no ':' is here, than we read the end */
if (*cur != ':') {
- end = memparse(cur, &cur);
+ end = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("crashkernel: Memory "
+ "value expected\n");
+ return -EINVAL;
+ }
+ cur = tmp;
if (end <= start) {
- printk(KERN_WARNING "crashkernel: end <= start\n");
+ pr_warning("crashkernel: end <= start\n");
return -EINVAL;
}
}

if (*cur != ':') {
- printk(KERN_WARNING "crashkernel: ':' expected\n");
+ pr_warning("crashkernel: ':' expected\n");
return -EINVAL;
}
cur++;

- size = memparse(cur, &cur);
- if (size < 0) {
- printk(KERN_WARNING "crashkernel: invalid size\n");
+ size = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("Memory value expected\n");
+ return -EINVAL;
+ }
+ cur = tmp;
+ if (size >= system_ram) {
+ pr_warning("crashkernel: invalid size\n");
return -EINVAL;
}

/* match ? */
- if (system_ram >= start && system_ram <= end) {
+ if (system_ram >= start && system_ram <= end) {
*crash_size = size;
break;
}
@@ -1213,8 +1228,15 @@ static int __init parse_crashkernel_mem(
if (*crash_size > 0) {
while (*cur != ' ' && *cur != '@')
cur++;
- if (*cur == '@')
- *crash_base = memparse(cur+1, &cur);
+ if (*cur == '@') {
+ cur++;
+ *crash_base = memparse(cur, &tmp);
+ if (cur == tmp) {
+ pr_warning("Memory value expected "
+ "after '@'\n");
+ return -EINVAL;
+ }
+ }
}

return 0;
@@ -1234,8 +1256,10 @@ static int __init parse_crashkernel_simp
char *cur = cmdline;

*crash_size = memparse(cmdline, &cur);
- if (cmdline == cur)
+ if (cmdline == cur) {
+ pr_warning("crashkernel: memory value expected\n");
return -EINVAL;
+ }

if (*cur == '@')
*crash_base = memparse(cur+1, &cur);

2007-09-26 21:16:48

by Oleg Verych

[permalink] [raw]
Subject: reviewed (Re: [patch 1/7] Extended crashkernel command line)

Wed, Sep 26, 2007 at 11:05:33PM +0200, Bernhard Walle:
> * Oleg Verych <[email protected]> [2007-09-26 20:18]:
> > >
> > > --- a/kernel/kexec.c
> > > +++ b/kernel/kexec.c
> > > @@ -1172,33 +1172,50 @@ static int __init parse_crashkernel_mem(
> > > do {
> > > unsigned long long start = 0, end = ULLONG_MAX;
> > > unsigned long long size = -1;
> >
> > no need in assigning values here, unless you plan to use them in case
> > of `return -EINVAL', but i can not see that,
>
> What about this (and yes, I tested with some wrong strings with Qemu):

Reviewed-by: Oleg Verych <[email protected]>

Thanks :D

> ----
>
> This patch improves error handling in parse_crashkernel_mem() by comparing
> the return pointer of memparse() with the input pointer and also replaces
> all printk(KERN_WARNING msg) with pr_warning(msg).
>
>
> Signed-off-by: Bernhard Walle <[email protected]>
_____