[RFC] ipv6: Change %pI6 format to output compacted addresses?

[RFC] ipv6: Change %pI6 format to output compacted addresses?

[Jens Rosenboom]

Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
which might be compacted to 2001:db8::1. The code to do this could be
adapted from inet_ntop in glibc, which would add about 80 lines to
lib/vsprintf.c. How do you guys value the tradeoff between more readable
logging and increased kernel size?

This was already mentioned in
http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684 but
noone seems to have taken up on it.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Brian Haley]

Jens Rosenboom wrote:
> Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
> which might be compacted to 2001:db8::1. The code to do this could be
> adapted from inet_ntop in glibc, which would add about 80 lines to
> lib/vsprintf.c. How do you guys value the tradeoff between more readable
> logging and increased kernel size?
> 
> This was already mentioned in
> http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684 but
> noone seems to have taken up on it.

I think if any changes are made they should try and follow:

http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt

For one thing, the code today doesn't print things like the v4-mapped
address correctly.

Anyways, can you try this patch, it's less than 40 new lines :)
It might be good enough, but could probably use some help.

-Brian


diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 756ccaf..58602ba 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -652,13 +652,46 @@ static char *ip6_addr_string(char *buf, char *end, u8 *addr,
 {
 	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
 	char *p = ip6_addr;
-	int i;
+	int i, needcolon = 0, printhi;
+	u16 *addr16 = (u16 *)addr;
+	enum { DC_START, DC_MIDDLE, DC_DONE } dcolon = DC_START;
+
+	/* omit leading zeros and shorten using "::" */
 
 	for (i = 0; i < 8; i++) {
-		p = pack_hex_byte(p, addr[2 * i]);
-		p = pack_hex_byte(p, addr[2 * i + 1]);
-		if (!(spec.flags & SPECIAL) && i != 7)
-			*p++ = ':';
+		if (!(spec.flags & SPECIAL)) {
+			if (addr16[i] == 0 && colon < DC_DONE) {
+				colon = DC_MIDDLE;
+				continue;
+			}
+			if (colon == DC_MIDDLE) {
+				colon = DC_DONE;
+				*p++ = ':';
+				*p++ = ':';
+			}  else if (needcolon)
+				*p++ = ':';
+		}
+		printhi = 0;
+		if (addr[2 * i]) {
+			if (addr[2 * i] > 0x0f)
+				p = pack_hex_byte(p, addr[2 * i]);
+			else
+				*p++ = hex_asc_lo(addr[2 * i]);
+			printhi++;
+		}
+		/*
+		 * If we printed the high-order bits we must print the
+		 * low-order ones, even if they're all zeros.
+		 */
+		if (printhi || addr[2 * i + 1] > 0x0f)
+			p = pack_hex_byte(p, addr[2 * i + 1]);
+		else if (addr[2 * i + 1])
+			*p++ = hex_asc_lo(addr[2 * i + 1]);
+		needcolon++;
+	}
+	if (colon == DC_MIDDLE) {
+		*p++ = ':';
+		*p++ = ':';
 	}
 	*p = '\0';
 	spec.flags &= ~SPECIAL;

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Wed, 2009-08-12 at 21:33 -0400, Brian Haley wrote:
> Jens Rosenboom wrote:
> > Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
> > which might be compacted to 2001:db8::1. The code to do this could be
> > adapted from inet_ntop in glibc, which would add about 80 lines to
> > lib/vsprintf.c. How do you guys value the tradeoff between more readable
> > logging and increased kernel size?
> > 
> > This was already mentioned in
> > http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684 but
> > noone seems to have taken up on it.
> 
> Anyways, can you try this patch, it's less than 40 new lines :)
> It might be good enough, but could probably use some help.

You'll need to invent a new %p qualifier type to allow
compressed representation.  Your patch will change current uses
with seq_<foo> output in net, which could break userspace.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Jens Rosenboom]

On Thu, 2009-08-13 at 03:39 -0700, Joe Perches wrote:
> On Wed, 2009-08-12 at 21:33 -0400, Brian Haley wrote:
> > Jens Rosenboom wrote:
> > > Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
> > > which might be compacted to 2001:db8::1. The code to do this could be
> > > adapted from inet_ntop in glibc, which would add about 80 lines to
> > > lib/vsprintf.c. How do you guys value the tradeoff between more readable
> > > logging and increased kernel size?
> > > 
> > > This was already mentioned in
> > > http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684 but
> > > noone seems to have taken up on it.
> > 
> > Anyways, can you try this patch, it's less than 40 new lines :)
> > It might be good enough, but could probably use some help.
> 
> You'll need to invent a new %p qualifier type to allow
> compressed representation.  Your patch will change current uses
> with seq_<foo> output in net, which could break userspace.

Would it be possible to transform this to using %pi6, as most of teh
seq_* stuff already does? It doesn't make sense to shorten the
un-colon-ed version anyway, I'll send an updated version of Brian's
patch soon.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Christoph Hellwig]

On Wed, Aug 12, 2009 at 05:39:20PM +0200, Jens Rosenboom wrote:
> Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
> which might be compacted to 2001:db8::1. The code to do this could be
> adapted from inet_ntop in glibc, which would add about 80 lines to
> lib/vsprintf.c. How do you guys value the tradeoff between more readable
> logging and increased kernel size?

A little note:  if you borrow code from glibc always make sure it's from
an old enough version that is still GPLv2+ licensed.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Jens Rosenboom]

On Thu, 2009-08-13 at 10:18 -0400, Christoph Hellwig wrote:
> On Wed, Aug 12, 2009 at 05:39:20PM +0200, Jens Rosenboom wrote:
> > Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
> > which might be compacted to 2001:db8::1. The code to do this could be
> > adapted from inet_ntop in glibc, which would add about 80 lines to
> > lib/vsprintf.c. How do you guys value the tradeoff between more readable
> > logging and increased kernel size?
> 
> A little note:  if you borrow code from glibc always make sure it's from
> an old enough version that is still GPLv2+ licensed.

The code for inet_ntop has as comment

 * author:
 *      Paul Vixie, 1996.

while the whole file is

Copyright (c) 1996-1999 by Internet Software Consortium.

so it should probably be old enough. But it also doesn't look too nice
to me, so I'll try to go without it for now.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Jens Rosenboom]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

On Wed, 2009-08-12 at 21:33 -0400, Brian Haley wrote:
> Jens Rosenboom wrote:
> > Currently the output looks like 2001:0db8:0000:0000:0000:0000:0000:0001
> > which might be compacted to 2001:db8::1. The code to do this could be
> > adapted from inet_ntop in glibc, which would add about 80 lines to
> > lib/vsprintf.c. How do you guys value the tradeoff between more readable
> > logging and increased kernel size?
> > 
> > This was already mentioned in
> > http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684 but
> > noone seems to have taken up on it.
> 
> I think if any changes are made they should try and follow:
> 
> http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
> 
> For one thing, the code today doesn't print things like the v4-mapped
> address correctly.
> 
> Anyways, can you try this patch, it's less than 40 new lines :)
> It might be good enough, but could probably use some help.

For a start, it didn't even compile. ;-) Here is a new version that also
fixes

- Leave %pi6 alone
- Don't compress a single :0:
- Do output 0

The results and also the remaining issues can be seen with the attached
test program, that also exposes a bug in glibc for v4-mapped addresses
from 0/16.

