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

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)



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

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

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"


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

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

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

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

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

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

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

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

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

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?


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

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

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.


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

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

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