[PATCH/RFC 0/2] lib: string: Remove duplicated function

[PATCH/RFC 0/2] lib: string: Remove duplicated function

[Rasmus Villemoes]

lib/string.c contains two functions, strnicmp and strncasecmp, which
do roughly the same thing, namely compare two strings
case-insensitively up to a given bound. They have slightly different
implementations, but the only important difference is that strncasecmp
doesn't handle len==0 appropriately; it effectively becomes strcasecmp
in that case. strnicmp correctly says that two strings are always
equal in their first 0 characters.

strncasecmp is the POSIX name for this functionality. The first patch
renames the non-broken version to the standard name, and makes
strnicmp into a wrapper for strncasecmp. In order not to cause in-tree
users of strnicmp to pay the cost of the extra indirection, the second
patch replaces the declaration of strnicmp in string.h with a
macro. When and if all callers of strnicmp have been updated, that
hack can be removed.

Arch-specific versions of these functions could complicate matters,
but fortunately the only #define of either __HAVE macro is in a
!__KERNEL__ section in arch/frv/include/asm/string.h.

The other problem is how to deal with modules that may be using
strnicmp. The proper fix would probably involve some alias/linker
magic, but I have no idea how to do that (hence the RFC).


Rasmus Villemoes (2):
  lib: string: Remove duplicated function
  lib: string: Make all calls to strnicmp into calls to strncasecmp

 include/linux/string.h |  2 +-
 lib/string.c           | 28 +++++++++++-----------------
 2 files changed, 12 insertions(+), 18 deletions(-)

-- 
2.0.4

[PATCH/RFC 1/2] lib: string: Remove duplicated function

[Rasmus Villemoes]

lib/string.c contains two functions, strnicmp and strncasecmp, which
do roughly the same thing, namely compare two strings
case-insensitively up to a given bound. They have slightly different
implementations, but the only important difference is that strncasecmp
doesn't handle len==0 appropriately; it effectively becomes strcasecmp
in that case. strnicmp correctly says that two strings are always
equal in their first 0 characters.

strncasecmp is the POSIX name for this functionality. So rename the
non-broken function to the standard name. To minimize the impact on
the rest of the kernel (and since both are exported to modules), make
strnicmp a wrapper for strncasecmp.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/string.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/lib/string.c b/lib/string.c
index 992bf30..92c33e1 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -27,14 +27,14 @@
 #include <linux/bug.h>
 #include <linux/errno.h>
 
-#ifndef __HAVE_ARCH_STRNICMP
+#ifndef __HAVE_ARCH_STRNCASECMP
 /**
- * strnicmp - Case insensitive, length-limited string comparison
+ * strncasecmp - Case insensitive, length-limited string comparison
  * @s1: One string
  * @s2: The other string
  * @len: the maximum number of characters to compare
  */
-int strnicmp(const char *s1, const char *s2, size_t len)
+int strncasecmp(const char *s1, const char *s2, size_t len)
 {
 	/* Yes, Virginia, it had better be unsigned */
 	unsigned char c1, c2;
@@ -56,6 +56,13 @@ int strnicmp(const char *s1, const char *s2, size_t len)
 	} while (--len);
 	return (int)c1 - (int)c2;
 }
+EXPORT_SYMBOL(strncasecmp);
+#endif
+#ifndef __HAVE_ARCH_STRNICMP
+int strnicmp(const char *s1, const char *s2, size_t len)
+{
+	return strncasecmp(s1, s2, len);
+}
 EXPORT_SYMBOL(strnicmp);
 #endif
 
@@ -73,20 +80,6 @@ int strcasecmp(const char *s1, const char *s2)
 EXPORT_SYMBOL(strcasecmp);
 #endif
 
