Re: [PATCH v4 3/4] x86, kdump: Change crashkernel_high/low= to crashkernel=,high/low

[Yinghai Lu <yinghai@kernel.org>]

On Thu, Apr 11, 2013 at 10:46 PM, HATAYAMA Daisuke
<d.hatayama@jp.fujitsu.com> wrote:
> (2013/04/11 4:39), Yinghai Lu wrote:
> <cut>
>> Index: linux-2.6/kernel/kexec.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/kexec.c
>> +++ linux-2.6/kernel/kexec.c
>> @@ -1368,35 +1368,108 @@ static int __init parse_crashkernel_simp
>>       return 0;
>>   }
>>
>> +#define SUFFIX_HIGH 0
>> +#define SUFFIX_LOW  1
>> +#define SUFFIX_NULL 2
>> +static __initdata char *suffix_tbl[] = {
>> +     [SUFFIX_HIGH] = ",high",
>> +     [SUFFIX_LOW]  = ",low",
>> +     [SUFFIX_NULL] = NULL,
>> +};
>> +
>>   /*
>> - * That function is the entry point for command line parsing and should be
>> - * called from the arch-specific code.
>> + * That function parses "suffix"  crashkernel command lines like
>> + *
>> + *   crashkernel=size,[high|low]
>> + *
>> + * It returns 0 on success and -EINVAL on failure.
>>    */
>> +static int __init parse_crashkernel_suffix(char *cmdline,
>> +                                        unsigned long long   *crash_size,
>> +                                        unsigned long long   *crash_base,
>> +                                        const char *suffix)
>> +{
>> +     char *cur = cmdline;
>> +
>> +     *crash_size = memparse(cmdline, &cur);
>> +     if (cmdline == cur) {
>> +             pr_warn("crashkernel: memory value expected\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* check with suffix */
>> +     if (!strncmp(cur, suffix, strlen(suffix)))
>> +             return 0;
>
> What kind of strings do you intend to be passed here? The syntax that
> matches this check would be
>
>   [0-9]+[kmgKMG]?(,low|,high){.*(,low|,high)}*

should be
[0-9]+[kmgKMG](,low|,high)

will tight the checking.

>
> though memparse() part might be less precise. For example, the following
> one passes the check since it ends with ",low" and  ",high" follows
> "12345K" to be parsed by memparse(). Is this within your intension for
> ease of implementation?
>
>   crashkernel=12345K,highfoobar,highabcd,low

it will be rejected.

crashkernel=12345K,highfoobar,highabcd,high
should be rejected too.

>
>> +
>> +     pr_warn("crashkernel: unrecognized char\n");
>> +     return -EINVAL;
>> +}
>
> I like functions checking errornous cases in the middle and ends with
> successful case. How about:
>
> /* check with suffix */
> if (strncmp(cur, suffix, strlen(suffix)) {
>     pr_warn("crashkernel: unrecognized char\n");
>     return -EINVAL;
> }
>
> return 0;

ok.