Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325181 > unrolled thread

[PATCH] vsprintf: do not append unset Scope ID to IPv6

Back to article view | Back to linux.kernel

Contents

  [PATCH] vsprintf: do not append unset Scope ID to IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-03 11:50 +0100
    [PATCH] vsprintf: flowinfo in IPv6 is optional too "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-03 13:20 +0100
      IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too Joe Perches <joe@perches.com> - 2016-02-03 19:00 +0100
        Re: IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-03 22:20 +0100
        Re: IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too Daniel Borkmann <daniel@iogearbox.net> - 2016-02-03 22:20 +0100
        Re: [PATCH] vsprintf: flowinfo in IPv6 is optional too Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-02-03 22:20 +0100
        Re: IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-03 22:20 +0100
    Re: [PATCH] vsprintf: do not append unset Scope ID to IPv6 Daniel Borkmann <daniel@iogearbox.net> - 2016-02-03 22:10 +0100
      Re: [PATCH] vsprintf: do not append unset Scope ID to IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-03 22:20 +0100
        Re: [PATCH] vsprintf: do not append unset Scope ID to IPv6 Joe Perches <joe@perches.com> - 2016-02-03 22:50 +0100
          Re: [PATCH] vsprintf: do not append unset Scope ID to IPv6 Daniel Borkmann <daniel@iogearbox.net> - 2016-02-03 23:10 +0100
          Re: [PATCH] vsprintf: do not append unset Scope ID to IPv6 "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-03 23:50 +0100
          [PATCH] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-04 00:00 +0100
            Re: [PATCH] vsprintf: automatic parameters for %pIS via 'a' Joe Perches <joe@perches.com> - 2016-02-04 00:10 +0100
              Re: [PATCH] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-04 00:30 +0100
              [PATCH v2] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-04 00:30 +0100
                Re: [PATCH v2] vsprintf: automatic parameters for %pIS via 'a' Joe Perches <joe@perches.com> - 2016-02-04 00:40 +0100
                  [PATCH v3] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-04 02:00 +0100
                Re: [PATCH v2] vsprintf: automatic parameters for %pIS via 'a' Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-02-05 01:10 +0100
                  Re: [PATCH v2] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-05 14:10 +0100
                  [PATCH v4] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-05 14:40 +0100
                  [PATCH] vsprintf: automatic parameters for %pIS via 'a' "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-02-05 14:40 +0100

Page 1 of 2  [1] 2  Next page →

#1325181 — [PATCH] vsprintf: do not append unset Scope ID to IPv6

The sockaddr_in6 has the sin6_scope_id parameter. This contains the
netdev index of the output device. When set to 0, sin6_scope_id is
considered to be "unset" -- it has no Scope ID (see RFC4007). When it is
set to >0, it has a Scope ID.

Other parts of the kernel respect htis. For example, code like this is
sometimes used to populate a sockaddr_in6 from a skb:

  sin6->sin6_scope_id = ipv6_iface_scope_id(ipv6_hdr(skb)->saddr, skb->skb_iif);

Such a code example makes a call to the ipv6_iface_scope_id function
which is defined as follows (in ipv6.h):

static inline __u32 ipv6_iface_scope_id(const struct in6_addr *addr, int iface)
{
	return __ipv6_addr_needs_scope_id(__ipv6_addr_type(addr)) ? iface : 0;
}

So, here, either it has Scope ID, in which case it's returned, or it
doesn't need a Scope ID, in which case 0 - "no scope id" - is returned.

Therefore, vsprintf should treat the 0 Scope ID the same way the rest of
the v6 stack and the RFC treats it: 0 Scope ID means no Scope ID. And if
there is no Scope ID, there is nothing to be printed.

So, this patch only prints the Scope ID when one exists, and does not
print a Scope ID when one does not exist.

It turns out, too, that this is what developers want. The most common
purpose of %pISpfsc is to show "what is in that sockaddr so that I can
have some idea of how to connect to it?"

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 48ff9c3..1b1b1c8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1194,7 +1194,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 		p = number(p, pend, ntohl(sa->sin6_flowinfo &
 					  IPV6_FLOWINFO_MASK), spec);
 	}
