[RFC PATCH 0/3] fix *pbl format support

[RFC PATCH 0/3] fix *pbl format support

[Maurizio Lombardi]

Hi,

I tried to fix the "*pb[l]" format issue while taking care of the problems
discussed in this thread:

https://lkml.org/lkml/2015/9/9/153

I would like to know whether this approach is more acceptable to you:

PATCH 1 modifies the code so that the printf_spec struct is not passed by value
anymore, instead a const pointer is used and the structure is copied to
a local variable only when necessary.

PATCH 2 modifies the bitmap_*_string() functions so they'll append "..." to the
output string whenever the buffer is not sufficiently large.

example of output:

*pb: cccccccc,...
*pbl: 1-2,5-7,...

PATCH 3 increases the size of printf_spec.field_width (from s16 to s32).

Maurizio Lombardi (3):
  lib/vsprintf.c: Do not pass printf_spec by value on stack.
  lib/vsprintf.c: append "..." if the *pb[l] output has been truncated.
  lib/vsprintf.c: increase the size of the field_width variable

 lib/vsprintf.c | 275 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 167 insertions(+), 108 deletions(-)

-- 
Maurizio Lombardi

[RFC PATCH 1/3] lib/vsprintf.c: Do not pass printf_spec by value on stack.

[Maurizio Lombardi]

The original code passes the structure by value on the stack,
this limits the size of the printf_spec structure because of
performance reasons.
This patch modifies the code so only a const pointer to the structure
is passed on the stack.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 lib/vsprintf.c | 225 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 118 insertions(+), 107 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b..8707d91 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -390,9 +390,10 @@ struct printf_spec {
 
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
-	     struct printf_spec spec)
+	     const struct printf_spec *specp)
 {
 	/* put_dec requires 2-byte alignment of the buffer. */
+	struct printf_spec spec = *specp;
 	char tmp[3 * sizeof(num)] __aligned(2);
 	char sign;
 	char locase;
@@ -508,9 +509,10 @@ char *number(char *buf, char *end, unsigned long long num,
 }
 
 static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+char *string(char *buf, char *end, const char *s, const struct printf_spec *specp)
 {
 	int len, i;
+	struct printf_spec spec = *specp;
 
 	if ((unsigned long)s < PAGE_SIZE)
 		s = "(null)";
@@ -557,7 +559,7 @@ static void widen(char *buf, char *end, unsigned len, unsigned spaces)
 }
 
 static noinline_for_stack
-char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
+char *dentry_name(char *buf, char *end, const struct dentry *d, const struct printf_spec *specp,
 		  const char *fmt)
 {
 	const char *array[4], *s;
@@ -585,7 +587,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 		}
 	}
 	s = array[--i];
