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

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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