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

Groups > linux.kernel > #1353387 > unrolled thread

[RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2016-03-08 21:50 +0100
    Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf  use Joe Perches <joe@perches.com> - 2016-03-08 22:10 +0100
      Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-03-11 01:00 +0100
    Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-03-09 14:10 +0100

#1353387 — [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

Passing overlapping source and destination is fragile, and in this
case we can even simplify the code and avoid the huge stack buffer by
using the %p extension for printing a small hex dump.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/usb/atm/usbatm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index db322d9ccb6e..fb47f9883056 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
 static int usbatm_print_packet(struct usbatm_data *instance,
 			       const unsigned char *data, int len)
 {
-	unsigned char buffer[256];
-	int i = 0, j = 0;
+	int i, j;
 
 	for (i = 0; i < len;) {
-		buffer[0] = '\0';
-		sprintf(buffer, "%.3d :", i);
-		for (j = 0; (j < 16) && (i < len); j++, i++)
-			sprintf(buffer, "%s %2.2x", buffer, data[i]);
-		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
+		j = min(16, len-i);
+		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
+		i += j;
 	}
 	return i;
 }
-- 
2.1.4

[toc] | [next] | [standalone]

#1353411 — Re: [RFC 7/7] USB: usbatm: avoid fragile and inefficient snprintf use

On Tue, 2016-03-08 at 21:40 +0100, Rasmus Villemoes wrote:
> Passing overlapping source and destination is fragile, and in this
> case we can even simplify the code and avoid the huge stack buffer by
> using the %p extension for printing a small hex dump.
[]
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
[]
> @@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
>  static int usbatm_print_packet(struct usbatm_data *instance,
>  			       const unsigned char *data, int len)
>  {
> -	unsigned char buffer[256];
> -	int i = 0, j = 0;
> +	int i, j;
>  
>  	for (i = 0; i < len;) {
> -		buffer[0] = '\0';
> -		sprintf(buffer, "%.3d :", i);
> -		for (j = 0; (j < 16) && (i < len); j++, i++)
> -			sprintf(buffer, "%s %2.2x", buffer, data[i]);
> -		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
> +		j = min(16, len-i);
> +		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
> +		i += j;
>  	}
>  	return i;
>  }

Maybe use print_dump_hex_debug()

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

#1355477

On Tue, Mar 08, 2016 at 01:01:09PM -0800, Joe Perches wrote:
> On Tue, 2016-03-08 at 21:40 +0100, Rasmus Villemoes wrote:
> > Passing overlapping source and destination is fragile, and in this
> > case we can even simplify the code and avoid the huge stack buffer by
> > using the %p extension for printing a small hex dump.
> []
> > diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> []
> > @@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
> >  static int usbatm_print_packet(struct usbatm_data *instance,
> >  			       const unsigned char *data, int len)
> >  {
> > -	unsigned char buffer[256];
> > -	int i = 0, j = 0;
> > +	int i, j;
> >  
> >  	for (i = 0; i < len;) {
> > -		buffer[0] = '\0';
> > -		sprintf(buffer, "%.3d :", i);
> > -		for (j = 0; (j < 16) && (i < len); j++, i++)
> > -			sprintf(buffer, "%s %2.2x", buffer, data[i]);
> > -		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
> > +		j = min(16, len-i);
> > +		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
> > +		i += j;
> >  	}
> >  	return i;
> >  }
> 
> Maybe use print_dump_hex_debug()

Yes, please use that instead.

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

#1354095

Hello.

On 3/8/2016 11:40 PM, Rasmus Villemoes wrote:

> Passing overlapping source and destination is fragile, and in this
> case we can even simplify the code and avoid the huge stack buffer by
> using the %p extension for printing a small hex dump.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   drivers/usb/atm/usbatm.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index db322d9ccb6e..fb47f9883056 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -1331,15 +1331,12 @@ MODULE_VERSION(DRIVER_VERSION);
>   static int usbatm_print_packet(struct usbatm_data *instance,
>   			       const unsigned char *data, int len)
>   {
> -	unsigned char buffer[256];
> -	int i = 0, j = 0;
> +	int i, j;
>
>   	for (i = 0; i < len;) {
> -		buffer[0] = '\0';
> -		sprintf(buffer, "%.3d :", i);
> -		for (j = 0; (j < 16) && (i < len); j++, i++)
> -			sprintf(buffer, "%s %2.2x", buffer, data[i]);
> -		dev_dbg(&instance->usb_intf->dev, "%s", buffer);
> +		j = min(16, len-i);

    Kernel style assumes spaces on either side of the operators, like below, no?

> +		dev_dbg(&instance->usb_intf->dev, "%.3d : %*ph", i, j, data + i);
> +		i += j;
>   	}
>   	return i;
>   }

MBR, Sergei

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web