-	for (n = 0; n != spec.precision; n++, buf++) {
+	for (n = 0; n != specp->precision; n++, buf++) {
 		char c = *s++;
 		if (!c) {
 			if (!i)
@@ -597,10 +599,10 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 			*buf = c;
 	}
 	rcu_read_unlock();
-	if (n < spec.field_width) {
+	if (n < specp->field_width) {
 		/* we want to pad the sucker */
-		unsigned spaces = spec.field_width - n;
-		if (!(spec.flags & LEFT)) {
+		unsigned spaces = specp->field_width - n;
+		if (!(specp->flags & LEFT)) {
 			widen(buf - n, end, n, spaces);
 			return buf + spaces;
 		}
@@ -615,9 +617,10 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 
 static noinline_for_stack
 char *symbol_string(char *buf, char *end, void *ptr,
-		    struct printf_spec spec, const char *fmt)
+		    const struct printf_spec *specp, const char *fmt)
 {
 	unsigned long value;
+	struct printf_spec spec = *specp;
 #ifdef CONFIG_KALLSYMS
 	char sym[KSYM_SYMBOL_LEN];
 #endif
@@ -634,19 +637,19 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	else
 		sprint_symbol_no_offset(sym, value);
 
-	return string(buf, end, sym, spec);
+	return string(buf, end, sym, &spec);
 #else
 	spec.field_width = 2 * sizeof(void *);
 	spec.flags |= SPECIAL | SMALL | ZEROPAD;
 	spec.base = 16;
 
-	return number(buf, end, value, spec);
+	return number(buf, end, value, &spec);
 #endif
 }
 
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
-		      struct printf_spec spec, const char *fmt)
+		      const struct printf_spec *specp, const char *fmt)
 {
 #ifndef IO_RSRC_PRINTK_SIZE
 #define IO_RSRC_PRINTK_SIZE	6
@@ -700,73 +703,73 @@ char *resource_string(char *buf, char *end, struct resource *res,
 
 	char *p = sym, *pend = sym + sizeof(sym);
 	int decode = (fmt[0] == 'R') ? 1 : 0;
-	const struct printf_spec *specp;
+	const struct printf_spec *sp;
 
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
-		p = string(p, pend, "io  ", str_spec);
-		specp = &io_spec;
+		p = string(p, pend, "io  ", &str_spec);
+		sp = &io_spec;
 	} else if (res->flags & IORESOURCE_MEM) {
-		p = string(p, pend, "mem ", str_spec);
-		specp = &mem_spec;
+		p = string(p, pend, "mem ", &str_spec);
+		sp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
-		p = string(p, pend, "irq ", str_spec);
-		specp = &dec_spec;
+		p = string(p, pend, "irq ", &str_spec);
+		sp = &dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
-		p = string(p, pend, "dma ", str_spec);
-		specp = &dec_spec;
+		p = string(p, pend, "dma ", &str_spec);
+		sp = &dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
-		p = string(p, pend, "bus ", str_spec);
-		specp = &bus_spec;
+		p = string(p, pend, "bus ", &str_spec);
+		sp = &bus_spec;
 	} else {
-		p = string(p, pend, "??? ", str_spec);
-		specp = &mem_spec;
+		p = string(p, pend, "??? ", &str_spec);
+		sp = &mem_spec;
 		decode = 0;
 	}
 	if (decode && res->flags & IORESOURCE_UNSET) {
-		p = string(p, pend, "size ", str_spec);
-		p = number(p, pend, resource_size(res), *specp);
+		p = string(p, pend, "size ", &str_spec);
+		p = number(p, pend, resource_size(res), sp);
 	} else {
-		p = number(p, pend, res->start, *specp);
+		p = number(p, pend, res->start, sp);
 		if (res->start != res->end) {
 			*p++ = '-';
-			p = number(p, pend, res->end, *specp);
+			p = number(p, pend, res->end, sp);
 		}
 	}
 	if (decode) {
 		if (res->flags & IORESOURCE_MEM_64)
-			p = string(p, pend, " 64bit", str_spec);
+			p = string(p, pend, " 64bit", &str_spec);
 		if (res->flags & IORESOURCE_PREFETCH)
-			p = string(p, pend, " pref", str_spec);
+			p = string(p, pend, " pref", &str_spec);
 		if (res->flags & IORESOURCE_WINDOW)
-			p = string(p, pend, " window", str_spec);
+			p = string(p, pend, " window", &str_spec);
 		if (res->flags & IORESOURCE_DISABLED)
-			p = string(p, pend, " disabled", str_spec);
+			p = string(p, pend, " disabled", &str_spec);
 	} else {
-		p = string(p, pend, " flags ", str_spec);
-		p = number(p, pend, res->flags, flag_spec);
+		p = string(p, pend, " flags ", &str_spec);
+		p = number(p, pend, res->flags, &flag_spec);
 	}
 	*p++ = ']';
 	*p = '\0';
 
-	return string(buf, end, sym, spec);
+	return string(buf, end, sym, specp);
 }
 
 static noinline_for_stack
-char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+char *hex_string(char *buf, char *end, u8 *addr, const struct printf_spec *specp,
 		 const char *fmt)
 {
 	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
 				   negative value, fallback to the default */
 	char separator;
 
-	if (spec.field_width == 0)
+	if (specp->field_width == 0)
 		/* nothing to print */
 		return buf;
 
 	if (ZERO_OR_NULL_PTR(addr))
 		/* NULL pointer */
-		return string(buf, end, NULL, spec);
+		return string(buf, end, NULL, specp);
 
 	switch (fmt[1]) {
 	case 'C':
@@ -783,8 +786,8 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		break;
 	}
 
-	if (spec.field_width > 0)
-		len = min_t(int, spec.field_width, 64);
+	if (specp->field_width > 0)
+		len = min_t(int, specp->field_width, 64);
 
 	for (i = 0; i < len; ++i) {
 		if (buf < end)
@@ -806,12 +809,13 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 
 static noinline_for_stack
 char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
-		    struct printf_spec spec, const char *fmt)
+		    const struct printf_spec *specp, const char *fmt)
 {
 	const int CHUNKSZ = 32;
-	int nr_bits = max_t(int, spec.field_width, 0);
+	int nr_bits = max_t(int, specp->field_width, 0);
 	int i, chunksz;
 	bool first = true;
+	struct printf_spec spec = *specp;
 
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .flags = SMALL | ZEROPAD, .base = 16 };
@@ -838,7 +842,7 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 		first = false;
 
 		spec.field_width = DIV_ROUND_UP(chunksz, 4);
-		buf = number(buf, end, val, spec);
+		buf = number(buf, end, val, &spec);
 
 		chunksz = CHUNKSZ;
 	}
@@ -847,12 +851,13 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 
 static noinline_for_stack
 char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
-			 struct printf_spec spec, const char *fmt)
+			 const struct printf_spec *specp, const char *fmt)
 {
-	int nr_bits = max_t(int, spec.field_width, 0);
+	int nr_bits = max_t(int, specp->field_width, 0);
 	/* current bit is 'cur', most recently seen range is [rbot, rtop] */
 	int cur, rbot, rtop;
 	bool first = true;
+	struct printf_spec spec = *specp;
 
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .base = 10 };
@@ -871,13 +876,13 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 		}
 		first = false;
 
-		buf = number(buf, end, rbot, spec);
+		buf = number(buf, end, rbot, &spec);
 		if (rbot < rtop) {
 			if (buf < end)
 				*buf = '-';
 			buf++;
 
-			buf = number(buf, end, rtop, spec);
+			buf = number(buf, end, rtop, &spec);
 		}
 
 		rbot = cur;
@@ -887,7 +892,7 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 
 static noinline_for_stack
 char *mac_address_string(char *buf, char *end, u8 *addr,
-			 struct printf_spec spec, const char *fmt)
+			 const struct printf_spec *specp, const char *fmt)
 {
 	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
 	char *p = mac_addr;
@@ -920,7 +925,7 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	}
 	*p = '\0';
 
-	return string(buf, end, mac_addr, spec);
+	return string(buf, end, mac_addr, specp);
 }
 
 static noinline_for_stack