-#ifndef __HAVE_ARCH_STRNCASECMP
-int strncasecmp(const char *s1, const char *s2, size_t n)
-{
-	int c1, c2;
-
-	do {
-		c1 = tolower(*s1++);
-		c2 = tolower(*s2++);
-	} while ((--n > 0) && c1 == c2 && c1 != 0);
-	return c1 - c2;
-}
-EXPORT_SYMBOL(strncasecmp);
-#endif
-
 #ifndef __HAVE_ARCH_STRCPY
 /**
  * strcpy - Copy a %NUL terminated string
-- 
2.0.4

[PATCH/RFC 2/2] lib: string: Make all calls to strnicmp into calls to strncasecmp

[Rasmus Villemoes]

The previous patch made strnicmp into a wrapper for strncasecmp. This
patch makes all in-tree users of strnicmp call strncasecmp directly,
while still making sure that the strnicmp symbol can be used by
out-of-tree modules. It should be considered a temporary hack until
all in-tree callers have been converted.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/string.h | 2 +-
 lib/string.c           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index d36977e..e6edfe5 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -41,7 +41,7 @@ extern int strcmp(const char *,const char *);
 extern int strncmp(const char *,const char *,__kernel_size_t);
 #endif
 #ifndef __HAVE_ARCH_STRNICMP
-extern int strnicmp(const char *, const char *, __kernel_size_t);
+#define strnicmp strncasecmp
 #endif
 #ifndef __HAVE_ARCH_STRCASECMP
 extern int strcasecmp(const char *s1, const char *s2);
diff --git a/lib/string.c b/lib/string.c
index 92c33e1..d171554 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -59,6 +59,7 @@ int strncasecmp(const char *s1, const char *s2, size_t len)
 EXPORT_SYMBOL(strncasecmp);
 #endif
 #ifndef __HAVE_ARCH_STRNICMP
+#undef strnicmp
 int strnicmp(const char *s1, const char *s2, size_t len)
 {
 	return strncasecmp(s1, s2, len);
-- 
2.0.4

Re: [PATCH/RFC 2/2] lib: string: Make all calls to strnicmp into calls to strncasecmp

[Dan Carpenter]

On Wed, Aug 27, 2014 at 09:36:02AM +0200, Rasmus Villemoes wrote:
> The previous patch made strnicmp into a wrapper for strncasecmp. This
> patch makes all in-tree users of strnicmp call strncasecmp directly,
> while still making sure that the strnicmp symbol can be used by
> out-of-tree modules. It should be considered a temporary hack until
> all in-tree callers have been converted.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Won't GCC just do the right thing without this second patch?

regards,
dan carpenter

Re: [PATCH/RFC 2/2] lib: string: Make all calls to strnicmp into calls to strncasecmp

[Rasmus Villemoes]

On Wed, Aug 27 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Aug 27, 2014 at 09:36:02AM +0200, Rasmus Villemoes wrote:
>> The previous patch made strnicmp into a wrapper for strncasecmp. This
>> patch makes all in-tree users of strnicmp call strncasecmp directly,
>> while still making sure that the strnicmp symbol can be used by
>> out-of-tree modules. It should be considered a temporary hack until
>> all in-tree callers have been converted.
>> 
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> Won't GCC just do the right thing without this second patch?
>

Not without LTO, I think. gcc can't really know how strnicmp is
implemented, so it has to emit a call to it.

Anyway, I was also planning on sending tree-wide patches doing
s/strnicmp/strncasecmp/, and then removing the hack from string.h, but I
first wanted to get feedback on the first patch and maybe some guidance
on how to properly deal with the module issue (e.g., does the kernel
need to export a strnicmp symbol forever?).

Rasmus

Re: [PATCH/RFC 2/2] lib: string: Make all calls to strnicmp into calls to strncasecmp

[Dan Carpenter]

On Wed, Aug 27, 2014 at 11:13:16AM +0200, Rasmus Villemoes wrote:
> Anyway, I was also planning on sending tree-wide patches doing
> s/strnicmp/strncasecmp/, and then removing the hack from string.h, but I
> first wanted to get feedback on the first patch and maybe some guidance
> on how to properly deal with the module issue (e.g., does the kernel
> need to export a strnicmp symbol forever?).

Once we remove the in kernel users then we can remove the function.
Don't worry about out of tree modules.

regards,
dan carpenter

Re: [PATCH/RFC 2/2] lib: string: Make all calls to strnicmp into calls to strncasecmp

[Rasmus Villemoes]

On Wed, Aug 27 2014, Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Aug 27, 2014 at 11:13:16AM +0200, Rasmus Villemoes wrote:
>> Anyway, I was also planning on sending tree-wide patches doing
>> s/strnicmp/strncasecmp/, and then removing the hack from string.h, but I
>> first wanted to get feedback on the first patch and maybe some guidance
>> on how to properly deal with the module issue (e.g., does the kernel
>> need to export a strnicmp symbol forever?).
>
> Once we remove the in kernel users then we can remove the function.
> Don't worry about out of tree modules.

OK, that makes everything simpler. Any objections to the first patch?
Andrew, could you take it through your tree?

Thanks,
Rasmus

Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function

[Andrew Morton]

On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> lib/string.c contains two functions, strnicmp and strncasecmp, which
> do roughly the same thing, namely compare two strings
> case-insensitively up to a given bound. They have slightly different
> implementations, but the only important difference is that strncasecmp
> doesn't handle len==0 appropriately; it effectively becomes strcasecmp
> in that case. strnicmp correctly says that two strings are always
> equal in their first 0 characters.
> 
> strncasecmp is the POSIX name for this functionality. So rename the
> non-broken function to the standard name. To minimize the impact on
> the rest of the kernel (and since both are exported to modules), make
> strnicmp a wrapper for strncasecmp.

I guess it's safe to assume that nobody was depending on the
strncasecmp() bug.

The existing strnicmp() implementation is rather verbose, but I expect
that avoiding the tolower() cost where possible makes sense.

Yes, please prepare the strnicmp()->strncasecmp() patches and let's get
them merged up.  After a kernel release or two we can zap the
back-compat wrapper.


And it isn't just "out of tree modules" that we should be concerned
about - we'll commonly find that "to be in tree" code is using
interfaces which we're trying to alter or remove, so it takes a cycle
or two to get everything propagated.  Most of this code can be found in
linux-next.

Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function

[Rasmus Villemoes]

On Fri, Sep 12 2014, Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> lib/string.c contains two functions, strnicmp and strncasecmp, which
>> do roughly the same thing, namely compare two strings
>> case-insensitively up to a given bound. They have slightly different
>> implementations, but the only important difference is that strncasecmp
>> doesn't handle len==0 appropriately; it effectively becomes strcasecmp
>> in that case. strnicmp correctly says that two strings are always
>> equal in their first 0 characters.
>> 
>> strncasecmp is the POSIX name for this functionality. So rename the
>> non-broken function to the standard name. To minimize the impact on
>> the rest of the kernel (and since both are exported to modules), make
>> strnicmp a wrapper for strncasecmp.
>
> I guess it's safe to assume that nobody was depending on the
> strncasecmp() bug.

Hm, I thought so as well, but decided to double check. I found one minor
issue; maybe Tejun can tell if my analysis is correct.

In drivers/ata/libata-core.c, ata_parse_force_one(), it is not
immediately clear to me that val cannot end up being the empty
string. With the buggy strncasecmp, the continue branch is always
followed (since fp->name is not empty); however, with strncasecmp with
the correct semantics, the empty string is obviously a prefix of every
fp->name. So even though the comment says that "1.5" is an ok
abbreviation of "1.5Gbps", I don't think the intention was to allow ""
to be an abbreviation of everything. Anyway, the worst that can happen
seems to be that "ambigious value" [sic] becomes the *reason instead
of "unknown value".

I may have overlooked similar issues; my checking was basically "is the
length parameter an explicit non-zero number or strlen() of some static
data in some table (where I assume empty strings do not lurk)".

> Yes, please prepare the strnicmp()->strncasecmp() patches and let's get
> them merged up.  After a kernel release or two we can zap the
> back-compat wrapper.

Will do. Is there a fixed SHA1 I can refer to in the commit logs?

> And it isn't just "out of tree modules" that we should be concerned
> about - we'll commonly find that "to be in tree" code is using
> interfaces which we're trying to alter or remove, so it takes a cycle
> or two to get everything propagated.  Most of this code can be found in
> linux-next.

Would it make sense to add a checkpatch warning to ensure new users
are not introduced, and then remove it when the wrapper is also removed?

Thanks,
Rasmus

Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function

[Joe Perches]

On Fri, 2014-09-12 at 11:01 +0200, Rasmus Villemoes wrote:
> Would it make sense to add a checkpatch warning to ensure new users
> are not introduced, and then remove it when the wrapper is also removed?

<shrug> Generally the rate of introduction is pretty low.

Generally, just making sure it's not used is good enough.

Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function

[Tejun Heo]

Hello,

On Fri, Sep 12, 2014 at 11:01:17AM +0200, Rasmus Villemoes wrote:
> On Fri, Sep 12 2014, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >> strncasecmp is the POSIX name for this functionality. So rename the
> >> non-broken function to the standard name. To minimize the impact on
> >> the rest of the kernel (and since both are exported to modules), make
> >> strnicmp a wrapper for strncasecmp.

Maybe it's a good idea to convert all existing users of strnicmp()
strncasecmp() and then mark the strnicmp() wrapper as __deprecated for
later removal?

> > I guess it's safe to assume that nobody was depending on the
> > strncasecmp() bug.
> 
> Hm, I thought so as well, but decided to double check. I found one minor
> issue; maybe Tejun can tell if my analysis is correct.
> 
> In drivers/ata/libata-core.c, ata_parse_force_one(), it is not
> immediately clear to me that val cannot end up being the empty
> string. With the buggy strncasecmp, the continue branch is always
> followed (since fp->name is not empty); however, with strncasecmp with
> the correct semantics, the empty string is obviously a prefix of every
> fp->name. So even though the comment says that "1.5" is an ok
> abbreviation of "1.5Gbps", I don't think the intention was to allow ""
> to be an abbreviation of everything. Anyway, the worst that can happen
> seems to be that "ambigious value" [sic] becomes the *reason instead
> of "unknown value".

Sounds right to me and it shouldn't matter at all.

> > Yes, please prepare the strnicmp()->strncasecmp() patches and let's get
> > them merged up.  After a kernel release or two we can zap the
> > back-compat wrapper.

Ooh, somebody already suggested the same. :)

Thanks.

-- 
tejun