-	if (have_s) {
+	if (have_s && sa->sin6_scope_id) {
 		*p++ = '%';
 		p = number(p, pend, sa->sin6_scope_id, spec);
 	}
-- 
2.7.0

[toc] | [next] | [standalone]

#1325324 — [PATCH] vsprintf: flowinfo in IPv6 is optional too

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1b1b1c8..85e6645 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1189,7 +1189,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 		*p++ = ':';
 		p = number(p, pend, ntohs(sa->sin6_port), spec);
 	}
-	if (have_f) {
+	if (have_f && (sa->sin6_flowinfo & IPV6_FLOWINFO_MASK)) {
 		*p++ = '/';
 		p = number(p, pend, ntohl(sa->sin6_flowinfo &
 					  IPV6_FLOWINFO_MASK), spec);
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1325747 — IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too

On Wed, 2016-02-03 at 13:13 +0100, Jason A. Donenfeld wrote:
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 1b1b1c8..85e6645 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1189,7 +1189,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
>  		*p++ = ':';
>  		p = number(p, pend, ntohs(sa->sin6_port), spec);
>  	}
> -	if (have_f) {
> +	if (have_f && (sa->sin6_flowinfo & IPV6_FLOWINFO_MASK)) {
>  		*p++ = '/';
>  		p = number(p, pend, ntohl(sa->sin6_flowinfo &
>  					  IPV6_FLOWINFO_MASK), spec);

Why does this matter at all?

The format string "%pIS[...]f" is not used currently in the kernel.

If one were to call out this 'f' qualifier to %pIS, wouldn't it
be better to show /0 than elide the / output completely?

[toc] | [prev] | [next] | [standalone]

#1325951 — Re: IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too

On Wed, Feb 3, 2016 at 10:13 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Another possibility also in regards to your other patch would be to have
> a different format string char option instead of 'f'/'s' that would then
> allow a developer for having both options to choose from. Dunno if it's
> really worth it, but if you have a use case that definitely needs it, then
> it'd be probably better. Less surprises during debugging, at least.

Just came to this too over in the other patch thread. I'll cook this up now.

[toc] | [prev] | [next] | [standalone]

#1325952 — Re: IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too

On 02/03/2016 06:56 PM, Joe Perches wrote:
> On Wed, 2016-02-03 at 13:13 +0100, Jason A. Donenfeld wrote:
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>> ---
>>   lib/vsprintf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 1b1b1c8..85e6645 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -1189,7 +1189,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
>>   		*p++ = ':';
>>   		p = number(p, pend, ntohs(sa->sin6_port), spec);
>>   	}
>> -	if (have_f) {
>> +	if (have_f && (sa->sin6_flowinfo & IPV6_FLOWINFO_MASK)) {
>>   		*p++ = '/';
>>   		p = number(p, pend, ntohl(sa->sin6_flowinfo &
>>   					  IPV6_FLOWINFO_MASK), spec);
>
> Why does this matter at all?
>
> The format string "%pIS[...]f" is not used currently in the kernel.
>
> If one were to call out this 'f' qualifier to %pIS, wouldn't it
> be better to show /0 than elide the / output completely?

+1

Another possibility also in regards to your other patch would be to have
a different format string char option instead of 'f'/'s' that would then
allow a developer for having both options to choose from. Dunno if it's
really worth it, but if you have a use case that definitely needs it, then
it'd be probably better. Less surprises during debugging, at least.

[toc] | [prev] | [next] | [standalone]

#1325955 — Re: [PATCH] vsprintf: flowinfo in IPv6 is optional too

On Wed, 2016-02-03 at 22:09 +0100, Jason A. Donenfeld wrote:
> On Wed, Feb 3, 2016 at 6:56 PM, Joe Perches <joe@perches.com> wrote:
> > Why does this matter at all?
> 
> Because I want to printk a sockaddr_in6, and have the most complete
> yet valid and standard representation I can. Displaying a /0 when
> there is no flow is confusing, ugly, and unhelpful, not to mention it
> doesn't accurately reflect what's happening. A flowlabel of zero
> means
> no flowlabel.

So, where do you want to put your result? dmesg? User-space application
via sysfs? 

Seems like here is discussion debug vs. release version of printed data
in the kernel.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1325960 — Re: IRe: [PATCH] vsprintf: flowinfo in IPv6 is optional too

On Wed, Feb 3, 2016 at 6:56 PM, Joe Perches <joe@perches.com> wrote:
> Why does this matter at all?

Because I want to printk a sockaddr_in6, and have the most complete
yet valid and standard representation I can. Displaying a /0 when
there is no flow is confusing, ugly, and unhelpful, not to mention it
doesn't accurately reflect what's happening. A flowlabel of zero means
no flowlabel.

[toc] | [prev] | [next] | [standalone]

#1325948

On 02/03/2016 11:41 AM, Jason A. Donenfeld wrote:
> The sockaddr_in6 has the sin6_scope_id parameter. This contains the
> netdev index of the output device. When set to 0, sin6_scope_id is
> considered to be "unset" -- it has no Scope ID (see RFC4007). When it is
> set to >0, it has a Scope ID.
[...]
> So, here, either it has Scope ID, in which case it's returned, or it
> doesn't need a Scope ID, in which case 0 - "no scope id" - is returned.
>
> Therefore, vsprintf should treat the 0 Scope ID the same way the rest of
> the v6 stack and the RFC treats it: 0 Scope ID means no Scope ID. And if
> there is no Scope ID, there is nothing to be printed.

I don't see an "issue" here. I.e. if the developer requested to append 's'
to the specifier, then it's expected to dump what is in the scope id, even
if that is 0 meaning "unset" scope.

It would be principle of least surprise -- I could imagine if not dumped,
then a developer would check the source code to find out whether he did a
mistake or it's expected behavior, just to make sure. That's what I would
do probably.

Is this causing a _real_ issue somewhere (I couldn't parse one out of your
commit message other than 'it would be nice to have')?

> So, this patch only prints the Scope ID when one exists, and does not
> print a Scope ID when one does not exist.
>
> It turns out, too, that this is what developers want. The most common
> purpose of %pISpfsc is to show "what is in that sockaddr so that I can
> have some idea of how to connect to it?"
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   lib/vsprintf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 48ff9c3..1b1b1c8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1194,7 +1194,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
>   		p = number(p, pend, ntohl(sa->sin6_flowinfo &
>   					  IPV6_FLOWINFO_MASK), spec);
>   	}
> -	if (have_s) {
> +	if (have_s && sa->sin6_scope_id) {
>   		*p++ = '%';
>   		p = number(p, pend, sa->sin6_scope_id, spec);
>   	}
>

[toc] | [prev] | [next] | [standalone]

#1325953

On Wed, Feb 3, 2016 at 10:07 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> I don't see an "issue" here. I.e. if the developer requested to append 's'
> to the specifier, then it's expected to dump what is in the scope id, even
> if that is 0 meaning "unset" scope.
>
> It would be principle of least surprise -- I could imagine if not dumped,
> then a developer would check the source code to find out whether he did a
> mistake or it's expected behavior, just to make sure. That's what I would
> do probably.

Showing something that's "unset" seems counter-intuitive.

The idea here is to be able to printk a sockaddr_in6, and have it show
something that looks like what the user would naturally pass to
getaddrinfo(3), which is entirely complete.

However, I could be convinced that this kind of behavior belongs in
it's own flag. Maybe I'll cook up a flag for that instead.

>
> Is this causing a _real_ issue somewhere (I couldn't parse one out of your
> commit message other than 'it would be nice to have')?

Nice to have.

[toc] | [prev] | [next] | [standalone]

#1325990

On Wed, 2016-02-03 at 22:14 +0100, Jason A. Donenfeld wrote:
> The idea here is to be able to printk a sockaddr_in6, and have it show
> something that looks like what the user would naturally pass to
> getaddrinfo(3), which is entirely complete.
> 
> However, I could be convinced that this kind of behavior belongs in
> it's own flag. Maybe I'll cook up a flag for that instead.

I think that'd be best.

Maybe using something like %pISG for this that
would optionally show these flow and scope values
only when non-zero.

Something like:
---
 lib/vsprintf.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6dc4288..2003c6f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1147,7 +1147,8 @@ static noinline_for_stack
 char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false, have_s = false, have_f = false, have_c = false;
+	u8 show_p = 0, show_s = 0, show_f = 0;
+	bool use_c = false;
 	char ip6_addr[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
 		      sizeof(":12345") + sizeof("/123456789") +
 		      sizeof("%1234567890")];
@@ -1160,43 +1161,50 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 	while (isalpha(*++fmt)) {
 		switch (*fmt) {
 		case 'p':
-			have_p = true;
+			show_p = 1;
 			break;
 		case 'f':
-			have_f = true;
+			show_f = 1;
 			break;
 		case 's':
-			have_s = true;
+			show_s = 1;
+			break;
+		case 'G':
+			show_p = 2;
+			show_f = 2;
+			show_s = 2;
 			break;
 		case 'c':
-			have_c = true;
+			use_c = true;
 			break;
 		}
 	}
 
-	if (have_p || have_s || have_f) {
+	if (show_p || show_s || show_f) {
 		*p = '[';
 		off = 1;
 	}
 
-	if (fmt6[0] == 'I' && have_c)
+	if (fmt6[0] == 'I' && use_c)
 		p = ip6_compressed_string(ip6_addr + off, addr);
 	else
 		p = ip6_string(ip6_addr + off, addr, fmt6);
 
-	if (have_p || have_s || have_f)
+	if (show_p || show_s || show_f)
 		*p++ = ']';
 
-	if (have_p) {
+	if (show_p) {
 		*p++ = ':';
 		p = number(p, pend, ntohs(sa->sin6_port), spec);
 	}
-	if (have_f) {
+	if (show_f == 1 ||
+	    (show_f == 2 && (sa->sin6_flowinfo & IPV6_FLOWINFO_MASK))) {
 		*p++ = '/';
 		p = number(p, pend, ntohl(sa->sin6_flowinfo &
 					  IPV6_FLOWINFO_MASK), spec);
 	}
-	if (have_s) {
+	if (show_s == 1 ||
+	    (show_s == 2 && sa->sin6_scope_id)) {
 		*p++ = '%';
 		p = number(p, pend, sa->sin6_scope_id, spec);
 	}

[toc] | [prev] | [next] | [standalone]

#1325995

On 02/03/2016 10:47 PM, Joe Perches wrote:
> On Wed, 2016-02-03 at 22:14 +0100, Jason A. Donenfeld wrote:
>> The idea here is to be able to printk a sockaddr_in6, and have it show
>> something that looks like what the user would naturally pass to
>> getaddrinfo(3), which is entirely complete.
>>
>> However, I could be convinced that this kind of behavior belongs in
>> it's own flag. Maybe I'll cook up a flag for that instead.
>
> I think that'd be best.

Agreed.

> Maybe using something like %pISG for this that
> would optionally show these flow and scope values
> only when non-zero.
>
> Something like:

Looks good to me, having this as a single generic option seems
even cleaner.

Thanks!

[toc] | [prev] | [next] | [standalone]

#1326079

I've got something a bit cleaner that more accurately reflects this
use case. Five minutes and I'll send over the patch.

[toc] | [prev] | [next] | [standalone]

#1326107 — [PATCH] vsprintf: automatic parameters for %pIS via 'a'

This patch adds a variable 'a' which indicates that the 'p',
'f', and 's' options should be toggled on or off depending on
whether or not those parameters are actually valid inside the
passed sockaddr. This is something that probably most users of
the %pIS family of functions will prefer to use.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 Documentation/printk-formats.txt |  6 ++++--
 lib/vsprintf.c                   | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5d1128b..22bae97 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -193,7 +193,7 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 	%piS	001.002.003.004	or 00010002000300040005000600070008
 	%pISc	1.2.3.4		or 1:2:3:4:5:6:7:8
 	%pISpc	1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345
-	%p[Ii]S[pfschnbl]
+	%p[Ii]S[pfsachnbl]
 
 	For printing an IP address without the need to distinguish whether it's
 	of type AF_INET or AF_INET6, a pointer to a valid 'struct sockaddr',
@@ -201,7 +201,9 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 
 	The additional 'p', 'f', and 's' specifiers are used to specify port
 	(IPv4, IPv6), flowinfo (IPv6) and scope (IPv6). Ports have a ':' prefix,
-	flowinfo a '/' and scope a '%', each followed by the actual value.
+	flowinfo a '/' and scope a '%', each followed by the actual value. If 'a'
+	is given, 'p', 'f', and 's' are activated or deactivated depending on
+	whether or not the sockaddr has a non-zero port, flowinfo, and scope.
 
 	In case of an IPv6 address the compressed IPv6 address as described by
 	http://tools.ietf.org/html/rfc5952 is being used if the additional
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 48ff9c3..b414a22 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1145,7 +1145,7 @@ static noinline_for_stack
 char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false, have_s = false, have_f = false, have_c = false;
+	bool have_p = false, have_s = false, have_f = false, have_c = false, have_a = false;
 	char ip6_addr[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
 		      sizeof(":12345") + sizeof("/123456789") +
 		      sizeof("%1234567890")];
@@ -1169,9 +1169,18 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 		case 'c':
 			have_c = true;
 			break;
+		case 'a':
+			have_a = true;
+			break;
 		}
 	}
 
+	if (have_a) {
+		have_p = sa->sin6_port != 0;
+		have_s = sa->sin6_scope_id != 0;
+		have_f = sa->sin6_flowinfo != 0;
+	}
+
 	if (have_p || have_s || have_f) {
 		*p = '[';
 		off = 1;
@@ -1207,7 +1216,7 @@ static noinline_for_stack
 char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false;
+	bool have_p = false, have_a = false;
 	char *p, ip4_addr[sizeof("255.255.255.255") + sizeof(":12345")];
 	char *pend = ip4_addr + sizeof(ip4_addr);
 	const u8 *addr = (const u8 *) &sa->sin_addr.s_addr;
@@ -1225,9 +1234,15 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 		case 'b':
 			fmt4[2] = *fmt;
 			break;
+		case 'a':
+			have_a = true;
+			break;
 		}
 	}
 
+	if (have_a)
+		have_p = sa->sin_port != 0;
+
 	p = ip4_string(ip4_addr, addr, fmt4);
 	if (have_p) {
 		*p++ = ':';
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1326118 — Re: [PATCH] vsprintf: automatic parameters for %pIS via 'a'

On Wed, 2016-02-03 at 23:53 +0100, Jason A. Donenfeld wrote:
> This patch adds a variable 'a' which indicates that the 'p',
> 'f', and 's' options should be toggled on or off depending on
> whether or not those parameters are actually valid inside the
> passed sockaddr.

OK

> This is something that probably most users of
> the %pIS family of functions will prefer to use.

More doubtful.

There isn't a single user of flowinfo or scope today.

These are the current uses of %pIS

      8 %pIS
     11 %pISc
      5 %pIScp
      2 %pISp
     23 %pISpc

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1169,9 +1169,18 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
>  		case 'c':
>  			have_c = true;
>  			break;
> +		case 'a':
> +			have_a = true;
> +			break;
>  		}
>  	}
>  
> +	if (have_a) {
> +		have_p = sa->sin6_port != 0;
> +		have_s = sa->sin6_scope_id != 0;
> +		have_f = sa->sin6_flowinfo != 0;

Doesn't this needs the mask tested?

		sa->sin6_flowinfo & IPV6_FLOWINFO_MASK

given the use of bool, the != 0 are unnecessary.

Other than that, looks good to me.

[toc] | [prev] | [next] | [standalone]

#1326184 — Re: [PATCH] vsprintf: automatic parameters for %pIS via 'a'

On Thu, Feb 4, 2016 at 12:05 AM, Joe Perches <joe@perches.com> wrote:
>
> Doesn't this needs the mask tested?
>
>                 sa->sin6_flowinfo & IPV6_FLOWINFO_MASK

Good catch. Fixing and will submit another.

>
> given the use of bool, the != 0 are unnecessary.

True.

> Other than that, looks good to me.

Great!

[toc] | [prev] | [next] | [standalone]

#1326199 — [PATCH v2] vsprintf: automatic parameters for %pIS via 'a'

This patch adds a variable 'a' which indicates that the 'p',
'f', and 's' options should be toggled on or off depending on
whether or not those parameters are actually valid inside the
passed sockaddr.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 Documentation/printk-formats.txt |  6 ++++--
 lib/vsprintf.c                   | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5d1128b..22bae97 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -193,7 +193,7 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 	%piS	001.002.003.004	or 00010002000300040005000600070008
 	%pISc	1.2.3.4		or 1:2:3:4:5:6:7:8
 	%pISpc	1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345
-	%p[Ii]S[pfschnbl]
+	%p[Ii]S[pfsachnbl]
 
 	For printing an IP address without the need to distinguish whether it's
 	of type AF_INET or AF_INET6, a pointer to a valid 'struct sockaddr',
@@ -201,7 +201,9 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 
 	The additional 'p', 'f', and 's' specifiers are used to specify port
 	(IPv4, IPv6), flowinfo (IPv6) and scope (IPv6). Ports have a ':' prefix,
-	flowinfo a '/' and scope a '%', each followed by the actual value.
+	flowinfo a '/' and scope a '%', each followed by the actual value. If 'a'
+	is given, 'p', 'f', and 's' are activated or deactivated depending on
+	whether or not the sockaddr has a non-zero port, flowinfo, and scope.
 
 	In case of an IPv6 address the compressed IPv6 address as described by
 	http://tools.ietf.org/html/rfc5952 is being used if the additional
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 48ff9c3..9a07284 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1145,7 +1145,7 @@ static noinline_for_stack
 char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false, have_s = false, have_f = false, have_c = false;
+	bool have_p = false, have_s = false, have_f = false, have_c = false, have_a = false;
 	char ip6_addr[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
 		      sizeof(":12345") + sizeof("/123456789") +
 		      sizeof("%1234567890")];
@@ -1169,9 +1169,18 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 		case 'c':
 			have_c = true;
 			break;
+		case 'a':
+			have_a = true;
+			break;
 		}
 	}
 
+	if (have_a) {
+		have_p = sa->sin6_port;
+		have_s = sa->sin6_scope_id;
+		have_f = sa->sin6_flowinfo & IPV6_FLOWINFO_MASK;
+	}
+
 	if (have_p || have_s || have_f) {
 		*p = '[';
 		off = 1;
@@ -1207,7 +1216,7 @@ static noinline_for_stack
 char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false;
+	bool have_p = false, have_a = false;
 	char *p, ip4_addr[sizeof("255.255.255.255") + sizeof(":12345")];
 	char *pend = ip4_addr + sizeof(ip4_addr);
 	const u8 *addr = (const u8 *) &sa->sin_addr.s_addr;
@@ -1225,9 +1234,15 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 		case 'b':
 			fmt4[2] = *fmt;
 			break;
+		case 'a':
+			have_a = true;
+			break;
 		}
 	}
 
+	if (have_a)
+		have_p = sa->sin_port != 0;
+
 	p = ip4_string(ip4_addr, addr, fmt4);
 	if (have_p) {
 		*p++ = ':';
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1326214 — Re: [PATCH v2] vsprintf: automatic parameters for %pIS via 'a'

On Thu, 2016-02-04 at 00:29 +0100, Jason A. Donenfeld wrote:
> This patch adds a variable 'a' which indicates that the 'p',
> 'f', and 's' options should be toggled on or off depending on
> whether or not those parameters are actually valid inside the
> passed sockaddr.

trivia:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1225,9 +1234,15 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
[]
> +	if (have_a)
> +		have_p = sa->sin_port != 0;
> +

different style than the v6 tests

[toc] | [prev] | [next] | [standalone]

#1326319 — [PATCH v3] vsprintf: automatic parameters for %pIS via 'a'

This patch adds a variable 'a' which indicates that the 'p',
'f', and 's' options should be toggled on or off depending on
whether or not those parameters are actually valid inside the
passed sockaddr.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 Documentation/printk-formats.txt |  6 ++++--
 lib/vsprintf.c                   | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5d1128b..22bae97 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -193,7 +193,7 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 	%piS	001.002.003.004	or 00010002000300040005000600070008
 	%pISc	1.2.3.4		or 1:2:3:4:5:6:7:8
 	%pISpc	1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345
-	%p[Ii]S[pfschnbl]
+	%p[Ii]S[pfsachnbl]
 
 	For printing an IP address without the need to distinguish whether it's
 	of type AF_INET or AF_INET6, a pointer to a valid 'struct sockaddr',
@@ -201,7 +201,9 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 
 	The additional 'p', 'f', and 's' specifiers are used to specify port
 	(IPv4, IPv6), flowinfo (IPv6) and scope (IPv6). Ports have a ':' prefix,
-	flowinfo a '/' and scope a '%', each followed by the actual value.
+	flowinfo a '/' and scope a '%', each followed by the actual value. If 'a'
+	is given, 'p', 'f', and 's' are activated or deactivated depending on
+	whether or not the sockaddr has a non-zero port, flowinfo, and scope.
 
 	In case of an IPv6 address the compressed IPv6 address as described by
 	http://tools.ietf.org/html/rfc5952 is being used if the additional
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 48ff9c3..cda27b4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1145,7 +1145,7 @@ static noinline_for_stack
 char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false, have_s = false, have_f = false, have_c = false;
+	bool have_p = false, have_s = false, have_f = false, have_c = false, have_a = false;
 	char ip6_addr[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
 		      sizeof(":12345") + sizeof("/123456789") +
 		      sizeof("%1234567890")];
@@ -1169,9 +1169,18 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 		case 'c':
 			have_c = true;
 			break;
+		case 'a':
+			have_a = true;
+			break;
 		}
 	}
 
+	if (have_a) {
+		have_p = sa->sin6_port;
+		have_s = sa->sin6_scope_id;
+		have_f = sa->sin6_flowinfo & IPV6_FLOWINFO_MASK;
+	}
+
 	if (have_p || have_s || have_f) {
 		*p = '[';
 		off = 1;
@@ -1207,7 +1216,7 @@ static noinline_for_stack
 char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 			 struct printf_spec spec, const char *fmt)
 {
-	bool have_p = false;
+	bool have_p = false, have_a = false;
 	char *p, ip4_addr[sizeof("255.255.255.255") + sizeof(":12345")];
 	char *pend = ip4_addr + sizeof(ip4_addr);
 	const u8 *addr = (const u8 *) &sa->sin_addr.s_addr;
@@ -1225,9 +1234,15 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 		case 'b':
 			fmt4[2] = *fmt;
 			break;
+		case 'a':
+			have_a = true;
+			break;
 		}
 	}
 