@@ -1074,7 +1079,7 @@ char *ip6_string(char *p, const char *addr, const char *fmt)
 
 static noinline_for_stack
 char *ip6_addr_string(char *buf, char *end, const u8 *addr,
-		      struct printf_spec spec, const char *fmt)
+		      const struct printf_spec *specp, const char *fmt)
 {
 	char ip6_addr[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255")];
 
@@ -1083,23 +1088,23 @@ char *ip6_addr_string(char *buf, char *end, const u8 *addr,
 	else
 		ip6_string(ip6_addr, addr, fmt);
 
-	return string(buf, end, ip6_addr, spec);
+	return string(buf, end, ip6_addr, specp);
 }
 
 static noinline_for_stack
 char *ip4_addr_string(char *buf, char *end, const u8 *addr,
-		      struct printf_spec spec, const char *fmt)
+		      const struct printf_spec *specp, const char *fmt)
 {
 	char ip4_addr[sizeof("255.255.255.255")];
 
 	ip4_string(ip4_addr, addr, fmt);
 
-	return string(buf, end, ip4_addr, spec);
+	return string(buf, end, ip4_addr, specp);
 }
 
 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)
+			 const struct printf_spec *specp, const char *fmt)
 {
 	bool have_p = false, have_s = false, have_f = false, have_c = false;
 	char ip6_addr[sizeof("[xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255]") +
@@ -1143,25 +1148,25 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 
 	if (have_p) {
 		*p++ = ':';
-		p = number(p, pend, ntohs(sa->sin6_port), spec);
+		p = number(p, pend, ntohs(sa->sin6_port), specp);
 	}
 	if (have_f) {
 		*p++ = '/';
 		p = number(p, pend, ntohl(sa->sin6_flowinfo &
-					  IPV6_FLOWINFO_MASK), spec);
+					  IPV6_FLOWINFO_MASK), specp);
 	}
 	if (have_s) {
 		*p++ = '%';
-		p = number(p, pend, sa->sin6_scope_id, spec);
+		p = number(p, pend, sa->sin6_scope_id, specp);
 	}
 	*p = '\0';
 
-	return string(buf, end, ip6_addr, spec);
+	return string(buf, end, ip6_addr, specp);
 }
 
 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)
+			 const struct printf_spec *specp, const char *fmt)
 {
 	bool have_p = false;
 	char *p, ip4_addr[sizeof("255.255.255.255") + sizeof(":12345")];
@@ -1187,15 +1192,15 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 	p = ip4_string(ip4_addr, addr, fmt4);
 	if (have_p) {
 		*p++ = ':';
-		p = number(p, pend, ntohs(sa->sin_port), spec);
+		p = number(p, pend, ntohs(sa->sin_port), specp);
 	}
 	*p = '\0';
 
-	return string(buf, end, ip4_addr, spec);
+	return string(buf, end, ip4_addr, specp);
 }
 
 static noinline_for_stack