To fully conform to the cited draft, we would still have to implement
v4-mapped and also check whether a second run of zeros would be longer
than the first one, although the draft also suggests that operators
should avoid using this kind of addresses, so maybe this second issue
can be neglected.

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 756ccaf..5710c65 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -652,13 +652,53 @@ static char *ip6_addr_string(char *buf, char *end,
u8 *addr,
 {
 	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing
zero */
 	char *p = ip6_addr;
-	int i;
+	int i, needcolon = 0, printhi;
+	u16 *addr16 = (u16 *)addr;
+	enum { DC_START, DC_MIDDLE, DC_DONE } colon = DC_START;
+
+	/* omit leading zeros and shorten using "::" */
 
-	for (i = 0; i < 8; i++) {
-		p = pack_hex_byte(p, addr[2 * i]);
-		p = pack_hex_byte(p, addr[2 * i + 1]);
-		if (!(spec.flags & SPECIAL) && i != 7)
+	if (!(spec.flags & SPECIAL)) {
+		for (i = 0; i < 8; i++) {
+			if (addr16[i] == 0 && addr16[i+1] == 0 && colon == DC_START) {
+				colon = DC_MIDDLE;
+				continue;
+			}
+			if (colon == DC_MIDDLE) {
+				if (addr16[i] == 0)
+					continue;
+				colon = DC_DONE;
+				*p++ = ':';
+				*p++ = ':';
+			}  else if (needcolon)
+				*p++ = ':';
+			printhi = 0;
+			if (addr[2 * i]) {
+				if (addr[2 * i] > 0x0f)
+					p = pack_hex_byte(p, addr[2 * i]);
+				else
+					*p++ = hex_asc_lo(addr[2 * i]);
+				printhi++;
+			}
+			/*
+		 	* If we printed the high-order bits we must print the
+		 	* low-order ones, even if they're all zeros.
+		 	*/
+			if (printhi || addr[2 * i + 1] > 0x0f)
+				p = pack_hex_byte(p, addr[2 * i + 1]);
+			else 
+				*p++ = hex_asc_lo(addr[2 * i + 1]);
+			needcolon++;
+		}
+		if (colon == DC_MIDDLE) {
 			*p++ = ':';
+			*p++ = ':';
+		}
+	} else {
+		for (i = 0; i < 8; i++) {
+			p = pack_hex_byte(p, addr[2 * i]);
+			p = pack_hex_byte(p, addr[2 * i + 1]);
+		}
 	}
 	*p = '\0';
 	spec.flags &= ~SPECIAL;


[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 867 bytes --]

/* Dirty test prog for %pI6 formatting */
/* Compile with: cc -o test test.c lib/vsprintf.o lib/ctype.o */

#include <arpa/inet.h>
extern int printf(const char *fmt, ...);
extern int sprintf(char * buf, const char *fmt, ...);
const char hex_asc[] = "0123456789abcdef";

sprint_symbol(void) { return 0; }
kallsyms_lookup(void) { return 0; }
warn_slowpath_null(void) { return 0;}

void dotest(void *addr) {
	char res[500], res2[500];

	inet_ntop(AF_INET6, addr, res2, 500);
	sprintf(res, "%pi6 %pI6", addr, addr);
	printf("%s %s\n", res, res2);
}

main() {
	unsigned int addr[8];
	int i;

	for(i=0;i<8;i++) addr[i]=0;

	dotest(addr);

	addr[3]=htonl(0x100);

	dotest(addr);

	addr[3]=htonl(0x10000);

	dotest(addr);

	addr[0]=htonl(0x20010db8);
	addr[1]=htonl(0x234);

	dotest(addr);

	addr[0]=htonl(0x20010000);

	dotest(addr);

	addr[3]=htonl(0xa);

	dotest(addr);
}

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Thu, 2009-08-13 at 15:52 +0200, Jens Rosenboom wrote:
> On Thu, 2009-08-13 at 03:39 -0700, Joe Perches wrote:
> > You'll need to invent a new %p qualifier type to allow
> > compressed representation.  Your patch will change current uses
> > with seq_<foo> output in net, which could break userspace.
> 
> Would it be possible to transform this to using %pi6, as most of teh
> seq_* stuff already does?

No.

There are applications that depend on the
current output representation.

I think it should be done with something like "%pi6c" 
rather than using another %p character because there
are a limited number of single characters available.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Chuck Lever]

On Aug 13, 2009, at 10:39 AM, Jens Rosenboom wrote:
> On Wed, 2009-08-12 at 21:33 -0400, Brian Haley wrote:
>> Jens Rosenboom wrote:
>>> Currently the output looks like  
>>> 2001:0db8:0000:0000:0000:0000:0000:0001
>>> which might be compacted to 2001:db8::1. The code to do this could  
>>> be
>>> adapted from inet_ntop in glibc, which would add about 80 lines to
>>> lib/vsprintf.c. How do you guys value the tradeoff between more  
>>> readable
>>> logging and increased kernel size?
>>>
>>> This was already mentioned in
>>> http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684  
>>> but
>>> noone seems to have taken up on it.
>>
>> I think if any changes are made they should try and follow:
>>
>> http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
>>
>> For one thing, the code today doesn't print things like the v4-mapped
>> address correctly.
>>
>> Anyways, can you try this patch, it's less than 40 new lines :)
>> It might be good enough, but could probably use some help.
>
> For a start, it didn't even compile. ;-) Here is a new version that  
> also
> fixes
>
> - Leave %pi6 alone
> - Don't compress a single :0:
> - Do output 0
>
> The results and also the remaining issues can be seen with the  
> attached
> test program, that also exposes a bug in glibc for v4-mapped addresses
> from 0/16.
>
> To fully conform to the cited draft, we would still have to implement
> v4-mapped and also check whether a second run of zeros would be longer
> than the first one, although the draft also suggests that operators
> should avoid using this kind of addresses, so maybe this second issue
> can be neglected.

If it is at all helpful, I recently proposed adding rpc_ntop() to  
sunrpc.ko to provide proper IPv6 shorthanding without changing %p[iI]6  
at all.  The patch was rejected, but there may be something here you  
can use.  The version of rpc_ntop() accepted for 2.6.32 does not  
provide shorthanding.

See the archived mail at http://www.spinics.net/lists/linux-nfs/msg08363.html

If you get proper IPv6 shorthanding into the kernel, RPC is one more  
consumer for you.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 756ccaf..5710c65 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -652,13 +652,53 @@ static char *ip6_addr_string(char *buf, char  
> *end,
> u8 *addr,
> {
> 	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing
> zero */
> 	char *p = ip6_addr;
> -	int i;
> +	int i, needcolon = 0, printhi;
> +	u16 *addr16 = (u16 *)addr;
> +	enum { DC_START, DC_MIDDLE, DC_DONE } colon = DC_START;
> +
> +	/* omit leading zeros and shorten using "::" */
>
> -	for (i = 0; i < 8; i++) {
> -		p = pack_hex_byte(p, addr[2 * i]);
> -		p = pack_hex_byte(p, addr[2 * i + 1]);
> -		if (!(spec.flags & SPECIAL) && i != 7)
> +	if (!(spec.flags & SPECIAL)) {
> +		for (i = 0; i < 8; i++) {
> +			if (addr16[i] == 0 && addr16[i+1] == 0 && colon == DC_START) {
> +				colon = DC_MIDDLE;
> +				continue;
> +			}
> +			if (colon == DC_MIDDLE) {
> +				if (addr16[i] == 0)
> +					continue;
> +				colon = DC_DONE;
> +				*p++ = ':';
> +				*p++ = ':';
> +			}  else if (needcolon)
> +				*p++ = ':';
> +			printhi = 0;
> +			if (addr[2 * i]) {
> +				if (addr[2 * i] > 0x0f)
> +					p = pack_hex_byte(p, addr[2 * i]);
> +				else
> +					*p++ = hex_asc_lo(addr[2 * i]);
> +				printhi++;
> +			}
> +			/*
> +		 	* If we printed the high-order bits we must print the
> +		 	* low-order ones, even if they're all zeros.
> +		 	*/
> +			if (printhi || addr[2 * i + 1] > 0x0f)
> +				p = pack_hex_byte(p, addr[2 * i + 1]);
> +			else
> +				*p++ = hex_asc_lo(addr[2 * i + 1]);
> +			needcolon++;
> +		}
> +		if (colon == DC_MIDDLE) {
> 			*p++ = ':';
> +			*p++ = ':';
> +		}
> +	} else {
> +		for (i = 0; i < 8; i++) {
> +			p = pack_hex_byte(p, addr[2 * i]);
> +			p = pack_hex_byte(p, addr[2 * i + 1]);
> +		}
> 	}
> 	*p = '\0';
> 	spec.flags &= ~SPECIAL;
>
> <test.c>

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Brian Haley]

Jens Rosenboom wrote:
>> Anyways, can you try this patch, it's less than 40 new lines :)
>> It might be good enough, but could probably use some help.
> 
> For a start, it didn't even compile. ;-)

It did on net-next-2.6 last night, weird.

> Here is a new version that also
> fixes
> 
> - Leave %pi6 alone
> - Don't compress a single :0:
> - Do output 0
> 
> The results and also the remaining issues can be seen with the attached
> test program, that also exposes a bug in glibc for v4-mapped addresses
> from 0/16.
> 
> To fully conform to the cited draft, we would still have to implement
> v4-mapped and also check whether a second run of zeros would be longer
> than the first one, although the draft also suggests that operators
> should avoid using this kind of addresses, so maybe this second issue
> can be neglected.

Yes, the "compress the most zeros" would be harder, and require two
passes over the address.  I had to cut corners somewhere :)

And regarding v4-mapped, the easy fix to that is just detect it and
call the IPv4 routine.  The attached patch does that, but without the
::ffff:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 756ccaf..5710c65 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -652,13 +652,53 @@ static char *ip6_addr_string(char *buf, char *end,
> u8 *addr,
>  {
>  	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing
> zero */
>  	char *p = ip6_addr;
> -	int i;
> +	int i, needcolon = 0, printhi;
> +	u16 *addr16 = (u16 *)addr;
> +	enum { DC_START, DC_MIDDLE, DC_DONE } colon = DC_START;
> +
> +	/* omit leading zeros and shorten using "::" */
>  
> -	for (i = 0; i < 8; i++) {
> -		p = pack_hex_byte(p, addr[2 * i]);
> -		p = pack_hex_byte(p, addr[2 * i + 1]);
> -		if (!(spec.flags & SPECIAL) && i != 7)
> +	if (!(spec.flags & SPECIAL)) {
> +		for (i = 0; i < 8; i++) {
> +			if (addr16[i] == 0 && addr16[i+1] == 0 && colon == DC_START) {

This will access the array out-of-bounds when i=7.

Another hack below.

-Brian


diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 756ccaf..ba70f2a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -647,25 +647,6 @@ static char *mac_address_string(char *buf, char *end, u8 *addr,
 	return string(buf, end, mac_addr, spec);
 }
 
-static char *ip6_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
-{
-	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
-	char *p = ip6_addr;
-	int i;
-
-	for (i = 0; i < 8; i++) {
-		p = pack_hex_byte(p, addr[2 * i]);
-		p = pack_hex_byte(p, addr[2 * i + 1]);
-		if (!(spec.flags & SPECIAL) && i != 7)
-			*p++ = ':';
-	}
-	*p = '\0';
-	spec.flags &= ~SPECIAL;
-
-	return string(buf, end, ip6_addr, spec);
-}
-
 static char *ip4_addr_string(char *buf, char *end, u8 *addr,
 				struct printf_spec spec)
 {
@@ -688,6 +669,73 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr,
 	return string(buf, end, ip4_addr, spec);
 }
 
+static char *ip6_addr_string(char *buf, char *end, u8 *addr,
+				struct printf_spec spec)
+{
+	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
+	char *p = ip6_addr;
+	int i, needcolon, printhi;
+	u16 *addr16 = (u16 *)addr;
+	u32 *addr32 = (u32 *)addr;
+	enum { DC_START, DC_MIDDLE, DC_DONE } colon = DC_START;
+
+	if (!(spec.flags & SPECIAL)) {
+		/* omit leading zeros and shorten using "::" */
+
+		/* v4mapped */
+		if ((addr32[0] | addr32[1] |
+		    (addr32[2] ^ htonl(0x0000ffff))) == 0)
+			return ip4_addr_string(buf, end, &addr[12], spec);
+
+		needcolon = 0;
+		for (i = 0; i < 8; i++) {
+			if (addr16[i] == 0 && i < 7 && addr16[i+1] == 0 &&
+			    colon == DC_START) {
+				colon = DC_MIDDLE;
+				continue;
+			}
+			if (colon == DC_MIDDLE) {
+				if (addr16[i] == 0)
+					continue;
+				colon = DC_DONE;
+				*p++ = ':';
+				*p++ = ':';
+			}  else if (needcolon)
+				*p++ = ':';
+			printhi = 0;
+			if (addr[2 * i]) {
+				if (addr[2 * i] > 0x0f)
+					p = pack_hex_byte(p, addr[2 * i]);
+				else
+					*p++ = hex_asc_lo(addr[2 * i]);
+				printhi++;
+			}
+			/*
+		 	* If we printed the high-order bits we must print the
+		 	* low-order ones, even if they're all zeros.
+		 	*/
+			if (printhi || addr[2 * i + 1] > 0x0f)
+				p = pack_hex_byte(p, addr[2 * i + 1]);
+			else 
+				*p++ = hex_asc_lo(addr[2 * i + 1]);
+			needcolon++;
+		}
+		if (colon == DC_MIDDLE) {
+			*p++ = ':';
+			*p++ = ':';
+		}
+	} else {
+		for (i = 0; i < 8; i++) {
+			p = pack_hex_byte(p, addr[2 * i]);
+			p = pack_hex_byte(p, addr[2 * i + 1]);
+		}
+	}
+	*p = '\0';
+	spec.flags &= ~SPECIAL;
+
+	return string(buf, end, ip6_addr, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Thu, 2009-08-13 at 12:27 -0400, Brian Haley wrote:
> Jens Rosenboom wrote:
> > Here is a new version that also
> > fixes
> > 
> > - Leave %pi6 alone
> > - Don't compress a single :0:
> > - Do output 0
> > The results and also the remaining issues can be seen with the attached
> > test program, that also exposes a bug in glibc for v4-mapped addresses
> > from 0/16.
> > To fully conform to the cited draft, we would still have to implement
> > v4-mapped and also check whether a second run of zeros would be longer
> > than the first one, although the draft also suggests that operators
> > should avoid using this kind of addresses, so maybe this second issue
> > can be neglected.
> 
> Yes, the "compress the most zeros" would be harder, and require two
> passes over the address.  I had to cut corners somewhere :)

2 things.

First a question, then a compilable but untested patch.

The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
with ipv4 last u32.

Can somebody tell me what I'm doing wrong when I link Jens' test?

cc -o test test_ipv6.c lib/vsprintf.o lib/ctype.o
lib/vsprintf.o: In function `global constructors keyed to
65535_0_simple_strtoul':
/home/joe/linux/linux-2.6/lib/vsprintf.c:1972: undefined reference to
`__gcov_init'
lib/vsprintf.o:(.data+0x28): undefined reference to `__gcov_merge_add'
collect2: ld returned 1 exit status


Now for the patch.  Perhaps something like this (compiled, untested)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 756ccaf..dd02842 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -630,7 +630,7 @@ static char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static char *mac_address_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+				struct printf_spec spec, const char *fmt)
 {
 	char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing zero */
 	char *p = mac_addr;
@@ -638,36 +638,128 @@ static char *mac_address_string(char *buf, char *end, u8 *addr,
 
 	for (i = 0; i < 6; i++) {
 		p = pack_hex_byte(p, addr[i]);
-		if (!(spec.flags & SPECIAL) && i != 5)
+		if (!(fmt[0] == 'm') && i != 5)
 			*p++ = ':';
 	}
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
 
 	return string(buf, end, mac_addr, spec);
 }
 
+typedef enum { DC_START, DC_MIDDLE, DC_DONE } ip6_colon_t;
+
+static char *ip6_compress_u16(char *p, u16 addr16, u16 addr16_next,
+			      ip6_colon_t *colon, bool *needcolon)
+{
+	bool printhi;
+	u8 hi;
+	u8 lo;
+
+	if (addr16 == 0 && addr16_next == 0 && *colon == DC_START) {
+		*colon = DC_MIDDLE;
+		return p;
+	}
+	if (*colon == DC_MIDDLE) {
+		if (addr16 == 0)
+			return p;
+		*colon = DC_DONE;
+		*p++ = ':';
+		*p++ = ':';
+	} else if (*needcolon)
+		*p++ = ':';
+
+	printhi = false;
+	hi = addr16 >> 8;
+	lo = addr16 & 0xff;
+	if (hi) {
+		if (hi > 0x0f)
+			p = pack_hex_byte(p, hi);
+		else
+			*p++ = hex_asc_lo(hi);
+		printhi = true;
+	}
+	/*
+	 * If we printed the high-order bits we must print the
+	 * low-order ones, even if they're all zeros.
+	 */
+	if (printhi || lo > 0x0f)
+		p = pack_hex_byte(p, lo);
+	else 
+		*p++ = hex_asc_lo(lo);
+	*needcolon = true;
+
+	return p;
+}
+
+static char *ip6_compressed_string(char *buf, char *end, u8 *addr,
+				   struct printf_spec spec, const char *fmt,
+				   char *ip6_addr)
+{
+	char *p = ip6_addr;
+	int i;
+	bool needcolon = false;
+	u16 *addr16 = (u16 *)addr;
+	ip6_colon_t colon = DC_START;
+
+	if (fmt[3] == '4') {		/* use :: and ipv4 */
+		for (i = 0; i < 6; i++) {
+			p = ip6_compress_u16(p, addr16[i], addr16[i+1],
+					     &colon, &needcolon);
+		}
+		if (colon != DC_DONE) {
+			*p++ = ':';
+			*p++ = ':';
+			colon = DC_DONE;
+		}
+		addr += 6 * 2;
+		for (i = 0; i < 4; i++) {
+			p = put_dec_trunc(p, *addr++);
+			if (i != 3)
+				*p++ = '.';
+		}
+	} else {
+		for (i = 0; i < 7; i++) {
+			p = ip6_compress_u16(p, addr16[i], addr16[i+1],
+					     &colon, &needcolon);
+		}
+		p = ip6_compress_u16(p, addr16[7], 0xff, &colon, &needcolon);
+	}
+
+	if (colon == DC_MIDDLE) {
+		*p++ = ':';
+		*p++ = ':';
+	}
+
+	*p = '\0';
+
+	return string(buf, end, ip6_addr, spec);
+}
+
 static char *ip6_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+			     struct printf_spec spec, const char *fmt)
 {
-	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
+	char ip6_addr[7 * 4 + 7 + 4 * 4]; /* (7 * 4 hex digits) + 7 colons +
+					   * ipv4 address, and trailing zero */
 	char *p = ip6_addr;
 	int i;
 
+	if (fmt[0] == 'i' && fmt[2] == 'c')
+		return ip6_compressed_string(buf, end, addr, spec, fmt,
+					     ip6_addr);
+
 	for (i = 0; i < 8; i++) {
 		p = pack_hex_byte(p, addr[2 * i]);
 		p = pack_hex_byte(p, addr[2 * i + 1]);
-		if (!(spec.flags & SPECIAL) && i != 7)
+		if (fmt[0] == 'i' && i != 7)
 			*p++ = ':';
 	}
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
 
 	return string(buf, end, ip6_addr, spec);
 }
 
 static char *ip4_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+			     struct printf_spec spec)
 {
 	char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and trailing zero */
 	char temp[3];	/* hold each IP quad in reverse order */
@@ -683,11 +775,18 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr,
 			*p++ = '.';
 	}
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
 
 	return string(buf, end, ip4_addr, spec);
 }
 
+static char *ip_addr_string(char *buf, char *end, u8 *addr,
+			    struct printf_spec spec, const char *fmt)
+{
+	if (fmt[1] == '6')
+		return ip6_addr_string(buf, end, addr, spec, fmt);
+	return ip4_addr_string(buf, end, addr, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -726,20 +825,19 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return symbol_string(buf, end, ptr, spec, *fmt);
 	case 'R':
 		return resource_string(buf, end, ptr, spec);
-	case 'm':
-		spec.flags |= SPECIAL;
-		/* Fallthrough */
-	case 'M':
-		return mac_address_string(buf, end, ptr, spec);
-	case 'i':
-		spec.flags |= SPECIAL;
-		/* Fallthrough */
-	case 'I':
-		if (fmt[1] == '6')
-			return ip6_addr_string(buf, end, ptr, spec);
-		if (fmt[1] == '4')
-			return ip4_addr_string(buf, end, ptr, spec);
-		spec.flags &= ~SPECIAL;
+	case 'm':			/* Colon separated: 00:01:02:03:04:05 */
+	case 'M':			/* Contiguous: 000102030405 */
+		return mac_address_string(buf, end, ptr, spec, fmt);
+	case 'i':			/*
+					 * Formatted IP supported
+					 * 4:	001.002.003.004
+					 * 6:	0001:0203:...:0708
+					 * 6c:	1::708
+					 * 6c4:	1::1.2.3.4
+					 */
+	case 'I':			/* Contiguous: */
+		if (fmt[1] == '4' || fmt[1] == '6')
+			ip_addr_string(buf, end, ptr, spec, fmt);
 		break;
 	}
 	spec.flags |= SMALL;

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Chuck Lever]

On Aug 13, 2009, at 2:10 PM, Joe Perches wrote:
> On Thu, 2009-08-13 at 12:27 -0400, Brian Haley wrote:
>> Jens Rosenboom wrote:
>>> Here is a new version that also
>>> fixes
>>>
>>> - Leave %pi6 alone
>>> - Don't compress a single :0:
>>> - Do output 0
>>> The results and also the remaining issues can be seen with the  
>>> attached
>>> test program, that also exposes a bug in glibc for v4-mapped  
>>> addresses
>>> from 0/16.
>>> To fully conform to the cited draft, we would still have to  
>>> implement
>>> v4-mapped and also check whether a second run of zeros would be  
>>> longer
>>> than the first one, although the draft also suggests that operators
>>> should avoid using this kind of addresses, so maybe this second  
>>> issue
>>> can be neglected.
>>
>> Yes, the "compress the most zeros" would be harder, and require two
>> passes over the address.  I had to cut corners somewhere :)
>
> 2 things.
>
> First a question, then a compilable but untested patch.
>
> The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
> with ipv4 last u32.

Why do these need to be separate?

> Can somebody tell me what I'm doing wrong when I link Jens' test?
>
> cc -o test test_ipv6.c lib/vsprintf.o lib/ctype.o
> lib/vsprintf.o: In function `global constructors keyed to
> 65535_0_simple_strtoul':
> /home/joe/linux/linux-2.6/lib/vsprintf.c:1972: undefined reference to
> `__gcov_init'
> lib/vsprintf.o:(.data+0x28): undefined reference to `__gcov_merge_add'
> collect2: ld returned 1 exit status
>
>
> Now for the patch.  Perhaps something like this (compiled, untested)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 756ccaf..dd02842 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -630,7 +630,7 @@ static char *resource_string(char *buf, char  
> *end, struct resource *res,
> }
>
> static char *mac_address_string(char *buf, char *end, u8 *addr,
> -				struct printf_spec spec)
> +				struct printf_spec spec, const char *fmt)
> {
> 	char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing  
> zero */
> 	char *p = mac_addr;
> @@ -638,36 +638,128 @@ static char *mac_address_string(char *buf,  
> char *end, u8 *addr,
>
> 	for (i = 0; i < 6; i++) {
> 		p = pack_hex_byte(p, addr[i]);
> -		if (!(spec.flags & SPECIAL) && i != 5)
> +		if (!(fmt[0] == 'm') && i != 5)
> 			*p++ = ':';
> 	}
> 	*p = '\0';
> -	spec.flags &= ~SPECIAL;
>
> 	return string(buf, end, mac_addr, spec);
> }
>
> +typedef enum { DC_START, DC_MIDDLE, DC_DONE } ip6_colon_t;
> +
> +static char *ip6_compress_u16(char *p, u16 addr16, u16 addr16_next,
> +			      ip6_colon_t *colon, bool *needcolon)
> +{
> +	bool printhi;
> +	u8 hi;
> +	u8 lo;
> +
> +	if (addr16 == 0 && addr16_next == 0 && *colon == DC_START) {
> +		*colon = DC_MIDDLE;
> +		return p;
> +	}
> +	if (*colon == DC_MIDDLE) {
> +		if (addr16 == 0)
> +			return p;
> +		*colon = DC_DONE;
> +		*p++ = ':';
> +		*p++ = ':';
> +	} else if (*needcolon)
> +		*p++ = ':';
> +
> +	printhi = false;
> +	hi = addr16 >> 8;
> +	lo = addr16 & 0xff;
> +	if (hi) {
> +		if (hi > 0x0f)
> +			p = pack_hex_byte(p, hi);
> +		else
> +			*p++ = hex_asc_lo(hi);
> +		printhi = true;
> +	}
> +	/*
> +	 * If we printed the high-order bits we must print the
> +	 * low-order ones, even if they're all zeros.
> +	 */
> +	if (printhi || lo > 0x0f)
> +		p = pack_hex_byte(p, lo);
> +	else
> +		*p++ = hex_asc_lo(lo);
> +	*needcolon = true;
> +
> +	return p;
> +}
> +
> +static char *ip6_compressed_string(char *buf, char *end, u8 *addr,
> +				   struct printf_spec spec, const char *fmt,
> +				   char *ip6_addr)
> +{
> +	char *p = ip6_addr;
> +	int i;
> +	bool needcolon = false;
> +	u16 *addr16 = (u16 *)addr;
> +	ip6_colon_t colon = DC_START;
> +
> +	if (fmt[3] == '4') {		/* use :: and ipv4 */
> +		for (i = 0; i < 6; i++) {
> +			p = ip6_compress_u16(p, addr16[i], addr16[i+1],
> +					     &colon, &needcolon);
> +		}
> +		if (colon != DC_DONE) {
> +			*p++ = ':';
> +			*p++ = ':';
> +			colon = DC_DONE;
> +		}
> +		addr += 6 * 2;
> +		for (i = 0; i < 4; i++) {
> +			p = put_dec_trunc(p, *addr++);
> +			if (i != 3)
> +				*p++ = '.';
> +		}
> +	} else {
> +		for (i = 0; i < 7; i++) {
> +			p = ip6_compress_u16(p, addr16[i], addr16[i+1],
> +					     &colon, &needcolon);
> +		}
> +		p = ip6_compress_u16(p, addr16[7], 0xff, &colon, &needcolon);
> +	}
> +
> +	if (colon == DC_MIDDLE) {
> +		*p++ = ':';
> +		*p++ = ':';
> +	}
> +
> +	*p = '\0';
> +
> +	return string(buf, end, ip6_addr, spec);
> +}
> +
> static char *ip6_addr_string(char *buf, char *end, u8 *addr,
> -				struct printf_spec spec)
> +			     struct printf_spec spec, const char *fmt)
> {
> -	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing  
> zero */
> +	char ip6_addr[7 * 4 + 7 + 4 * 4]; /* (7 * 4 hex digits) + 7 colons +
> +					   * ipv4 address, and trailing zero */
> 	char *p = ip6_addr;
> 	int i;
>
> +	if (fmt[0] == 'i' && fmt[2] == 'c')
> +		return ip6_compressed_string(buf, end, addr, spec, fmt,
> +					     ip6_addr);
> +
> 	for (i = 0; i < 8; i++) {
> 		p = pack_hex_byte(p, addr[2 * i]);
> 		p = pack_hex_byte(p, addr[2 * i + 1]);
> -		if (!(spec.flags & SPECIAL) && i != 7)
> +		if (fmt[0] == 'i' && i != 7)
> 			*p++ = ':';
> 	}
> 	*p = '\0';
> -	spec.flags &= ~SPECIAL;
>
> 	return string(buf, end, ip6_addr, spec);
> }
>
> static char *ip4_addr_string(char *buf, char *end, u8 *addr,
> -				struct printf_spec spec)
> +			     struct printf_spec spec)
> {
> 	char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and  
> trailing zero */
> 	char temp[3];	/* hold each IP quad in reverse order */
> @@ -683,11 +775,18 @@ static char *ip4_addr_string(char *buf, char  
> *end, u8 *addr,
> 			*p++ = '.';
> 	}
> 	*p = '\0';
> -	spec.flags &= ~SPECIAL;
>
> 	return string(buf, end, ip4_addr, spec);
> }
>
> +static char *ip_addr_string(char *buf, char *end, u8 *addr,
> +			    struct printf_spec spec, const char *fmt)
> +{
> +	if (fmt[1] == '6')
> +		return ip6_addr_string(buf, end, addr, spec, fmt);
> +	return ip4_addr_string(buf, end, addr, spec);
> +}
> +
> /*
>  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>  * by an extra set of alphanumeric characters that are extended format
> @@ -726,20 +825,19 @@ static char *pointer(const char *fmt, char  
> *buf, char *end, void *ptr,
> 		return symbol_string(buf, end, ptr, spec, *fmt);
> 	case 'R':
> 		return resource_string(buf, end, ptr, spec);
> -	case 'm':
> -		spec.flags |= SPECIAL;
> -		/* Fallthrough */
> -	case 'M':
> -		return mac_address_string(buf, end, ptr, spec);
> -	case 'i':
> -		spec.flags |= SPECIAL;
> -		/* Fallthrough */
> -	case 'I':
> -		if (fmt[1] == '6')
> -			return ip6_addr_string(buf, end, ptr, spec);
> -		if (fmt[1] == '4')
> -			return ip4_addr_string(buf, end, ptr, spec);
> -		spec.flags &= ~SPECIAL;
> +	case 'm':			/* Colon separated: 00:01:02:03:04:05 */
> +	case 'M':			/* Contiguous: 000102030405 */
> +		return mac_address_string(buf, end, ptr, spec, fmt);
> +	case 'i':			/*
> +					 * Formatted IP supported
> +					 * 4:	001.002.003.004
> +					 * 6:	0001:0203:...:0708
> +					 * 6c:	1::708
> +					 * 6c4:	1::1.2.3.4
> +					 */
> +	case 'I':			/* Contiguous: */
> +		if (fmt[1] == '4' || fmt[1] == '6')
> +			ip_addr_string(buf, end, ptr, spec, fmt);
> 		break;
> 	}
> 	spec.flags |= SMALL;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Thu, 2009-08-13 at 14:15 -0400, Chuck Lever wrote:
> On Aug 13, 2009, at 2:10 PM, Joe Perches wrote:
> > The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
> > with ipv4 last u32.
> 
> Why do these need to be separate?

Just an option.
I think it possible somebody will want "1::" instead of "1::0.0.0.0"

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Chuck Lever]

On Aug 13, 2009, at 2:21 PM, Joe Perches wrote:
> On Thu, 2009-08-13 at 14:15 -0400, Chuck Lever wrote:
>> On Aug 13, 2009, at 2:10 PM, Joe Perches wrote:
>>> The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
>>> with ipv4 last u32.
>>
>> Why do these need to be separate?
>
> Just an option.
> I think it possible somebody will want "1::" instead of "1::0.0.0.0"

Hrm.

Do you have a use case?  Really, it's pretty easy to tell when the  
mapped v4 presentation format should be used.  See  
ipv6_addr_v4mapped().  Otherwise the mapped v4 presentation format  
should never be used.

A problem with the existing %p[iI] implementation is that each call  
site has to have logic that figures out the address family of the  
address before calling sprintf().  This makes it difficult to use this  
facility with, for example, debugging messages, since you have to add  
address family detection logic at every debugging message call site.   
Lots of clutter and duplicated code.

With %p6ic4, each call site now has to see that it's an IPv6 address,  
and then decide if the address is a mapped v4 address or not.  It's  
the same logic everywhere.

It seems to me it would be a lot more useful if we had a new %p6  
formatter that handled all types of IPv6 addresses properly, the way  
inet_ntop(3) does in user space.  (Or even a new formatter that could  
handle both address families).

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Thu, 2009-08-13 at 14:39 -0400, Chuck Lever wrote:
> On Aug 13, 2009, at 2:21 PM, Joe Perches wrote:
> > On Thu, 2009-08-13 at 14:15 -0400, Chuck Lever wrote:
> >> On Aug 13, 2009, at 2:10 PM, Joe Perches wrote:
> >>> The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
> >>> with ipv4 last u32.
> >> Why do these need to be separate?
> > Just an option.
> > I think it possible somebody will want "1::" instead of "1::0.0.0.0"
> Hrm.
> With %p6ic4, each call site now has to see that it's an IPv6 address,  
> and then decide if the address is a mapped v4 address or not.  It's  
> the same logic everywhere.

I suppose ipv6_addr_v4mapped(addr) could be tested instead

Perhaps this on top of last:

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd02842..7ce34a7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -25,6 +25,7 @@
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
+#include <net/ipv6.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -701,7 +702,7 @@ static char *ip6_compressed_string(char *buf, char *end, u8 *addr,
 	u16 *addr16 = (u16 *)addr;
 	ip6_colon_t colon = DC_START;
 
-	if (fmt[3] == '4') {		/* use :: and ipv4 */
+	if (ipv6_addr_v4mapped((const struct in6_addr *)addr)) {
 		for (i = 0; i < 6; i++) {
 			p = ip6_compress_u16(p, addr16[i], addr16[i+1],
 					     &colon, &needcolon);
@@ -832,8 +833,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 					 * Formatted IP supported
 					 * 4:	001.002.003.004
 					 * 6:	0001:0203:...:0708
-					 * 6c:	1::708
-					 * 6c4:	1::1.2.3.4
+					 * 6c:	1::708 or 1::1.2.3.4
 					 */
 	case 'I':			/* Contiguous: */
 		if (fmt[1] == '4' || fmt[1] == '6')

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Brian Haley]

Joe Perches wrote:
> On Thu, 2009-08-13 at 12:27 -0400, Brian Haley wrote:
>> Jens Rosenboom wrote:
>>> Here is a new version that also
>>> fixes
>>>
>>> - Leave %pi6 alone
>>> - Don't compress a single :0:
>>> - Do output 0
>>> The results and also the remaining issues can be seen with the attached
>>> test program, that also exposes a bug in glibc for v4-mapped addresses
>>> from 0/16.
>>> To fully conform to the cited draft, we would still have to implement
>>> v4-mapped and also check whether a second run of zeros would be longer
>>> than the first one, although the draft also suggests that operators
>>> should avoid using this kind of addresses, so maybe this second issue
>>> can be neglected.
>> Yes, the "compress the most zeros" would be harder, and require two
>> passes over the address.  I had to cut corners somewhere :)
> 
> 2 things.
> 
> First a question, then a compilable but untested patch.
> 
> The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
> with ipv4 last u32.
> 
> Can somebody tell me what I'm doing wrong when I link Jens' test?
> 
> cc -o test test_ipv6.c lib/vsprintf.o lib/ctype.o
> lib/vsprintf.o: In function `global constructors keyed to
> 65535_0_simple_strtoul':
> /home/joe/linux/linux-2.6/lib/vsprintf.c:1972: undefined reference to
> `__gcov_init'
> lib/vsprintf.o:(.data+0x28): undefined reference to `__gcov_merge_add'
> collect2: ld returned 1 exit status

Is your arch "um"?  Seems like those are only defined there, I'm building
a straight x86 kernel.

> Now for the patch.  Perhaps something like this (compiled, untested)

This core dumps when running "test", I'm still trying to track down why.

I think we're thinking too hard about this, I would think we'd always
want to print the shortened IPv6 address in debugging messages with %pI6.
The %pi6 places need to stay since they're an API to userspace.  I don't
think we need the extra "c" and "c4" support.

One comment on a quick scan of the code:

>  static char *ip6_addr_string(char *buf, char *end, u8 *addr,
> -				struct printf_spec spec)
> +			     struct printf_spec spec, const char *fmt)
>  {
> -	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
> +	char ip6_addr[7 * 4 + 7 + 4 * 4]; /* (7 * 4 hex digits) + 7 colons +
> +					   * ipv4 address, and trailing zero */

ip6_addr[8 * 5] is fine here, we won't ever have all eight plus an IPv4 address.

-Brian

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Brian Haley]

Joe Perches wrote:
> I suppose ipv6_addr_v4mapped(addr) could be tested instead
> 
> Perhaps this on top of last:
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index dd02842..7ce34a7 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -25,6 +25,7 @@
>  #include <linux/kallsyms.h>
>  #include <linux/uaccess.h>
>  #include <linux/ioport.h>
> +#include <net/ipv6.h>

I was trying to avoid this by open-coding the v4mapped check, didn't
know if IPv6-specific headers should be pulled-into this generic
code, even though we're printing IPv6 addresses.

-Brian

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Brian Haley]

Chuck Lever wrote:
> On Aug 13, 2009, at 2:21 PM, Joe Perches wrote:
>> On Thu, 2009-08-13 at 14:15 -0400, Chuck Lever wrote:
>>> On Aug 13, 2009, at 2:10 PM, Joe Perches wrote:
>>>> The patch allows "%p6ic" for compressed and "%p6ic4" for compressed
>>>> with ipv4 last u32.
>>>
>>> Why do these need to be separate?
>>
>> Just an option.
>> I think it possible somebody will want "1::" instead of "1::0.0.0.0"
> 
> Hrm.
> 
> Do you have a use case?  Really, it's pretty easy to tell when the
> mapped v4 presentation format should be used.  See
> ipv6_addr_v4mapped().  Otherwise the mapped v4 presentation format
> should never be used.
> 
> A problem with the existing %p[iI] implementation is that each call site
> has to have logic that figures out the address family of the address
> before calling sprintf().  This makes it difficult to use this facility
> with, for example, debugging messages, since you have to add address
> family detection logic at every debugging message call site.  Lots of
> clutter and duplicated code.
> 
> With %p6ic4, each call site now has to see that it's an IPv6 address,
> and then decide if the address is a mapped v4 address or not.  It's the
> same logic everywhere.
> 
> It seems to me it would be a lot more useful if we had a new %p6
> formatter that handled all types of IPv6 addresses properly, the way
> inet_ntop(3) does in user space.  (Or even a new formatter that could
> handle both address families).

I would agree that this could be better, maybe after playing with this
some more it will be obvious what that something is.  I'd be willing
to review any thoughts you have :)

-Brian

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Thu, 2009-08-13 at 16:24 -0400, Brian Haley wrote:
> Is your arch "um"?  Seems like those are only defined there, I'm building
> a straight x86 kernel.

Nope.

I did make allyesconfig ; make lib/vsprintf.o lib ctype.o
allnoconfig works though.

> This core dumps when running "test", I'm still trying to track down why.

missing return on ip_addr_string

> I think we're thinking too hard about this, I would think we'd always
> want to print the shortened IPv6 address in debugging messages with %pI6.

True, but you can't tell in sprintf as it's used in seq.

for instance:
net/sunrpc/svcauth_unix.c:		seq_printf(m, "%s %pI6 %s\n", im->m_class, &addr, dom);

> The %pi6 places need to stay since they're an API to userspace.  I don't
> think we need the extra "c" and "c4" support.

I'm pretty sure it can't change and a new form is needed
so %pi6c should be OK.

I'd rather not use another %p<foo> letter.

> One comment on a quick scan of the code:
> ip6_addr[8 * 5] is fine here, we won't ever have all eight plus an IPv4 address.

I'm fixing it up and will resubmit something working in a little while.

cheers, Joe

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Chuck Lever]

On Aug 13, 2009, at 4:34 PM, Joe Perches wrote:
> On Thu, 2009-08-13 at 16:24 -0400, Brian Haley wrote:
>> Is your arch "um"?  Seems like those are only defined there, I'm  
>> building
>> a straight x86 kernel.
>
> Nope.
>
> I did make allyesconfig ; make lib/vsprintf.o lib ctype.o
> allnoconfig works though.
>
>> This core dumps when running "test", I'm still trying to track down  
>> why.
>
> missing return on ip_addr_string
>
>> I think we're thinking too hard about this, I would think we'd always
>> want to print the shortened IPv6 address in debugging messages with  
>> %pI6.
>
> True, but you can't tell in sprintf as it's used in seq.
>
> for instance:
> net/sunrpc/svcauth_unix.c:		seq_printf(m, "%s %pI6 %s\n", im- 
> >m_class, &addr, dom);

This one might be a bad example.  RPC IPv6 support, especially server  
side, isn't written in stone yet.  User space may not even be ready  
for an IPv6 address here; I can check.  If user space happens to be  
flexible here, then it won't matter if this particular instance is  
shorthanded or not.

[ I would think user space in general should be using inet_pton(3)  
everywhere for such interfaces, so the format of these addresses  
wouldn't matter so much.  Probably impossible at this point. ]

>> The %pi6 places need to stay since they're an API to userspace.  I  
>> don't
>> think we need the extra "c" and "c4" support.
>
> I'm pretty sure it can't change and a new form is needed
> so %pi6c should be OK.
>
> I'd rather not use another %p<foo> letter.

I'm not arguing one way or the other, but it would be useful if  
someone could check exactly what the dependencies are right now.  It  
seems like we're speculating a bit.

>> One comment on a quick scan of the code:
>> ip6_addr[8 * 5] is fine here, we won't ever have all eight plus an  
>> IPv4 address.
>
> I'm fixing it up and will resubmit something working in a little  
> while.
>
> cheers, Joe

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Joe Perches]

On Thu, 2009-08-13 at 17:02 -0400, Chuck Lever wrote:
> On Aug 13, 2009, at 4:34 PM, Joe Perches wrote:
> > net/sunrpc/svcauth_unix.c:		seq_printf(m, "%s %pI6 %s\n", im- 
> > >m_class, &addr, dom);
> 
> This one might be a bad example.

There are 9 of them in net

$ grep -rP --include=*.[ch] "%pI6" net | grep seq_ 
net/sctp/ipv6.c:	seq_printf(seq, "%pI6 ", &addr->v6.sin6_addr);
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c:	return seq_printf(s, "src=%pI6 dst=%pI6 ",
net/ipv6/ip6mr.c:		seq_printf(seq, "%pI6 %pI6 %-3hd",
net/netfilter/xt_hashlimit.c:		return seq_printf(s, "%ld %pI6:%u->%pI6:%u %u %u %u\n",
net/netfilter/xt_recent.c:		seq_printf(seq, "src=%pI6 ttl: %u last_seen: %lu oldest_pkt: %u",
net/netfilter/ipvs/ip_vs_ctl.c:				seq_printf(seq, "%s  [%pI6]:%04X %s ",
net/netfilter/ipvs/ip_vs_conn.c:			seq_printf(seq, "%-3s %pI6 %04X %pI6 %04X %pI6 %04X %-11s %7lu\n",
net/netfilter/ipvs/ip_vs_conn.c:			seq_printf(seq, "%-3s %pI6 %04X %pI6 %04X %pI6 %04X %-11s %-6s %7lu\n",
net/sunrpc/svcauth_unix.c:		seq_printf(m, "%s %pI6 %s\n", im->m_class, &addr, dom);

> [ I would think user space in general should be using inet_pton(3)  
> everywhere for such interfaces, so the format of these addresses  
> wouldn't matter so much.  Probably impossible at this point. ]

David Miller is authoritative here.

> I'm not arguing one way or the other, but it would be useful if  
> someone could check exactly what the dependencies are right now.  It  
> seems like we're speculating a bit.

cheers, Joe

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Thu, 13 Aug 2009 14:13:42 -0700

> On Thu, 2009-08-13 at 17:02 -0400, Chuck Lever wrote:
>> [ I would think user space in general should be using inet_pton(3)  
>> everywhere for such interfaces, so the format of these addresses  
>> wouldn't matter so much.  Probably impossible at this point. ]
> 
> David Miller is authoritative here.

In the final analysis, the risk is just too high to break
userspace.  So let's play conservative here and not change
the output for currently user visible stuff.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Jens Rosenboom]

On Thu, 2009-08-13 at 16:31 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Thu, 13 Aug 2009 14:13:42 -0700
> 
> > On Thu, 2009-08-13 at 17:02 -0400, Chuck Lever wrote:
> >> [ I would think user space in general should be using inet_pton(3)  
> >> everywhere for such interfaces, so the format of these addresses  
> >> wouldn't matter so much.  Probably impossible at this point. ]
> > 
> > David Miller is authoritative here.
> 
> In the final analysis, the risk is just too high to break
> userspace.  So let's play conservative here and not change
> the output for currently user visible stuff.

So just to clarify, do you want us to drop the whole thread and stay
with the clumsy output, or would you be o.k. with adding a new 
%p{something} and use that for kernel messages and maybe do some slow
migration of other stuff where possible?

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[David Miller]

From: Jens Rosenboom <jens@mcbone.net>
Date: Fri, 14 Aug 2009 08:22:05 +0200

> On Thu, 2009-08-13 at 16:31 -0700, David Miller wrote:
>> From: Joe Perches <joe@perches.com>
>> Date: Thu, 13 Aug 2009 14:13:42 -0700
>> 
>> > On Thu, 2009-08-13 at 17:02 -0400, Chuck Lever wrote:
>> >> [ I would think user space in general should be using inet_pton(3)  
>> >> everywhere for such interfaces, so the format of these addresses  
>> >> wouldn't matter so much.  Probably impossible at this point. ]
>> > 
>> > David Miller is authoritative here.
>> 
>> In the final analysis, the risk is just too high to break
>> userspace.  So let's play conservative here and not change
>> the output for currently user visible stuff.
> 
> So just to clarify, do you want us to drop the whole thread and stay
> with the clumsy output, or would you be o.k. with adding a new 
> %p{something} and use that for kernel messages and maybe do some slow
> migration of other stuff where possible?

You tell me what part of this you don't understand:

	So let's play conservative here and not change
	the output for currently user visible stuff.

I can't figure out a way to express that more clearly than I did.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Jens Rosenboom]

On Fri, 2009-08-14 at 00:15 -0700, David Miller wrote:
> From: Jens Rosenboom <jens@mcbone.net>
> Date: Fri, 14 Aug 2009 08:22:05 +0200
> 
> > On Thu, 2009-08-13 at 16:31 -0700, David Miller wrote:
> >> From: Joe Perches <joe@perches.com>
> >> Date: Thu, 13 Aug 2009 14:13:42 -0700
> >> 
> >> > On Thu, 2009-08-13 at 17:02 -0400, Chuck Lever wrote:
> >> >> [ I would think user space in general should be using inet_pton(3)  
> >> >> everywhere for such interfaces, so the format of these addresses  
> >> >> wouldn't matter so much.  Probably impossible at this point. ]
> >> > 
> >> > David Miller is authoritative here.
> >> 
> >> In the final analysis, the risk is just too high to break
> >> userspace.  So let's play conservative here and not change
> >> the output for currently user visible stuff.
> > 
> > So just to clarify, do you want us to drop the whole thread and stay
> > with the clumsy output, or would you be o.k. with adding a new 
> > %p{something} and use that for kernel messages and maybe do some slow
> > migration of other stuff where possible?
> 
> You tell me what part of this you don't understand:
> 
> 	So let's play conservative here and not change
> 	the output for currently user visible stuff.
> 
> I can't figure out a way to express that more clearly than I did.

I wasn't sure whether "currently user visible stuff" would mean "user
space interfaces" like sys/proc-fs, which the first quoted post asked
about, or also kernel messages.

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[Chuck Lever]

On Aug 13, 2009, at 5:13 PM, Joe Perches wrote:
> On Thu, 2009-08-13 at 17:02 -0400, Chuck Lever wrote:
>> On Aug 13, 2009, at 4:34 PM, Joe Perches wrote:
>>> net/sunrpc/svcauth_unix.c:		seq_printf(m, "%s %pI6 %s\n", im-
>>>> m_class, &addr, dom);
>>
>> This one might be a bad example.
>
> There are 9 of them in net
>
> $ grep -rP --include=*.[ch] "%pI6" net | grep seq_
> net/sctp/ipv6.c:	seq_printf(seq, "%pI6 ", &addr->v6.sin6_addr);
> net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c:	return seq_printf(s,  
> "src=%pI6 dst=%pI6 ",
> net/ipv6/ip6mr.c:		seq_printf(seq, "%pI6 %pI6 %-3hd",
> net/netfilter/xt_hashlimit.c:		return seq_printf(s, "%ld %pI6:%u-> 
> %pI6:%u %u %u %u\n",
> net/netfilter/xt_recent.c:		seq_printf(seq, "src=%pI6 ttl: %u  
> last_seen: %lu oldest_pkt: %u",
> net/netfilter/ipvs/ip_vs_ctl.c:				seq_printf(seq, "%s  [%pI6]:%04X  
> %s ",
> net/netfilter/ipvs/ip_vs_conn.c:			seq_printf(seq, "%-3s %pI6 %04X  
> %pI6 %04X %pI6 %04X %-11s %7lu\n",
> net/netfilter/ipvs/ip_vs_conn.c:			seq_printf(seq, "%-3s %pI6 %04X  
> %pI6 %04X %pI6 %04X %-11s %-6s %7lu\n",
> net/sunrpc/svcauth_unix.c:		seq_printf(m, "%s %pI6 %s\n", im- 
> >m_class, &addr, dom);

I checked with the NFSD maintainer.  He thinks this last one is for  
debugging.  It's hard to tell just by looking whether a seq_printf()  
call site actually has a strong user space dependence.

>> [ I would think user space in general should be using inet_pton(3)
>> everywhere for such interfaces, so the format of these addresses
>> wouldn't matter so much.  Probably impossible at this point. ]
>
> David Miller is authoritative here.

I've seen enough to agree that a new formatter is a reasonable  
approach.  Thanks for checking.

>> I'm not arguing one way or the other, but it would be useful if
>> someone could check exactly what the dependencies are right now.  It
>> seems like we're speculating a bit.
>
> cheers, Joe

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC] ipv6: Change %pI6 format to output compacted addresses?

[David Miller]

From: Jens Rosenboom <jens@mcbone.net>
Date: Fri, 14 Aug 2009 10:15:39 +0200

> I wasn't sure whether "currently user visible stuff" would mean "user
> space interfaces" like sys/proc-fs, which the first quoted post asked
> about, or also kernel messages.

I'd say that kernel log messages are OK to tinker with, whereas procfs
and sysfs file contents are not.

[PATCH] lib/vsprintf.c: Add "%pI6c" - print pointer as compressed ipv6 address

[Joe Perches]

On Fri, 2009-08-14 at 13:12 -0700, David Miller wrote:
> I'd say that kernel log messages are OK to tinker with, whereas procfs
> and sysfs file contents are not.

Here's a patch to start that tinkering with log messages

Add functions to format and print a compressed ipv6 address
Does longest 0 match "::" compression
Added an #include <net/addrconf.h>
Changed currently unused "%pi4" to use leading 0s (001.002.003.004)
Changed code to not modify "spec"

'I' [46] for IPv4/IPv6 addresses printed in the usual way
    IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
    IPv6 uses colon separated network-order 16 bit hex with leading 0's
'i' [46] for 'raw' IPv4/IPv6 addresses
    IPv6 omits the colons (01020304...0f)
    IPv4 uses dot-separated decimal with leading 0's (010.123.045.006)
'I6c' for IPv6 addresses printed as specified by
    http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt

Signed-off-by: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c |  205 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 158 insertions(+), 47 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 756ccaf..9b79536 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -25,6 +25,7 @@
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
+#include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -630,60 +631,162 @@ static char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static char *mac_address_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+				struct printf_spec spec, const char *fmt)
 {
-	char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing zero */
+	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
 	char *p = mac_addr;
 	int i;
 
 	for (i = 0; i < 6; i++) {
 		p = pack_hex_byte(p, addr[i]);
-		if (!(spec.flags & SPECIAL) && i != 5)
+		if (fmt[0] == 'M' && i != 5)
 			*p++ = ':';
 	}
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
 
 	return string(buf, end, mac_addr, spec);
 }
 
-static char *ip6_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+static char *ip4_string(char *p, const u8 *addr, bool leading_zeros)
 {
-	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
-	char *p = ip6_addr;
 	int i;
 
+	for (i = 0; i < 4; i++) {
+		char temp[3];	/* hold each IP quad in reverse order */
+		int digits = put_dec_trunc(temp, addr[i]) - temp;
+		if (leading_zeros) {
+			if (digits < 3)
+				*p++ = '0';
+			if (digits < 2)
+				*p++ = '0';
+		}
+		/* reverse the digits in the quad */
+		while (digits--)
+			*p++ = temp[digits];
+		if (i < 3)
+			*p++ = '.';
+	}
+
+	*p = '\0';
+	return p;
+}
+
+static char *ip6_compressed_string(char *p, const struct in6_addr *addr)
+{
+	int i;
+	int j;
+	int range;
+	unsigned char zerolength[8];
+	int longest = 0;
+	int colonpos = -1;
+	u16 word;
+	u8 hi;
+	u8 lo;
+	bool printhi;
+	bool needcolon = false;
+	bool useIPv4 = ipv6_addr_v4mapped(addr) || ipv6_addr_is_isatap(addr);
+
+	memset(zerolength, 0, sizeof(zerolength));
+
+	if (useIPv4)
+		range = 6;
+	else
+		range = 8;
+
+	/* find position of longest 0 run */
+	for (i = 0; i < range; i++) {
+		for (j = i; j < range; j++) {
+			if (addr->s6_addr16[j] != 0)
+				break;
+			zerolength[i]++;
+		}
+	}
+	for (i = 0; i < range; i++) {
+		if (zerolength[i] > longest) {
+			longest = zerolength[i];
+			colonpos = i;
+		}
+	}
+	if (colonpos != -1 && zerolength[colonpos] < 2)
+		colonpos = -1;
+
+	for (i = 0; i < range; i++) {
+		if (i == colonpos) {
+			if (needcolon || i == 0)
+				*p++ = ':';
+			*p++ = ':';
+			needcolon = false;
+			while (i + 1 < range && addr->s6_addr16[i + 1] == 0) {
+				i++;	/* skip successive 0s */
+			}
+			continue;
+		}
+		if (needcolon) {
+			*p++ = ':';
+			needcolon = false;
+		}
+		/* hex u16 without leading 0s */
+		word = ntohs(addr->s6_addr16[i]);
+		hi = word >> 8;
+		lo = word & 0xff;
+		printhi = false;
+		if (hi) {
+			if (hi > 0x0f)
+				p = pack_hex_byte(p, hi);
+			else
+				*p++ = hex_asc_lo(hi);
+			printhi = true;
+		}
+		if (printhi || lo > 0x0f)
+			p = pack_hex_byte(p, lo);
+		else
+			*p++ = hex_asc_lo(lo);
+		needcolon = true;
+	}
+
+	if (useIPv4) {
+		if (needcolon)
+			*p++ = ':';
+		p = ip4_string(p, &addr->s6_addr[12], false);
+	}
+
+	*p = '\0';
+	return p;
+}
+
+static char *ip6_string(char *p, const struct in6_addr *addr, const char *fmt)
+{
+	int i;
 	for (i = 0; i < 8; i++) {
-		p = pack_hex_byte(p, addr[2 * i]);
-		p = pack_hex_byte(p, addr[2 * i + 1]);
-		if (!(spec.flags & SPECIAL) && i != 7)
+		p = pack_hex_byte(p, addr->s6_addr[2 * i]);
+		p = pack_hex_byte(p, addr->s6_addr[2 * i + 1]);
+		if (fmt[0] == 'I' && i != 7)
 			*p++ = ':';
 	}
+
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
+	return p;
+}
+
+static char *ip6_addr_string(char *buf, char *end, const u8 *addr,
+			     struct printf_spec spec, const char *fmt)
+{
+	char ip6_addr[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255")];
+
+	if (fmt[0] == 'I' && fmt[2] == 'c')
+		ip6_compressed_string(ip6_addr, (const struct in6_addr *)addr);
+	else
+		ip6_string(ip6_addr, (const struct in6_addr *)addr, fmt);
 
 	return string(buf, end, ip6_addr, spec);
 }
 
-static char *ip4_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
+			     struct printf_spec spec, const char *fmt)
 {
-	char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and trailing zero */
-	char temp[3];	/* hold each IP quad in reverse order */
-	char *p = ip4_addr;
-	int i, digits;
+	char ip4_addr[sizeof("255.255.255.255")];
 
-	for (i = 0; i < 4; i++) {
-		digits = put_dec_trunc(temp, addr[i]) - temp;
-		/* reverse the digits in the quad */
-		while (digits--)
-			*p++ = temp[digits];
-		if (i != 3)
-			*p++ = '.';
-	}
-	*p = '\0';
-	spec.flags &= ~SPECIAL;
+	ip4_string(ip4_addr, addr, fmt[0] == 'i');
 
 	return string(buf, end, ip4_addr, spec);
 }
@@ -702,11 +805,15 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr,
  *       addresses (not the name nor the flags)
  * - 'M' For a 6-byte MAC address, it prints the address in the
  *       usual colon-separated hex notation
- * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way (dot-separated
- *       decimal for v4 and colon separated network-order 16 bit hex for v6)
- * - 'i' [46] for 'raw' IPv4/IPv6 addresses, IPv6 omits the colons, IPv4 is
- *       currently the same
- *
+ * - 'm' For a 6-byte MAC address, it prints the hex address without colons
+ * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
+ *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
+ *       IPv6 uses colon separated network-order 16 bit hex with leading 0's
+ * - 'i' [46] for 'raw' IPv4/IPv6 addresses
+ *       IPv6 omits the colons (01020304...0f)
+ *       IPv4 uses dot-separated decimal with leading 0's (010.123.045.006)
+ * - 'I6c' for IPv6 addresses printed as specified by
+ *       http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
@@ -726,20 +833,24 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return symbol_string(buf, end, ptr, spec, *fmt);
 	case 'R':
 		return resource_string(buf, end, ptr, spec);
-	case 'm':
-		spec.flags |= SPECIAL;
-		/* Fallthrough */
-	case 'M':
-		return mac_address_string(buf, end, ptr, spec);
-	case 'i':
-		spec.flags |= SPECIAL;
-		/* Fallthrough */
-	case 'I':
-		if (fmt[1] == '6')
-			return ip6_addr_string(buf, end, ptr, spec);
-		if (fmt[1] == '4')
-			return ip4_addr_string(buf, end, ptr, spec);
-		spec.flags &= ~SPECIAL;
+	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
+	case 'm':			/* Contiguous: 000102030405 */
+		return mac_address_string(buf, end, ptr, spec, fmt);
+	case 'I':			/* Formatted IP supported
+					 * 4:	1.2.3.4
+					 * 6:	0001:0203:...:0708
+					 * 6c:	1::708 or 1::1.2.3.4
+					 */
+	case 'i':			/* Contiguous:
+					 * 4:	001.002.003.004
+					 * 6:   000102...0f
+					 */
+		switch (fmt[1]) {
+		case '6':
+			return ip6_addr_string(buf, end, ptr, spec, fmt);
+		case '4':
+			return ip4_addr_string(buf, end, ptr, spec, fmt);
+		}
 		break;
 	}
 	spec.flags |= SMALL;
-- 
1.6.3.1.10.g659a0.dirty

[RFC PATCH] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Joe Perches]

Hi Chuck.

Here's a tentative patch that adds that facility you wanted in
this thread.
http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684

This patch is on top of this patch:
http://marc.info/?l=linux-netdev&m=125034992003220&w=2

I'm not sure it's great or even useful, but just for discussion.

Use style:
* - 'N' For network socket (sockaddr) pointers
*       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
*       May be used with any combination of additional specifiers below
*       'p' decimal socket port number for IPv[46]: ":12345"
*       'f' decimal flowinfo for IPv6: "/123456789"
*       's' decimal scope_id number for IPv6: "%1234567890"
*       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6: "1::c0a8:a:1234"

I think using ":" as the separator for the port number, while common,
and already frequently used in kernel source (see bullet 2 in):
http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
"Section 6: Notes on Combining IPv6 Addresses with Port Numbers".
is not good for readability.

Perhaps this style should be changed to the "[ipv6]:port" described
in the draft above.

cheers, Joe

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9b79536..b3cbc38 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -791,6 +791,90 @@ static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, ip4_addr, spec);
 }
 
+static char *u32_dec_val(char *p, u32 val)
+{
+	char temp[9];
+	int digits;
+	u32 hi_val = val / 100000;
+	char *pos;
+	pos = put_dec_trunc(temp, val%100000);
+	if (hi_val)
+		pos = put_dec_trunc(pos, hi_val);
+	digits = pos - temp;
+	/* reverse the digits in temp */
+	while (digits--)
+		*p++ = temp[digits];
+	return p;
+}
+
+static char *socket_addr_string(char *buf, char *end,
+				const struct sockaddr *sa,
+				struct printf_spec spec, const char *fmt)
+{
+	char addr[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255") +
+		  sizeof(":12345") +
+		  sizeof("%1234567890") +
+		  sizeof("/123456789")];
+	char *p;
+	struct sockaddr_in *sa4 = (struct sockaddr_in *)sa;
+	struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *)sa;
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		p = ip4_string(addr, (const u8 *)&sa4->sin_addr.s_addr, false);
+		break;
+	case AF_INET6:
+		p = ip6_compressed_string(addr, &sa6->sin6_addr);
+		break;
+	default: {
+		struct printf_spec num_spec = {
+			.base = 16,
+			.precision = -1,
+			.field_width = 2 * sizeof(void *),
+			.flags = SPECIAL | SMALL | ZEROPAD,
+		};
+
+		p = strcpy(addr, "Bad socket address: ")
+			+ sizeof("Bad socket address: ");
+		p = number(p, addr + sizeof(addr), (unsigned long)sa, num_spec);
+		break;
+	}
+	}
+
+	while (isalpha(*++fmt)) {
+		switch (*fmt) {
+		case 'p':
+			*p++ = ':';
+			switch (sa->sa_family) {
+			case AF_INET:
+				p = u32_dec_val(p,ntohs(sa4->sin_port));
+				break;
+			case AF_INET6:
+				p = u32_dec_val(p,ntohs(sa6->sin6_port));
+				break;
+			}
+			break;
+		case 's':
+			*p++ = '%';
+			switch (sa->sa_family) {
+			case AF_INET6:
+				p = u32_dec_val(p, sa6->sin6_scope_id);
+			}
+			break;
+		case 'f':
+			*p++ = '/';
+			switch (sa->sa_family) {
+			case AF_INET6:
+				p = u32_dec_val(p, ntohl(sa6->sin6_flowinfo &
+							 IPV6_FLOWINFO_MASK));
+			}
+			break;
+		}
+	}
+	*p = '\0';
+	return string(buf, end, addr, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -814,6 +898,13 @@ static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
  *       IPv4 uses dot-separated decimal with leading 0's (010.123.045.006)
  * - 'I6c' for IPv6 addresses printed as specified by
  *       http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
+ * - 'N' For network socket (sockaddr) pointers
+ *       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
+ *       May be used with any combination of additional specifiers below
+ *       'p' decimal socket port number for IPv[46]: ":12345"
+ *       'f' decimal flowinfo for IPv6: "/123456789"
+ *       's' decimal scope_id number for IPv6: "%1234567890"
+ *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6: "1::c0a8:a:1234"
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
@@ -852,7 +943,10 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return ip4_addr_string(buf, end, ptr, spec, fmt);
 		}
 		break;
+	case 'N':
+		return socket_addr_string(buf, end, ptr, spec, fmt);
 	}
+
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
 		spec.field_width = 2*sizeof(void *);

Re: [PATCH] lib/vsprintf.c: Add "%pI6c" - print pointer as compressed ipv6 address

[Jens Rosenboom]

On Sat, 2009-08-15 at 08:24 -0700, Joe Perches wrote:
> On Fri, 2009-08-14 at 13:12 -0700, David Miller wrote:
> > I'd say that kernel log messages are OK to tinker with, whereas procfs
> > and sysfs file contents are not.
> 
> Here's a patch to start that tinkering with log messages

Two small optimizations:

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9b79536..a80ef3d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -677,12 +677,11 @@ static char *ip6_compressed_string(char *p, const
struct in6_addr *addr)
 	int j;
 	int range;
 	unsigned char zerolength[8];
-	int longest = 0;
+	int longest = 1;
 	int colonpos = -1;
 	u16 word;
 	u8 hi;
 	u8 lo;
-	bool printhi;
 	bool needcolon = false;
 	bool useIPv4 = ipv6_addr_v4mapped(addr) || ipv6_addr_is_isatap(addr);
 
@@ -707,8 +706,6 @@ static char *ip6_compressed_string(char *p, const
struct in6_addr *addr)
 			colonpos = i;
 		}
 	}
-	if (colonpos != -1 && zerolength[colonpos] < 2)
-		colonpos = -1;
 
 	for (i = 0; i < range; i++) {
 		if (i == colonpos) {
@@ -729,15 +726,13 @@ static char *ip6_compressed_string(char *p, const
struct in6_addr *addr)
 		word = ntohs(addr->s6_addr16[i]);
 		hi = word >> 8;
 		lo = word & 0xff;
-		printhi = false;
 		if (hi) {
 			if (hi > 0x0f)
 				p = pack_hex_byte(p, hi);
 			else
 				*p++ = hex_asc_lo(hi);
-			printhi = true;
 		}
-		if (printhi || lo > 0x0f)
+		if (hi || lo > 0x0f)
 			p = pack_hex_byte(p, lo);
 		else
 			*p++ = hex_asc_lo(lo);

Also I'm wondering whether it makes sense to pull the format code
checking into all the sub-routines. It might be easier to maintain if it
is all kept together in pointer().

[PATCH V2] lib/vsprintf.c: Add "%pI6c" - print pointer as compressed ipv6 address

[Joe Perches]

On Mon, 2009-08-17 at 17:18 +0200, Jens Rosenboom wrote: 
> Two small optimizations:

Thanks.  Another is to use the calculated "longest" to
increment i after the :: instead of counting 0's again.

Another possibility might be to make all the ip formatting
in vsprintf #ifdef'd on CONFIG_NET.

> Also I'm wondering whether it makes sense to pull the format code
> checking into all the sub-routines. 

It isn't clear to me what you're asking for.

> It might be easier to maintain if it 
> is all kept together in pointer().

Can you explain more thoroughly please?

If you mean not passing const char *fmt to the sub-routines,
I think not doing so makes it harder to maintain and extend.

I think it will be useful to extend the 'S' resource_string
capability to use fmt to allow specific bits to be selected
of the resource identifier to be printed.

See the idea in:  http://lkml.org/lkml/2009/4/17/105

Here's the modified patch with your suggestions:

cheers,

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 756ccaf..cb8a112 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -25,6 +25,7 @@
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
+#include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -630,60 +631,156 @@ static char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static char *mac_address_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+				struct printf_spec spec, const char *fmt)
 {
-	char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing zero */
+	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
 	char *p = mac_addr;
 	int i;
 
 	for (i = 0; i < 6; i++) {
 		p = pack_hex_byte(p, addr[i]);
-		if (!(spec.flags & SPECIAL) && i != 5)
+		if (fmt[0] == 'M' && i != 5)
 			*p++ = ':';
 	}
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
 
 	return string(buf, end, mac_addr, spec);
 }
 
-static char *ip6_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+static char *ip4_string(char *p, const u8 *addr, bool leading_zeros)
+{
+	int i;
+
+	for (i = 0; i < 4; i++) {
+		char temp[3];	/* hold each IP quad in reverse order */
+		int digits = put_dec_trunc(temp, addr[i]) - temp;
+		if (leading_zeros) {
+			if (digits < 3)
+				*p++ = '0';
+			if (digits < 2)
+				*p++ = '0';
+		}
+		/* reverse the digits in the quad */
+		while (digits--)
+			*p++ = temp[digits];
+		if (i < 3)
+			*p++ = '.';
+	}
+
+	*p = '\0';
+	return p;
+}
+
+static char *ip6_compressed_string(char *p, const struct in6_addr *addr)
 {
-	char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */
-	char *p = ip6_addr;
 	int i;
+	int j;
+	int range;
+	unsigned char zerolength[8];
+	int longest = 1;
+	int colonpos = -1;
+	u16 word;
+	u8 hi;
+	u8 lo;
+	bool needcolon = false;
+	bool useIPv4 = ipv6_addr_v4mapped(addr) || ipv6_addr_is_isatap(addr);
+
+	memset(zerolength, 0, sizeof(zerolength));
+
+	if (useIPv4)
+		range = 6;
+	else
+		range = 8;
+
+	/* find position of longest 0 run */
+	for (i = 0; i < range; i++) {
+		for (j = i; j < range; j++) {
+			if (addr->s6_addr16[j] != 0)
+				break;
+			zerolength[i]++;
+		}
+	}
+	for (i = 0; i < range; i++) {
+		if (zerolength[i] > longest) {
+			longest = zerolength[i];
+			colonpos = i;
+		}
+	}
+
+	/* emit address */
+	for (i = 0; i < range; i++) {
+		if (i == colonpos) {
+			if (needcolon || i == 0)
+				*p++ = ':';
+			*p++ = ':';
+			needcolon = false;
+			i += longest - 1;
+			continue;
+		}
+		if (needcolon) {
+			*p++ = ':';
+			needcolon = false;
+		}
+		/* hex u16 without leading 0s */
+		word = ntohs(addr->s6_addr16[i]);
+		hi = word >> 8;
+		lo = word & 0xff;
+		if (hi) {
+			if (hi > 0x0f)
+				p = pack_hex_byte(p, hi);
+			else
+				*p++ = hex_asc_lo(hi);
+		}
+		if (hi || lo > 0x0f)
+			p = pack_hex_byte(p, lo);
+		else
+			*p++ = hex_asc_lo(lo);
+		needcolon = true;
+	}
+
+	if (useIPv4) {
+		if (needcolon)
+			*p++ = ':';
+		p = ip4_string(p, &addr->s6_addr[12], false);
+	}
 
+	*p = '\0';
+	return p;
+}
+
+static char *ip6_string(char *p, const struct in6_addr *addr, const char *fmt)
+{
+	int i;
 	for (i = 0; i < 8; i++) {
-		p = pack_hex_byte(p, addr[2 * i]);
-		p = pack_hex_byte(p, addr[2 * i + 1]);
-		if (!(spec.flags & SPECIAL) && i != 7)
+		p = pack_hex_byte(p, addr->s6_addr[2 * i]);
+		p = pack_hex_byte(p, addr->s6_addr[2 * i + 1]);
+		if (fmt[0] == 'I' && i != 7)
 			*p++ = ':';
 	}
+
 	*p = '\0';
-	spec.flags &= ~SPECIAL;
+	return p;
+}
+
+static char *ip6_addr_string(char *buf, char *end, const u8 *addr,
+			     struct printf_spec spec, const char *fmt)
+{
+	char ip6_addr[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255")];
+
+	if (fmt[0] == 'I' && fmt[2] == 'c')
+		ip6_compressed_string(ip6_addr, (const struct in6_addr *)addr);
+	else
+		ip6_string(ip6_addr, (const struct in6_addr *)addr, fmt);
 
 	return string(buf, end, ip6_addr, spec);
 }
 
-static char *ip4_addr_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec)
+static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
+			     struct printf_spec spec, const char *fmt)
 {
-	char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and trailing zero */
-	char temp[3];	/* hold each IP quad in reverse order */
-	char *p = ip4_addr;
-	int i, digits;
+	char ip4_addr[sizeof("255.255.255.255")];
 
-	for (i = 0; i < 4; i++) {
-		digits = put_dec_trunc(temp, addr[i]) - temp;
-		/* reverse the digits in the quad */
-		while (digits--)
-			*p++ = temp[digits];
-		if (i != 3)
-			*p++ = '.';
-	}
-	*p = '\0';
-	spec.flags &= ~SPECIAL;
+	ip4_string(ip4_addr, addr, fmt[0] == 'i');
 
 	return string(buf, end, ip4_addr, spec);
 }
@@ -702,11 +799,15 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr,
  *       addresses (not the name nor the flags)
  * - 'M' For a 6-byte MAC address, it prints the address in the
  *       usual colon-separated hex notation
- * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way (dot-separated
- *       decimal for v4 and colon separated network-order 16 bit hex for v6)
- * - 'i' [46] for 'raw' IPv4/IPv6 addresses, IPv6 omits the colons, IPv4 is
- *       currently the same
- *
+ * - 'm' For a 6-byte MAC address, it prints the hex address without colons
+ * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
+ *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
+ *       IPv6 uses colon separated network-order 16 bit hex with leading 0's
+ * - 'i' [46] for 'raw' IPv4/IPv6 addresses
+ *       IPv6 omits the colons (01020304...0f)
+ *       IPv4 uses dot-separated decimal with leading 0's (010.123.045.006)
+ * - 'I6c' for IPv6 addresses printed as specified by
+ *       http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
@@ -726,20 +827,24 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return symbol_string(buf, end, ptr, spec, *fmt);
 	case 'R':
 		return resource_string(buf, end, ptr, spec);
-	case 'm':
-		spec.flags |= SPECIAL;
-		/* Fallthrough */
-	case 'M':
-		return mac_address_string(buf, end, ptr, spec);
-	case 'i':
-		spec.flags |= SPECIAL;
-		/* Fallthrough */
-	case 'I':
-		if (fmt[1] == '6')
-			return ip6_addr_string(buf, end, ptr, spec);
-		if (fmt[1] == '4')
-			return ip4_addr_string(buf, end, ptr, spec);
-		spec.flags &= ~SPECIAL;
+	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
+	case 'm':			/* Contiguous: 000102030405 */
+		return mac_address_string(buf, end, ptr, spec, fmt);
+	case 'I':			/* Formatted IP supported
+					 * 4:	1.2.3.4
+					 * 6:	0001:0203:...:0708
+					 * 6c:	1::708 or 1::1.2.3.4
+					 */
+	case 'i':			/* Contiguous:
+					 * 4:	001.002.003.004
+					 * 6:   000102...0f
+					 */
+		switch (fmt[1]) {
+		case '6':
+			return ip6_addr_string(buf, end, ptr, spec, fmt);
+		case '4':
+			return ip4_addr_string(buf, end, ptr, spec, fmt);
+		}
 		break;
 	}
 	spec.flags |= SMALL;


cheers, Joe

Re: [PATCH V2] lib/vsprintf.c: Add "%pI6c" - print pointer as compressed ipv6 address

[Jens Rosenboom]

On Mon, 2009-08-17 at 15:29 -0700, Joe Perches wrote:
> On Mon, 2009-08-17 at 17:18 +0200, Jens Rosenboom wrote: 
[...]
> > Also I'm wondering whether it makes sense to pull the format code
> > checking into all the sub-routines. 
> 
> It isn't clear to me what you're asking for.
> 
> > It might be easier to maintain if it 
> > is all kept together in pointer().
> 
> Can you explain more thoroughly please?
> 
> If you mean not passing const char *fmt to the sub-routines,
> I think not doing so makes it harder to maintain and extend.
> 
> I think it will be useful to extend the 'S' resource_string
> capability to use fmt to allow specific bits to be selected
> of the resource identifier to be printed.
> 
> See the idea in:  http://lkml.org/lkml/2009/4/17/105

You were right in assuming the intention of my seemingly badly explained
comments, but in this context I agree with you, that it makes sense the
way you did it.

> Here's the modified patch with your suggestions:
> 

Ran it through my test-cases and they all look fine.

Tested-by: Jens Rosenboom <jens@mcbone.net>

Re: [RFC PATCH] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Chuck Lever]

On Aug 16, 2009, at 12:10 AM, Joe Perches wrote:
> Hi Chuck.
>
> Here's a tentative patch that adds that facility you wanted in
> this thread.
> http://kerneltrap.org/mailarchive/linux-netdev/2008/11/25/4231684
>
> This patch is on top of this patch:
> http://marc.info/?l=linux-netdev&m=125034992003220&w=2
>
> I'm not sure it's great or even useful, but just for discussion.

I think having at least a generic socket address formatter would be  
very helpful.  The kernel RPC code can use that immediately for  
generating debugging messages, generating "universal  
addresses" (patches already accepted for 2.6.32) and for generating  
presentation addresses to pass to user space (lockd does this now).

> Use style:
> * - 'N' For network socket (sockaddr) pointers
> *       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
> *       May be used with any combination of additional specifiers  
> below
> *       'p' decimal socket port number for IPv[46]: ":12345"
> *       'f' decimal flowinfo for IPv6: "/123456789"
> *       's' decimal scope_id number for IPv6: "%1234567890"
> *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6:  
> "1::c0a8:a:1234"
>
> I think using ":" as the separator for the port number, while common,
> and already frequently used in kernel source (see bullet 2 in):
> http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
> "Section 6: Notes on Combining IPv6 Addresses with Port Numbers".
> is not good for readability.
>
> Perhaps this style should be changed to the "[ipv6]:port" described
> in the draft above.

I agree that the port number convention is tricky, and some kernel  
areas handle it differently than others. Perhaps having a separate  
family-agnostic formatter for printing the port number (or scope ID,  
etc.) might allow greatest flexibility.

The RPC code, for example, displays port numbers in decimal and in  
"hi.lo" format (the high byte is printed in decimal, then the low byte  
is printed in decimal.  The two values are separated by a dot.  For  
example, port 2049 is displayed as "8.1").

> cheers, Joe
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 9b79536..b3cbc38 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -791,6 +791,90 @@ static char *ip4_addr_string(char *buf, char  
> *end, const u8 *addr,
> 	return string(buf, end, ip4_addr, spec);
> }
>
> +static char *u32_dec_val(char *p, u32 val)
> +{
> +	char temp[9];
> +	int digits;
> +	u32 hi_val = val / 100000;
> +	char *pos;
> +	pos = put_dec_trunc(temp, val%100000);
> +	if (hi_val)
> +		pos = put_dec_trunc(pos, hi_val);
> +	digits = pos - temp;
> +	/* reverse the digits in temp */
> +	while (digits--)
> +		*p++ = temp[digits];
> +	return p;
> +}
> +
> +static char *socket_addr_string(char *buf, char *end,
> +				const struct sockaddr *sa,
> +				struct printf_spec spec, const char *fmt)
> +{
> +	char addr[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255") +
> +		  sizeof(":12345") +
> +		  sizeof("%1234567890") +
> +		  sizeof("/123456789")];
> +	char *p;
> +	struct sockaddr_in *sa4 = (struct sockaddr_in *)sa;
> +	struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *)sa;
> +
> +	switch (sa->sa_family) {
> +	case AF_INET:
> +		p = ip4_string(addr, (const u8 *)&sa4->sin_addr.s_addr, false);
> +		break;
> +	case AF_INET6:
> +		p = ip6_compressed_string(addr, &sa6->sin6_addr);
> +		break;
> +	default: {
> +		struct printf_spec num_spec = {
> +			.base = 16,
> +			.precision = -1,
> +			.field_width = 2 * sizeof(void *),
> +			.flags = SPECIAL | SMALL | ZEROPAD,
> +		};
> +
> +		p = strcpy(addr, "Bad socket address: ")
> +			+ sizeof("Bad socket address: ");
> +		p = number(p, addr + sizeof(addr), (unsigned long)sa, num_spec);
> +		break;
> +	}
> +	}
> +
> +	while (isalpha(*++fmt)) {
> +		switch (*fmt) {
> +		case 'p':
> +			*p++ = ':';
> +			switch (sa->sa_family) {
> +			case AF_INET:
> +				p = u32_dec_val(p,ntohs(sa4->sin_port));
> +				break;
> +			case AF_INET6:
> +				p = u32_dec_val(p,ntohs(sa6->sin6_port));
> +				break;
> +			}
> +			break;
> +		case 's':
> +			*p++ = '%';
> +			switch (sa->sa_family) {
> +			case AF_INET6:
> +				p = u32_dec_val(p, sa6->sin6_scope_id);
> +			}
> +			break;
> +		case 'f':
> +			*p++ = '/';
> +			switch (sa->sa_family) {
> +			case AF_INET6:
> +				p = u32_dec_val(p, ntohl(sa6->sin6_flowinfo &
> +							 IPV6_FLOWINFO_MASK));
> +			}
> +			break;
> +		}
> +	}
> +	*p = '\0';
> +	return string(buf, end, addr, spec);
> +}
> +
> /*
>  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>  * by an extra set of alphanumeric characters that are extended format
> @@ -814,6 +898,13 @@ static char *ip4_addr_string(char *buf, char  
> *end, const u8 *addr,
>  *       IPv4 uses dot-separated decimal with leading 0's  
> (010.123.045.006)
>  * - 'I6c' for IPv6 addresses printed as specified by
>  *       http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
> + * - 'N' For network socket (sockaddr) pointers
> + *       if sa_family is IPv4, output is %pI4; if IPv6, output is  
> %pI6c
> + *       May be used with any combination of additional specifiers  
> below
> + *       'p' decimal socket port number for IPv[46]: ":12345"
> + *       'f' decimal flowinfo for IPv6: "/123456789"
> + *       's' decimal scope_id number for IPv6: "%1234567890"
> + *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6:  
> "1::c0a8:a:1234"
>  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>  * function pointers are really function descriptors, which contain a
>  * pointer to the real address.
> @@ -852,7 +943,10 @@ static char *pointer(const char *fmt, char  
> *buf, char *end, void *ptr,
> 			return ip4_addr_string(buf, end, ptr, spec, fmt);
> 		}
> 		break;
> +	case 'N':
> +		return socket_addr_string(buf, end, ptr, spec, fmt);
> 	}
> +
> 	spec.flags |= SMALL;
> 	if (spec.field_width == -1) {
> 		spec.field_width = 2*sizeof(void *);
>
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

[RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Joe Perches]

On Wed, 2009-08-19 at 10:26 -0400, Chuck Lever wrote:
> On Aug 16, 2009, at 12:10 AM, Joe Perches wrote:

> I think having at least a generic socket address formatter would be  
> very helpful.  The kernel RPC code can use that immediately for  
> generating debugging messages, generating "universal  
> addresses" (patches already accepted for 2.6.32) and for generating  
> presentation addresses to pass to user space (lockd does this now).

> > Use style:
> > * - 'N' For network socket (sockaddr) pointers
> > *       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
> > *       May be used with any combination of additional specifiers  
> > below
> > *       'p' decimal socket port number for IPv[46]: ":12345"
> > *       'f' decimal flowinfo for IPv6: "/123456789"
> > *       's' decimal scope_id number for IPv6: "%1234567890"
> > *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6:  
> > "1::c0a8:a:1234"
> >
> > I think using ":" as the separator for the port number, while common,
> > and already frequently used in kernel source (see bullet 2 in):
> > http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
> > "Section 6: Notes on Combining IPv6 Addresses with Port Numbers".
> > is not good for readability.
> >
> > Perhaps this style should be changed to the "[ipv6]:port" described
> > in the draft above.
> 
> I agree that the port number convention is tricky, and some kernel  
> areas handle it differently than others. Perhaps having a separate  
> family-agnostic formatter for printing the port number (or scope ID,  
> etc.) might allow greatest flexibility.

I'm not too sure there's much value in ever more formatters for
relatively simple things or in printing something like port or
scope without the ip address.

%hu ntohs(sa6->sin6_port)
or
%d or 0x%x ntohl(sa6->sin6_flowinfo)

seem pretty straightforward to me.

> The RPC code, for example, displays port numbers in decimal and in  
> "hi.lo" format (the high byte is printed in decimal, then the low byte  
> is printed in decimal.  The two values are separated by a dot.  For  
> example, port 2049 is displayed as "8.1").

Here's another tentative patch that does both using square
brackets around IPv6 addresses with a port number and
allows selection of the port number style ":dddd" or ":hi.lo"

It also adds a "pI6n" form for IPv6 addresses with 7 colons
and no leading 0's.

lib/vsprintf.c: Add '%pN' print formatted struct sock_addr *
    
Use styles:
* - 'N' For network socket (sockaddr) pointers
*       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
*       May be used with any combination of additional specifiers below
*       'p' decimal socket port number for IPv[46]: ":12345"
*       'P' decimal socket port number for IPv6: ":1.255" (hi.lo)
*           if IPv6 and port number is selected, square brackets
*           will surround the IPv6 address for 'p' and 'P'
*       'f' decimal flowinfo for IPv6: "/123456789"
*       's' decimal scope_id number for IPv6: "%1234567890"
*       %pNp will print IPv4: "1.2.3.4:1234"
*                       IPv6: "[1::c0a8:a]:1234"
*       %pNP will print IPv4: "1.2.3.4:1.255
*                       IPv6: "[1::c0a8:a]:1.255"
    
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index cb8a112..139f310 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -647,22 +647,73 @@ static char *mac_address_string(char *buf, char *end, u8 *addr,
 	return string(buf, end, mac_addr, spec);
 }
 
-static char *ip4_string(char *p, const u8 *addr, bool leading_zeros)
+static char *u8_to_dec(char *p, u8 val, bool leadingzeros)
+{
+	char temp[3];			/* holds val in reverse order */
+	int digits = put_dec_trunc(temp, val) - temp;
+
+	if (leadingzeros) {
+		if (digits < 3)
+			*p++ = '0';
+		if (digits < 2)
+			*p++ = '0';
+	}
+	/* reverse the digits */
+	while (digits--)
+		*p++ = temp[digits];
+
+	return p;
+}
+
+static char *u16_to_hex(char *p, u16 word, bool leadingzeros)
+{
+	u8 hi = word >> 8;
+	u8 lo = word & 0xff;
+	if (leadingzeros) {
+		p = pack_hex_byte(p, hi);
+		p = pack_hex_byte(p, lo);
+	} else {
+		if (hi) {
+			if (hi > 0x0f)
+				p = pack_hex_byte(p, hi);
+			else
+				*p++ = hex_asc_lo(hi);
+		}
+		if (hi || lo > 0x0f)
+			p = pack_hex_byte(p, lo);
+		else
+			*p++ = hex_asc_lo(lo);
+	}
+
+	return p;
+}
+
+static char *u32_dec_val(char *p, u32 val)
+{
+	char temp[9];			/* holds val in reverse order */
+	int digits;
+	char *pos;
+
+	if (val < 100000)
+		pos = put_dec_trunc(temp, val);
+	else {
+		pos = put_dec_trunc(temp, val % 100000);
+		pos = put_dec_trunc(pos, val / 100000);
+	}
+	digits = pos - temp;
+	/* reverse the digits */
+	while (digits--)
+		*p++ = temp[digits];
+
+	return p;
+}
+
+static char *ip4_string(char *p, const u8 *addr, bool leadingzeros)
 {
 	int i;
 
 	for (i = 0; i < 4; i++) {
-		char temp[3];	/* hold each IP quad in reverse order */
-		int digits = put_dec_trunc(temp, addr[i]) - temp;
-		if (leading_zeros) {
-			if (digits < 3)
-				*p++ = '0';
-			if (digits < 2)
-				*p++ = '0';
-		}
-		/* reverse the digits in the quad */
-		while (digits--)
-			*p++ = temp[digits];
+		p = u8_to_dec(p, addr[i], leadingzeros);
 		if (i < 3)
 			*p++ = '.';
 	}
@@ -679,9 +730,6 @@ static char *ip6_compressed_string(char *p, const struct in6_addr *addr)
 	unsigned char zerolength[8];
 	int longest = 1;
 	int colonpos = -1;
-	u16 word;
-	u8 hi;
-	u8 lo;
 	bool needcolon = false;
 	bool useIPv4 = ipv6_addr_v4mapped(addr) || ipv6_addr_is_isatap(addr);
 
@@ -721,20 +769,7 @@ static char *ip6_compressed_string(char *p, const struct in6_addr *addr)
 			*p++ = ':';
 			needcolon = false;
 		}
-		/* hex u16 without leading 0s */
-		word = ntohs(addr->s6_addr16[i]);
-		hi = word >> 8;
-		lo = word & 0xff;
-		if (hi) {
-			if (hi > 0x0f)
-				p = pack_hex_byte(p, hi);
-			else
-				*p++ = hex_asc_lo(hi);
-		}
-		if (hi || lo > 0x0f)
-			p = pack_hex_byte(p, lo);
-		else
-			*p++ = hex_asc_lo(lo);
+		p = u16_to_hex(p, ntohs(addr->s6_addr16[i]), false);
 		needcolon = true;
 	}
 
@@ -751,9 +786,10 @@ static char *ip6_compressed_string(char *p, const struct in6_addr *addr)
 static char *ip6_string(char *p, const struct in6_addr *addr, const char *fmt)
 {
 	int i;
+	bool leadingzeros = !(fmt[0] == 'I' && fmt[2] == 'n');
+
 	for (i = 0; i < 8; i++) {
-		p = pack_hex_byte(p, addr->s6_addr[2 * i]);
-		p = pack_hex_byte(p, addr->s6_addr[2 * i + 1]);
+		p = u16_to_hex(p, ntohs(addr->s6_addr16[i]), leadingzeros);
 		if (fmt[0] == 'I' && i != 7)
 			*p++ = ':';
 	}
@@ -785,6 +821,102 @@ static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, ip4_addr, spec);
 }
 
+static char *port_string(char *p, u16 port, const char *fmt)
+{
+	if (*fmt == 'p')
+		p = u32_dec_val(p, port);
+	else {
+		p = u8_to_dec(p, port >> 8, false);
+		*p++ = '.';
+		p = u8_to_dec(p, port & 0xff, false);
+	}
+
+	return p;
+}
+
+static char *socket_addr_string(char *buf, char *end,
+				const struct sockaddr *sa,
+				struct printf_spec spec, const char *fmt)
+{
+	char baddress[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]"
+			     ":255.255"		/* port # or :12345 */
+			     "%1234567890"	/* IPv6 scope id */
+			     "/123456789")];	/* IPv6 flow mask */
+	char *p;
+	char *addr;
+	struct sockaddr_in *sa4 = (struct sockaddr_in *)sa;
+	struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *)sa;
+
+	/* Start with a bracket in case the address is IPv6 with a port # */
+	baddress[0] = '[';
+	addr = &baddress[1];		/* address without leading '[' */
+	p = addr;
+
+	switch (sa->sa_family) {
+	case AF_INET:
+		p = ip4_string(addr, (const u8 *)&sa4->sin_addr.s_addr, false);
+		break;
+	case AF_INET6:
+		p = ip6_compressed_string(addr, &sa6->sin6_addr);
+		break;
+	default: {
+		struct printf_spec num_spec = {
+			.base = 16,
+			.precision = -1,
+			.field_width = 2 * sizeof(void *),
+			.flags = SPECIAL | SMALL | ZEROPAD,
+		};
+		size_t msglen;
+
+		p = strcpy(baddress, "Bad socket address: ");
+		msglen = strlen(p);
+		p = number(p + msglen, p + sizeof(baddress) - msglen,
+			   (unsigned long)sa, num_spec);
+		break;
+	}
+	}
+
+	while (isalpha(*++fmt)) {
+		switch (*fmt) {
+		case 'p':
+		case 'P':
+			switch (sa->sa_family) {
+			case AF_INET:
+				*p++ = ':';
+				p = port_string(p, ntohs(sa4->sin_port), fmt);
+				break;
+			case AF_INET6:
+				/* Use the initial bracket */
+				if (addr > baddress)
+					addr--;
+				*p++ = ']';
+				*p++ = ':';
+				p = port_string(p, ntohs(sa6->sin6_port), fmt);
+				break;
+			}
+			break;
+		case 's':
+			switch (sa->sa_family) {
+			case AF_INET6:
+				*p++ = '%';
+				p = u32_dec_val(p, sa6->sin6_scope_id);
+			}
+			break;
+		case 'f':
+			switch (sa->sa_family) {
+			case AF_INET6:
+				*p++ = '/';
+				p = u32_dec_val(p, ntohl(sa6->sin6_flowinfo &
+							 IPV6_FLOWINFO_MASK));
+			}
+			break;
+		}
+	}
+
+	*p = '\0';
+	return string(buf, end, addr, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -808,6 +940,15 @@ static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
  *       IPv4 uses dot-separated decimal with leading 0's (010.123.045.006)
  * - 'I6c' for IPv6 addresses printed as specified by
  *       http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
+ * - 'I6n' IPv6: colon separated network-order 16 bit hex without leading 0's
+ * - 'N' For network socket (sockaddr) pointers
+ *       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
+ *       May be used with any combination of additional specifiers below
+ *       'p' decimal socket port number for IPv[46]: ":12345"
+ *       'P' decimal socket port numbers using hi/lo u8: ":0.255"
+ *       's' decimal scope_id number for IPv6: "%123456789"
+ *       'f' decimal flowinfo for IPv6: "/123456789"
+ *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6: "[1::c0a8:a]:1234"
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
@@ -834,6 +975,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 					 * 4:	1.2.3.4
 					 * 6:	0001:0203:...:0708
 					 * 6c:	1::708 or 1::1.2.3.4
+					 * 6n:  1:2:3:0:0:0:789:abcd
 					 */
 	case 'i':			/* Contiguous:
 					 * 4:	001.002.003.004
@@ -846,7 +988,10 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return ip4_addr_string(buf, end, ptr, spec, fmt);
 		}
 		break;
+	case 'N':
+		return socket_addr_string(buf, end, ptr, spec, fmt);
 	}
+
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
 		spec.field_width = 2*sizeof(void *);

Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Chuck Lever]

On Aug 19, 2009, at 4:44 PM, Joe Perches wrote:
> On Wed, 2009-08-19 at 10:26 -0400, Chuck Lever wrote:
>> On Aug 16, 2009, at 12:10 AM, Joe Perches wrote:
>
>> I think having at least a generic socket address formatter would be
>> very helpful.  The kernel RPC code can use that immediately for
>> generating debugging messages, generating "universal
>> addresses" (patches already accepted for 2.6.32) and for generating
>> presentation addresses to pass to user space (lockd does this now).
>
>>> Use style:
>>> * - 'N' For network socket (sockaddr) pointers
>>> *       if sa_family is IPv4, output is %pI4; if IPv6, output is  
>>> %pI6c
>>> *       May be used with any combination of additional specifiers
>>> below
>>> *       'p' decimal socket port number for IPv[46]: ":12345"
>>> *       'f' decimal flowinfo for IPv6: "/123456789"
>>> *       's' decimal scope_id number for IPv6: "%1234567890"
>>> *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6:
>>> "1::c0a8:a:1234"
>>>
>>> I think using ":" as the separator for the port number, while  
>>> common,
>>> and already frequently used in kernel source (see bullet 2 in):
>>> http://www.ietf.org/id/draft-kawamura-ipv6-text- 
>>> representation-03.txt
>>> "Section 6: Notes on Combining IPv6 Addresses with Port Numbers".
>>> is not good for readability.
>>>
>>> Perhaps this style should be changed to the "[ipv6]:port" described
>>> in the draft above.
>>
>> I agree that the port number convention is tricky, and some kernel
>> areas handle it differently than others. Perhaps having a separate
>> family-agnostic formatter for printing the port number (or scope ID,
>> etc.) might allow greatest flexibility.
>
> I'm not too sure there's much value in ever more formatters for
> relatively simple things or in printing something like port or
> scope without the ip address.
>
> %hu ntohs(sa6->sin6_port)
> or
> %d or 0x%x ntohl(sa6->sin6_flowinfo)
>
> seem pretty straightforward to me.

It's the same issue for port numbers as it is for addresses: though  
the port field is the same size in each, it's at different offsets in  
AF_INET and AF_INET6 addresses, so extra logic is still needed to sort  
that at each call site.

As an example, an rpcbind query cares about the port number result,  
but usually not the address.  A human-readable message could show the  
returned port number, but leave out the address.  Without a separate  
formatter, you would still need extra logic around each debugging  
message (for example) to choose how to print the port number.

You could probably argue, though, that this is a less common need than  
printing addresses.

>> The RPC code, for example, displays port numbers in decimal and in
>> "hi.lo" format (the high byte is printed in decimal, then the low  
>> byte
>> is printed in decimal.  The two values are separated by a dot.  For
>> example, port 2049 is displayed as "8.1").
>
> Here's another tentative patch that does both using square
> brackets around IPv6 addresses with a port number and
> allows selection of the port number style ":dddd" or ":hi.lo"

The "hi.lo" form is always separated from the address by a dot, not by  
a colon, and square brackets are never used in that case.  The hi.lo  
format is probably enough of a special case that a separate specifier  
for that one is unnecessary.  Having a hexadecimal port number option  
might be more useful.  The hexadecimal form is also used by the  
kernel's RPC implementation, fwiw.

> It also adds a "pI6n" form for IPv6 addresses with 7 colons
> and no leading 0's.

What would the 7 colons form be used for?

> lib/vsprintf.c: Add '%pN' print formatted struct sock_addr *
>
> Use styles:
> * - 'N' For network socket (sockaddr) pointers
> *       if sa_family is IPv4, output is %pI4; if IPv6, output is %pI6c
> *       May be used with any combination of additional specifiers  
> below
> *       'p' decimal socket port number for IPv[46]: ":12345"
> *       'P' decimal socket port number for IPv6: ":1.255" (hi.lo)
> *           if IPv6 and port number is selected, square brackets
> *           will surround the IPv6 address for 'p' and 'P'
> *       'f' decimal flowinfo for IPv6: "/123456789"
> *       's' decimal scope_id number for IPv6: "%1234567890"
> *       %pNp will print IPv4: "1.2.3.4:1234"
> *                       IPv6: "[1::c0a8:a]:1234"
> *       %pNP will print IPv4: "1.2.3.4:1.255
> *                       IPv6: "[1::c0a8:a]:1.255"
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index cb8a112..139f310 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -647,22 +647,73 @@ static char *mac_address_string(char *buf,  
> char *end, u8 *addr,
> 	return string(buf, end, mac_addr, spec);
> }
>
> -static char *ip4_string(char *p, const u8 *addr, bool leading_zeros)
> +static char *u8_to_dec(char *p, u8 val, bool leadingzeros)
> +{
> +	char temp[3];			/* holds val in reverse order */
> +	int digits = put_dec_trunc(temp, val) - temp;
> +
> +	if (leadingzeros) {
> +		if (digits < 3)
> +			*p++ = '0';
> +		if (digits < 2)
> +			*p++ = '0';
> +	}
> +	/* reverse the digits */
> +	while (digits--)
> +		*p++ = temp[digits];
> +
> +	return p;
> +}
> +
> +static char *u16_to_hex(char *p, u16 word, bool leadingzeros)
> +{
> +	u8 hi = word >> 8;
> +	u8 lo = word & 0xff;
> +	if (leadingzeros) {
> +		p = pack_hex_byte(p, hi);
> +		p = pack_hex_byte(p, lo);
> +	} else {
> +		if (hi) {
> +			if (hi > 0x0f)
> +				p = pack_hex_byte(p, hi);
> +			else
> +				*p++ = hex_asc_lo(hi);
> +		}
> +		if (hi || lo > 0x0f)
> +			p = pack_hex_byte(p, lo);
> +		else
> +			*p++ = hex_asc_lo(lo);
> +	}
> +
> +	return p;
> +}
> +
> +static char *u32_dec_val(char *p, u32 val)
> +{
> +	char temp[9];			/* holds val in reverse order */
> +	int digits;
> +	char *pos;
> +
> +	if (val < 100000)
> +		pos = put_dec_trunc(temp, val);
> +	else {
> +		pos = put_dec_trunc(temp, val % 100000);
> +		pos = put_dec_trunc(pos, val / 100000);
> +	}
> +	digits = pos - temp;
> +	/* reverse the digits */
> +	while (digits--)
> +		*p++ = temp[digits];
> +
> +	return p;
> +}
> +
> +static char *ip4_string(char *p, const u8 *addr, bool leadingzeros)
> {
> 	int i;
>
> 	for (i = 0; i < 4; i++) {
> -		char temp[3];	/* hold each IP quad in reverse order */
> -		int digits = put_dec_trunc(temp, addr[i]) - temp;
> -		if (leading_zeros) {
> -			if (digits < 3)
> -				*p++ = '0';
> -			if (digits < 2)
> -				*p++ = '0';
> -		}
> -		/* reverse the digits in the quad */
> -		while (digits--)
> -			*p++ = temp[digits];
> +		p = u8_to_dec(p, addr[i], leadingzeros);
> 		if (i < 3)
> 			*p++ = '.';
> 	}
> @@ -679,9 +730,6 @@ static char *ip6_compressed_string(char *p,  
> const struct in6_addr *addr)
> 	unsigned char zerolength[8];
> 	int longest = 1;
> 	int colonpos = -1;
> -	u16 word;
> -	u8 hi;
> -	u8 lo;
> 	bool needcolon = false;
> 	bool useIPv4 = ipv6_addr_v4mapped(addr) || ipv6_addr_is_isatap(addr);
>
> @@ -721,20 +769,7 @@ static char *ip6_compressed_string(char *p,  
> const struct in6_addr *addr)
> 			*p++ = ':';
> 			needcolon = false;
> 		}
> -		/* hex u16 without leading 0s */
> -		word = ntohs(addr->s6_addr16[i]);
> -		hi = word >> 8;
> -		lo = word & 0xff;
> -		if (hi) {
> -			if (hi > 0x0f)
> -				p = pack_hex_byte(p, hi);
> -			else
> -				*p++ = hex_asc_lo(hi);
> -		}
> -		if (hi || lo > 0x0f)
> -			p = pack_hex_byte(p, lo);
> -		else
> -			*p++ = hex_asc_lo(lo);
> +		p = u16_to_hex(p, ntohs(addr->s6_addr16[i]), false);
> 		needcolon = true;
> 	}
>
> @@ -751,9 +786,10 @@ static char *ip6_compressed_string(char *p,  
> const struct in6_addr *addr)
> static char *ip6_string(char *p, const struct in6_addr *addr, const  
> char *fmt)
> {
> 	int i;
> +	bool leadingzeros = !(fmt[0] == 'I' && fmt[2] == 'n');
> +
> 	for (i = 0; i < 8; i++) {
> -		p = pack_hex_byte(p, addr->s6_addr[2 * i]);
> -		p = pack_hex_byte(p, addr->s6_addr[2 * i + 1]);
> +		p = u16_to_hex(p, ntohs(addr->s6_addr16[i]), leadingzeros);
> 		if (fmt[0] == 'I' && i != 7)
> 			*p++ = ':';
> 	}
> @@ -785,6 +821,102 @@ static char *ip4_addr_string(char *buf, char  
> *end, const u8 *addr,
> 	return string(buf, end, ip4_addr, spec);
> }
>
> +static char *port_string(char *p, u16 port, const char *fmt)
> +{
> +	if (*fmt == 'p')
> +		p = u32_dec_val(p, port);
> +	else {
> +		p = u8_to_dec(p, port >> 8, false);
> +		*p++ = '.';
> +		p = u8_to_dec(p, port & 0xff, false);
> +	}
> +
> +	return p;
> +}
> +
> +static char *socket_addr_string(char *buf, char *end,
> +				const struct sockaddr *sa,
> +				struct printf_spec spec, const char *fmt)
> +{
> +	char baddress[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx: 
> 255.255.255.255]"
> +			     ":255.255"		/* port # or :12345 */
> +			     "%1234567890"	/* IPv6 scope id */
> +			     "/123456789")];	/* IPv6 flow mask */
> +	char *p;
> +	char *addr;
> +	struct sockaddr_in *sa4 = (struct sockaddr_in *)sa;
> +	struct sockaddr_in6 *sa6 = (struct sockaddr_in6 *)sa;
> +
> +	/* Start with a bracket in case the address is IPv6 with a port # */
> +	baddress[0] = '[';
> +	addr = &baddress[1];		/* address without leading '[' */
> +	p = addr;
> +
> +	switch (sa->sa_family) {
> +	case AF_INET:
> +		p = ip4_string(addr, (const u8 *)&sa4->sin_addr.s_addr, false);
> +		break;
> +	case AF_INET6:
> +		p = ip6_compressed_string(addr, &sa6->sin6_addr);
> +		break;
> +	default: {
> +		struct printf_spec num_spec = {
> +			.base = 16,
> +			.precision = -1,
> +			.field_width = 2 * sizeof(void *),
> +			.flags = SPECIAL | SMALL | ZEROPAD,
> +		};
> +		size_t msglen;
> +
> +		p = strcpy(baddress, "Bad socket address: ");
> +		msglen = strlen(p);
> +		p = number(p + msglen, p + sizeof(baddress) - msglen,
> +			   (unsigned long)sa, num_spec);
> +		break;
> +	}
> +	}
> +
> +	while (isalpha(*++fmt)) {
> +		switch (*fmt) {
> +		case 'p':
> +		case 'P':
> +			switch (sa->sa_family) {
> +			case AF_INET:
> +				*p++ = ':';
> +				p = port_string(p, ntohs(sa4->sin_port), fmt);
> +				break;
> +			case AF_INET6:
> +				/* Use the initial bracket */
> +				if (addr > baddress)
> +					addr--;
> +				*p++ = ']';
> +				*p++ = ':';
> +				p = port_string(p, ntohs(sa6->sin6_port), fmt);
> +				break;
> +			}
> +			break;
> +		case 's':
> +			switch (sa->sa_family) {
> +			case AF_INET6:
> +				*p++ = '%';
> +				p = u32_dec_val(p, sa6->sin6_scope_id);
> +			}
> +			break;
> +		case 'f':
> +			switch (sa->sa_family) {
> +			case AF_INET6:
> +				*p++ = '/';
> +				p = u32_dec_val(p, ntohl(sa6->sin6_flowinfo &
> +							 IPV6_FLOWINFO_MASK));
> +			}
> +			break;
> +		}
> +	}
> +
> +	*p = '\0';
> +	return string(buf, end, addr, spec);
> +}
> +
> /*
>  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>  * by an extra set of alphanumeric characters that are extended format
> @@ -808,6 +940,15 @@ static char *ip4_addr_string(char *buf, char  
> *end, const u8 *addr,
>  *       IPv4 uses dot-separated decimal with leading 0's  
> (010.123.045.006)
>  * - 'I6c' for IPv6 addresses printed as specified by
>  *       http://www.ietf.org/id/draft-kawamura-ipv6-text-representation-03.txt
> + * - 'I6n' IPv6: colon separated network-order 16 bit hex without  
> leading 0's
> + * - 'N' For network socket (sockaddr) pointers
> + *       if sa_family is IPv4, output is %pI4; if IPv6, output is  
> %pI6c
> + *       May be used with any combination of additional specifiers  
> below
> + *       'p' decimal socket port number for IPv[46]: ":12345"
> + *       'P' decimal socket port numbers using hi/lo u8: ":0.255"
> + *       's' decimal scope_id number for IPv6: "%123456789"
> + *       'f' decimal flowinfo for IPv6: "/123456789"
> + *       so %pNp will print if IPv4 "1.2.3.4:1234", if IPv6:  
> "[1::c0a8:a]:1234"
>  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>  * function pointers are really function descriptors, which contain a
>  * pointer to the real address.
> @@ -834,6 +975,7 @@ static char *pointer(const char *fmt, char *buf,  
> char *end, void *ptr,
> 					 * 4:	1.2.3.4
> 					 * 6:	0001:0203:...:0708
> 					 * 6c:	1::708 or 1::1.2.3.4
> +					 * 6n:  1:2:3:0:0:0:789:abcd
> 					 */
> 	case 'i':			/* Contiguous:
> 					 * 4:	001.002.003.004
> @@ -846,7 +988,10 @@ static char *pointer(const char *fmt, char  
> *buf, char *end, void *ptr,
> 			return ip4_addr_string(buf, end, ptr, spec, fmt);
> 		}
> 		break;
> +	case 'N':
> +		return socket_addr_string(buf, end, ptr, spec, fmt);
> 	}
> +
> 	spec.flags |= SMALL;
> 	if (spec.field_width == -1) {
> 		spec.field_width = 2*sizeof(void *);
>
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Joe Perches]

On Wed, 2009-08-19 at 18:20 -0400, Chuck Lever wrote:
> On Aug 19, 2009, at 4:44 PM, Joe Perches wrote:
> > I'm not too sure there's much value in ever more formatters for
> > relatively simple things or in printing something like port or
> > scope without the ip address.
> >
> > %hu ntohs(sa6->sin6_port)
> > or
> > %d or 0x%x ntohl(sa6->sin6_flowinfo)
> >
> > seem pretty straightforward to me.
> 
> It's the same issue for port numbers as it is for addresses: though  
> the port field is the same size in each, it's at different offsets in  
> AF_INET and AF_INET6 addresses, so extra logic is still needed to sort  
> that at each call site.
> 
> As an example, an rpcbind query cares about the port number result,  
> but usually not the address.  A human-readable message could show the  
> returned port number, but leave out the address.  Without a separate  
> formatter, you would still need extra logic around each debugging  
> message (for example) to choose how to print the port number.
> 
> You could probably argue, though, that this is a less common need than  
> printing addresses.

I suppose an address exclude could be added to %pN
if really needed.  Maybe "%pNx(specifiers)"

> > Here's another tentative patch that does both using square
> > brackets around IPv6 addresses with a port number and
> > allows selection of the port number style ":dddd" or ":hi.lo"
> 
> The "hi.lo" form is always separated from the address by a dot, not by  
> a colon, and square brackets are never used in that case.  The hi.lo  
> format is probably enough of a special case that a separate specifier  
> for that one is unnecessary.

Is the RPC code never used with a ipv6_addr_v4mapped address?
::192.168.0.1.1.1 would look pretty ugly

> Having a hexadecimal port number option  
> might be more useful.  The hexadecimal form is also used by the  
> kernel's RPC implementation, fwiw.

Easy enough. %pNhp or some such.

> > It also adds a "pI6n" form for IPv6 addresses with 7 colons
> > and no leading 0's.
> What would the 7 colons form be used for?

Shorter output requiring colon parsing, no :: parsing.
It's easier for humans to visually scan than the
leading 0 form.

Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Chuck Lever]

On Aug 19, 2009, at 6:36 PM, Joe Perches wrote:
> On Wed, 2009-08-19 at 18:20 -0400, Chuck Lever wrote:
>> On Aug 19, 2009, at 4:44 PM, Joe Perches wrote:
>>> I'm not too sure there's much value in ever more formatters for
>>> relatively simple things or in printing something like port or
>>> scope without the ip address.
>>>
>>> %hu ntohs(sa6->sin6_port)
>>> or
>>> %d or 0x%x ntohl(sa6->sin6_flowinfo)
>>>
>>> seem pretty straightforward to me.
>>
>> It's the same issue for port numbers as it is for addresses: though
>> the port field is the same size in each, it's at different offsets in
>> AF_INET and AF_INET6 addresses, so extra logic is still needed to  
>> sort
>> that at each call site.

I should add that the scope ID and flowinfo tag are IPv6 only, so they  
really don't have the same problem.  If you have to print a scope ID,  
you pretty much know you are dealing only with sockaddr_in6.

>> As an example, an rpcbind query cares about the port number result,
>> but usually not the address.  A human-readable message could show the
>> returned port number, but leave out the address.  Without a separate
>> formatter, you would still need extra logic around each debugging
>> message (for example) to choose how to print the port number.
>>
>> You could probably argue, though, that this is a less common need  
>> than
>> printing addresses.
>
> I suppose an address exclude could be added to %pN
> if really needed.  Maybe "%pNx(specifiers)"

>>> Here's another tentative patch that does both using square
>>> brackets around IPv6 addresses with a port number and
>>> allows selection of the port number style ":dddd" or ":hi.lo"
>>
>> The "hi.lo" form is always separated from the address by a dot, not  
>> by
>> a colon, and square brackets are never used in that case.  The hi.lo
>> format is probably enough of a special case that a separate specifier
>> for that one is unnecessary.
>
> Is the RPC code never used with a ipv6_addr_v4mapped address?
> ::192.168.0.1.1.1 would look pretty ugly

As I understand it, TI-RPC does not ever use mapped v4 addresses on  
the wire (only pure IPv4 and IPv6).  So they shouldn't actually appear  
in the universal address format, in practice.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[Joe Perches]

On Wed, 2009-08-19 at 15:36 -0700, Joe Perches wrote:
> > Having a hexadecimal port number option  
> > might be more useful.  The hexadecimal form is also used by the  
> > kernel's RPC implementation, fwiw.
> 
> Easy enough.

Do you have an opinion on this style patch to lib/vsprint?
Where does it fall on the useless <-> desirable scale?

Re: [RFC PATCH V2] lib/vsprintf.c: Add struct sockaddr * "%pN<foo>" output

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Wed, 19 Aug 2009 21:24:06 -0700

> On Wed, 2009-08-19 at 15:36 -0700, Joe Perches wrote:
>> > Having a hexadecimal port number option  
>> > might be more useful.  The hexadecimal form is also used by the  
>> > kernel's RPC implementation, fwiw.
>> 
>> Easy enough.
> 
> Do you have an opinion on this style patch to lib/vsprint?
> Where does it fall on the useless <-> desirable scale?

Who me?

I'm just following this thread loosely, and just plan to review it
on a whole once things seem to quiet down and the major issues
seem to be worked out.

I really have no hard opinion on anything like this, sorry for
the lack of feedback, I simply have none :)

Re: [PATCH V2] lib/vsprintf.c: Add "%pI6c" - print pointer as compressed ipv6 address

[David Miller]

From: Jens Rosenboom <jens@mcbone.net>
Date: Tue, 18 Aug 2009 15:48:56 +0200

> On Mon, 2009-08-17 at 15:29 -0700, Joe Perches wrote:
>> Here's the modified patch with your suggestions:
>> 
> 
> Ran it through my test-cases and they all look fine.
> 
> Tested-by: Jens Rosenboom <jens@mcbone.net>
> 

Applied, thanks everyone.