* [dpdk-dev] [PATCH] pmd_ring: fix coverity issue
@ 2016-11-01 3:48 Mauricio Vasquez B
2016-11-01 14:17 ` Ferruh Yigit
2016-11-01 19:55 ` [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check Mauricio Vasquez B
0 siblings, 2 replies; 10+ messages in thread
From: Mauricio Vasquez B @ 2016-11-01 3:48 UTC (permalink / raw)
To: bruce.richardson; +Cc: dev
internals->data will never be NULL, so the check is not necessary.
Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
Coverity issue: 137873
Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
---
drivers/net/ring/rte_eth_ring.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 6d2a8c1..5ca00ed 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -599,17 +599,15 @@ rte_pmd_ring_remove(const char *name)
eth_dev_stop(eth_dev);
- if (eth_dev->data) {
- internals = eth_dev->data->dev_private;
- if (internals->action == DEV_CREATE) {
- /*
- * it is only necessary to delete the rings in rx_queues because
- * they are the same used in tx_queues
- */
- for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
- r = eth_dev->data->rx_queues[i];
- rte_ring_free(r->rng);
- }
+ internals = eth_dev->data->dev_private;
+ if (internals->action == DEV_CREATE) {
+ /*
+ * it is only necessary to delete the rings in rx_queues because
+ * they are the same used in tx_queues
+ */
+ for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+ r = eth_dev->data->rx_queues[i];
+ rte_ring_free(r->rng);
}
rte_free(eth_dev->data->rx_queues);
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH] pmd_ring: fix coverity issue
2016-11-01 3:48 [dpdk-dev] [PATCH] pmd_ring: fix coverity issue Mauricio Vasquez B
@ 2016-11-01 14:17 ` Ferruh Yigit
2016-11-01 19:55 ` [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check Mauricio Vasquez B
1 sibling, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2016-11-01 14:17 UTC (permalink / raw)
To: Mauricio Vasquez B, bruce.richardson; +Cc: dev
Hi Mauricio,
On 11/1/2016 3:48 AM, Mauricio Vasquez B wrote:
> internals->data will never be NULL, so the check is not necessary.
>
> Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
> Coverity issue: 137873
>
> Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
> ---
Thank you for the patch.
But "fix coverity issue" is not very descriptive title on its own.
Can you please describe what is really done in the patch, perhaps
something like:
"net/ring: remove unnecessary NULL check" ?
Thanks,
ferruh
^ permalink raw reply [flat|nested] 10+ messages in thread* [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check
2016-11-01 3:48 [dpdk-dev] [PATCH] pmd_ring: fix coverity issue Mauricio Vasquez B
2016-11-01 14:17 ` Ferruh Yigit
@ 2016-11-01 19:55 ` Mauricio Vasquez B
2016-11-02 11:38 ` Ferruh Yigit
2016-11-02 13:46 ` [dpdk-dev] [PATCH v3] " Mauricio Vasquez B
1 sibling, 2 replies; 10+ messages in thread
From: Mauricio Vasquez B @ 2016-11-01 19:55 UTC (permalink / raw)
To: bruce.richardson; +Cc: dev, ferruh.yigit
Coverity detected this as an issue because internals->data will never be NULL,
then the check is not necessary.
Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
Coverity issue: 137873
Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
---
drivers/net/ring/rte_eth_ring.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 6d2a8c1..5ca00ed 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -599,17 +599,15 @@ rte_pmd_ring_remove(const char *name)
eth_dev_stop(eth_dev);
- if (eth_dev->data) {
- internals = eth_dev->data->dev_private;
- if (internals->action == DEV_CREATE) {
- /*
- * it is only necessary to delete the rings in rx_queues because
- * they are the same used in tx_queues
- */
- for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
- r = eth_dev->data->rx_queues[i];
- rte_ring_free(r->rng);
- }
+ internals = eth_dev->data->dev_private;
+ if (internals->action == DEV_CREATE) {
+ /*
+ * it is only necessary to delete the rings in rx_queues because
+ * they are the same used in tx_queues
+ */
+ for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+ r = eth_dev->data->rx_queues[i];
+ rte_ring_free(r->rng);
}
rte_free(eth_dev->data->rx_queues);
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check
2016-11-01 19:55 ` [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check Mauricio Vasquez B
@ 2016-11-02 11:38 ` Ferruh Yigit
2016-11-02 12:49 ` Fulvio Risso
2016-11-02 13:46 ` [dpdk-dev] [PATCH v3] " Mauricio Vasquez B
1 sibling, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2016-11-02 11:38 UTC (permalink / raw)
To: Mauricio Vasquez B, bruce.richardson; +Cc: dev
Hi Mauricio,
On 11/1/2016 7:55 PM, Mauricio Vasquez B wrote:
> Coverity detected this as an issue because internals->data will never be NULL,
> then the check is not necessary.
>
> Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
> Coverity issue: 137873
>
> Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
> ---
> drivers/net/ring/rte_eth_ring.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> index 6d2a8c1..5ca00ed 100644
> --- a/drivers/net/ring/rte_eth_ring.c
> +++ b/drivers/net/ring/rte_eth_ring.c
> @@ -599,17 +599,15 @@ rte_pmd_ring_remove(const char *name)
>
> eth_dev_stop(eth_dev);
>
> - if (eth_dev->data) {
> - internals = eth_dev->data->dev_private;
> - if (internals->action == DEV_CREATE) {
> - /*
> - * it is only necessary to delete the rings in rx_queues because
> - * they are the same used in tx_queues
> - */
> - for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> - r = eth_dev->data->rx_queues[i];
> - rte_ring_free(r->rng);
> - }
> + internals = eth_dev->data->dev_private;
> + if (internals->action == DEV_CREATE) {
> + /*
> + * it is only necessary to delete the rings in rx_queues because
> + * they are the same used in tx_queues
> + */
> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> + r = eth_dev->data->rx_queues[i];
> + rte_ring_free(r->rng);
> }
>
> rte_free(eth_dev->data->rx_queues);
This patch not only removes the NULL check but also changes the logic.
after patch rx_queues, tx_queues and dev_private only freed if action is
DEV_CREATE which is wrong.
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check
2016-11-02 11:38 ` Ferruh Yigit
@ 2016-11-02 12:49 ` Fulvio Risso
2016-11-02 13:15 ` Ferruh Yigit
0 siblings, 1 reply; 10+ messages in thread
From: Fulvio Risso @ 2016-11-02 12:49 UTC (permalink / raw)
To: Ferruh Yigit, Mauricio Vasquez B, bruce.richardson; +Cc: dev
Dear Ferruh,
Maybe I'm wrong, but I cannot see your point.
The code is absolutely the same, only the following line
if (eth_dev->data) {
is actually removed.
fulvio
On 02/11/2016 12:38, Ferruh Yigit wrote:
> Hi Mauricio,
>
> On 11/1/2016 7:55 PM, Mauricio Vasquez B wrote:
>> Coverity detected this as an issue because internals->data will never be NULL,
>> then the check is not necessary.
>>
>> Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
>> Coverity issue: 137873
>>
>> Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
>> ---
>> drivers/net/ring/rte_eth_ring.c | 20 +++++++++-----------
>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
>> index 6d2a8c1..5ca00ed 100644
>> --- a/drivers/net/ring/rte_eth_ring.c
>> +++ b/drivers/net/ring/rte_eth_ring.c
>> @@ -599,17 +599,15 @@ rte_pmd_ring_remove(const char *name)
>>
>> eth_dev_stop(eth_dev);
>>
>> - if (eth_dev->data) {
>> - internals = eth_dev->data->dev_private;
>> - if (internals->action == DEV_CREATE) {
>> - /*
>> - * it is only necessary to delete the rings in rx_queues because
>> - * they are the same used in tx_queues
>> - */
>> - for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> - r = eth_dev->data->rx_queues[i];
>> - rte_ring_free(r->rng);
>> - }
>> + internals = eth_dev->data->dev_private;
>> + if (internals->action == DEV_CREATE) {
>> + /*
>> + * it is only necessary to delete the rings in rx_queues because
>> + * they are the same used in tx_queues
>> + */
>> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
>> + r = eth_dev->data->rx_queues[i];
>> + rte_ring_free(r->rng);
>> }
>>
>> rte_free(eth_dev->data->rx_queues);
>
> This patch not only removes the NULL check but also changes the logic.
> after patch rx_queues, tx_queues and dev_private only freed if action is
> DEV_CREATE which is wrong.
>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check
2016-11-02 12:49 ` Fulvio Risso
@ 2016-11-02 13:15 ` Ferruh Yigit
2016-11-02 13:50 ` Mauricio Vasquez
0 siblings, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2016-11-02 13:15 UTC (permalink / raw)
To: Fulvio Risso, Mauricio Vasquez B, bruce.richardson; +Cc: dev
On 11/2/2016 12:49 PM, Fulvio Risso wrote:
> Dear Ferruh,
> Maybe I'm wrong, but I cannot see your point.
> The code is absolutely the same, only the following line
>
> if (eth_dev->data) {
>
> is actually removed.
Please double check the condition "rx_queues" freed:
before the patch:
==========
if (eth_dev->data) {
internals = eth_dev->data->dev_private;
if (internals->action == DEV_CREATE) {
/*
* it is only necessary to delete the rings in rx_queues because
* they are the same used in tx_queues
*/
for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
r = eth_dev->data->rx_queues[i];
rte_ring_free(r->rng);
}
}
rte_free(eth_dev->data->rx_queues);
rte_free(eth_dev->data->tx_queues);
rte_free(eth_dev->data->dev_private);
}
==========
After the patch:
==========
internals = eth_dev->data->dev_private;
if (internals->action == DEV_CREATE) {
/*
* it is only necessary to delete the rings in rx_queues because
* they are the same used in tx_queues
*/
for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
r = eth_dev->data->rx_queues[i];
rte_ring_free(r->rng);
}
rte_free(eth_dev->data->rx_queues);
rte_free(eth_dev->data->tx_queues);
rte_free(eth_dev->data->dev_private);
}
==========
Thanks,
ferruh
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check
2016-11-02 13:15 ` Ferruh Yigit
@ 2016-11-02 13:50 ` Mauricio Vasquez
0 siblings, 0 replies; 10+ messages in thread
From: Mauricio Vasquez @ 2016-11-02 13:50 UTC (permalink / raw)
To: Ferruh Yigit, Fulvio Risso, bruce.richardson; +Cc: dev
Dear Ferruh,
You are right, I messed up the brackets.
I already sent v3.
Thanks,
Mauricio.
On 11/02/2016 08:15 AM, Ferruh Yigit wrote:
> On 11/2/2016 12:49 PM, Fulvio Risso wrote:
>> Dear Ferruh,
>> Maybe I'm wrong, but I cannot see your point.
>> The code is absolutely the same, only the following line
>>
>> if (eth_dev->data) {
>>
>> is actually removed.
> Please double check the condition "rx_queues" freed:
>
> before the patch:
> ==========
> if (eth_dev->data) {
> internals = eth_dev->data->dev_private;
> if (internals->action == DEV_CREATE) {
> /*
> * it is only necessary to delete the rings in rx_queues because
> * they are the same used in tx_queues
> */
> for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> r = eth_dev->data->rx_queues[i];
> rte_ring_free(r->rng);
> }
> }
>
> rte_free(eth_dev->data->rx_queues);
> rte_free(eth_dev->data->tx_queues);
> rte_free(eth_dev->data->dev_private);
> }
> ==========
>
>
> After the patch:
> ==========
> internals = eth_dev->data->dev_private;
> if (internals->action == DEV_CREATE) {
> /*
> * it is only necessary to delete the rings in rx_queues because
> * they are the same used in tx_queues
> */
> for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> r = eth_dev->data->rx_queues[i];
> rte_ring_free(r->rng);
> }
>
> rte_free(eth_dev->data->rx_queues);
> rte_free(eth_dev->data->tx_queues);
> rte_free(eth_dev->data->dev_private);
> }
> ==========
>
>
> Thanks,
> ferruh
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [dpdk-dev] [PATCH v3] net/ring: remove unnecessary NULL check
2016-11-01 19:55 ` [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check Mauricio Vasquez B
2016-11-02 11:38 ` Ferruh Yigit
@ 2016-11-02 13:46 ` Mauricio Vasquez B
2016-11-02 14:05 ` Ferruh Yigit
1 sibling, 1 reply; 10+ messages in thread
From: Mauricio Vasquez B @ 2016-11-02 13:46 UTC (permalink / raw)
To: bruce.richardson; +Cc: dev, ferruh.yigit
Coverity detected this as an issue because internals->data will never be NULL,
then the check is not necessary.
Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
Coverity issue: 137873
Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
---
drivers/net/ring/rte_eth_ring.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 6d2a8c1..c1767c4 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -599,24 +599,22 @@ rte_pmd_ring_remove(const char *name)
eth_dev_stop(eth_dev);
- if (eth_dev->data) {
- internals = eth_dev->data->dev_private;
- if (internals->action == DEV_CREATE) {
- /*
- * it is only necessary to delete the rings in rx_queues because
- * they are the same used in tx_queues
- */
- for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
- r = eth_dev->data->rx_queues[i];
- rte_ring_free(r->rng);
- }
+ internals = eth_dev->data->dev_private;
+ if (internals->action == DEV_CREATE) {
+ /*
+ * it is only necessary to delete the rings in rx_queues because
+ * they are the same used in tx_queues
+ */
+ for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+ r = eth_dev->data->rx_queues[i];
+ rte_ring_free(r->rng);
}
-
- rte_free(eth_dev->data->rx_queues);
- rte_free(eth_dev->data->tx_queues);
- rte_free(eth_dev->data->dev_private);
}
+ rte_free(eth_dev->data->rx_queues);
+ rte_free(eth_dev->data->tx_queues);
+ rte_free(eth_dev->data->dev_private);
+
rte_free(eth_dev->data);
rte_eth_dev_release_port(eth_dev);
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v3] net/ring: remove unnecessary NULL check
2016-11-02 13:46 ` [dpdk-dev] [PATCH v3] " Mauricio Vasquez B
@ 2016-11-02 14:05 ` Ferruh Yigit
2016-11-07 13:08 ` Thomas Monjalon
0 siblings, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2016-11-02 14:05 UTC (permalink / raw)
To: Mauricio Vasquez B, bruce.richardson; +Cc: dev
On 11/2/2016 1:46 PM, Mauricio Vasquez B wrote:
> Coverity detected this as an issue because internals->data will never be NULL,
> then the check is not necessary.
>
> Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
> Coverity issue: 137873
>
> Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
> ---
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [dpdk-dev] [PATCH v3] net/ring: remove unnecessary NULL check
2016-11-02 14:05 ` Ferruh Yigit
@ 2016-11-07 13:08 ` Thomas Monjalon
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2016-11-07 13:08 UTC (permalink / raw)
To: Mauricio Vasquez B; +Cc: dev, Ferruh Yigit, bruce.richardson
2016-11-02 14:05, Ferruh Yigit:
> On 11/2/2016 1:46 PM, Mauricio Vasquez B wrote:
> > Coverity detected this as an issue because internals->data will never be NULL,
> > then the check is not necessary.
> >
> > Fixes: d082c0395bf6 ("ring: fix memory leak when detaching")
> > Coverity issue: 137873
> >
> > Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
>
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied, thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-11-07 13:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-01 3:48 [dpdk-dev] [PATCH] pmd_ring: fix coverity issue Mauricio Vasquez B
2016-11-01 14:17 ` Ferruh Yigit
2016-11-01 19:55 ` [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check Mauricio Vasquez B
2016-11-02 11:38 ` Ferruh Yigit
2016-11-02 12:49 ` Fulvio Risso
2016-11-02 13:15 ` Ferruh Yigit
2016-11-02 13:50 ` Mauricio Vasquez
2016-11-02 13:46 ` [dpdk-dev] [PATCH v3] " Mauricio Vasquez B
2016-11-02 14:05 ` Ferruh Yigit
2016-11-07 13:08 ` Thomas Monjalon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).