-char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+char *escaped_string(char *buf, char *end, u8 *addr, const struct printf_spec *specp,
 		     const char *fmt)
 {
 	bool found = true;
@@ -1203,11 +1208,11 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	unsigned int flags = 0;
 	int len;
 
-	if (spec.field_width == 0)
+	if (specp->field_width == 0)
 		return buf;				/* nothing to print */
 
 	if (ZERO_OR_NULL_PTR(addr))
-		return string(buf, end, NULL, spec);	/* NULL pointer */
+		return string(buf, end, NULL, specp);	/* NULL pointer */
 
 
 	do {
@@ -1242,7 +1247,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	if (!flags)
 		flags = ESCAPE_ANY_NP;
 
-	len = spec.field_width < 0 ? 1 : spec.field_width;
+	len = specp->field_width < 0 ? 1 : specp->field_width;
 
 	/*
 	 * string_escape_mem() writes as many characters as it can to
@@ -1256,7 +1261,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 
 static noinline_for_stack
 char *uuid_string(char *buf, char *end, const u8 *addr,
-		  struct printf_spec spec, const char *fmt)
+		  const struct printf_spec *specp, const char *fmt)
 {
 	char uuid[sizeof("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx")];
 	char *p = uuid;
@@ -1298,26 +1303,28 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 		} while (*(++p));
 	}
 
-	return string(buf, end, uuid, spec);
+	return string(buf, end, uuid, specp);
 }
 
 static
 char *netdev_feature_string(char *buf, char *end, const u8 *addr,
-		      struct printf_spec spec)
+		      const struct printf_spec *specp)
 {
+	struct printf_spec spec = *specp;
 	spec.flags |= SPECIAL | SMALL | ZEROPAD;
 	if (spec.field_width == -1)
 		spec.field_width = 2 + 2 * sizeof(netdev_features_t);
 	spec.base = 16;
 
-	return number(buf, end, *(const netdev_features_t *)addr, spec);
+	return number(buf, end, *(const netdev_features_t *)addr, &spec);
 }
 
 static noinline_for_stack
 char *address_val(char *buf, char *end, const void *addr,
-		  struct printf_spec spec, const char *fmt)
+		  const struct printf_spec *specp, const char *fmt)
 {
 	unsigned long long num;
+	struct printf_spec spec = *specp;
 
 	spec.flags |= SPECIAL | SMALL | ZEROPAD;
 	spec.base = 16;
@@ -1334,29 +1341,32 @@ char *address_val(char *buf, char *end, const void *addr,
 		break;
 	}
 
-	return number(buf, end, num, spec);
+	return number(buf, end, num, &spec);
 }
 
 static noinline_for_stack
-char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
+char *clock(char *buf, char *end, struct clk *clk, const struct printf_spec *specp,
 	    const char *fmt)
 {
 	if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
-		return string(buf, end, NULL, spec);
+		return string(buf, end, NULL, specp);
 
 	switch (fmt[1]) {
 	case 'r':
-		return number(buf, end, clk_get_rate(clk), spec);
+		return number(buf, end, clk_get_rate(clk), specp);
 
 	case 'n':
 	default:
 #ifdef CONFIG_COMMON_CLK
-		return string(buf, end, __clk_get_name(clk), spec);
+		return string(buf, end, __clk_get_name(clk), specp);
 #else
-		spec.base = 16;
-		spec.field_width = sizeof(unsigned long) * 2 + 2;
-		spec.flags |= SPECIAL | SMALL | ZEROPAD;
-		return number(buf, end, (unsigned long)clk, spec);
+		{
+			struct printf_spec spec = *specp;
+			spec.base = 16;
+			spec.field_width = sizeof(unsigned long) * 2 + 2;
+			spec.flags |= SPECIAL | SMALL | ZEROPAD;
+			return number(buf, end, (unsigned long)clk, &spec);
+		}
 #endif
 	}
 }
@@ -1455,8 +1465,9 @@ int kptr_restrict __read_mostly;
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
-	      struct printf_spec spec)
+	      const struct printf_spec *specp)
 {
+	struct printf_spec spec = *specp;
 	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
 
 	if (!ptr && *fmt != 'K') {
@@ -1466,7 +1477,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		 */
 		if (spec.field_width == -1)
 			spec.field_width = default_width;
-		return string(buf, end, "(null)", spec);
+		return string(buf, end, "(null)", &spec);
 	}
 
 	switch (*fmt) {
@@ -1477,24 +1488,24 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'S':
 	case 's':
 	case 'B':
-		return symbol_string(buf, end, ptr, spec, fmt);
+		return symbol_string(buf, end, ptr, &spec, fmt);
 	case 'R':
 	case 'r':
-		return resource_string(buf, end, ptr, spec, fmt);
+		return resource_string(buf, end, ptr, &spec, fmt);
 	case 'h':
-		return hex_string(buf, end, ptr, spec, fmt);
+		return hex_string(buf, end, ptr, &spec, fmt);
 	case 'b':
 		switch (fmt[1]) {
 		case 'l':
-			return bitmap_list_string(buf, end, ptr, spec, fmt);
+			return bitmap_list_string(buf, end, ptr, &spec, fmt);
 		default:
-			return bitmap_string(buf, end, ptr, spec, fmt);
+			return bitmap_string(buf, end, ptr, &spec, fmt);
 		}
 	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
 					/* [mM]R (Reverse order; Bluetooth) */
-		return mac_address_string(buf, end, ptr, spec, fmt);
+		return mac_address_string(buf, end, ptr, &spec, fmt);
 	case 'I':			/* Formatted IP supported
 					 * 4:	1.2.3.4
 					 * 6:	0001:0203:...:0708
@@ -1506,9 +1517,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 					 */
 		switch (fmt[1]) {
 		case '6':
-			return ip6_addr_string(buf, end, ptr, spec, fmt);
+			return ip6_addr_string(buf, end, ptr, &spec, fmt);
 		case '4':
-			return ip4_addr_string(buf, end, ptr, spec, fmt);
+			return ip4_addr_string(buf, end, ptr, &spec, fmt);
 		case 'S': {
 			const union {
 				struct sockaddr		raw;
@@ -1518,18 +1529,18 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 
 			switch (sa->raw.sa_family) {
 			case AF_INET:
-				return ip4_addr_string_sa(buf, end, &sa->v4, spec, fmt);
+				return ip4_addr_string_sa(buf, end, &sa->v4, &spec, fmt);
 			case AF_INET6:
-				return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
+				return ip6_addr_string_sa(buf, end, &sa->v6, &spec, fmt);
 			default:
-				return string(buf, end, "(invalid address)", spec);
+				return string(buf, end, "(invalid address)", &spec);
 			}}
 		}
 		break;
 	case 'E':
-		return escaped_string(buf, end, ptr, spec, fmt);
+		return escaped_string(buf, end, ptr, &spec, fmt);
 	case 'U':
-		return uuid_string(buf, end, ptr, spec, fmt);
+		return uuid_string(buf, end, ptr, &spec, fmt);
 	case 'V':
 		{
 			va_list va;
@@ -1549,7 +1560,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				      in_nmi())) {
 			if (spec.field_width == -1)
 				spec.field_width = default_width;
-			return string(buf, end, "pK-error", spec);
+			return string(buf, end, "pK-error", &spec);
 		}
 
 		switch (kptr_restrict) {
@@ -1585,19 +1596,19 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'N':
 		switch (fmt[1]) {
 		case 'F':
-			return netdev_feature_string(buf, end, ptr, spec);
+			return netdev_feature_string(buf, end, ptr, &spec);
 		}
 		break;
 	case 'a':
-		return address_val(buf, end, ptr, spec, fmt);
+		return address_val(buf, end, ptr, &spec, fmt);
 	case 'd':
-		return dentry_name(buf, end, ptr, spec, fmt);
+		return dentry_name(buf, end, ptr, &spec, fmt);
 	case 'C':
-		return clock(buf, end, ptr, spec, fmt);
+		return clock(buf, end, ptr, &spec, fmt);
 	case 'D':
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
-				   spec, fmt);
+				   &spec, fmt);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
@@ -1606,7 +1617,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 	spec.base = 16;
 
-	return number(buf, end, (unsigned long) ptr, spec);
+	return number(buf, end, (unsigned long) ptr, &spec);
 }
 
 /*
@@ -1927,12 +1938,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		}
 
 		case FORMAT_TYPE_STR:
-			str = string(str, end, va_arg(args, char *), spec);
+			str = string(str, end, va_arg(args, char *), &spec);
 			break;
 
 		case FORMAT_TYPE_PTR:
 			str = pointer(fmt, str, end, va_arg(args, void *),
-				      spec);
+				      &spec);
 			while (isalnum(*fmt))
 				fmt++;
 			break;
@@ -1988,7 +1999,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				num = va_arg(args, unsigned int);
 			}
 
-			str = number(str, end, num, spec);
+			str = number(str, end, num, &spec);
 		}
 	}
 
@@ -2364,12 +2375,12 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		case FORMAT_TYPE_STR: {
 			const char *str_arg = args;
 			args += strlen(str_arg) + 1;
-			str = string(str, end, (char *)str_arg, spec);
+			str = string(str, end, (char *)str_arg, &spec);
 			break;
 		}
 
 		case FORMAT_TYPE_PTR:
-			str = pointer(fmt, str, end, get_arg(void *), spec);
+			str = pointer(fmt, str, end, get_arg(void *), &spec);
 			while (isalnum(*fmt))
 				fmt++;
 			break;
@@ -2418,7 +2429,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 				num = get_arg(int);
 			}
 
-			str = number(str, end, num, spec);
+			str = number(str, end, num, &spec);
 		} /* default: */
 		} /* switch(spec.type) */
 	} /* while(*fmt) */