+	if (have_a)
+		have_p = sa->sin_port;
+
 	p = ip4_string(ip4_addr, addr, fmt4);
 	if (have_p) {
 		*p++ = ':';
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1327363 — Re: [PATCH v2] vsprintf: automatic parameters for %pIS via 'a'

On Thu, Feb 04 2016, "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> This patch adds a variable 'a' which indicates that the 'p',
> 'f', and 's' options should be toggled on or off depending on
> whether or not those parameters are actually valid inside the
> passed sockaddr.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  Documentation/printk-formats.txt |  6 ++++--
>  lib/vsprintf.c                   | 19 +++++++++++++++++--
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 5d1128b..22bae97 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -193,7 +193,7 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
>  	%piS	001.002.003.004	or 00010002000300040005000600070008
>  	%pISc	1.2.3.4		or 1:2:3:4:5:6:7:8
>  	%pISpc	1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345
> -	%p[Ii]S[pfschnbl]
> +	%p[Ii]S[pfsachnbl]
>  
>  	For printing an IP address without the need to distinguish whether it's
>  	of type AF_INET or AF_INET6, a pointer to a valid 'struct sockaddr',
> @@ -201,7 +201,9 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
>  
>  	The additional 'p', 'f', and 's' specifiers are used to specify port
>  	(IPv4, IPv6), flowinfo (IPv6) and scope (IPv6). Ports have a ':' prefix,
> -	flowinfo a '/' and scope a '%', each followed by the actual value.
> +	flowinfo a '/' and scope a '%', each followed by the actual value. If 'a'
> +	is given, 'p', 'f', and 's' are activated or deactivated depending on
> +	whether or not the sockaddr has a non-zero port, flowinfo, and scope.
>  
>  	In case of an IPv6 address the compressed IPv6 address as described by
>  	http://tools.ietf.org/html/rfc5952 is being used if the additional
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 48ff9c3..9a07284 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1145,7 +1145,7 @@ static noinline_for_stack
>  char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
>  			 struct printf_spec spec, const char *fmt)
>  {
> -	bool have_p = false, have_s = false, have_f = false, have_c = false;
> +	bool have_p = false, have_s = false, have_f = false, have_c = false, have_a = false;
>  	char ip6_addr[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
>  		      sizeof(":12345") + sizeof("/123456789") +
>  		      sizeof("%1234567890")];
> @@ -1169,9 +1169,18 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
>  		case 'c':
>  			have_c = true;
>  			break;
> +		case 'a':
> +			have_a = true;
> +			break;
>  		}
>  	}
>  
> +	if (have_a) {
> +		have_p = sa->sin6_port;
> +		have_s = sa->sin6_scope_id;
> +		have_f = sa->sin6_flowinfo & IPV6_FLOWINFO_MASK;
> +	}
> +

