[PATCH] more improvement to dev_alloc_name -- strnchr

[PATCH] more improvement to dev_alloc_name -- strnchr

[Stephen Hemminger]

On Fri, 16 Jan 2004 20:23:39 -0500
Alex Pankratov <ap@cipherica.com> wrote:

> Stephen,
> 
> perhaps it'd make sense to bail out from dev_alloc_name()
> without iterating if the name is not wildcarded ? 

Okay, this patch avoids the loop if no wildcarding present, and keeps
the same behaviour as original.  It adds the string function strnchr
to avoid searching past the maximum ifname size to find the format character.

diff -Nru a/include/linux/string.h b/include/linux/string.h
--- a/include/linux/string.h	Mon Jan 19 11:30:38 2004
+++ b/include/linux/string.h	Mon Jan 19 11:30:38 2004
@@ -52,6 +52,9 @@
 #ifndef __HAVE_ARCH_STRCHR
 extern char * strchr(const char *,int);
 #endif
+#ifndef __HAVE_ARCH_STRNCHR
+extern char * strnchr(const char *, size_t, int);
+#endif
 #ifndef __HAVE_ARCH_STRRCHR
 extern char * strrchr(const char *,int);
 #endif
diff -Nru a/lib/string.c b/lib/string.c
--- a/lib/string.c	Mon Jan 19 11:30:38 2004
+++ b/lib/string.c	Mon Jan 19 11:30:38 2004
@@ -271,6 +271,22 @@
 }
 #endif
 