-- 
Maurizio Lombardi

[RFC PATCH 2/3] lib/vsprintf.c: append "..." if the *pb[l] output has been truncated.

[Maurizio Lombardi]

The *pb[l] format may generate a very long string that could exaust
the output buffer capacity;
when such event happens the output could be misleading,
it may appear valid but part of it has been truncated.

This patch modifies the bitmap_*_string() functions so they will append
"..." to the output to inform the user that a truncation happened.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 lib/vsprintf.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8707d91..f49bf54 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -816,6 +816,7 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 	int i, chunksz;
 	bool first = true;
 	struct printf_spec spec = *specp;
+	const char *buf_start = buf;
 
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .flags = SMALL | ZEROPAD, .base = 16 };
@@ -846,6 +847,29 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 
 		chunksz = CHUNKSZ;
 	}
+
+	if (buf >= end && buf_start != end) {
+		int spc = 0;
+		char *trunc = end - 1;
+
+		while (trunc > buf_start) {
+			if (*trunc == ',' && spc > 3) {
+				trunc++;
+				break;
+			}
+			trunc--;
+			spc++;
+		}
+
+		if (spc > 3) {
+			trunc[0] = '.';
+			trunc[1] = '.';
+			trunc[2] = '.';
+			trunc[3] = '\0';
+		} else
+			trunc[0] = '\0';
+	}
+
 	return buf;
 }
 
@@ -858,6 +882,7 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 	int cur, rbot, rtop;
 	bool first = true;
 	struct printf_spec spec = *specp;
+	const char *buf_start = buf;
 
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .base = 10 };
@@ -887,6 +912,29 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 
 		rbot = cur;
 	}
+
+	if (buf >= end && buf_start != end) {
+		int spc = 0;
+		char *trunc = end - 1;
+
+		while (trunc > buf_start) {
+			if (*trunc == ',' && spc > 3) {
+				trunc++;
+				break;
+			}
+			trunc--;
+			spc++;
+		}
+
+		if (spc > 3) {
+			trunc[0] = '.';
+			trunc[1] = '.';
+			trunc[2] = '.';
+			trunc[3] = '\0';
+		} else
+			trunc[0] = '\0';
+	}
+
 	return buf;
 }
 
-- 
Maurizio Lombardi

[RFC PATCH 3/3] lib/vsprintf.c: increase the size of the field_width variable

[Maurizio Lombardi]

When printing a bitmap using the "%*pb[l]" printk format
a 16 bit variable (field_width) is used to store the size of the bitmap.
In some cases 16 bits are not sufficient, the variable overflows and
printk does not work as expected.

This patch fixes the problem by changing the type of field_width to s32.

How to reproduce the bug:

1.load scsi_debug
# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1

2.create VG
# vgcreate tsvg /dev/sdb
  Physical volume "/dev/sdb" successfully created
  Volume group "tsvg" successfully created

3. Bitmap should be set, but still empty
# cat /sys/bus/pseudo/drivers/scsi_debug/map

Expected results:
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f49bf54..9712260 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -384,8 +384,8 @@ struct printf_spec {
 	u8	flags;		/* flags to number() */
 	u8	base;		/* number base, 8, 10 or 16 only */
 	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
-	s16	field_width;	/* width of output field */
 	s16	precision;	/* # of digits/chars */
+	s32	field_width;	/* width of output field */
 };
 
 static noinline_for_stack
-- 
Maurizio Lombardi

Re: [RFC PATCH 0/3] fix *pbl format support

[Rasmus Villemoes]

On Wed, Sep 16 2015, Maurizio Lombardi <mlombard@redhat.com> wrote:

> Hi,
>
> I tried to fix the "*pb[l]" format issue while taking care of the problems
> discussed in this thread:
>
> https://lkml.org/lkml/2015/9/9/153
>
> I would like to know whether this approach is more acceptable to you:
>
> PATCH 1 modifies the code so that the printf_spec struct is not passed by value
> anymore, instead a const pointer is used and the structure is copied to
> a local variable only when necessary.
>

If we want to fix the problem with 3/3, then this seems obviously
necessary. There may be stuff we want to optimize later (for example, I
don't think we should always make a local copy of the entire struct; if
we're only modifying one of the fields, it's better to copy that field
to a local variable and use that).

Nit: Please don't say that the parameter is passed around _on the
stack_. Making it fit in 8 bytes is very much so that sane architectures
have a chance to pass it in a register, and _how_ parameters are passed
around is in any case very arch-dependent. Just say "struct printf_spec
is passed by value".

>
> PATCH 2 modifies the bitmap_*_string() functions so they'll append
>"..." to the
> output string whenever the buffer is not sufficiently large.
>
> example of output:
>
> *pb: cccccccc,...
> *pbl: 1-2,5-7,...