First of all I don't think vsprintf.c should be extended with yet
another piece of unused code, so I hope there's an actual user coming.

Second, please add tests to lib/test_printf.c.

Third, wouldn't it be more convenient/flexible if a meant "print those
that are present _and_ requested"; that is, the above would be

> +		have_p = have_p && sa->sin6_port;
> +		have_s = have_s && sa->sin6_scope_id;
> +		have_f = have_f && sa->sin6_flowinfo & IPV6_FLOWINFO_MASK;

that way one could say %pISaf and have the flowinfo if and only if it's
present, without also unintentionally turning on port or scope_id.

Rasmus

[toc] | [prev] | [next] | [standalone]

#1327651 — Re: [PATCH v2] vsprintf: automatic parameters for %pIS via 'a'

On Fri, Feb 5, 2016 at 1:06 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> First of all I don't think vsprintf.c should be extended with yet
> another piece of unused code, so I hope there's an actual user coming.

There is. Don't worry.

>
> Second, please add tests to lib/test_printf.c.

Sure, no problem. I'll add this in the next version.

>
> Third, wouldn't it be more convenient/flexible if a meant "print those
> that are present _and_ requested"; that is, the above would be
>
>> +             have_p = have_p && sa->sin6_port;
>> +             have_s = have_s && sa->sin6_scope_id;
>> +             have_f = have_f && sa->sin6_flowinfo & IPV6_FLOWINFO_MASK;
>
> that way one could say %pISaf and have the flowinfo if and only if it's
> present, without also unintentionally turning on port or scope_id.

That's a great idea. I'll make this change too for the next version.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web