sysfs_format_mac

sysfs_format_mac

[David Miller]

I assume that the only reason sysfs_format_mac() exists at all, and
don't just use scnprintf() calls with "%pm\n", is that some devices
have MAC addresses which are not 6 bytes in length.

But it's such a waste to have this special case piece of code
just for that.

It would make so much more sense to allow specifying a length
specifier to %pm and friends.

Re: sysfs_format_mac

[Joe Perches]

On Tue, 2013-07-16 at 12:56 -0700, David Miller wrote:
> 
> I assume that the only reason sysfs_format_mac() exists at all, and
> don't just use scnprintf() calls with "%pm\n", is that some devices
> have MAC addresses which are not 6 bytes in length.
> 
> But it's such a waste to have this special case piece of code
> just for that.
> 
> It would make so much more sense to allow specifying a length
> specifier to %pm and friends.

scnprintf("%*ph", (int)size, buffer)

Re: sysfs_format_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 16 Jul 2013 13:46:26 -0700

> On Tue, 2013-07-16 at 12:56 -0700, David Miller wrote:
>> 
>> I assume that the only reason sysfs_format_mac() exists at all, and
>> don't just use scnprintf() calls with "%pm\n", is that some devices
>> have MAC addresses which are not 6 bytes in length.
>> 
>> But it's such a waste to have this special case piece of code
>> just for that.
>> 
>> It would make so much more sense to allow specifying a length
>> specifier to %pm and friends.
> 
> scnprintf("%*ph", (int)size, buffer)

Yes, exactly, something like that.

Re: sysfs_format_mac

[Joe Perches]

On Tue, 2013-07-16 at 13:58 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Tue, 16 Jul 2013 13:46:26 -0700
> > On Tue, 2013-07-16 at 12:56 -0700, David Miller wrote:
[]
> >> It would make so much more sense to allow specifying a length
> >> specifier to %pm and friends.
> > 
> > scnprintf("%*ph", (int)size, buffer)
> 
> Yes, exactly, something like that.

That works now.

Re: sysfs_format_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 16 Jul 2013 14:12:49 -0700

> On Tue, 2013-07-16 at 13:58 -0700, David Miller wrote:
>> From: Joe Perches <joe@perches.com>
>> Date: Tue, 16 Jul 2013 13:46:26 -0700
>> > On Tue, 2013-07-16 at 12:56 -0700, David Miller wrote:
> []
>> >> It would make so much more sense to allow specifying a length
>> >> specifier to %pm and friends.
>> > 
>> > scnprintf("%*ph", (int)size, buffer)
>> 
>> Yes, exactly, something like that.
> 
> That works now.

Oh, hehe, indeed.

I'm about to test the following.

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 5359560..2307062 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -401,27 +401,8 @@ struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
 }
 EXPORT_SYMBOL(alloc_etherdev_mqs);
 
-static size_t _format_mac_addr(char *buf, int buflen,
-			       const unsigned char *addr, int len)
-{
-	int i;
-	char *cp = buf;
-
-	for (i = 0; i < len; i++) {
-		cp += scnprintf(cp, buflen - (cp - buf), "%02x", addr[i]);
-		if (i == len - 1)
-			break;
-		cp += scnprintf(cp, buflen - (cp - buf), ":");
-	}
-	return cp - buf;
-}
-
 ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
 {
-	size_t l;
-
-	l = _format_mac_addr(buf, PAGE_SIZE, addr, len);
-	l += scnprintf(buf + l, PAGE_SIZE - l, "\n");
-	return (ssize_t)l;
+	return scnprintf(buf, PAGE_SIZE, "%*phC", len, addr);
 }
 EXPORT_SYMBOL(sysfs_format_mac);

Re: sysfs_format_mac

[Joe Perches]

On Tue, 2013-07-16 at 16:25 -0700, David Miller wrote:
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
[]
>  ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
[]
> -       l = _format_mac_addr(buf, PAGE_SIZE, addr, len);
> -       l += scnprintf(buf + l, PAGE_SIZE - l, "\n");
[]
> +       return scnprintf(buf, PAGE_SIZE, "%*phC", len, addr);

missing newline?

	scnprintf(buf, PAGE_ SIZE, "%*phC\n", len, addr);

Re: sysfs_format_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 16 Jul 2013 16:32:12 -0700

> On Tue, 2013-07-16 at 16:25 -0700, David Miller wrote:
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> []
>>  ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
> []
>> -       l = _format_mac_addr(buf, PAGE_SIZE, addr, len);
>> -       l += scnprintf(buf + l, PAGE_SIZE - l, "\n");
> []
>> +       return scnprintf(buf, PAGE_SIZE, "%*phC", len, addr);
> 
> missing newline?
> 
> 	scnprintf(buf, PAGE_ SIZE, "%*phC\n", len, addr);

Good catch.

Re: sysfs_format_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 16 Jul 2013 16:32:12 -0700

> On Tue, 2013-07-16 at 16:25 -0700, David Miller wrote:
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> []
>>  ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
> []
>> -       l = _format_mac_addr(buf, PAGE_SIZE, addr, len);
>> -       l += scnprintf(buf + l, PAGE_SIZE - l, "\n");
> []
>> +       return scnprintf(buf, PAGE_SIZE, "%*phC", len, addr);
> 
> missing newline?
> 
> 	scnprintf(buf, PAGE_ SIZE, "%*phC\n", len, addr);

Thanks again Joe, I just commited the following patch:

--------------------
[PATCH] net: Fix sysfs_format_mac() code duplication.

It's just a duplicate implementation of "%*phC".  Thanks to Joe
Perches for showing that we had exactly this support in the
lib/vsprintf.c code already.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ethernet/eth.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 5359560..be1f64d 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -401,27 +401,8 @@ struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
 }
 EXPORT_SYMBOL(alloc_etherdev_mqs);
 
-static size_t _format_mac_addr(char *buf, int buflen,
-			       const unsigned char *addr, int len)
-{
-	int i;
-	char *cp = buf;
-
-	for (i = 0; i < len; i++) {
-		cp += scnprintf(cp, buflen - (cp - buf), "%02x", addr[i]);
-		if (i == len - 1)
-			break;
-		cp += scnprintf(cp, buflen - (cp - buf), ":");
-	}
-	return cp - buf;
-}
-
 ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
 {
-	size_t l;
-
-	l = _format_mac_addr(buf, PAGE_SIZE, addr, len);
-	l += scnprintf(buf + l, PAGE_SIZE - l, "\n");
-	return (ssize_t)l;
+	return scnprintf(buf, PAGE_SIZE, "%*phC\n", len, addr);
 }
 EXPORT_SYMBOL(sysfs_format_mac);
-- 
1.7.11.7