[dpdk-dev,v4,09/18] net/qede: strncpy length constant and NUL
Checks
Commit Message
Signed-off-by: Andy Green <andy@warmcat.com>
---
drivers/net/qede/base/ecore_int.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
>
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
> drivers/net/qede/base/ecore_int.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/qede/base/ecore_int.c
> b/drivers/net/qede/base/ecore_int.c
> index f43781ba4..d9e22b5ed 100644
> --- a/drivers/net/qede/base/ecore_int.c
> +++ b/drivers/net/qede/base/ecore_int.c
> @@ -6,6 +6,8 @@
> * See LICENSE.qede_pmd for copyright and licensing details.
> */
>
> +#include <rte_string_fns.h>
> +
> #include "bcm_osal.h"
> #include "ecore.h"
> #include "ecore_spq.h"
> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> p_aeu->bit_name,
> num);
> else
> - OSAL_STRNCPY(bit_name,
> - p_aeu->bit_name,
> - 30);
> + strlcpy(bit_name,
> + p_aeu->bit_name,
> + sizeof(bit_name));
>
> /* We now need to pass bitmask in its
> * correct position.
I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
However, this modifies base driver code, so it is up to the maintainers to make that decision.
(CC'ing maintainers here).
Also, missing fixes line and CC stable.
Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
Cc: stable@dpdk.org
On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Friday, May 11, 2018 2:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
>> NUL
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>> drivers/net/qede/base/ecore_int.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/qede/base/ecore_int.c
>> b/drivers/net/qede/base/ecore_int.c
>> index f43781ba4..d9e22b5ed 100644
>> --- a/drivers/net/qede/base/ecore_int.c
>> +++ b/drivers/net/qede/base/ecore_int.c
>> @@ -6,6 +6,8 @@
>> * See LICENSE.qede_pmd for copyright and licensing details.
>> */
>>
>> +#include <rte_string_fns.h>
>> +
>> #include "bcm_osal.h"
>> #include "ecore.h"
>> #include "ecore_spq.h"
>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>> p_aeu->bit_name,
>> num);
>> else
>> - OSAL_STRNCPY(bit_name,
>> - p_aeu->bit_name,
>> - 30);
>> + strlcpy(bit_name,
>> + p_aeu->bit_name,
>> + sizeof(bit_name));
>>
>> /* We now need to pass bitmask in its
>> * correct position.
>
> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
> modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
>
> However, this modifies base driver code, so it is up to the maintainers to make that decision.
> (CC'ing maintainers here).
There's no value for any OSAL_* that simply defines itself to be the
same as the direct api, as does OSAL_STRNCPY.
It's better to just remove any OSAL_* that calls straight through since
all it does is obfuscate what the code does, for no benefit.
> Also, missing fixes line and CC stable.
>
> Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
The idea of this "Fixes" thing is to look up in git blame what is being
edited is it? If so what's the benefit of that over using git if you
ever want to know that?
-Andy
> Cc: stable@dpdk.org
>
> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 11:48 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
>
>
>
> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >> Sent: Friday, May 11, 2018 2:46 AM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >> constant and NUL
> >>
> >> Signed-off-by: Andy Green <andy@warmcat.com>
> >> ---
> >> drivers/net/qede/base/ecore_int.c | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/qede/base/ecore_int.c
> >> b/drivers/net/qede/base/ecore_int.c
> >> index f43781ba4..d9e22b5ed 100644
> >> --- a/drivers/net/qede/base/ecore_int.c
> >> +++ b/drivers/net/qede/base/ecore_int.c
> >> @@ -6,6 +6,8 @@
> >> * See LICENSE.qede_pmd for copyright and licensing details.
> >> */
> >>
> >> +#include <rte_string_fns.h>
> >> +
> >> #include "bcm_osal.h"
> >> #include "ecore.h"
> >> #include "ecore_spq.h"
> >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >> p_aeu->bit_name,
> >> num);
> >> else
> >> - OSAL_STRNCPY(bit_name,
> >> - p_aeu->bit_name,
> >> - 30);
> >> + strlcpy(bit_name,
> >> + p_aeu->bit_name,
> >> + sizeof(bit_name));
> >>
> >> /* We now need to pass bitmask in its
> >> * correct position.
> >
> > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
> > modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> >
> > However, this modifies base driver code, so it is up to the maintainers to make
> that decision.
> > (CC'ing maintainers here).
>
> There's no value for any OSAL_* that simply defines itself to be the same as the
> direct api, as does OSAL_STRNCPY.
>
> It's better to just remove any OSAL_* that calls straight through since all it does
> is obfuscate what the code does, for no benefit.
I agree. Since this is modifying base driver code,
the maintainers can decide what to do with this.
>
> > Also, missing fixes line and CC stable.
> >
> > Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
>
> The idea of this "Fixes" thing is to look up in git blame what is being edited is it?
> If so what's the benefit of that over using git if you ever want to know that?
>
This is important mainly to see which releases needed this patch backported.
I am replying to your patches with this info, to save you some time
(I know you are feeling the pain of this overhead :P).
> -Andy
>
> > Cc: stable@dpdk.org
> >
On 05/11/2018 08:48 PM, De Lara Guarch, Pablo wrote:
>
>
>> -----Original Message-----
>> From: Andy Green [mailto:andy@warmcat.com]
>> Sent: Friday, May 11, 2018 11:48 AM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
>> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
>> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
>> NUL
>>
>>
>>
>> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>>>> Sent: Friday, May 11, 2018 2:46 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
>>>> constant and NUL
>>>>
>>>> Signed-off-by: Andy Green <andy@warmcat.com>
>>>> ---
>>>> drivers/net/qede/base/ecore_int.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/qede/base/ecore_int.c
>>>> b/drivers/net/qede/base/ecore_int.c
>>>> index f43781ba4..d9e22b5ed 100644
>>>> --- a/drivers/net/qede/base/ecore_int.c
>>>> +++ b/drivers/net/qede/base/ecore_int.c
>>>> @@ -6,6 +6,8 @@
>>>> * See LICENSE.qede_pmd for copyright and licensing details.
>>>> */
>>>>
>>>> +#include <rte_string_fns.h>
>>>> +
>>>> #include "bcm_osal.h"
>>>> #include "ecore.h"
>>>> #include "ecore_spq.h"
>>>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
>>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
>>>> p_aeu->bit_name,
>>>> num);
>>>> else
>>>> - OSAL_STRNCPY(bit_name,
>>>> - p_aeu->bit_name,
>>>> - 30);
>>>> + strlcpy(bit_name,
>>>> + p_aeu->bit_name,
>>>> + sizeof(bit_name));
>>>>
>>>> /* We now need to pass bitmask in its
>>>> * correct position.
>>>
>>> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY and
>>> modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
>>>
>>> However, this modifies base driver code, so it is up to the maintainers to make
>> that decision.
>>> (CC'ing maintainers here).
>>
>> There's no value for any OSAL_* that simply defines itself to be the same as the
>> direct api, as does OSAL_STRNCPY.
>>
>> It's better to just remove any OSAL_* that calls straight through since all it does
>> is obfuscate what the code does, for no benefit.
>
> I agree. Since this is modifying base driver code,
> the maintainers can decide what to do with this.
>
>>
>>> Also, missing fixes line and CC stable.
>>>
>>> Fixes: 8427c6647964 ("net/qede/base: add attention formatting string")
>>
>> The idea of this "Fixes" thing is to look up in git blame what is being edited is it?
>> If so what's the benefit of that over using git if you ever want to know that?
>>
>
> This is important mainly to see which releases needed this patch backported.
> I am replying to your patches with this info, to save you some time
> (I know you are feeling the pain of this overhead :P).
Yeah: I appreciate the help. Some of the current rules make assumptions
about burning time for no real benefit that assume the contributor has
no choice but to comply. But that is simply not true for all potential
contributors.
I will integrate the comments tomorrow morning (ie, +12h) and push
again. I will look closer at the missing include thing, but since build
stops, telling me it can't find the include file, and the patch fixes
it, there are a limited amount of possible root causes.
-Andy
>
>> -Andy
>>
>>> Cc: stable@dpdk.org
>>>
> -----Original Message-----
> From: Andy Green [mailto:andy@warmcat.com]
> Sent: Friday, May 11, 2018 2:39 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil, Harish
> <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and
> NUL
>
>
>
> On 05/11/2018 08:48 PM, De Lara Guarch, Pablo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andy Green [mailto:andy@warmcat.com]
> >> Sent: Friday, May 11, 2018 11:48 AM
> >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> >> dev@dpdk.org
> >> Cc: stable@dpdk.org; Mody, Rasesh <Rasesh.Mody@cavium.com>; Patil,
> >> Harish <Harish.Patil@cavium.com>; Shahed.Shaikh@cavium.com
> >> Subject: Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >> constant and NUL
> >>
> >>
> >>
> >> On 05/11/2018 06:43 PM, De Lara Guarch, Pablo wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> >>>> Sent: Friday, May 11, 2018 2:46 AM
> >>>> To: dev@dpdk.org
> >>>> Subject: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length
> >>>> constant and NUL
> >>>>
> >>>> Signed-off-by: Andy Green <andy@warmcat.com>
> >>>> ---
> >>>> drivers/net/qede/base/ecore_int.c | 8 +++++---
> >>>> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/qede/base/ecore_int.c
> >>>> b/drivers/net/qede/base/ecore_int.c
> >>>> index f43781ba4..d9e22b5ed 100644
> >>>> --- a/drivers/net/qede/base/ecore_int.c
> >>>> +++ b/drivers/net/qede/base/ecore_int.c
> >>>> @@ -6,6 +6,8 @@
> >>>> * See LICENSE.qede_pmd for copyright and licensing details.
> >>>> */
> >>>>
> >>>> +#include <rte_string_fns.h>
> >>>> +
> >>>> #include "bcm_osal.h"
> >>>> #include "ecore.h"
> >>>> #include "ecore_spq.h"
> >>>> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> >>>> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> >>>> p_aeu->bit_name,
> >>>> num);
> >>>> else
> >>>> - OSAL_STRNCPY(bit_name,
> >>>> - p_aeu->bit_name,
> >>>> - 30);
> >>>> + strlcpy(bit_name,
> >>>> + p_aeu->bit_name,
> >>>> + sizeof(bit_name));
> >>>>
> >>>> /* We now need to pass bitmask in its
> >>>> * correct position.
> >>>
> >>> I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY
> >>> and modify the macro to use strlcpy, so we avoid further uses of that
> strlcpy.
> >>>
> >>> However, this modifies base driver code, so it is up to the
> >>> maintainers to make
> >> that decision.
> >>> (CC'ing maintainers here).
> >>
> >> There's no value for any OSAL_* that simply defines itself to be the
> >> same as the direct api, as does OSAL_STRNCPY.
> >>
> >> It's better to just remove any OSAL_* that calls straight through
> >> since all it does is obfuscate what the code does, for no benefit.
> >
> > I agree. Since this is modifying base driver code, the maintainers can
> > decide what to do with this.
> >
> >>
> >>> Also, missing fixes line and CC stable.
> >>>
> >>> Fixes: 8427c6647964 ("net/qede/base: add attention formatting
> >>> string")
> >>
> >> The idea of this "Fixes" thing is to look up in git blame what is being edited is
> it?
> >> If so what's the benefit of that over using git if you ever want to know that?
> >>
> >
> > This is important mainly to see which releases needed this patch backported.
> > I am replying to your patches with this info, to save you some time (I
> > know you are feeling the pain of this overhead :P).
>
> Yeah: I appreciate the help. Some of the current rules make assumptions about
> burning time for no real benefit that assume the contributor has no choice but
> to comply. But that is simply not true for all potential contributors.
>
> I will integrate the comments tomorrow morning (ie, +12h) and push again. I
> will look closer at the missing include thing, but since build stops, telling me it
> can't find the include file, and the patch fixes it, there are a limited amount of
> possible root causes.
Thanks for your time, Andy.
Pablo
> > >>
> > >> +#include <rte_string_fns.h>
> > >> +
> > >> #include "bcm_osal.h"
> > >> #include "ecore.h"
> > >> #include "ecore_spq.h"
> > >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t
> > >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > >> p_aeu->bit_name,
> > >> num);
> > >> else
> > >> - OSAL_STRNCPY(bit_name,
> > >> - p_aeu->bit_name,
> > >> - 30);
> > >> + strlcpy(bit_name,
> > >> + p_aeu->bit_name,
> > >> + sizeof(bit_name));
> > >>
> > >> /* We now need to pass bitmask in its
> > >> * correct position.
> > >
> > > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY
> > > and modify the macro to use strlcpy, so we avoid further uses of that strlcpy.
> > >
> > > However, this modifies base driver code, so it is up to the
> > > maintainers to make
> > that decision.
> > > (CC'ing maintainers here).
> >
> > There's no value for any OSAL_* that simply defines itself to be the
> > same as the direct api, as does OSAL_STRNCPY.
> >
> > It's better to just remove any OSAL_* that calls straight through
> > since all it does is obfuscate what the code does, for no benefit.
>
> I agree. Since this is modifying base driver code, the maintainers can decide
> what to do with this.
Hi,
For this series, you can continue with s/OSAL_STRNCPY/strlcpy/ for this instance.
I will send a patch to cleanup OSAL_* once your series gets applied.
Thanks,
Shahed
@@ -6,6 +6,8 @@
* See LICENSE.qede_pmd for copyright and licensing details.
*/
+#include <rte_string_fns.h>
+
#include "bcm_osal.h"
#include "ecore.h"
#include "ecore_spq.h"
@@ -1104,9 +1106,9 @@ static enum _ecore_status_t ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
p_aeu->bit_name,
num);
else
- OSAL_STRNCPY(bit_name,
- p_aeu->bit_name,
- 30);
+ strlcpy(bit_name,
+ p_aeu->bit_name,
+ sizeof(bit_name));
/* We now need to pass bitmask in its
* correct position.