| Message ID | 20161212161142.13998-1-mike.holmes@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 12/12/16 19:11, Mike Holmes wrote: > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/odp_pool.c | 4 ++-- > platform/linux-generic/pktio/socket_mmap.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c > index 415c9fa..858bdd8 100644 > --- a/platform/linux-generic/odp_pool.c > +++ b/platform/linux-generic/odp_pool.c > @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, > pool->s.blk_freelist = NULL; > > /* Initialization will increment these to their target vals */ > - odp_atomic_store_u32(&pool->s.bufcount, 0); > - odp_atomic_store_u32(&pool->s.blkcount, 0); > + (void)odp_atomic_store_u32(&pool->s.bufcount, 0); > + (void)odp_atomic_store_u32(&pool->s.blkcount, 0); it's void function, not needed. > > uint8_t *buf = udata_base_addr - buf_stride; > uint8_t *udat = udata_stride == 0 ? NULL : > diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c > index 9655668..f0f893e 100644 > --- a/platform/linux-generic/pktio/socket_mmap.c > +++ b/platform/linux-generic/pktio/socket_mmap.c > @@ -458,7 +458,7 @@ static int mmap_sock(pkt_sock_mmap_t *pkt_sock) > > static void mmap_unmap_sock(pkt_sock_mmap_t *pkt_sock) > { > - munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > + (void)munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); it's better to check return code then hide return check. Maxim. > free(pkt_sock->rx_ring.rd); > free(pkt_sock->tx_ring.rd); > } >
On 12 December 2016 at 11:48, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/12/16 19:11, Mike Holmes wrote: > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > > --- > > platform/linux-generic/odp_pool.c | 4 ++-- > > platform/linux-generic/pktio/socket_mmap.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/platform/linux-generic/odp_pool.c > b/platform/linux-generic/odp_pool.c > > index 415c9fa..858bdd8 100644 > > --- a/platform/linux-generic/odp_pool.c > > +++ b/platform/linux-generic/odp_pool.c > > @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, > > pool->s.blk_freelist = NULL; > > > > /* Initialization will increment these to their target > vals */ > > - odp_atomic_store_u32(&pool->s.bufcount, 0); > > - odp_atomic_store_u32(&pool->s.blkcount, 0); > > + (void)odp_atomic_store_u32(&pool->s.bufcount, 0); > > + (void)odp_atomic_store_u32(&pool->s.blkcount, 0); > > it's void function, not needed. > Coverity disagrees and I like the human documentation aspect, by reading this I know that it was not forgotten and I dont have to go look it up to know that. > > > > > uint8_t *buf = udata_base_addr - buf_stride; > > uint8_t *udat = udata_stride == 0 ? NULL : > > diff --git a/platform/linux-generic/pktio/socket_mmap.c > b/platform/linux-generic/pktio/socket_mmap.c > > index 9655668..f0f893e 100644 > > --- a/platform/linux-generic/pktio/socket_mmap.c > > +++ b/platform/linux-generic/pktio/socket_mmap.c > > @@ -458,7 +458,7 @@ static int mmap_sock(pkt_sock_mmap_t *pkt_sock) > > > > static void mmap_unmap_sock(pkt_sock_mmap_t *pkt_sock) > > { > > - munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > > + (void)munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > > it's better to check return code then hide return check. > SEE! now that I put it explicitly it becomes obvious that it was a mistake - will fix > > Maxim. > > > free(pkt_sock->rx_ring.rd); > > free(pkt_sock->tx_ring.rd); > > } > > > > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
On 12/12/16 19:58, Mike Holmes wrote: > > > On 12 December 2016 at 11:48, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 12/12/16 19:11, Mike Holmes wrote: > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org <mailto:mike.holmes@linaro.org>> > > --- > > platform/linux-generic/odp_pool.c | 4 ++-- > > platform/linux-generic/pktio/socket_mmap.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c > > index 415c9fa..858bdd8 100644 > > --- a/platform/linux-generic/odp_pool.c > > +++ b/platform/linux-generic/odp_pool.c > > @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, > > pool->s.blk_freelist = NULL; > > > > /* Initialization will increment these to their target vals */ > > - odp_atomic_store_u32(&pool->s.bufcount, 0); > > - odp_atomic_store_u32(&pool->s.blkcount, 0); > > + (void)odp_atomic_store_u32(&pool->s.bufcount, 0); > > + (void)odp_atomic_store_u32(&pool->s.blkcount, 0); > > > it's void function, not needed. > > > Coverity disagrees and I like the human documentation aspect, by > reading this I know that it was not forgotten and I dont have to go look > it up to know that. > > It looks like Coverity bug: _STATIC void odp_atomic_store_u32(odp_atomic_u32_t *atom, uint32_t val) { __atomic_store_n(&atom->v, val, __ATOMIC_RELAXED); } #if 0 #define ODP_ABI_COMPAT 1 #define _STATIC #else #define ODP_ABI_COMPAT 0 #define _STATIC static inline #endif So just mark it as false positive. Maxim. > > > > > > uint8_t *buf = udata_base_addr - buf_stride; > > uint8_t *udat = udata_stride == 0 ? NULL : > > diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c > > index 9655668..f0f893e 100644 > > --- a/platform/linux-generic/pktio/socket_mmap.c > > +++ b/platform/linux-generic/pktio/socket_mmap.c > > @@ -458,7 +458,7 @@ static int mmap_sock(pkt_sock_mmap_t *pkt_sock) > > > > static void mmap_unmap_sock(pkt_sock_mmap_t *pkt_sock) > > { > > - munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > > + (void)munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > > it's better to check return code then hide return check. > > > SEE! now that I put it explicitly it becomes obvious that it was a > mistake - will fix > > > > Maxim. > > > free(pkt_sock->rx_ring.rd); > > free(pkt_sock->tx_ring.rd); > > } > > > > > > > -- > Mike Holmes > Program Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>* **│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > > __ > >
On 12 December 2016 at 12:01, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/12/16 19:58, Mike Holmes wrote: > > > > > > On 12 December 2016 at 11:48, Maxim Uvarov <maxim.uvarov@linaro.org > > <mailto:maxim.uvarov@linaro.org>> wrote: > > > > On 12/12/16 19:11, Mike Holmes wrote: > > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org <mailto: > mike.holmes@linaro.org>> > > > --- > > > platform/linux-generic/odp_pool.c | 4 ++-- > > > platform/linux-generic/pktio/socket_mmap.c | 2 +- > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/platform/linux-generic/odp_pool.c > b/platform/linux-generic/odp_pool.c > > > index 415c9fa..858bdd8 100644 > > > --- a/platform/linux-generic/odp_pool.c > > > +++ b/platform/linux-generic/odp_pool.c > > > @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, > > > pool->s.blk_freelist = NULL; > > > > > > /* Initialization will increment these to their > target vals */ > > > - odp_atomic_store_u32(&pool->s.bufcount, 0); > > > - odp_atomic_store_u32(&pool->s.blkcount, 0); > > > + (void)odp_atomic_store_u32(&pool->s.bufcount, 0); > > > + (void)odp_atomic_store_u32(&pool->s.blkcount, 0); > > > > > > it's void function, not needed. > > > > > > Coverity disagrees and I like the human documentation aspect, by > > reading this I know that it was not forgotten and I dont have to go look > > it up to know that. > > > > > > It looks like Coverity bug: > > _STATIC void odp_atomic_store_u32(odp_atomic_u32_t *atom, uint32_t val) > { > __atomic_store_n(&atom->v, val, __ATOMIC_RELAXED); > } > > #if 0 > #define ODP_ABI_COMPAT 1 > #define _STATIC > #else > #define ODP_ABI_COMPAT 0 > #define _STATIC static inline > #endif > > > So just mark it as false positive. > It is a human thing - it is self documenting, we know we did not intend to check the return as soon as we read the code. > > Maxim. > > > > > > > > > > > uint8_t *buf = udata_base_addr - buf_stride; > > > uint8_t *udat = udata_stride == 0 ? NULL : > > > diff --git a/platform/linux-generic/pktio/socket_mmap.c > b/platform/linux-generic/pktio/socket_mmap.c > > > index 9655668..f0f893e 100644 > > > --- a/platform/linux-generic/pktio/socket_mmap.c > > > +++ b/platform/linux-generic/pktio/socket_mmap.c > > > @@ -458,7 +458,7 @@ static int mmap_sock(pkt_sock_mmap_t *pkt_sock) > > > > > > static void mmap_unmap_sock(pkt_sock_mmap_t *pkt_sock) > > > { > > > - munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > > > + (void)munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); > > > > it's better to check return code then hide return check. > > > > > > SEE! now that I put it explicitly it becomes obvious that it was a > > mistake - will fix > > > > > > > > Maxim. > > > > > free(pkt_sock->rx_ring.rd); > > > free(pkt_sock->tx_ring.rd); > > > } > > > > > > > > > > > > > -- > > Mike Holmes > > Program Manager - Linaro Networking Group > > Linaro.org <http://www.linaro.org/>* **│ *Open source software for ARM > SoCs > > "Work should be fun and collaborative, the rest follows" > > > > __ > > > > > > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
> > > > --- a/platform/linux-generic/odp_pool.c > > > > +++ b/platform/linux-generic/odp_pool.c > > > > @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, > > > > pool->s.blk_freelist = NULL; > > > > > > > > /* Initialization will increment these to their > > target vals */ > > > > - odp_atomic_store_u32(&pool->s.bufcount, 0); > > > > - odp_atomic_store_u32(&pool->s.blkcount, 0); > > > > + (void)odp_atomic_store_u32(&pool->s.bufcount, 0); > > > > + (void)odp_atomic_store_u32(&pool->s.blkcount, 0); > > > > > > > > > it's void function, not needed. > > > > > > > > > Coverity disagrees and I like the human documentation aspect, by > > > reading this I know that it was not forgotten and I dont have to go > look > > > it up to know that. > > > > > > > > > > It looks like Coverity bug: > > > > _STATIC void odp_atomic_store_u32(odp_atomic_u32_t *atom, uint32_t val) > > { > > __atomic_store_n(&atom->v, val, __ATOMIC_RELAXED); > > } > > > > #if 0 > > #define ODP_ABI_COMPAT 1 > > #define _STATIC > > #else > > #define ODP_ABI_COMPAT 0 > > #define _STATIC static inline > > #endif > > > > > > So just mark it as false positive. > > > > It is a human thing - it is self documenting, we know we did not intend to > check the return as soon as we read the code. > Casting function return values to void, especially when those are "void" already, looks suspicious and ugly. It's a Coverity bug and should be handled as such. -Petri
On 13 December 2016 at 05:31, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia-bell-labs.com> wrote: > > > > > --- a/platform/linux-generic/odp_pool.c > > > > > +++ b/platform/linux-generic/odp_pool.c > > > > > @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, > > > > > pool->s.blk_freelist = NULL; > > > > > > > > > > /* Initialization will increment these to their > > > target vals */ > > > > > - odp_atomic_store_u32(&pool->s.bufcount, 0); > > > > > - odp_atomic_store_u32(&pool->s.blkcount, 0); > > > > > + (void)odp_atomic_store_u32(&pool->s.bufcount, > 0); > > > > > + (void)odp_atomic_store_u32(&pool->s.blkcount, > 0); > > > > > > > > > > > > it's void function, not needed. > > > > > > > > > > > > Coverity disagrees and I like the human documentation aspect, by > > > > reading this I know that it was not forgotten and I dont have to go > > look > > > > it up to know that. > > > > > > > > > > > > > > It looks like Coverity bug: > > > > > > _STATIC void odp_atomic_store_u32(odp_atomic_u32_t *atom, uint32_t > val) > > > { > > > __atomic_store_n(&atom->v, val, __ATOMIC_RELAXED); > > > } > > > > > > #if 0 > > > #define ODP_ABI_COMPAT 1 > > > #define _STATIC > > > #else > > > #define ODP_ABI_COMPAT 0 > > > #define _STATIC static inline > > > #endif > > > > > > > > > So just mark it as false positive. > > > > > > > It is a human thing - it is self documenting, we know we did not intend > to > > check the return as soon as we read the code. > > > > Casting function return values to void, especially when those are "void" > already, looks suspicious and ugly. It's a Coverity bug and should be > handled as such. > It might be our bug, coverity needs to be told about a number of things and we have not configured it, we just ignore things on a case by case basis. As noted v2 will fix the correct case found in the last run. > > -Petri > > > > -- Mike Holmes Program Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows"
diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c index 415c9fa..858bdd8 100644 --- a/platform/linux-generic/odp_pool.c +++ b/platform/linux-generic/odp_pool.c @@ -433,8 +433,8 @@ odp_pool_t _pool_create(const char *name, pool->s.blk_freelist = NULL; /* Initialization will increment these to their target vals */ - odp_atomic_store_u32(&pool->s.bufcount, 0); - odp_atomic_store_u32(&pool->s.blkcount, 0); + (void)odp_atomic_store_u32(&pool->s.bufcount, 0); + (void)odp_atomic_store_u32(&pool->s.blkcount, 0); uint8_t *buf = udata_base_addr - buf_stride; uint8_t *udat = udata_stride == 0 ? NULL : diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c index 9655668..f0f893e 100644 --- a/platform/linux-generic/pktio/socket_mmap.c +++ b/platform/linux-generic/pktio/socket_mmap.c @@ -458,7 +458,7 @@ static int mmap_sock(pkt_sock_mmap_t *pkt_sock) static void mmap_unmap_sock(pkt_sock_mmap_t *pkt_sock) { - munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); + (void)munmap(pkt_sock->mmap_base, pkt_sock->mmap_len); free(pkt_sock->rx_ring.rd); free(pkt_sock->tx_ring.rd); }
Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- platform/linux-generic/odp_pool.c | 4 ++-- platform/linux-generic/pktio/socket_mmap.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) -- 2.9.3