Message ID | 1478030140-7127-1-git-send-email-mauricio.vasquez@polito.it (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 0114D3277; Tue, 1 Nov 2016 20:55:53 +0100 (CET) Received: from fm1nodo5.polito.it (fm1nodo5.polito.it [130.192.180.13]) by dpdk.org (Postfix) with ESMTP id 2B7ED326D for <dev@dpdk.org>; Tue, 1 Nov 2016 20:55:52 +0100 (CET) Received: from polito.it (frontmail2.polito.it [130.192.180.42]) by fm1nodo5.polito.it with ESMTP id uA1Jtptg002683-uA1Jtpti002683 (version=TLSv1.0 cipher=DHE-RSA-AES256-SHA bits=256 verify=CAFAIL); Tue, 1 Nov 2016 20:55:51 +0100 Received: from [190.9.209.39] (account d040768@polito.it HELO localhost.localdomain) by polito.it (CommuniGate Pro SMTP 6.1.9) with ESMTPSA id 54260221; Tue, 01 Nov 2016 20:55:51 +0100 From: Mauricio Vasquez B <mauricio.vasquez@polito.it> To: bruce.richardson@intel.com Date: Tue, 1 Nov 2016 14:55:40 -0500 Message-Id: <1478030140-7127-1-git-send-email-mauricio.vasquez@polito.it> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1477972113-2600-1-git-send-email-mauricio.vasquez@polito.it> References: <1477972113-2600-1-git-send-email-mauricio.vasquez@polito.it> X-FEAS-SYSTEM-WL: 130.192.180.42 Cc: dev@dpdk.org, ferruh.yigit@intel.com Subject: [dpdk-dev] [PATCH v2] net/ring: remove unnecessary NULL check X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Mauricio Vasquez
Nov. 1, 2016, 7:55 p.m. UTC
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(-)
Comments
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. >
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. > >> >
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
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 >
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);