Patchwork [2/2] usb: dwc3: debug: purge usage of strcat

login
register
mail settings
Submitter Felipe Balbi
Date Feb. 5, 2019, 11:11 a.m.
Message ID <20190205111118.26842-2-felipe.balbi@linux.intel.com>
Download mbox | patch
Permalink /patch/718285/
State New
Headers show

Comments

Felipe Balbi - Feb. 5, 2019, 11:11 a.m.
Now that buffer size is always passed around, we don't need to rely on
strcat anymore.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/debug.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
Jack Pham - Feb. 5, 2019, 10:42 p.m.
Hi Felipe,

On Tue, Feb 05, 2019 at 01:11:18PM +0200, Felipe Balbi wrote:
> Now that buffer size is always passed around, we don't need to rely on
> strcat anymore.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  drivers/usb/dwc3/debug.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
> index e925a6b73005..6759a7efd8d5 100644
> --- a/drivers/usb/dwc3/debug.h
> +++ b/drivers/usb/dwc3/debug.h
> @@ -545,21 +545,25 @@ static inline const char *dwc3_ep_event_string(char *str, size_t size,
>  				status & DEPEVT_STATUS_TRANSFER_ACTIVE ?
>  				" (Active)" : " (Not Active)");
>  
> +		len = strlen(str);
> +

I don't see 'len' used after this. Did you mean to s/ret/len/ in the
snprintf() calls below?

Alternatively couldn't you just use 'ret' as an accumulator each time
snprintf() is called to keep count of the bytes written to the buffer
instead of needing to call strlen() [O(n) each time] multiple times?

Thanks,
Jack

>  		/* Control Endpoints */
>  		if (epnum <= 1) {
>  			int phase = DEPEVT_STATUS_CONTROL_PHASE(event->status);
>  
>  			switch (phase) {
>  			case DEPEVT_STATUS_CONTROL_DATA:
> -				strcat(str, " [Data Phase]");
> +				snprintf(str + ret, size - ret,
> +						" [Data Phase]");
>  				break;
>  			case DEPEVT_STATUS_CONTROL_STATUS:
> -				strcat(str, " [Status Phase]");
> +				snprintf(str + ret, size - ret,
> +						" [Status Phase]");
>  			}
>  		}
>  		break;
>  	case DWC3_DEPEVT_RXTXFIFOEVT:
> -		strcat(str, "FIFO");
> +		snprintf(str + ret, size - ret, "FIFO");
>  		break;
>  	case DWC3_DEPEVT_STREAMEVT:
>  		status = event->status;
> @@ -571,13 +575,13 @@ static inline const char *dwc3_ep_event_string(char *str, size_t size,
>  			break;
>  		case DEPEVT_STREAMEVT_NOTFOUND:
>  		default:
> -			strcat(str, " Stream Not Found");
> +			snprintf(str + ret, size - ret, " Stream Not Found");
>  			break;
>  		}
>  
>  		break;
>  	case DWC3_DEPEVT_EPCMDCMPLT:
> -		strcat(str, "Endpoint Command Complete");
> +		snprintf(str + ret, size - ret, "Endpoint Command Complete");
>  		break;
>  	default:
>  		snprintf(str, size, "UNKNOWN");
Felipe Balbi - Feb. 6, 2019, 6:29 a.m.
Hi,

Jack Pham <jackp@codeaurora.org> writes:
> On Tue, Feb 05, 2019 at 01:11:18PM +0200, Felipe Balbi wrote:
>> Now that buffer size is always passed around, we don't need to rely on
>> strcat anymore.
>> 
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>>  drivers/usb/dwc3/debug.h | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>> index e925a6b73005..6759a7efd8d5 100644
>> --- a/drivers/usb/dwc3/debug.h
>> +++ b/drivers/usb/dwc3/debug.h
>> @@ -545,21 +545,25 @@ static inline const char *dwc3_ep_event_string(char *str, size_t size,
>>  				status & DEPEVT_STATUS_TRANSFER_ACTIVE ?
>>  				" (Active)" : " (Not Active)");
>>  
>> +		len = strlen(str);
>> +
>
> I don't see 'len' used after this. Did you mean to s/ret/len/ in the
> snprintf() calls below?
>
> Alternatively couldn't you just use 'ret' as an accumulator each time
> snprintf() is called to keep count of the bytes written to the buffer
> instead of needing to call strlen() [O(n) each time] multiple times?

