[PATCH] pmd_ring: fix coverity issue

[PATCH] pmd_ring: fix coverity issue

[Mauricio Vasquez B]

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

Re: [PATCH] pmd_ring: fix coverity issue

[Ferruh Yigit]

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

[PATCH v2] net/ring: remove unnecessary NULL check

[Mauricio Vasquez B]

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

Re: [PATCH v2] net/ring: remove unnecessary NULL check

[Ferruh Yigit]

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.

> 

Re: [PATCH v2] net/ring: remove unnecessary NULL check

[Fulvio Risso]

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.
>
>>
>

Re: [PATCH v2] net/ring: remove unnecessary NULL check

[Ferruh Yigit]

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

[PATCH v3] net/ring: remove unnecessary NULL check

[Mauricio Vasquez B]

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

Re: [PATCH v2] net/ring: remove unnecessary NULL check

[Mauricio Vasquez]

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
>

Re: [PATCH v3] net/ring: remove unnecessary NULL check

[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>

Re: [PATCH v3] net/ring: remove unnecessary NULL check

[Thomas Monjalon]

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