[dpdk-dev,12/18] drivers: net: sfc: fix another strncpy size and NUL
Checks
Commit Message
---
drivers/net/sfc/sfc_ethdev.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
Comments
Many thanks. I guess the most of below notes are applicable to many other
patches in the series.
Signed-off-by: , Fixes: and Cc: stable@dpdk.org tags are missing. See [1].
Changeset summary should start from "net/sfc: "
I.e. something like:
net/sfc: fix strncpy size and NUL
(it looks like "another" is useless in the original subject)
In general all patches should pass ./devtools/check-git-log.sh and
./devtools/checkpatches.sh
(which requires path to Linux kernel checkpatches.pl).
Andrew.
[1]
http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line
On 05/08/2018 07:30 AM, Andy Green wrote:
> ---
> drivers/net/sfc/sfc_ethdev.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index e9bb283e0..bd5f17f33 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>
> for (i = 0; i < EFX_MAC_NSTATS; ++i) {
> if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
> - if (xstats_names != NULL && nstats < xstats_count)
> + if (xstats_names != NULL && nstats < xstats_count) {
> strncpy(xstats_names[nstats].name,
> efx_mac_stat_name(sa->nic, i),
> - sizeof(xstats_names[0].name));
> + sizeof(xstats_names[0].name) - 1);
> + xstats_names[0].name[
> + sizeof(xstats_names[0].name) - 1] = '\0';
> + }
In fact strlcpy() should be used.
> nstats++;
> }
> }
>
On 05/08/2018 03:36 PM, Andrew Rybchenko wrote:
> Many thanks. I guess the most of below notes are applicable to many other
> patches in the series.
>
> Signed-off-by: , Fixes: and Cc: stable@dpdk.org tags are missing. See [1].
Everybody's project has different prejudices.
> Changeset summary should start from "net/sfc: "
> I.e. something like:
> net/sfc: fix strncpy size and NUL
Yeah if that's what you like.
> (it looks like "another" is useless in the original subject)
It captures my feeling at having to wade through making 18 fixes before
I could compile the project on current Fedora.
> In general all patches should pass ./devtools/check-git-log.sh and
> ./devtools/checkpatches.sh
> (which requires path to Linux kernel checkpatches.pl).
Can you help me understand why adding CRLFs at 80 cols on the gcc errors
I pasted helps anything at all? The patches actually fix problems in
the code.
If you don't care about Coverity, let me know and I will register this
project there and send you fixes when I have time.
> Andrew.
>
> [1]
> http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line
>
> On 05/08/2018 07:30 AM, Andy Green wrote:
>> ---
>> drivers/net/sfc/sfc_ethdev.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
>> index e9bb283e0..bd5f17f33 100644
>> --- a/drivers/net/sfc/sfc_ethdev.c
>> +++ b/drivers/net/sfc/sfc_ethdev.c
>> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>>
>> for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>> if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
>> - if (xstats_names != NULL && nstats < xstats_count)
>> + if (xstats_names != NULL && nstats < xstats_count) {
>> strncpy(xstats_names[nstats].name,
>> efx_mac_stat_name(sa->nic, i),
>> - sizeof(xstats_names[0].name));
>> + sizeof(xstats_names[0].name) - 1);
>> + xstats_names[0].name[
>> + sizeof(xstats_names[0].name) - 1] = '\0';
>> + }
>
> In fact strlcpy() should be used.
Fair enough. Last time I looked it wasn't in glibc but seems it is now.
-Andy
>> nstats++;
>> }
>> }
>>
>
On Tue, May 08, 2018 at 04:18:41PM +0800, Andy Green wrote:
>
>
> On 05/08/2018 03:36 PM, Andrew Rybchenko wrote:
> > Many thanks. I guess the most of below notes are applicable to many other
> > patches in the series.
> >
> > Signed-off-by: , Fixes: and Cc: stable@dpdk.org tags are missing. See [1].
>
> Everybody's project has different prejudices.
>
> > Changeset summary should start from "net/sfc: "
> > I.e. something like:
> > net/sfc: fix strncpy size and NUL
>
> Yeah if that's what you like.
>
> > (it looks like "another" is useless in the original subject)
>
> It captures my feeling at having to wade through making 18 fixes before I
> could compile the project on current Fedora.
>
> > In general all patches should pass ./devtools/check-git-log.sh and
> > ./devtools/checkpatches.sh
> > (which requires path to Linux kernel checkpatches.pl).
>
> Can you help me understand why adding CRLFs at 80 cols on the gcc errors I
> pasted helps anything at all? The patches actually fix problems in the
> code.
>
I don't think there is any need to wrap the error messages since they are
pasted verbatim.
> If you don't care about Coverity, let me know and I will register this
> project there and send you fixes when I have time.
>
FYI: The DPDK project is already registered in coverity and scanned regularly.
We do try and fix as many issues it flags as possible, but not everything
gets dealt with as quickly as we'd like, sadly.
https://scan.coverity.com/projects/dpdk-data-plane-development-kit
Regards,
/Bruce
On 05/08/2018 11:18 AM, Andy Green wrote:
> On 05/08/2018 03:36 PM, Andrew Rybchenko wrote:
>> (it looks like "another" is useless in the original subject)
>
> It captures my feeling at having to wade through making 18 fixes
> before I could compile the project on current Fedora.
I see.
>> In general all patches should pass ./devtools/check-git-log.sh and
>> ./devtools/checkpatches.sh
>> (which requires path to Linux kernel checkpatches.pl).
>
> Can you help me understand why adding CRLFs at 80 cols on the gcc
> errors I pasted helps anything at all? The patches actually fix
> problems in the code.
Seeing GCC errors which patch fixes is useful to see. Yes, I agree that
it is real problem in the code.
> If you don't care about Coverity, let me know and I will register this
> project there and send you fixes when I have time.
dpdk is registered at Coverity and we get reports from time to time.
>
>> Andrew.
>>
>> [1]
>> http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-subject-line
>>
>> On 05/08/2018 07:30 AM, Andy Green wrote:
>>> ---
>>> drivers/net/sfc/sfc_ethdev.c | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/sfc/sfc_ethdev.c
>>> b/drivers/net/sfc/sfc_ethdev.c
>>> index e9bb283e0..bd5f17f33 100644
>>> --- a/drivers/net/sfc/sfc_ethdev.c
>>> +++ b/drivers/net/sfc/sfc_ethdev.c
>>> @@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
>>> for (i = 0; i < EFX_MAC_NSTATS; ++i) {
>>> if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
>>> - if (xstats_names != NULL && nstats < xstats_count)
>>> + if (xstats_names != NULL && nstats < xstats_count) {
>>> strncpy(xstats_names[nstats].name,
>>> efx_mac_stat_name(sa->nic, i),
>>> - sizeof(xstats_names[0].name));
>>> + sizeof(xstats_names[0].name) - 1);
>>> + xstats_names[0].name[
>>> + sizeof(xstats_names[0].name) - 1] = '\0';
>>> + }
>>
>> In fact strlcpy() should be used.
> Fair enough. Last time I looked it wasn't in glibc but seems it is now.
As far as I know it is not in glibc, but dpdk has internal fallback if
the function
is not available from external libs.
Andrew.
@@ -662,10 +662,13 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
for (i = 0; i < EFX_MAC_NSTATS; ++i) {
if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
- if (xstats_names != NULL && nstats < xstats_count)
+ if (xstats_names != NULL && nstats < xstats_count) {
strncpy(xstats_names[nstats].name,
efx_mac_stat_name(sa->nic, i),
- sizeof(xstats_names[0].name));
+ sizeof(xstats_names[0].name) - 1);
+ xstats_names[0].name[
+ sizeof(xstats_names[0].name) - 1] = '\0';
+ }
nstats++;
}
}