Re: [PATCH v2 1/2] cmdline_parse: Convert no- prefix into =no for OPT_CUSTOM

[Don Slutz <dslutz@verizon.com>]

On 07/30/14 03:58, Jan Beulich wrote:
>>>> On 29.07.14 at 21:57, <dslutz@verizon.com> wrote:
>> This allows converting OPT_BOOL into OPT_CUSTOM and still do what is
>> expected.
>>
>> Note: if both no- and = provided, pass provided = if not a zero
>> length string.  Also log a warning about this.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   xen/common/kernel.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>> index 7e83353..9857159 100644
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -135,7 +135,20 @@ void __init cmdline_parse(const char *cmdline)
>>                       parse_size_and_unit(optval, NULL));
>>                   break;
>>               case OPT_CUSTOM:
>> -                ((void (*)(const char *))param->var)(optval);
>> +                if ( !bool_assert )
>> +                {
>> +                    if ( *optval )
>> +                    {
>> +                        printk(XENLOG_WARNING
>> +                               "Warning: ignored no- prefix for %s=%s\n",
>> +                               param->name, optval);
> Log messages printed before the consoles get initialized are of limited
> value (they would only show up in "xl dmesg" or equivalent output).
> Therefore I'm not really certain this is worthwhile.

I am happy to go either way.

>> +                        ((void (*)(const char *))param->var)(optval);
>> +                    }
>> +                    else
>> +                        ((void (*)(const char *))param->var)("no");
>> +                }
>> +                else
>> +                    ((void (*)(const char *))param->var)(optval);
> Considering this repeated cast I'd prefer coding this quite
> differently, not the least because you also can't pass a literal
> "no" to the parsing functions (as they're permitted to alter the
> string), and at once also ignoring bogus "no-<name>=<value>"
> options instead of trying to assign a (perhaps unexpected)
> meaning:
>
> convert "no-" command line option prefix into "=no" for OPT_CUSTOM
>
> ... to allow restoring/retaining previous behavior for options getting
> converted from boolean to custom.
>
> Reported-by: Don Slutz <dslutz@verizon.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -135,6 +135,13 @@ void __init cmdline_parse(const char *cm
>                       parse_size_and_unit(optval, NULL));
>                   break;
>               case OPT_CUSTOM:
> +                if ( !bool_assert )
> +                {
> +                    if ( *optval )
> +                        break;
> +                    safe_strcpy(opt, "no");
> +                    optval = opt;
> +                }
>                   ((void (*)(const char *))param->var)(optval);
>                   break;
>               default:
>
>

Without the log message, I think it might help to include the part about
ignoring bogus "no-<name>=<value>"... aka stacked inversions in the
commit message.  This is because boolean options do still support stacked
inversions, which is valid under the statement "Explicitly specifying any
value other than those listed above is undefined, as is stacking a `no-`
prefix with an explicit value."

I am happy with the actual code change.

    -Don Slutz