Checks
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
Commit Message
Stephen Hemminger
Jan. 24, 2022, 12:04 a.m. UTC
Remove redundant NULL pointer checks before free functions
found by nullfree.cocci
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
examples/ip_pipeline/cli.c | 12 ++++--------
examples/ip_pipeline/cryptodev.c | 6 ++----
examples/ip_pipeline/thread.c | 6 ++----
3 files changed, 8 insertions(+), 16 deletions(-)
Comments
> -----Original Message----- > From: Stephen Hemminger <stephen@networkplumber.org> > Sent: Monday, January 24, 2022 12:04 AM > To: dev@dpdk.org > Cc: Stephen Hemminger <stephen@networkplumber.org>; Dumitrescu, > Cristian <cristian.dumitrescu@intel.com> > Subject: [PATCH 05/82] examples/ip_pipeline: remove unnecessary NULL > checks > > Remove redundant NULL pointer checks before free functions > found by nullfree.cocci > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > examples/ip_pipeline/cli.c | 12 ++++-------- > examples/ip_pipeline/cryptodev.c | 6 ++---- > examples/ip_pipeline/thread.c | 6 ++---- > 3 files changed, 8 insertions(+), 16 deletions(-) > Hi Stephen, The rte_ring_free() and rte_mempool_free() do not state in their API description that freeing a NULL pointer is harmless. Before pushing these changes, please add the necessary note in the API header files for these functions. In the absence of the clear note in their API description, the user is forced to check for the NULL pointer. I agree that the implementation of these functions does the right think and exits early when the input pointer is NULL, but there is no guarantee that the implementation is not going to change. Agree? The stdlib free() and the rte_free() do have the clear API description note that freeing a NULL object is harmless, so removing the NULL check before their call is indeed safe. This stands for all the patches in this set. Regards, Cristian
On Mon, 24 Jan 2022 10:17:23 +0000 "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote: > > -----Original Message----- > > From: Stephen Hemminger <stephen@networkplumber.org> > > Sent: Monday, January 24, 2022 12:04 AM > > To: dev@dpdk.org > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Dumitrescu, > > Cristian <cristian.dumitrescu@intel.com> > > Subject: [PATCH 05/82] examples/ip_pipeline: remove unnecessary NULL > > checks > > > > Remove redundant NULL pointer checks before free functions > > found by nullfree.cocci > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > --- > > examples/ip_pipeline/cli.c | 12 ++++-------- > > examples/ip_pipeline/cryptodev.c | 6 ++---- > > examples/ip_pipeline/thread.c | 6 ++---- > > 3 files changed, 8 insertions(+), 16 deletions(-) > > > > Hi Stephen, > > The rte_ring_free() and rte_mempool_free() do not state in their API description that freeing a NULL pointer is harmless. Before pushing these changes, please add the necessary note in the API header files for these functions. Okay, will fix the documentation then. The implemeentation has always done the right thing. > In the absence of the clear note in their API description, the user is forced to check for the NULL pointer. I agree that the implementation of these functions does the right think and exits early when the input pointer is NULL, but there is no guarantee that the implementation is not going to change. Agree? No. I don't agree. All XXX_free() functions should accept NULL and handle it in the same way. Any inconsistency is an obvious source of bugs. > > The stdlib free() and the rte_free() do have the clear API description note that freeing a NULL object is harmless, so removing the NULL check before their call is indeed safe. Will add first patch to fix the documentation to match the current implementation.
diff --git a/examples/ip_pipeline/cli.c b/examples/ip_pipeline/cli.c index 0c372675a919..c918f30e06f3 100644 --- a/examples/ip_pipeline/cli.c +++ b/examples/ip_pipeline/cli.c @@ -3773,10 +3773,8 @@ parse_free_sym_crypto_param_data(struct rte_table_action_sym_crypto_params *p) switch (xform[i]->type) { case RTE_CRYPTO_SYM_XFORM_CIPHER: - if (p->cipher_auth.cipher_iv.val) - free(p->cipher_auth.cipher_iv.val); - if (p->cipher_auth.cipher_iv_update.val) - free(p->cipher_auth.cipher_iv_update.val); + free(p->cipher_auth.cipher_iv.val); + free(p->cipher_auth.cipher_iv_update.val); break; case RTE_CRYPTO_SYM_XFORM_AUTH: if (p->cipher_auth.auth_iv.val) @@ -3785,10 +3783,8 @@ parse_free_sym_crypto_param_data(struct rte_table_action_sym_crypto_params *p) free(p->cipher_auth.cipher_iv_update.val); break; case RTE_CRYPTO_SYM_XFORM_AEAD: - if (p->aead.iv.val) - free(p->aead.iv.val); - if (p->aead.aad.val) - free(p->aead.aad.val); + free(p->aead.iv.val); + free(p->aead.aad.val); break; default: continue; diff --git a/examples/ip_pipeline/cryptodev.c b/examples/ip_pipeline/cryptodev.c index 9997d974563f..cd0f9c3d4eac 100644 --- a/examples/ip_pipeline/cryptodev.c +++ b/examples/ip_pipeline/cryptodev.c @@ -151,10 +151,8 @@ cryptodev_create(const char *name, struct cryptodev_params *params) return cryptodev; error_exit: - if (cryptodev->mp_create) - rte_mempool_free(cryptodev->mp_create); - if (cryptodev->mp_init) - rte_mempool_free(cryptodev->mp_init); + rte_mempool_free(cryptodev->mp_create); + rte_mempool_free(cryptodev->mp_init); free(cryptodev); diff --git a/examples/ip_pipeline/thread.c b/examples/ip_pipeline/thread.c index b2e97327052f..82d5f87c384c 100644 --- a/examples/ip_pipeline/thread.c +++ b/examples/ip_pipeline/thread.c @@ -92,11 +92,9 @@ thread_free(void) continue; /* MSGQs */ - if (t->msgq_req) - rte_ring_free(t->msgq_req); + rte_ring_free(t->msgq_req); - if (t->msgq_rsp) - rte_ring_free(t->msgq_rsp); + rte_ring_free(t->msgq_rsp); } }