diff mbox

[dpdk-dev,09/18] drivers: net: qede: fix strncpy constant and NUL

Message ID 152575381332.56689.17827274556476490336.stgit@localhost.localdomain (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply issues

Commit Message

Andy Green May 8, 2018, 4:30 a.m. UTC 
  ---
 drivers/net/qede/base/ecore_int.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Shaikh, Shahed May 8, 2018, 5:59 p.m. UTC | #1 
> -----Original Message-----

> From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green

> Sent: Monday, May 7, 2018 11:30 PM

> To: dev@dpdk.org

> Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and

> NUL

> 

> 

> ---

>  drivers/net/qede/base/ecore_int.c |   10 ++++++----

>  1 file changed, 6 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/net/qede/base/ecore_int.c

> b/drivers/net/qede/base/ecore_int.c

> index f43781ba4..c809d84ef 100644

> --- a/drivers/net/qede/base/ecore_int.c

> +++ b/drivers/net/qede/base/ecore_int.c

> @@ -1103,10 +1103,12 @@ static enum _ecore_status_t

> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,

>  						OSAL_SNPRINTF(bit_name, 30,

>  							      p_aeu->bit_name,

>  							      num);

> -					else

> -						OSAL_STRNCPY(bit_name,

> -							     p_aeu->bit_name,

> -							     30);

> +					else {

> +						strncpy(bit_name,

> +							p_aeu->bit_name,

> +							sizeof(bit_name) - 1);

> +						bit_name[sizeof(bit_name) - 1]

> = '\0';

> +					}


I think you can retain OSAL_STRNCPY and just replace 30 with 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.

Thanks,
Shahed
> 

>  					/* We now need to pass bitmask in its

>  					 * correct position.
Bruce Richardson May 8, 2018, 7:53 p.m. UTC | #2 
On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> > Sent: Monday, May 7, 2018 11:30 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and
> > NUL
> > 
> > 
> > ---
> >  drivers/net/qede/base/ecore_int.c |   10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/qede/base/ecore_int.c
> > b/drivers/net/qede/base/ecore_int.c
> > index f43781ba4..c809d84ef 100644
> > --- a/drivers/net/qede/base/ecore_int.c
> > +++ b/drivers/net/qede/base/ecore_int.c
> > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >  						OSAL_SNPRINTF(bit_name, 30,
> >  							      p_aeu->bit_name,
> >  							      num);
> > -					else
> > -						OSAL_STRNCPY(bit_name,
> > -							     p_aeu->bit_name,
> > -							     30);
> > +					else {
> > +						strncpy(bit_name,
> > +							p_aeu->bit_name,
> > +							sizeof(bit_name) - 1);
> > +						bit_name[sizeof(bit_name) - 1]
> > = '\0';
> > +					}
> 
> I think you can retain OSAL_STRNCPY and just replace 30 with 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.

Can that actually be fixed inside OSAL_STRNCPY itself, rather than having
each use needing to explicitly null-terminate?
Shaikh, Shahed May 8, 2018, 8:02 p.m. UTC | #3 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson
> Sent: Tuesday, May 8, 2018 2:53 PM
> To: dev-bounces@dpdk.org
> Cc: Andy Green <andy@warmcat.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant
> and NUL
> 
> On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
> >
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
> > > Sent: Monday, May 7, 2018 11:30 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy
> > > constant and NUL
> > >
> > >
> > > ---
> > >  drivers/net/qede/base/ecore_int.c |   10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/qede/base/ecore_int.c
> > > b/drivers/net/qede/base/ecore_int.c
> > > index f43781ba4..c809d84ef 100644
> > > --- a/drivers/net/qede/base/ecore_int.c
> > > +++ b/drivers/net/qede/base/ecore_int.c
> > > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > >  						OSAL_SNPRINTF(bit_name, 30,
> > >  							      p_aeu->bit_name,
> > >  							      num);
> > > -					else
> > > -						OSAL_STRNCPY(bit_name,
> > > -							     p_aeu->bit_name,
> > > -							     30);
> > > +					else {
> > > +						strncpy(bit_name,
> > > +							p_aeu->bit_name,
> > > +							sizeof(bit_name) - 1);
> > > +						bit_name[sizeof(bit_name) - 1]
> > > = '\0';
> > > +					}
> >
> > I think you can retain OSAL_STRNCPY and just replace 30 with
> 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.
> 
> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each
> use needing to explicitly null-terminate?

Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.

Thanks,
Shahed
Andy Green May 8, 2018, 10:07 p.m. UTC | #4 
On 05/09/2018 04:02 AM, Shaikh, Shahed wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson
>> Sent: Tuesday, May 8, 2018 2:53 PM
>> To: dev-bounces@dpdk.org
>> Cc: Andy Green <andy@warmcat.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant
>> and NUL
>>
>> On Tue, May 08, 2018 at 05:59:47PM +0000, dev-bounces@dpdk.org wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
>>>> Sent: Monday, May 7, 2018 11:30 PM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy
>>>> constant and NUL
>>>>
>>>>
>>>> ---
>>>>   drivers/net/qede/base/ecore_int.c |   10 ++++++----
>>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/qede/base/ecore_int.c
>>>> b/drivers/net/qede/base/ecore_int.c
>>>> index f43781ba4..c809d84ef 100644
>>>> --- a/drivers/net/qede/base/ecore_int.c
>>>> +++ b/drivers/net/qede/base/ecore_int.c
>>>> @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
>>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>>>   						OSAL_SNPRINTF(bit_name, 30,
>>>>   							      p_aeu->bit_name,
>>>>   							      num);
>>>> -					else
>>>> -						OSAL_STRNCPY(bit_name,
>>>> -							     p_aeu->bit_name,
>>>> -							     30);
>>>> +					else {
>>>> +						strncpy(bit_name,
>>>> +							p_aeu->bit_name,
>>>> +							sizeof(bit_name) - 1);
>>>> +						bit_name[sizeof(bit_name) - 1]
>>>> = '\0';
>>>> +					}
>>>
>>> I think you can retain OSAL_STRNCPY and just replace 30 with
>> 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you did.
>>
>> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each
>> use needing to explicitly null-terminate?
> 
> Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.

Doesn't it make more sense to get rid of OSAL_* that bring nothing at 
all to the party?

#define OSAL_SPRINTF(name, pattern, ...) \
         sprintf(name, pattern, ##__VA_ARGS__)
#define OSAL_SNPRINTF(buf, size, format, ...) \
         snprintf(buf, size, format, ##__VA_ARGS__)
#define OSAL_STRLEN(string) strlen(string)
#define OSAL_STRCPY(dst, string) strcpy(dst, string)
#define OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len)
#define OSAL_STRCMP(str1, str2) strcmp(str1, str2)

Do I miss the point or these are just cruft?

-Andy

> Thanks,
> Shahed
>
Shaikh, Shahed May 8, 2018, 11:33 p.m. UTC | #5 
> >>> I think you can retain OSAL_STRNCPY and just replace 30 with

> >> 'bit_name[sizeof(bit_name) - 1'  and then set last byte with '\0' just like you

> did.

> >>

> >> Can that actually be fixed inside OSAL_STRNCPY itself, rather than

> >> having each use needing to explicitly null-terminate?

> >

> > Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.

> 

> Doesn't it make more sense to get rid of OSAL_* that bring nothing at all to the

> party?

> 

> #define OSAL_SPRINTF(name, pattern, ...) \

>          sprintf(name, pattern, ##__VA_ARGS__) #define OSAL_SNPRINTF(buf, size,

> format, ...) \

>          snprintf(buf, size, format, ##__VA_ARGS__) #define OSAL_STRLEN(string)

> strlen(string) #define OSAL_STRCPY(dst, string) strcpy(dst, string) #define

> OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len) #define

> OSAL_STRCMP(str1, str2) strcmp(str1, str2)

> 

> Do I miss the point or these are just cruft?


Hi Andy,

I'll send a cleanup patch for this. For now, you can go ahead with original patch.

Thanks,
Shahed

Acked-by: Shahed Shaikh <shahed.shaikh@cavium.com>


> 

> -Andy

> 

> > Thanks,

> > Shahed

> >
diff mbox

Patch

diff --git a/drivers/net/qede/base/ecore_int.c b/drivers/net/qede/base/ecore_int.c
index f43781ba4..c809d84ef 100644
--- a/drivers/net/qede/base/ecore_int.c
+++ b/drivers/net/qede/base/ecore_int.c
@@ -1103,10 +1103,12 @@  static enum _ecore_status_t ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
 						OSAL_SNPRINTF(bit_name, 30,
 							      p_aeu->bit_name,
 							      num);
-					else
-						OSAL_STRNCPY(bit_name,
-							     p_aeu->bit_name,
-							     30);
+					else {
+						strncpy(bit_name,
+							p_aeu->bit_name,
+							sizeof(bit_name) - 1);
+						bit_name[sizeof(bit_name) - 1] = '\0';
+					}
 
 					/* We now need to pass bitmask in its
 					 * correct position.