+1 on Mike’s suggestion. Have debugged one too many accidental assignments that were supposed to be tests over the years. I personally would like to see brackets around single lines on conditionals as well.
From: [email protected] [mailto:[email protected]] On Behalf Of Mike Holmes Sent: Wednesday, November 19, 2014 4:23 PM To: Maxim Uvarov Cc: lng-odp Subject: Re: [lng-odp] [PATCH v3] add implement for odp_shm_free On 19 November 2014 16:13, Maxim Uvarov <[email protected]<mailto:[email protected]>> wrote: On 11/19/2014 11:21 PM, Mike Holmes wrote: On 19 November 2014 10:36, Yan Songming <[email protected]<mailto:[email protected]> <mailto:[email protected]<mailto:[email protected]>>> wrote: New API implementing odp_shm_free to match the odp_shm_reserve. Signed-off-by: Yan Songming <[email protected]<mailto:[email protected]> <mailto:[email protected]<mailto:[email protected]>>> --- v3 change the return value of odp_shm_free. v2 fix the problem which Maxim found. --- .../linux-generic/include/api/odp_shared_memory.h | 2 +- platform/linux-generic/odp_shared_memory.c | 43 +++++++++++++++++++--- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h index ff6f9a9..d42e272 100644 --- a/platform/linux-generic/include/api/odp_shared_memory.h +++ b/platform/linux-generic/include/api/odp_shared_memory.h @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, * @param[in] shm Block handle * From the implementation below we can be more precise about the behaviour of this API * @retval 0 for success @retval 0 if the handle is already free @retval 0 if the handle free succeeds - * @retval 1 on failure + * @retval -1 on failure @retval -1 on failure to free the handle */ int odp_shm_free(odp_shm_t shm); diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c index 24a5d60..bc0ab55 100644 --- a/platform/linux-generic/odp_shared_memory.c +++ b/platform/linux-generic/odp_shared_memory.c @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t *index) return 0; } +int odp_shm_free(odp_shm_t shm) +{ + uint32_t i; + int ret; + odp_shm_block_t *shm_block; + uint64_t alloc_size; + + i = from_handle(shm); + if (odp_shm_tbl->block[i].addr == NULL) { Would it be better to follow the slightly better habit of reversing the constant with the variable NULL == odp_shm_tbl->block[i].addr why is that better? I always write tested variable first. http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 Basically the reverse habit if applied uniformly removes the chance of the linked silly mistake, if we adopt that for ODP we will make it harder to suffer from the issue. But we would have to do it uniformly for best effect so that during a review it catches your eye. Maxim. + /* free block */ Is this comment adding value ? + ODP_DBG("odp_shm_free: Free block\n"); + return 0; + } + + odp_spinlock_lock(&odp_shm_tbl->lock); + shm_block = &odp_shm_tbl->block[i]; + + alloc_size = shm_block->size + shm_block->align; + ret = munmap(shm_block->addr, alloc_size); + if (0 != ret) { + ODP_DBG("odp_shm_free: munmap failed\n"); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return -1; + } + + if (shm_block->flags & ODP_SHM_PROC) { + ret = shm_unlink(shm_block->name); + if (0 != ret) { + ODP_DBG("odp_shm_free: shm_unlink failed\n"); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return -1; + } + } + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return 0; +} odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, uint32_t flags) @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, return block->hdl; } -int odp_shm_free(odp_shm_t shm ODP_UNUSED) -{ - ODP_UNIMPLEMENTED(); - return 0; -} - odp_shm_t odp_shm_lookup(const char *name) { uint32_t i; -- 1.8.3.1 _______________________________________________ lng-odp mailing list [email protected]<mailto:[email protected]> <mailto:[email protected]<mailto:[email protected]>> http://lists.linaro.org/mailman/listinfo/lng-odp -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP _______________________________________________ lng-odp mailing list [email protected]<mailto:[email protected]> http://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list [email protected]<mailto:[email protected]> http://lists.linaro.org/mailman/listinfo/lng-odp -- Mike Holmes Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