This part I really don't like. We shouldn't make the output depend on
the size of the output buffer (other than truncating it if necessary, of
course).

I haven't looked carefully at your code, but it does seem that you make
sure that at least the return value is as expected, which will make
kasprintf work. But it seems there is another kasprintf
problem. [reminder: kasprintf works by doing a va_copy, then doing a
first call of vsnprintf, passing NULL for the buffer and 0 for the
length to determine the size to allocate, and then doing the actual
formatting with a second call]

+	if (buf >= end && buf_start != end) {
+		int spc = 0;
+		char *trunc = end - 1;
+
+		while (trunc > buf_start) {
+			if (*trunc == ',' && spc > 3) {
+				trunc++;
+				break;
+			}
+			trunc--;
+			spc++;
+		}

On the first call from kasprintf, we have end == NULL + 0 == NULL.
Suppose the format is "hello world %pb". By the time the bitmap helper
is called, we have advanced buf away from end, so the stored buf_start
is != end, and also of course buf >= end. Then we set trunc = (void*)-1,
and trunc will continue to be > buf_start for a very very long time...

I may have misread, or it might be fixable, but I really don't like
playing these subtle games. snprintf already provides a method to
reliably detect truncation; it is up to the user to decide whether and
how to deal with that. But yes, this of course requires that snprintf
actually attempted to format the entire bitmap, which in turn requires
some way to pass the correct size all the way through to the bitmap
formatter.

> PATCH 3 increases the size of printf_spec.field_width (from s16 to s32).

I'm not yet completely convinced this is the right solution. Obviously,
if other problems with the small .field_width size show up, this might
be necessary, but as long as it's only the %pb formatter (and so far
only a single user of that), I think smaller/other hammers should be
thought about. So far I think there've been two alternatives: (1)
reintroduce the dedicated bitmap pretty printer(s), (2) my half-ugly
proposal allowing the user to pass struct printf_bitmap to the %pbh[l]
specifier. I'll try to actually code up (2).

Rasmus

Re: [RFC PATCH 0/3] fix *pbl format support

[Maurizio Lombardi]

Hi,

On 09/16/2015 02:27 PM, Rasmus Villemoes wrote:
> 
> If we want to fix the problem with 3/3, then this seems obviously
> necessary. There may be stuff we want to optimize later (for example, I
> don't think we should always make a local copy of the entire struct;

Yes I know, I just tried not to break anything in the process,
optimizations can be done later.

> I haven't looked carefully at your code, but it does seem that you make
> sure that at least the return value is as expected, which will make
> kasprintf work. But it seems there is another kasprintf
> problem. [reminder: kasprintf works by doing a va_copy, then doing a
> first call of vsnprintf, passing NULL for the buffer and 0 for the
> length to determine the size to allocate, and then doing the actual
> formatting with a second call]

Ah, you're right, PATCH 2 is broken because I didn't think to the case
you described.
Please ignore it, thanks for catching this.

> I'm not yet completely convinced this is the right solution. Obviously,
> if other problems with the small .field_width size show up, this might
> be necessary, but as long as it's only the %pb formatter (and so far
> only a single user of that), I think smaller/other hammers should be
> thought about. So far I think there've been two alternatives: (1)
> reintroduce the dedicated bitmap pretty printer(s)

I have no problem with that, at least it will work again.

Thanks for the review,
Maurizio Lombardi

Re: [RFC PATCH 0/3] fix *pbl format support

[Tejun Heo]

Hello,

On Wed, Sep 16, 2015 at 02:27:23PM +0200, Rasmus Villemoes wrote:
> If we want to fix the problem with 3/3, then this seems obviously
> necessary. There may be stuff we want to optimize later (for example, I
> don't think we should always make a local copy of the entire struct; if
> we're only modifying one of the fields, it's better to copy that field
> to a local variable and use that).

Yeap.

...
> I may have misread, or it might be fixable, but I really don't like
> playing these subtle games. snprintf already provides a method to
> reliably detect truncation; it is up to the user to decide whether and
> how to deal with that. But yes, this of course requires that snprintf
> actually attempted to format the entire bitmap, which in turn requires
> some way to pass the correct size all the way through to the bitmap
> formatter.

Agreed again.

> > PATCH 3 increases the size of printf_spec.field_width (from s16 to s32).
> 
> I'm not yet completely convinced this is the right solution. Obviously,
> if other problems with the small .field_width size show up, this might
> be necessary, but as long as it's only the %pb formatter (and so far
> only a single user of that), I think smaller/other hammers should be
> thought about. So far I think there've been two alternatives: (1)
> reintroduce the dedicated bitmap pretty printer(s), (2) my half-ugly
> proposal allowing the user to pass struct printf_bitmap to the %pbh[l]
> specifier. I'll try to actually code up (2).

I suppose (2) could work too but we really should strive to provide
something convenient to print[fk] users.  The balance here is pretty
one-sided.

Thanks.

-- 
tejun

Re: [RFC PATCH 0/3] fix *pbl format support

[Rasmus Villemoes]

On Wed, Sep 16 2015, Tejun Heo <tj@kernel.org> wrote:

>
> I suppose (2) could work too but we really should strive to provide
> something convenient to print[fk] users.  The balance here is pretty
> one-sided.
>

I just remembered: I noticed a while ago that the qualifier member is
only used inside format_decode (in the end, the information is folded
into the type member), so one might as well use a local variable for
that. This gives option (3): Make field_width a 24 bit bitfield. I think
that should be sufficient for any realistic bitmap that one would
print. The patch is also rather small. Surprisingly, bloat-o-meter even
says that it's also a net win in code size:

add/remove: 18/16 grow/shrink: 3/2 up/down: 5551/-5775 (-224)

