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.

--

[ message continues ]

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;
--

[ message continues ]

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.


--

[ message continues ]

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.

--

[ message continues ]

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.

--

[ message continues ]

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 ...
[ message continues ]

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  

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




--

[ message continues ]

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

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 = ...
[ message continues ]

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) ...
[ message continues ]

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




--

[ message continues ]

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


--

[ message continues ]

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
--

[ message continues ]

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')



--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

Is your arch "um"?  Seems like those are only defined there, I'm building

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.


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

-Brian
--

[ message continues ]

Nope.

I did make allyesconfig ; make lib/vsprintf.o lib ctype.o


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

for instance:

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


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

cheers, Joe

--

[ message continues ]

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  

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  

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

[ message continues ]

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",


cheers, Joe

--

[ message continues ]

From: Joe Perches <joe@perches.com>

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.
--

[ message continues ]

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?


--

[ message continues ]

From: Jens Rosenboom <jens@mcbone.net>

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.
--

[ message continues ]

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.


--

[ message continues ]

From: Jens Rosenboom <jens@mcbone.net>

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

[ message continues ]

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 ...
[ message continues ]

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") +
+		  ...
[ message continues ]

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  

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  

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



--

[ message continues ]

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)


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 ...
[ message continues ]

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  

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  


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



--

[ message continues ]

I suppose an address exclude could be added to %pN



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

--

[ message continues ]

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,  

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
--

[ message continues ]

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

--

[ message continues ]

From: Joe Perches <joe@perches.com>

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 :)
--

[ message continues ]

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().

--

[ message continues ]

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


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 ...
[ message continues ]

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

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

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

--

[ message continues ]

From: Jens Rosenboom <jens@mcbone.net>

Applied, thanks everyone.
--

[ message continues ]

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()  

I've seen enough to agree that a new formatter is a reasonable  

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

[ message continues ]

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

--

[ message continues ]

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.


--

[ message continues ]