Re: [Xen-devel] [PATCH] cmdline: treat hyphens and underscores the same

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@citrix.com>
Subject: Re: [Xen-devel] [PATCH] cmdline: treat hyphens and underscores the same
Date: Thu, 5 Dec 2019 16:27:36 +0000	[thread overview]
Message-ID: <8dc4166d-45c3-0a5c-8782-78de1b74ad36@xen.org> (raw)
In-Reply-To: <aa92bd0c-f35c-2bf3-d665-4977f83bb0d5@suse.com>

Hi,

On 05/12/2019 15:33, Jan Beulich wrote:
> In order to avoid permanently having to ask that no new command line
> options using underscores be introduced (albeit I'm likely to still make
> remarks), and in order to also allow extending the use of hyphens to
> pre-existing ones, introduce custom comparison functions treating both
> characters as matching.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -72,6 +72,11 @@ Some options take a comma separated list
>   Some parameters act as combinations of the above, most commonly a mix
>   of Boolean and String.  These are noted in the relevant sections.
>   
> +### Spelling
> +
> +Parameter names may include hyphens or underscores.  These are
> +generally being treated as matching one another by the parsing logic.
> +
>   ## Parameter details
>   
>   ### acpi
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -23,6 +23,49 @@ enum system_state system_state = SYS_STA
>   xen_commandline_t saved_cmdline;
>   static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
>   
> +static int cdiff(unsigned char c1, unsigned char c2)

This is not obvious from the name and the implementation what it does 
(it took me a few minutes to figure it out). So I think you want to add 
a comment.

> +{
> +    int res = c1 - c2;
> +
> +    if ( res && (c1 ^ c2) == ('-' ^ '_') &&
> +         (c1 == '-' || c1 == '_') )
> +        res = 0;
> +
> +    return res;
> +}
> +
> +/*
> + * String comparison functions mostly matching strcmp() / strncmp(),
> + * except that they treat '-' and '_' as matching one another.
> + */
> +static int _strcmp(const char *s1, const char *s2)

I thought we were trying to avoid new function name with leading _?

But it is really worth to implement both strcmp and strncmp rather than 
using the latter to implement the former?

I know this involve using strlen, but I am not convinced this will be 
noticeable at boot.

> +{
> +    int res;
> +
> +    for ( ; ; ++s1, ++s2 )
> +    {
> +        res = cdiff(*s1, *s2);
> +        if ( res || !*s1 )
> +            break;
> +    }
> +
> +    return res;
> +}
> +
> +static int _strncmp(const char *s1, const char *s2, size_t n)
> +{
> +    int res = 0;
> +
> +    for ( ; n--; ++s1, ++s2 )
> +    {
> +        res = cdiff(*s1, *s2);
> +        if ( res || !*s1 )
> +            break;
> +    }
> +
> +    return res;
> +}
> +
>   static int assign_integer_param(const struct kernel_param *param, uint64_t val)
>   {
>       switch ( param->len )
> @@ -94,7 +137,7 @@ static int parse_params(const char *cmdl
>   
>           /* Boolean parameters can be inverted with 'no-' prefix. */
>           key = optkey;
> -        bool_assert = !!strncmp("no-", optkey, 3);
> +        bool_assert = !!_strncmp("no-", optkey, 3);
>           if ( !bool_assert )
>               optkey += 3;
>   
> @@ -105,11 +148,11 @@ static int parse_params(const char *cmdl
>               int rctmp;
>               const char *s;
>   
> -            if ( strcmp(param->name, optkey) )
> +            if ( _strcmp(param->name, optkey) )
>               {
>                   if ( param->type == OPT_CUSTOM && q &&
>                        strlen(param->name) == q + 1 - opt &&
> -                     !strncmp(param->name, opt, q + 1 - opt) )
> +                     !_strncmp(param->name, opt, q + 1 - opt) )
>                   {
>                       found = true;
>                       optval[-1] = '=';
> @@ -275,7 +318,7 @@ int parse_bool(const char *s, const char
>   int parse_boolean(const char *name, const char *s, const char *e)
>   {
>       size_t slen, nlen;
> -    int val = !!strncmp(s, "no-", 3);
> +    int val = !!_strncmp(s, "no-", 3);
>   
>       if ( !val )
>           s += 3;
> @@ -284,7 +327,7 @@ int parse_boolean(const char *name, cons
>       nlen = strlen(name);
>   
>       /* Does s now start with name? */
> -    if ( slen < nlen || strncmp(s, name, nlen) )
> +    if ( slen < nlen || _strncmp(s, name, nlen) )
>           return -1;
>   
>       /* Exact, unadorned name?  Result depends on the 'no-' prefix. */
> @@ -304,7 +347,7 @@ int cmdline_strcmp(const char *frag, con
>       for ( ; ; frag++, name++ )
>       {
>           unsigned char f = *frag, n = *name;
> -        int res = f - n;
> +        int res = cdiff(f, n);
>   
>           if ( res || n == '\0' )
>           {
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel