login
Search
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; --
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. --
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. --
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 ...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 --
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 = ...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) ...-- Chuck Lever chuck[dot]lever[at]oracle[dot]com --
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 --
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')
--
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 --
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 --
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 --
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 --
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 --
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 --
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. --
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?
--
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. --
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. --
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. --
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 ...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") + + ...
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 --
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 ...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 --
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. --
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 --
Do you have an opinion on this style patch to lib/vsprint? Where does it fall on the useless <-> desirable scale? --
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 :) --
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().
--
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 ...
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> --
From: Jens Rosenboom <jens@mcbone.net> Applied, thanks everyone. --
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 --
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. --
