Patchwork [RFC] vsprintf: Add %p*D extension for 80211 SSIDs

login
register
mail settings
Submitter Joe Perches
Date Jan. 7, 2013, 4:49 a.m.
Message ID <1357534195.21481.31.camel@joe-AO722>
Download mbox | patch
Permalink /patch/209854/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Joe Perches - Jan. 7, 2013, 4:49 a.m.
On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> Maybe these days this should be another vsprintf %p extension
> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> 
> (or maybe extend %ph for ssids with %*phs, length, array)

Maybe like this:

 lib/vsprintf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang - Jan. 7, 2013, 6:07 a.m.
于 2013年01月07日 12:49, Joe Perches 写道:
> On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
>> Maybe these days this should be another vsprintf %p extension
>> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
>>
>> (or maybe extend %ph for ssids with %*phs, length, array)
> 

  excuse me:
    I do not quite know how to reply the [RFC PATCH].
    it would be better if you can tell me how to do for [RFC PATCH], thanks.

    :-)


  at least for me:

    although it is good idea to add common, widely used features in public tools function.

    after searching the relative source code:
      it seems print ssid is not quite widely used (although it is a common feature).
        it is used by drivers/net/wireless/libertas
        it is used by drivers/net/wireless/ipw2x00
        no additional using in current kernel source code wide.
      if another modules want to print ssid,
        they can still call print_ssid now (EXPORT_SYMBOL in net/wireless).

    so at least now, it is not necessary to add this feature to public tools function.

  Regards

gchen.