+#ifndef __HAVE_ARCH_STRNCHR
+/**
+ * strnchr - Find a character in a length limited string
+ * @s: The string to be searched
+ * @count: The number of characters to be searched
+ * @c: The character to search for
+ */
+char *strnchr(const char *s, size_t count, int c)
+{
+	for (; count-- && *s != '\0'; ++s)
+		if (*s == (char) c)
+			return (char *) s;
+	return NULL;
+}
+#endif
+
 #ifndef __HAVE_ARCH_STRLEN
 /**
  * strlen - Find the length of a string
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c	Mon Jan 19 11:30:38 2004
+++ b/net/core/dev.c	Mon Jan 19 11:30:38 2004
@@ -697,42 +697,43 @@
 
 int dev_alloc_name(struct net_device *dev, const char *name)
 {
-	int i;
+	int i = 0;
 	char buf[IFNAMSIZ];
 	const char *p;
 	const int max_netdevices = 8*PAGE_SIZE;
 	long *inuse;
 	struct net_device *d;
 
-	/*
-	 * Verify the string as this thing may have come from
-	 * the user.  There must be either one "%d" and no other "%"
-	 * characters, or no "%" characters at all.
-	 */
-	p = strchr(name, '%');
-	if (p && ((p[1] != 'd' || strchr(p + 2, '%'))
-		  || (p - name) > IFNAMSIZ-2))
-		return -EINVAL;
+	p = strnchr(name, IFNAMSIZ-1, '%');
+	if (p) {
+		/*
+		 * Verify the string as this thing may have come from
+		 * the user.  There must be either one "%d" and no other "%"
+		 * characters.
+		 */
+		if (p[1] != 'd' || strchr(p + 2, '%'))
+			return -EINVAL;
 
-	/* Use one page as a bit array of possible slots */
-	inuse = (long *) get_zeroed_page(GFP_ATOMIC);
-	if (!inuse)
-		return -ENOMEM;
+		/* Use one page as a bit array of possible slots */
+		inuse = (long *) get_zeroed_page(GFP_ATOMIC);
+		if (!inuse)
+			return -ENOMEM;
 
-	for (d = dev_base; d; d = d->next) {
-		if (!sscanf(d->name, name, &i))
-			continue;
-		if (i < 0 || i >= max_netdevices)
-			continue;
+		for (d = dev_base; d; d = d->next) {
+			if (!sscanf(d->name, name, &i))
+				continue;
+			if (i < 0 || i >= max_netdevices)
+				continue;
 
-		/*  avoid cases where sscanf is not exact inverse of printf */
-		snprintf(buf, sizeof(buf), name, i);
-		if (!strncmp(buf, d->name, IFNAMSIZ))
-			set_bit(i, inuse);
-	}
+			/*  avoid cases where sscanf is not exact inverse of printf */
+			snprintf(buf, sizeof(buf), name, i);
+			if (!strncmp(buf, d->name, IFNAMSIZ))
+				set_bit(i, inuse);
+		}
 
-	i = find_first_zero_bit(inuse, max_netdevices);
-	free_page((unsigned long) inuse);
+		i = find_first_zero_bit(inuse, max_netdevices);
+		free_page((unsigned long) inuse);
+	}
 
 	snprintf(buf, sizeof(buf), name, i);
 	if (!__dev_get_by_name(buf)) {

Re: [PATCH] more improvement to dev_alloc_name -- strnchr

[Andi Kleen]

On Mon, 19 Jan 2004 11:32:04 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> +#ifndef __HAVE_ARCH_STRNCHR

Please drop the ifdef. Don't want to encourage anybody to write
strrchr() in assembly.

-Andi

Re: [PATCH] more improvement to dev_alloc_name -- strnchr

[Stephen Hemminger]

On Mon, 19 Jan 2004 21:06:05 +0100
Andi Kleen <ak@suse.de> wrote:

> On Mon, 19 Jan 2004 11:32:04 -0800
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > +#ifndef __HAVE_ARCH_STRNCHR
> 
> Please drop the ifdef. Don't want to encourage anybody to write
> strrchr() in assembly.
> 
> -Andi

I assume you mean strnchr not strrchr. Mainly just following the style
of all the other string routines.
What if gcc does it inline in some future version?

Re: [PATCH] more improvement to dev_alloc_name -- strnchr

[Andi Kleen]

On Mon, 19 Jan 2004 13:07:44 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> On Mon, 19 Jan 2004 21:06:05 +0100
> Andi Kleen <ak@suse.de> wrote:
> 
> > On Mon, 19 Jan 2004 11:32:04 -0800
> > Stephen Hemminger <shemminger@osdl.org> wrote:
> > 
> > > +#ifndef __HAVE_ARCH_STRNCHR
> > 
> > Please drop the ifdef. Don't want to encourage anybody to write
> > strrchr() in assembly.
> > 
> > -Andi
> 
> I assume you mean strnchr not strrchr. Mainly just following the style
> of all the other string routines.

Yep, strnchr. 

> What if gcc does it inline in some future version?

Not sure what it has to do with that. The #ifdef __HAVE_ARCH_* stuff 
is that architectures with crazy enough hackers can add assembly
optimized functions if they want. But it clearly doesn't make any sense
with this function (in fact it doesn't make much sense with any string
function except memset/memcpy), so better not encourage it.

-Andi 

Re: [PATCH] more improvement to dev_alloc_name -- strnchr

[David S. Miller]

On Mon, 19 Jan 2004 22:15:15 +0100
Andi Kleen <ak@suse.de> wrote:

> On Mon, 19 Jan 2004 13:07:44 -0800
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > What if gcc does it inline in some future version?
> 
> Not sure what it has to do with that. The #ifdef __HAVE_ARCH_* stuff 
> is that architectures with crazy enough hackers can add assembly
> optimized functions if they want. But it clearly doesn't make any sense
> with this function (in fact it doesn't make much sense with any string
> function except memset/memcpy), so better not encourage it.

Sometimes it is just used on a platform to force a call to the gcc builtin.

I think it's perfectly reasonable what Stephen has done.

Re: [PATCH] more improvement to dev_alloc_name -- strnchr

[Alex Pankratov]

Andi Kleen wrote:
> 
> Not sure what it has to do with that. The #ifdef __HAVE_ARCH_* stuff 
> is that architectures with crazy enough hackers can add assembly
> optimized functions if they want. But it clearly doesn't make any sense
> with this function (in fact it doesn't make much sense with any string
> function except memset/memcpy) ... [snip]

.. as well as memchr/memrchr/memcmp and strlen. Just nitpicking :)

Re: [PATCH] more improvement to dev_alloc_name -- strnchr

[David S. Miller]

On Mon, 19 Jan 2004 11:32:04 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Okay, this patch avoids the loop if no wildcarding present, and keeps
> the same behaviour as original.  It adds the string function strnchr
> to avoid searching past the maximum ifname size to find the format character.

I'm deferring this until the next 2.6.x-pre starts up as well.
Please resend at that time.

Thanks Stephen.