yeah, this is leftover. Just forgot to remove it.
Felipe Balbi - Feb. 6, 2019, 6:31 a.m.
Felipe Balbi <felipe.balbi@linux.intel.com> writes:

> Hi,
>
> Jack Pham <jackp@codeaurora.org> writes:
>> On Tue, Feb 05, 2019 at 01:11:18PM +0200, Felipe Balbi wrote:
>>> Now that buffer size is always passed around, we don't need to rely on
>>> strcat anymore.
>>> 
>>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>>> ---
>>>  drivers/usb/dwc3/debug.h | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>>> index e925a6b73005..6759a7efd8d5 100644
>>> --- a/drivers/usb/dwc3/debug.h
>>> +++ b/drivers/usb/dwc3/debug.h
>>> @@ -545,21 +545,25 @@ static inline const char *dwc3_ep_event_string(char *str, size_t size,
>>>  				status & DEPEVT_STATUS_TRANSFER_ACTIVE ?
>>>  				" (Active)" : " (Not Active)");
>>>  
>>> +		len = strlen(str);
>>> +
>>
>> I don't see 'len' used after this. Did you mean to s/ret/len/ in the
>> snprintf() calls below?
>>
>> Alternatively couldn't you just use 'ret' as an accumulator each time
>> snprintf() is called to keep count of the bytes written to the buffer
>> instead of needing to call strlen() [O(n) each time] multiple times?
>
> yeah, this is leftover. Just forgot to remove it.

Just to be clear, removed on a separate patch :-)

Patch

diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index e925a6b73005..6759a7efd8d5 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -545,21 +545,25 @@  static inline const char *dwc3_ep_event_string(char *str, size_t size,
 				status & DEPEVT_STATUS_TRANSFER_ACTIVE ?
 				" (Active)" : " (Not Active)");
 
+		len = strlen(str);
+
 		/* Control Endpoints */
 		if (epnum <= 1) {
 			int phase = DEPEVT_STATUS_CONTROL_PHASE(event->status);
 
 			switch (phase) {
 			case DEPEVT_STATUS_CONTROL_DATA:
-				strcat(str, " [Data Phase]");
+				snprintf(str + ret, size - ret,
+						" [Data Phase]");
 				break;
 			case DEPEVT_STATUS_CONTROL_STATUS:
-				strcat(str, " [Status Phase]");
+				snprintf(str + ret, size - ret,
+						" [Status Phase]");
 			}
 		}
 		break;
 	case DWC3_DEPEVT_RXTXFIFOEVT:
-		strcat(str, "FIFO");
+		snprintf(str + ret, size - ret, "FIFO");
 		break;
 	case DWC3_DEPEVT_STREAMEVT:
 		status = event->status;
@@ -571,13 +575,13 @@  static inline const char *dwc3_ep_event_string(char *str, size_t size,
 			break;
 		case DEPEVT_STREAMEVT_NOTFOUND:
 		default:
-			strcat(str, " Stream Not Found");
+			snprintf(str + ret, size - ret, " Stream Not Found");
 			break;
 		}
 
 		break;
 	case DWC3_DEPEVT_EPCMDCMPLT:
-		strcat(str, "Endpoint Command Complete");
+		snprintf(str + ret, size - ret, "Endpoint Command Complete");
 		break;
 	default:
 		snprintf(str, size, "UNKNOWN");