> Maybe like this:
> 
>  lib/vsprintf.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index fab33a9..98916a0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -26,6 +26,7 @@
>  #include <linux/math64.h>
>  #include <linux/uaccess.h>
>  #include <linux/ioport.h>
> +#include <linux/ieee80211.h>
>  #include <net/addrconf.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
> @@ -660,10 +661,59 @@ char *resource_string(char *buf, char *end, struct resource *res,
>  }
>  
>  static noinline_for_stack
> +char *ssid_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> +		  const char *fmt)
> +{
> +	int i, len = 1;		/* if we pass %*p, field width remains
> +				   negative value, fallback to the default */
> +
> +	if (spec.field_width == 0)
> +		/* nothing to print */
> +		return buf;
> +
> +	if (ZERO_OR_NULL_PTR(addr))
> +		/* NULL pointer */
> +		return string(buf, end, NULL, spec);
> +
> +	if (spec.field_width > 0)
> +		len = min_t(int, spec.field_width, IEEE80211_MAX_SSID_LEN);
> +
> +	for (i = 0; i < len && buf < end; i++) {
> +		if (isprint(addr[i])) {
> +			*buf++ = addr[i];
> +			continue;
> +		}
> +		*buf++ = '\\';
> +		if (buf >= end)
> +			continue;
> +		if (addr[i] == '\0')
> +			*buf++ = '0';
> +		else if (addr[i] == '\n')
> +			*buf++ = 'n';
> +		else if (addr[i] == '\r')
> +			*buf++ = 'r';
> +		else if (addr[i] == '\t')
> +			*buf++ = 't';
> +		else if (addr[i] == '\\')
> +			*buf++ = '\\';
> +		else {
> +			if (buf < end)
> +				*buf++ = ((addr[i] >> 6) & 7) + '0';
> +			if (buf < end)
> +				*buf++ = ((addr[i] >> 3) & 7) + '0';
> +			if (buf < end)
> +				*buf++ = ((addr[i] >> 0) & 7) + '0';
> +		}
> +	}
> +
> +	return buf;
> +}
> +
> +static noinline_for_stack
>  char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  		 const char *fmt)
>  {
> -	int i, len = 1;		/* if we pass '%ph[CDN]', field witdh remains
> +	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
>  				   negative value, fallback to the default */
>  	char separator;
>  
> @@ -695,7 +745,6 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  
>  	for (i = 0; i < len && buf < end - 1; i++) {
>  		buf = hex_byte_pack(buf, addr[i]);
> -
>  		if (buf < end && separator && i != len - 1)
>  			*buf++ = separator;
>  	}
> @@ -1016,6 +1065,8 @@ int kptr_restrict __read_mostly;
>   *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
>   *           little endian output byte order is:
>   *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
> + * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
> +         characters with a leading \ and octal or [0nrt\]
>   * - 'V' For a struct va_format which contains a format string * and va_list *,
>   *       call vsnprintf(->format, *->va_list).
>   *       Implements a "recursive vsnprintf".
> @@ -1088,6 +1139,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		break;
>  	case 'U':
>  		return uuid_string(buf, end, ptr, spec, fmt);
> +	case 'D':
> +		return ssid_string(buf, end, ptr, spec, fmt);
>  	case 'V':
>  		{
>  			va_list va;
> 
> 
> 
>
Joe Perches - Jan. 7, 2013, 6:37 a.m.
On Mon, 2013-01-07 at 14:07 +0800, Chen Gang wrote:
> 于 2013年01月07日 12:49, Joe Perches 写道:
> > On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> >> Maybe these days this should be another vsprintf %p extension
> >> like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> >>
> >> (or maybe extend %ph for ssids with %*phs, length, array)
> > 
>   excuse me:
>     I do not quite know how to reply the [RFC PATCH].
>     it would be better if you can tell me how to do for [RFC PATCH], thanks.

You did fine except you unnecessarily quoted the entire original email.
Remember to trim your replies please.  _lots_ of people read these
mailing lists and unnecessary quoting wastes all of their times.

>   at least for me:
>     although it is good idea to add common, widely used features in public tools function.

It's akin to most of the minor uuid/bluetooth mac, etc codes
in vsprintf I've added.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang - Jan. 7, 2013, 6:58 a.m.
于 2013年01月07日 14:37, Joe Perches 写道:
> You did fine except you unnecessarily quoted the entire original email.
> Remember to trim your replies please.  _lots_ of people read these
> mailing lists and unnecessary quoting wastes all of their times.
> 
  ok, thank you.

  I should notice, next time.  :-)

>> >   at least for me:
>> >     although it is good idea to add common, widely used features in public tools function.
> It's akin to most of the minor uuid/bluetooth mac, etc codes

  after see the comments for function pointer in lib/vsprintf.c
  sorry for I do not think it is suitable to add ssid in vsprintf
    all formate features in vsprintf are face to kernel wide, not for one sub system
      one exception is for mac and ip:
        they have already been well known in kernel wide
        every one knows about mac and ip.
    I feel, our ssid is not quite well known in kernel wide

  maybe current place (net/wireless/) is not quite suitable for print_ssid:
    for bluetooth, it is not belong to net/wireless.
    if necessary, I suggest to reconstruct it within net sub system wide, not in kernel wide.


  (it is also my fault for my original reply, it is not quite clear)


  Regards
Johannes Berg - Jan. 7, 2013, 7:47 a.m.
On Sun, 2013-01-06 at 20:49 -0800, Joe Perches wrote:
> On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> > Maybe these days this should be another vsprintf %p extension
> > like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> > 
> > (or maybe extend %ph for ssids with %*phs, length, array)
> 
> Maybe like this:

> + * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
> +         characters with a leading \ and octal or [0nrt\]

Honestly, I'm not sure it's worth it. print_ssid() is used in two or
three legacy drivers only, not in any modern driver, and is unlikely to
be used in the more modern drivers due to tracing etc.

johannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 7, 2013, 5:22 p.m.
On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
> On Sun, 2013-01-06 at 20:49 -0800, Joe Perches wrote:
> > On Sun, 2013-01-06 at 19:19 -0800, Joe Perches wrote:
> > > Maybe these days this should be another vsprintf %p extension
> > > like %pM when the DECLARE_MAC_BUF/print_mac uses were converted.
> > > 
> > > (or maybe extend %ph for ssids with %*phs, length, array)
> > 
> > Maybe like this:
> 
> > + * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
> > +         characters with a leading \ and octal or [0nrt\]
> 
> Honestly, I'm not sure it's worth it.

Neither am I really, the only value I see in it is the
reduction in stack consumption by 100 bytes or so per use.

> print_ssid() is used in two or
> three legacy drivers only, not in any modern driver, and is unlikely to
> be used in the more modern drivers due to tracing etc.

Swell.  It was just another way to correct those overrun
errors Chen Gang found.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang - Jan. 8, 2013, 2:57 a.m.
于 2013年01月08日 01:22, Joe Perches 写道:
> On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
>> > print_ssid() is used in two or
>> > three legacy drivers only, not in any modern driver, and is unlikely to
>> > be used in the more modern drivers due to tracing etc.
> Swell.  It was just another way to correct those overrun
> errors Chen Gang found.
> 

  sorry, I am not quite clear about what you said.
Joe Perches - Jan. 8, 2013, 3:11 a.m.
On Tue, 2013-01-08 at 10:57 +0800, Chen Gang wrote:
> 于 2013年01月08日 01:22, Joe Perches 写道:
> > On Mon, 2013-01-07 at 08:47 +0100, Johannes Berg wrote:
> >> > print_ssid() is used in two or
> >> > three legacy drivers only, not in any modern driver, and is unlikely to
> >> > be used in the more modern drivers due to tracing etc.
> > Swell.  It was just another way to correct those overrun
> > errors Chen Gang found.
> > 
>   sorry, I am not quite clear about what you said.

You found some overrun errors and proposed a patch.
Your solution could output incomplete SSIDs.

I proposed a different patch that would fully output
any binary/non-ascii printable SSID.

I also proposed a different mechanism to avoid the
overrun via printk and also avoid possibly excessive
stack consumption as a means for John Linville to
select 1 of the above options.

I don't care what is done with any of those proposed
patches.

Clear?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang - Jan. 8, 2013, 3:20 a.m.
于 2013年01月08日 11:11, Joe Perches 写道:
> On Tue, 2013-01-08 at 10:57 +0800, Chen Gang wrote:
>>   sorry, I am not quite clear about what you said.
> 
> You found some overrun errors and proposed a patch.
> Your solution could output incomplete SSIDs.
> 
> I proposed a different patch that would fully output
> any binary/non-ascii printable SSID.
> 
> I also proposed a different mechanism to avoid the
> overrun via printk and also avoid possibly excessive
> stack consumption as a means for John Linville to
> select 1 of the above options.
> 
> I don't care what is done with any of those proposed
> patches.
> 
> Clear?
> 

  ok

Patch

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index fab33a9..98916a0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -26,6 +26,7 @@ 
 #include <linux/math64.h>
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
+#include <linux/ieee80211.h>
 #include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
@@ -660,10 +661,59 @@  char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
+char *ssid_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		  const char *fmt)
+{
+	int i, len = 1;		/* if we pass %*p, field width remains
+				   negative value, fallback to the default */
+
+	if (spec.field_width == 0)
+		/* nothing to print */
+		return buf;
+
+	if (ZERO_OR_NULL_PTR(addr))
+		/* NULL pointer */
+		return string(buf, end, NULL, spec);
+
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, IEEE80211_MAX_SSID_LEN);
+
+	for (i = 0; i < len && buf < end; i++) {
+		if (isprint(addr[i])) {
+			*buf++ = addr[i];
+			continue;
+		}
+		*buf++ = '\\';
+		if (buf >= end)
+			continue;
+		if (addr[i] == '\0')
+			*buf++ = '0';
+		else if (addr[i] == '\n')
+			*buf++ = 'n';
+		else if (addr[i] == '\r')
+			*buf++ = 'r';
+		else if (addr[i] == '\t')
+			*buf++ = 't';
+		else if (addr[i] == '\\')
+			*buf++ = '\\';
+		else {
+			if (buf < end)
+				*buf++ = ((addr[i] >> 6) & 7) + '0';
+			if (buf < end)
+				*buf++ = ((addr[i] >> 3) & 7) + '0';
+			if (buf < end)
+				*buf++ = ((addr[i] >> 0) & 7) + '0';
+		}
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		 const char *fmt)
 {
-	int i, len = 1;		/* if we pass '%ph[CDN]', field witdh remains
+	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
 				   negative value, fallback to the default */
 	char separator;
 
@@ -695,7 +745,6 @@  char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 
 	for (i = 0; i < len && buf < end - 1; i++) {
 		buf = hex_byte_pack(buf, addr[i]);
-
 		if (buf < end && separator && i != len - 1)
 			*buf++ = separator;
 	}
@@ -1016,6 +1065,8 @@  int kptr_restrict __read_mostly;
  *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
  *           little endian output byte order is:
  *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
+ * - 'D' For a 80211 SSID, it prints the SSID escaping any non-printable
+         characters with a leading \ and octal or [0nrt\]
  * - 'V' For a struct va_format which contains a format string * and va_list *,
  *       call vsnprintf(->format, *->va_list).
  *       Implements a "recursive vsnprintf".
@@ -1088,6 +1139,8 @@  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		break;
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
+	case 'D':
+		return ssid_string(buf, end, ptr, spec, fmt);
 	case 'V':
 		{
 			va_list va;