On 19 November 2014 17:13, Robbie King (robking) <[email protected]> wrote:
> +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. > I have suffered from this also, subsequent work adds code to the "if" but forgets to add braces We would need to update the checkpatch rule to allow it however > > > *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]> wrote: > > On 11/19/2014 11:21 PM, Mike Holmes wrote: > > > > On 19 November 2014 10:36, Yan Songming <[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]>> > > > --- > 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]> > 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 > > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > -- > > *Mike Holmes* > > Linaro Sr Technical Manager > > LNG - ODP > -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