(the huge absolute numbers are due to stuff like 

pointer                                        -    1148   +1148
pointer.isra                                1116       -   -1116

so the functions haven't actually changed that much). I'll have to check
how this can be (smaller might still be worse), but at least it doesn't
seem to be a catastrophe in terms of .text bloat.

[Grr, why don't we have a way to do a compile-time assert outside
function context?]

Only compile-tested.

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date: Wed, 16 Sep 2015 22:06:14 +0200
Subject: [RFC] lib/vsprintf.c: expand field_width to 24 bits

Maurizio Lombardi reported a problem with the %pb extension: It
doesn't work for sufficiently large bitmaps, since the size is stashed
in the field_width field of the struct printf_spec, which is currently
an s16. Concretely, this manifested itself in
/sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
printer got a size of 0, which is the 16 bit truncation of the actual
bitmap size.

We do want to keep struct printf_spec at 8 bytes so that it can
cheaply be passed by value. The qualifier field is only used for
internal bookkeeping in format_decode, so we might as well use a local
variable for that. This gives us an additional 8 bits, which we can
then use for the field width. To stay in 8 bytes, we need to do a
little rearranging and make the type member a bitfield as well.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b43b99..cce2a780a82e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -380,13 +380,13 @@ enum format_type {
 };
 
 struct printf_spec {
-	u8	type;		/* format_type enum */
+	u8	type:8;		/* format_type enum */
+	s32	field_width:24;	/* width of output field */
 	u8	flags;		/* flags to number() */
 	u8	base;		/* number base, 8, 10 or 16 only */
-	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
-	s16	field_width;	/* width of output field */
 	s16	precision;	/* # of digits/chars */
 };
+extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
 
 static noinline_for_stack
 char *number(char *buf, char *end, unsigned long long num,
@@ -1633,6 +1633,7 @@ static noinline_for_stack
 int format_decode(const char *fmt, struct printf_spec *spec)
 {
 	const char *start = fmt;
+	char qualifier;
 
 	/* we finished early by reading the field width */
 	if (spec->type == FORMAT_TYPE_WIDTH) {
@@ -1715,16 +1716,16 @@ precision:
 
 qualifier:
 	/* get the conversion qualifier */
-	spec->qualifier = -1;
+	qualifier = 0;
 	if (*fmt == 'h' || _tolower(*fmt) == 'l' ||
 	    _tolower(*fmt) == 'z' || *fmt == 't') {
-		spec->qualifier = *fmt++;
-		if (unlikely(spec->qualifier == *fmt)) {
-			if (spec->qualifier == 'l') {
-				spec->qualifier = 'L';
+		qualifier = *fmt++;
+		if (unlikely(qualifier == *fmt)) {
+			if (qualifier == 'l') {
+				qualifier = 'L';
 				++fmt;
-			} else if (spec->qualifier == 'h') {
-				spec->qualifier = 'H';
+			} else if (qualifier == 'h') {
+				qualifier = 'H';
 				++fmt;
 			}
 		}
@@ -1781,19 +1782,19 @@ qualifier:
 		return fmt - start;
 	}
 
-	if (spec->qualifier == 'L')
+	if (qualifier == 'L')
 		spec->type = FORMAT_TYPE_LONG_LONG;
-	else if (spec->qualifier == 'l') {
+	else if (qualifier == 'l') {
 		BUILD_BUG_ON(FORMAT_TYPE_ULONG + SIGN != FORMAT_TYPE_LONG);
 		spec->type = FORMAT_TYPE_ULONG + (spec->flags & SIGN);
-	} else if (_tolower(spec->qualifier) == 'z') {
+	} else if (_tolower(qualifier) == 'z') {
 		spec->type = FORMAT_TYPE_SIZE_T;
-	} else if (spec->qualifier == 't') {
+	} else if (qualifier == 't') {
 		spec->type = FORMAT_TYPE_PTRDIFF;
-	} else if (spec->qualifier == 'H') {
+	} else if (qualifier == 'H') {
 		BUILD_BUG_ON(FORMAT_TYPE_UBYTE + SIGN != FORMAT_TYPE_BYTE);
 		spec->type = FORMAT_TYPE_UBYTE + (spec->flags & SIGN);
-	} else if (spec->qualifier == 'h') {
+	} else if (qualifier == 'h') {
 		BUILD_BUG_ON(FORMAT_TYPE_USHORT + SIGN != FORMAT_TYPE_SHORT);
 		spec->type = FORMAT_TYPE_USHORT + (spec->flags & SIGN);
 	} else {
-- 
2.1.3

Re: [RFC PATCH 0/3] fix *pbl format support

[Maurizio Lombardi]

Hi Rasmus,

On 09/16/2015 10:35 PM, Rasmus Villemoes wrote:
> 
> I just remembered: I noticed a while ago that the qualifier member is
> only used inside format_decode (in the end, the information is folded
> into the type member), so one might as well use a local variable for
> that. This gives option (3): Make field_width a 24 bit bitfield. I think
> that should be sufficient for any realistic bitmap that one would
> print. The patch is also rather small. Surprisingly, bloat-o-meter even
> says that it's also a net win in code size:

I tested your patch with scsi-debug and it works for me:

# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
# cat /sys/bus/pseudo/drivers/scsi_debug/map

# vgcreate tsvg /dev/sdb
  Physical volume "/dev/sdb" successfully created
  Volume group "tsvg" successfully created
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15
# lvcreate -V200m -l99%FREE -T tsvg/pool -n lv1 --discards ignore
  Logical volume "lv1" created.
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-31,2048-2055,501760-501871

Thanks,
Maurizio Lombardi


> 
> add/remove: 18/16 grow/shrink: 3/2 up/down: 5551/-5775 (-224)
> 
> (the huge absolute numbers are due to stuff like 
> 
> pointer                                        -    1148   +1148
> pointer.isra                                1116       -   -1116
> 
> so the functions haven't actually changed that much). I'll have to check
> how this can be (smaller might still be worse), but at least it doesn't
> seem to be a catastrophe in terms of .text bloat.
> 
> [Grr, why don't we have a way to do a compile-time assert outside
> function context?]
> 
> Only compile-tested.
> 
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Date: Wed, 16 Sep 2015 22:06:14 +0200
> Subject: [RFC] lib/vsprintf.c: expand field_width to 24 bits
> 
> Maurizio Lombardi reported a problem with the %pb extension: It
> doesn't work for sufficiently large bitmaps, since the size is stashed
> in the field_width field of the struct printf_spec, which is currently
> an s16. Concretely, this manifested itself in
> /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
> printer got a size of 0, which is the 16 bit truncation of the actual
> bitmap size.
> 
> We do want to keep struct printf_spec at 8 bytes so that it can
> cheaply be passed by value. The qualifier field is only used for
> internal bookkeeping in format_decode, so we might as well use a local
> variable for that. This gives us an additional 8 bits, which we can
> then use for the field width. To stay in 8 bytes, we need to do a
> little rearranging and make the type member a bitfield as well.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 95cd63b43b99..cce2a780a82e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -380,13 +380,13 @@ enum format_type {
>  };
>  
>  struct printf_spec {
> -	u8	type;		/* format_type enum */
> +	u8	type:8;		/* format_type enum */
> +	s32	field_width:24;	/* width of output field */
>  	u8	flags;		/* flags to number() */
>  	u8	base;		/* number base, 8, 10 or 16 only */
> -	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
> -	s16	field_width;	/* width of output field */
>  	s16	precision;	/* # of digits/chars */
>  };
> +extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
>  
>  static noinline_for_stack
>  char *number(char *buf, char *end, unsigned long long num,
> @@ -1633,6 +1633,7 @@ static noinline_for_stack
>  int format_decode(const char *fmt, struct printf_spec *spec)
>  {
>  	const char *start = fmt;
> +	char qualifier;
>  
>  	/* we finished early by reading the field width */
>  	if (spec->type == FORMAT_TYPE_WIDTH) {
> @@ -1715,16 +1716,16 @@ precision:
>  
>  qualifier:
>  	/* get the conversion qualifier */
> -	spec->qualifier = -1;
> +	qualifier = 0;
>  	if (*fmt == 'h' || _tolower(*fmt) == 'l' ||
>  	    _tolower(*fmt) == 'z' || *fmt == 't') {
> -		spec->qualifier = *fmt++;
> -		if (unlikely(spec->qualifier == *fmt)) {
> -			if (spec->qualifier == 'l') {
> -				spec->qualifier = 'L';
> +		qualifier = *fmt++;
> +		if (unlikely(qualifier == *fmt)) {
> +			if (qualifier == 'l') {
> +				qualifier = 'L';
>  				++fmt;
> -			} else if (spec->qualifier == 'h') {
> -				spec->qualifier = 'H';
> +			} else if (qualifier == 'h') {
> +				qualifier = 'H';
>  				++fmt;
>  			}
>  		}
> @@ -1781,19 +1782,19 @@ qualifier:
>  		return fmt - start;
>  	}
>  
> -	if (spec->qualifier == 'L')
> +	if (qualifier == 'L')
>  		spec->type = FORMAT_TYPE_LONG_LONG;
> -	else if (spec->qualifier == 'l') {
> +	else if (qualifier == 'l') {
>  		BUILD_BUG_ON(FORMAT_TYPE_ULONG + SIGN != FORMAT_TYPE_LONG);
>  		spec->type = FORMAT_TYPE_ULONG + (spec->flags & SIGN);
> -	} else if (_tolower(spec->qualifier) == 'z') {
> +	} else if (_tolower(qualifier) == 'z') {
>  		spec->type = FORMAT_TYPE_SIZE_T;
> -	} else if (spec->qualifier == 't') {
> +	} else if (qualifier == 't') {
>  		spec->type = FORMAT_TYPE_PTRDIFF;
> -	} else if (spec->qualifier == 'H') {
> +	} else if (qualifier == 'H') {
>  		BUILD_BUG_ON(FORMAT_TYPE_UBYTE + SIGN != FORMAT_TYPE_BYTE);
>  		spec->type = FORMAT_TYPE_UBYTE + (spec->flags & SIGN);
> -	} else if (spec->qualifier == 'h') {
> +	} else if (qualifier == 'h') {
>  		BUILD_BUG_ON(FORMAT_TYPE_USHORT + SIGN != FORMAT_TYPE_SHORT);
>  		spec->type = FORMAT_TYPE_USHORT + (spec->flags & SIGN);
>  	} else {
> 

Re: [RFC PATCH 0/3] fix *pbl format support

[Rasmus Villemoes]

On Mon, Sep 21 2015, Maurizio Lombardi <mlombard@redhat.com> wrote:

> Hi Rasmus,
>
> On 09/16/2015 10:35 PM, Rasmus Villemoes wrote:
>> 
>> I just remembered: I noticed a while ago that the qualifier member is
>> only used inside format_decode (in the end, the information is folded
>> into the type member), so one might as well use a local variable for
>> that. This gives option (3): Make field_width a 24 bit bitfield. I think
>> that should be sufficient for any realistic bitmap that one would
>> print. The patch is also rather small. Surprisingly, bloat-o-meter even
>> says that it's also a net win in code size:
>
> I tested your patch with scsi-debug and it works for me:

Thanks for testing it! What do people think of this option?

Rasmus