Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function
Hello, On Fri, Sep 12, 2014 at 11:01:17AM +0200, Rasmus Villemoes wrote: > On Fri, Sep 12 2014, Andrew Morton wrote: > > > On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function
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? Generally the rate of introduction is pretty low. Generally, just making sure it's not used is good enough. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function
On Fri, Sep 12 2014, Andrew Morton wrote: > On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH/RFC 1/2] lib: string: Remove duplicated function
On Wed, 27 Aug 2014 09:36:01 +0200 Rasmus Villemoes 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH/RFC 1/2] lib: string: Remove duplicated function
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
---
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
#include
-#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
