| Message ID | 1422982108-13813-15-git-send-email-ola.liljedahl@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 4 February 2015 at 14:50, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl >> Sent: Tuesday, February 03, 2015 6:48 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCHv5 14/18] api: odp_shared_memory.h: updated >> return descriptions >> >> Updated doxygen descriptions, particularly the @return/@retval >> descriptions. >> No change of implementation necessary. >> Documented API calls which are guaranteed to handle invalid/stale handles. >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> --- >> (This document/code contribution attached is provided under the terms of >> agreement LES-LTM-21309) >> >> include/odp/api/shared_memory.h | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/include/odp/api/shared_memory.h >> b/include/odp/api/shared_memory.h >> index 5b27b6b..18764ce 100644 >> --- a/include/odp/api/shared_memory.h >> +++ b/include/odp/api/shared_memory.h >> @@ -71,7 +71,8 @@ typedef struct odp_shm_info_t { >> * @param[in] flags Shared memory parameter flags (ODP_SHM_*). >> * Default value is 0. >> * >> - * @return Pointer to the reserved block, or NULL >> + * @return Handle of the reserved block >> + * @retval NULL on failure >> */ >> odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t >> align, >> uint32_t flags); >> @@ -84,9 +85,8 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t >> size, uint64_t align, >> * >> * @param[in] shm Block handle >> * >> - * @retval 0 if the handle is already free >> - * @retval 0 if the handle free succeeds >> - * @retval -1 on failure to free the handle >> + * @retval 0 on success >> + * @retval <0 on failure to free the handle >> */ >> int odp_shm_free(odp_shm_t shm); >> >> @@ -96,7 +96,7 @@ int odp_shm_free(odp_shm_t shm); >> * @param[in] name Name of the block >> * >> * @return A handle to the block if it is found by name >> - * @retval #ODP_SHM_INVALID if the block is not found >> + * @retval ODP_SHM_INVALID on failure >> */ >> odp_shm_t odp_shm_lookup(const char *name); >> >> @@ -106,18 +106,21 @@ odp_shm_t odp_shm_lookup(const char *name); >> * >> * @param[in] shm Block handle >> * >> - * @return Memory block address, or NULL on error >> + * @return Memory block address > > > > Missing the failure case. No this was intentional. How can odp_shm_addr() fail? What are those errors? Can the function spuriously return NULL? Either the shm handle is valid and the function returns an address. Or the handle is invalid and the behavior is undefined (possibly function does not return at all). If we require odp_shm_addr() to detect invalid shm handles, this behavior and requirement needs to be specified. How can an ODP implementer otherwise know what needs to be implemented? How can the test program writer know what scenarios to test and their expected outcomes? > > @retval NULL on failure > > > -Petri > > >> */ >> void *odp_shm_addr(odp_shm_t shm); >> >> >> /** >> * Shared memory block info >> + * @note This is the only shared memory API function which accepts >> invalid >> + * shm handles (any bit value) without causing undefined behavior. >> * >> * @param[in] shm Block handle >> * @param[out] info Block info pointer for output >> * >> - * @return 0 on success, otherwise non-zero >> + * @retval 0 on success >> + * @retval <0 on failure >> */ >> int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info); >> >> -- >> 1.9.1 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
On 4 February 2015 at 16:34, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >> Sent: Wednesday, February 04, 2015 4:08 PM >> To: Savolainen, Petri (NSN - FI/Espoo) >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv5 14/18] api: odp_shared_memory.h: updated >> return descriptions >> >> On 4 February 2015 at 14:50, Savolainen, Petri (NSN - FI/Espoo) >> <petri.savolainen@nsn.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> >> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl >> >> Sent: Tuesday, February 03, 2015 6:48 PM >> >> To: lng-odp@lists.linaro.org >> >> Subject: [lng-odp] [PATCHv5 14/18] api: odp_shared_memory.h: updated >> >> return descriptions >> >> >> >> Updated doxygen descriptions, particularly the @return/@retval >> >> descriptions. >> >> No change of implementation necessary. >> >> Documented API calls which are guaranteed to handle invalid/stale >> handles. >> >> >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> >> --- >> >> (This document/code contribution attached is provided under the terms >> of >> >> agreement LES-LTM-21309) >> >> >> >> include/odp/api/shared_memory.h | 17 ++++++++++------- >> >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> >> >> diff --git a/include/odp/api/shared_memory.h >> >> b/include/odp/api/shared_memory.h >> >> index 5b27b6b..18764ce 100644 >> >> --- a/include/odp/api/shared_memory.h >> >> +++ b/include/odp/api/shared_memory.h >> >> @@ -71,7 +71,8 @@ typedef struct odp_shm_info_t { >> >> * @param[in] flags Shared memory parameter flags (ODP_SHM_*). >> >> * Default value is 0. >> >> * >> >> - * @return Pointer to the reserved block, or NULL >> >> + * @return Handle of the reserved block >> >> + * @retval NULL on failure >> >> */ >> >> odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t >> >> align, >> >> uint32_t flags); >> >> @@ -84,9 +85,8 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t >> >> size, uint64_t align, >> >> * >> >> * @param[in] shm Block handle >> >> * >> >> - * @retval 0 if the handle is already free >> >> - * @retval 0 if the handle free succeeds >> >> - * @retval -1 on failure to free the handle >> >> + * @retval 0 on success >> >> + * @retval <0 on failure to free the handle >> >> */ >> >> int odp_shm_free(odp_shm_t shm); >> >> >> >> @@ -96,7 +96,7 @@ int odp_shm_free(odp_shm_t shm); >> >> * @param[in] name Name of the block >> >> * >> >> * @return A handle to the block if it is found by name >> >> - * @retval #ODP_SHM_INVALID if the block is not found >> >> + * @retval ODP_SHM_INVALID on failure >> >> */ >> >> odp_shm_t odp_shm_lookup(const char *name); >> >> >> >> @@ -106,18 +106,21 @@ odp_shm_t odp_shm_lookup(const char *name); >> >> * >> >> * @param[in] shm Block handle >> >> * >> >> - * @return Memory block address, or NULL on error >> >> + * @return Memory block address >> > >> > >> > >> > Missing the failure case. >> No this was intentional. >> How can odp_shm_addr() fail? What are those errors? Can the function >> spuriously return NULL? >> >> Either the shm handle is valid and the function returns an address. >> Or the handle is invalid and the behavior is undefined (possibly >> function does not return at all). >> >> If we require odp_shm_addr() to detect invalid shm handles, this >> behavior and requirement needs to be specified. How can an ODP >> implementer otherwise know what needs to be implemented? How can the >> test program writer know what scenarios to test and their expected >> outcomes? > > Shm is a shared resource. The address is not necessary coded into the handle. I didn't expect the address to be encoded into the handle. > > Due to a synchronization error between threads another thread may have freed the shm. If implementation can catch that (shm status changed to "free"), it can return NULL. If it cannot catch, it may e.g. crash (undefined behavior). So for passing stale shm handles, the behavior is undefined. Why do we have to specify a return value for that? I think you are creating a situation that cannot be tested. There is no reliable way of making odp_shm_addr() return NULL. But to superficially conform to the API spec, the caller should still test for NULL return value. How do we test this situation? How do we get code coverage? > > I think we agreed that, functions which access a shared resource may return error vs. functions which access non-shared resources (buffer, packet, tmo, event) only cannot return error on a bad handle (but abort or crash). I think behavior needs to be well defined. Either a function detects invalid handles and returns some specific value or code for that, validation programs can then test for compliance. Or passing of invalid handles invokes undefined behavior and we don't specify what will happen and no test is needed (or possible). > > Thread A: Thread B: > > // success > if (odp_shm_xxx(shm, ...)) > ERROR(); > > // success > if (odp_shm_xxx(shm, ...)) > ERROR(); > > odp_shm_free(shm, ...) > > > // fail/abort/crash depending > // on implementation. A robust > // implementation catches the > // error and returns an error > // all ways. But then we should specify that the ODP implementation has to detect invalid handles (and I don't think we can limit this to just stale handles) and return some error code. > if (odp_shm_xxx(shm, ...)) > ERROR(); > > > > -Petri > > >> >> >> > >> > @retval NULL on failure >> > >> > >> > -Petri >> > >> > >> >> */ >> >> void *odp_shm_addr(odp_shm_t shm); >> >> >> >> >> >> /** >> >> * Shared memory block info >> >> + * @note This is the only shared memory API function which accepts >> >> invalid >> >> + * shm handles (any bit value) without causing undefined behavior. >> >> * >> >> * @param[in] shm Block handle >> >> * @param[out] info Block info pointer for output >> >> * >> >> - * @return 0 on success, otherwise non-zero >> >> + * @retval 0 on success >> >> + * @retval <0 on failure >> >> */ >> >> int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info); >> >> >> >> -- >> >> 1.9.1 >> >> >> >> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org >> >> http://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/include/odp/api/shared_memory.h b/include/odp/api/shared_memory.h index 5b27b6b..18764ce 100644 --- a/include/odp/api/shared_memory.h +++ b/include/odp/api/shared_memory.h @@ -71,7 +71,8 @@ typedef struct odp_shm_info_t { * @param[in] flags Shared memory parameter flags (ODP_SHM_*). * Default value is 0. * - * @return Pointer to the reserved block, or NULL + * @return Handle of the reserved block + * @retval NULL on failure */ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, uint32_t flags); @@ -84,9 +85,8 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, * * @param[in] shm Block handle * - * @retval 0 if the handle is already free - * @retval 0 if the handle free succeeds - * @retval -1 on failure to free the handle + * @retval 0 on success + * @retval <0 on failure to free the handle */ int odp_shm_free(odp_shm_t shm); @@ -96,7 +96,7 @@ int odp_shm_free(odp_shm_t shm); * @param[in] name Name of the block * * @return A handle to the block if it is found by name - * @retval #ODP_SHM_INVALID if the block is not found + * @retval ODP_SHM_INVALID on failure */ odp_shm_t odp_shm_lookup(const char *name); @@ -106,18 +106,21 @@ odp_shm_t odp_shm_lookup(const char *name); * * @param[in] shm Block handle * - * @return Memory block address, or NULL on error + * @return Memory block address */ void *odp_shm_addr(odp_shm_t shm); /** * Shared memory block info + * @note This is the only shared memory API function which accepts invalid + * shm handles (any bit value) without causing undefined behavior. * * @param[in] shm Block handle * @param[out] info Block info pointer for output * - * @return 0 on success, otherwise non-zero + * @retval 0 on success + * @retval <0 on failure */ int odp_shm_info(odp_shm_t shm, odp_shm_info_t *info);
Updated doxygen descriptions, particularly the @return/@retval descriptions. No change of implementation necessary. Documented API calls which are guaranteed to handle invalid/stale handles. Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> --- (This document/code contribution attached is provided under the terms of agreement LES-LTM-21309) include/odp/api/shared_memory.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)