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

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

[Jan Beulich]

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)
+{
+    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)
+{
+    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' )
         {

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

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

[Julien Grall]

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

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

[Jan Beulich]

On 05.12.2019 17:27, Julien Grall wrote:
> On 05/12/2019 15:33, Jan Beulich wrote:
>> --- 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.

Sure, done.

>> +{
>> +    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 _?

We're trying to avoid new name space violations. Such are
- identifiers starting with two underscores,
- identifiers starting with an underscore and an upper case letter,
- identifiers of non-static symbols starting with an underscore.

> 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.

To be honest - it didn't even occur to me. The functions seemed
simple enough to not have one use the other. If others agree
with you, I'd be fine doing as you suggest.

Jan

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

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

[Julien Grall]

Hi Jan,

On 05/12/2019 16:50, Jan Beulich wrote:
> On 05.12.2019 17:27, Julien Grall wrote:
>> On 05/12/2019 15:33, Jan Beulich wrote:
>>> --- 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.
> 
> Sure, done.
> 
>>> +{
>>> +    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 _?
> 
> We're trying to avoid new name space violations. Such are
> - identifiers starting with two underscores,
> - identifiers starting with an underscore and an upper case letter,
> - identifiers of non-static symbols starting with an underscore.

I am not sure to understand why non-static symbols only. This would 
prevent you to use the the non-static symbol if you happen to re-use the 
same name.

Anyway, how about calling it cmdline_strncmp()? This would be easier to 
spot misuse on review (i.e using strncmp() rather than _strncmp()).

Cheers,

-- 
Julien Grall

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

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

[George Dunlap]

On 12/6/19 2:46 PM, Julien Grall wrote:
> Hi Jan,
> 
> On 05/12/2019 16:50, Jan Beulich wrote:
>> On 05.12.2019 17:27, Julien Grall wrote:
>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>> --- 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.
>>
>> Sure, done.
>>
>>>> +{
>>>> +    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 _?
>>
>> We're trying to avoid new name space violations. Such are
>> - identifiers starting with two underscores,
>> - identifiers starting with an underscore and an upper case letter,
>> - identifiers of non-static symbols starting with an underscore.
> 
> I am not sure to understand why non-static symbols only. This would
> prevent you to use the the non-static symbol if you happen to re-use the
> same name.
> 
> Anyway, how about calling it cmdline_strncmp()? This would be easier to
> spot misuse on review (i.e using strncmp() rather than _strncmp()).

FWIW I was thinking something similar.

 -George

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

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

[Jan Beulich]

On 06.12.2019 15:46, Julien Grall wrote:
> On 05/12/2019 16:50, Jan Beulich wrote:
>> On 05.12.2019 17:27, Julien Grall wrote:
>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>> +/*
>>>> + * 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 _?
>>
>> We're trying to avoid new name space violations. Such are
>> - identifiers starting with two underscores,
>> - identifiers starting with an underscore and an upper case letter,
>> - identifiers of non-static symbols starting with an underscore.
> 
> I am not sure to understand why non-static symbols only. This would 
> prevent you to use the the non-static symbol if you happen to re-use the 
> same name.

I'm afraid I don't understand. Anyway - what I've listed above is
what the language standard mandates.

> Anyway, how about calling it cmdline_strncmp()? This would be easier to 
> spot misuse on review (i.e using strncmp() rather than _strncmp()).

We already have cmdline_strcmp(), or else I would indeed have used
this prefix. No prefix (other than the lone underscore) seemed the
next best option.

Jan

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

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

[Julien Grall]

Hi,

On 06/12/2019 16:06, Jan Beulich wrote:
> On 06.12.2019 15:46, Julien Grall wrote:
>> On 05/12/2019 16:50, Jan Beulich wrote:
>>> On 05.12.2019 17:27, Julien Grall wrote:
>>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>>> +/*
>>>>> + * 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 _?
>>>
>>> We're trying to avoid new name space violations. Such are
>>> - identifiers starting with two underscores,
>>> - identifiers starting with an underscore and an upper case letter,
>>> - identifiers of non-static symbols starting with an underscore.
>>
>> I am not sure to understand why non-static symbols only. This would
>> prevent you to use the the non-static symbol if you happen to re-use the
>> same name.
> 
> I'm afraid I don't understand. Anyway - what I've listed above is
> what the language standard mandates.
AFAIU, for a given unit, there is only one pool of identifiers. So you 
could not have an identifier used at the same time by a non-static and a 
static symbol (that's exclusing the weak attribute). So it feels 
slightly strange to only cover the non-static symbols.

>> Anyway, how about calling it cmdline_strncmp()? This would be easier to
>> spot misuse on review (i.e using strncmp() rather than _strncmp()).
> 
> We already have cmdline_strcmp(), or else I would indeed have used
> this prefix. No prefix (other than the lone underscore) seemed the
> next best option.

As we parse an option, how about opt_strncmp()?

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

On 06.12.2019 17:20, Julien Grall wrote:
> Hi,
> 
> On 06/12/2019 16:06, Jan Beulich wrote:
>> On 06.12.2019 15:46, Julien Grall wrote:
>>> On 05/12/2019 16:50, Jan Beulich wrote:
>>>> On 05.12.2019 17:27, Julien Grall wrote:
>>>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>>>> +/*
>>>>>> + * 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 _?
>>>>
>>>> We're trying to avoid new name space violations. Such are
>>>> - identifiers starting with two underscores,
>>>> - identifiers starting with an underscore and an upper case letter,
>>>> - identifiers of non-static symbols starting with an underscore.
>>>
>>> I am not sure to understand why non-static symbols only. This would
>>> prevent you to use the the non-static symbol if you happen to re-use the
>>> same name.
>>
>> I'm afraid I don't understand. Anyway - what I've listed above is
>> what the language standard mandates.
> AFAIU, for a given unit, there is only one pool of identifiers. So you 
> could not have an identifier used at the same time by a non-static and a 
> static symbol (that's exclusing the weak attribute). So it feels 
> slightly strange to only cover the non-static symbols.

I guess I'm still not getting your point. What the above tells
us is that static symbols may start with an underscore (but
not followed by another one or an uppercase letter). Non-static
symbols may not.

>>> Anyway, how about calling it cmdline_strncmp()? This would be easier to
>>> spot misuse on review (i.e using strncmp() rather than _strncmp()).
>>
>> We already have cmdline_strcmp(), or else I would indeed have used
>> this prefix. No prefix (other than the lone underscore) seemed the
>> next best option.
> 
> As we parse an option, how about opt_strncmp()?

I'd still like _strncmp() better here.

Jan

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

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

[Julien Grall]

Hi Jan,

On 06/12/2019 16:42, Jan Beulich wrote:
> On 06.12.2019 17:20, Julien Grall wrote:
>> Hi,
>>
>> On 06/12/2019 16:06, Jan Beulich wrote:
>>> On 06.12.2019 15:46, Julien Grall wrote:
>>>> On 05/12/2019 16:50, Jan Beulich wrote:
>>>>> On 05.12.2019 17:27, Julien Grall wrote:
>>>>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>> Anyway, how about calling it cmdline_strncmp()? This would be easier to
>>>> spot misuse on review (i.e using strncmp() rather than _strncmp()).
>>>
>>> We already have cmdline_strcmp(), or else I would indeed have used
>>> this prefix. No prefix (other than the lone underscore) seemed the
>>> next best option.
>>
>> As we parse an option, how about opt_strncmp()?
> 
> I'd still like _strncmp() better here.

Adding an _ in front of the name does not make very obvious the 
differences with the non-underscore very.

This should really be limited to things that cannot be named otherwise. 
In this particular case, there are other possible name a bit longer but 
more descriptive.

Cheers,

-- 
Julien Grall

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

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

[George Dunlap]

On 12/6/19 4:42 PM, Jan Beulich wrote:
> On 06.12.2019 17:20, Julien Grall wrote:
>> Hi,
>>
>> On 06/12/2019 16:06, Jan Beulich wrote:
>>> On 06.12.2019 15:46, Julien Grall wrote:
>>>> On 05/12/2019 16:50, Jan Beulich wrote:
>>>>> On 05.12.2019 17:27, Julien Grall wrote:
>>>>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>>>>> +/*
>>>>>>> + * 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 _?
>>>>>
>>>>> We're trying to avoid new name space violations. Such are
>>>>> - identifiers starting with two underscores,
>>>>> - identifiers starting with an underscore and an upper case letter,
>>>>> - identifiers of non-static symbols starting with an underscore.
>>>>
>>>> I am not sure to understand why non-static symbols only. This would
>>>> prevent you to use the the non-static symbol if you happen to re-use the
>>>> same name.
>>>
>>> I'm afraid I don't understand. Anyway - what I've listed above is
>>> what the language standard mandates.
>> AFAIU, for a given unit, there is only one pool of identifiers. So you 
>> could not have an identifier used at the same time by a non-static and a 
>> static symbol (that's exclusing the weak attribute). So it feels 
>> slightly strange to only cover the non-static symbols.
> 
> I guess I'm still not getting your point. What the above tells
> us is that static symbols may start with an underscore (but
> not followed by another one or an uppercase letter). Non-static
> symbols may not.
> 
>>>> Anyway, how about calling it cmdline_strncmp()? This would be easier to
>>>> spot misuse on review (i.e using strncmp() rather than _strncmp()).
>>>
>>> We already have cmdline_strcmp(), or else I would indeed have used
>>> this prefix. No prefix (other than the lone underscore) seemed the
>>> next best option.
>>
>> As we parse an option, how about opt_strncmp()?
> 
> I'd still like _strncmp() better here.

Why?  It doesn't tell you anything at all about what's special about the
function.  In fact, I'd say it's confusing -- the "_" doesn't normally
mean, "do something different and special", but "do the core of
something which other things might call".

I'd much prefer opt_strncmp() than _strncmp().

 -George


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

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

[Jan Beulich]

On 09.12.2019 15:06, George Dunlap wrote:
> On 12/6/19 4:42 PM, Jan Beulich wrote:
>> On 06.12.2019 17:20, Julien Grall wrote:
>>> Hi,
>>>
>>> On 06/12/2019 16:06, Jan Beulich wrote:
>>>> On 06.12.2019 15:46, Julien Grall wrote:
>>>>> On 05/12/2019 16:50, Jan Beulich wrote:
>>>>>> On 05.12.2019 17:27, Julien Grall wrote:
>>>>>>> On 05/12/2019 15:33, Jan Beulich wrote:
>>>>>>>> +/*
>>>>>>>> + * 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 _?
>>>>>>
>>>>>> We're trying to avoid new name space violations. Such are
>>>>>> - identifiers starting with two underscores,
>>>>>> - identifiers starting with an underscore and an upper case letter,
>>>>>> - identifiers of non-static symbols starting with an underscore.
>>>>>
>>>>> I am not sure to understand why non-static symbols only. This would
>>>>> prevent you to use the the non-static symbol if you happen to re-use the
>>>>> same name.
>>>>
>>>> I'm afraid I don't understand. Anyway - what I've listed above is
>>>> what the language standard mandates.
>>> AFAIU, for a given unit, there is only one pool of identifiers. So you 
>>> could not have an identifier used at the same time by a non-static and a 
>>> static symbol (that's exclusing the weak attribute). So it feels 
>>> slightly strange to only cover the non-static symbols.
>>
>> I guess I'm still not getting your point. What the above tells
>> us is that static symbols may start with an underscore (but
>> not followed by another one or an uppercase letter). Non-static
>> symbols may not.
>>
>>>>> Anyway, how about calling it cmdline_strncmp()? This would be easier to
>>>>> spot misuse on review (i.e using strncmp() rather than _strncmp()).
>>>>
>>>> We already have cmdline_strcmp(), or else I would indeed have used
>>>> this prefix. No prefix (other than the lone underscore) seemed the
>>>> next best option.
>>>
>>> As we parse an option, how about opt_strncmp()?
>>
>> I'd still like _strncmp() better here.
> 
> Why?  It doesn't tell you anything at all about what's special about the
> function.  In fact, I'd say it's confusing -- the "_" doesn't normally
> mean, "do something different and special", but "do the core of
> something which other things might call".
> 
> I'd much prefer opt_strncmp() than _strncmp().

Noted - will do.

Jan

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