| Message ID | 1463581455-24846-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, May 18, 2016 at 9:24 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Accodring to API return code for fail case is ODP_TIMER_POOL_INVALID > and errno set event if it's defined to NULL. Also add check on pool > alloc that input parameter is not invalid. > https://bugs.linaro.org/show_bug.cgi?id=2139 > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > v2: added missed ) on if statement. (looks like forgot to git ---ammend > it ) > > platform/linux-generic/odp_timer.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index a6d3332..8696074 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -227,16 +227,15 @@ static inline odp_timer_t tp_idx_to_handle(struct > odp_timer_pool_s *tp, > static void itimer_init(odp_timer_pool *tp); > static void itimer_fini(odp_timer_pool *tp); > > -static odp_timer_pool *odp_timer_pool_new( > - const char *_name, > - const odp_timer_pool_param_t *param) > +static odp_timer_pool_t odp_timer_pool_new(const char *_name, > + const odp_timer_pool_param_t > *param) > { > uint32_t tp_idx = odp_atomic_fetch_add_u32(&num_timer_pools, 1); > if (odp_unlikely(tp_idx >= MAX_TIMER_POOLS)) { > /* Restore the previous value */ > odp_atomic_sub_u32(&num_timer_pools, 1); > __odp_errno = ENFILE; /* Table overflow */ > - return NULL; > + return ODP_TIMER_POOL_INVALID; > } > size_t sz0 = ODP_ALIGN_ROUNDUP(sizeof(odp_timer_pool), > ODP_CACHE_LINE_SIZE); > @@ -804,10 +803,9 @@ odp_timer_pool_create(const char *name, > /* Verify that we have a valid (non-zero) timer resolution */ > if (param->res_ns == 0) { > __odp_errno = EINVAL; > - return NULL; > + return ODP_TIMER_POOL_INVALID; > } > - odp_timer_pool_t tp = odp_timer_pool_new(name, param); > - return tp; > + return odp_timer_pool_new(name, param); > } > > void odp_timer_pool_start(void) > @@ -855,15 +853,17 @@ odp_timer_t odp_timer_alloc(odp_timer_pool_t tpid, > odp_queue_t queue, > void *user_ptr) > { > + odp_timer_t hdl; > + > + if (odp_unlikely(tpid == ODP_TIMER_POOL_INVALID)) > + ODP_ABORT("Invalid timer pool.\n"); > if (odp_unlikely(queue == ODP_QUEUE_INVALID)) > ODP_ABORT("%s: Invalid queue handle\n", tpid->name); > Is an abort here really necessary? Why not just ODP_DBG() a diagnostic and return ODP_TIMER_INVALID ? > /* We don't care about the validity of user_ptr because we will not > * attempt to dereference it */ > - odp_timer_t hdl = timer_alloc(tpid, queue, user_ptr); > - if (odp_likely(hdl != ODP_TIMER_INVALID)) { > - /* Success */ > - return hdl; > - } > + hdl = timer_alloc(tpid, queue, user_ptr); > + if (odp_likely(hdl != ODP_TIMER_INVALID)) > + return hdl; /* Success */ > /* errno set by timer_alloc() */ > return ODP_TIMER_INVALID; > This can be simplified to just: return timer_alloc(tpid, queue, user_ptr); > } > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 18 May 2016 at 16:24, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Accodring to API return code for fail case is ODP_TIMER_POOL_INVALID > and errno set event if it's defined to NULL. Also add check on pool > alloc that input parameter is not invalid. > https://bugs.linaro.org/show_bug.cgi?id=2139 If odp_timer_pool is a pointer in linux-generic, then it is OK for the implementation to return NULL for an invalid timer pool handle. The implementation should be allowed to use constants (e.g. NULL) for types which are under its control (e.g. odp_timer_pool) and not have to use abstract values (ODP_TIMER_POOL_INVALID). This is the implementation, it is not directly portable and reusable with other definitions of the timer types. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > v2: added missed ) on if statement. (looks like forgot to git ---ammend > it ) > > platform/linux-generic/odp_timer.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index a6d3332..8696074 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -227,16 +227,15 @@ static inline odp_timer_t tp_idx_to_handle(struct > odp_timer_pool_s *tp, > static void itimer_init(odp_timer_pool *tp); > static void itimer_fini(odp_timer_pool *tp); > > -static odp_timer_pool *odp_timer_pool_new( > - const char *_name, > - const odp_timer_pool_param_t *param) > +static odp_timer_pool_t odp_timer_pool_new(const char *_name, > + const odp_timer_pool_param_t > *param) > { > uint32_t tp_idx = odp_atomic_fetch_add_u32(&num_timer_pools, 1); > if (odp_unlikely(tp_idx >= MAX_TIMER_POOLS)) { > /* Restore the previous value */ > odp_atomic_sub_u32(&num_timer_pools, 1); > __odp_errno = ENFILE; /* Table overflow */ > - return NULL; > + return ODP_TIMER_POOL_INVALID; > } > size_t sz0 = ODP_ALIGN_ROUNDUP(sizeof(odp_timer_pool), > ODP_CACHE_LINE_SIZE); > @@ -804,10 +803,9 @@ odp_timer_pool_create(const char *name, > /* Verify that we have a valid (non-zero) timer resolution */ > if (param->res_ns == 0) { > __odp_errno = EINVAL; > - return NULL; > + return ODP_TIMER_POOL_INVALID; > } > - odp_timer_pool_t tp = odp_timer_pool_new(name, param); > - return tp; > + return odp_timer_pool_new(name, param); > } > > void odp_timer_pool_start(void) > @@ -855,15 +853,17 @@ odp_timer_t odp_timer_alloc(odp_timer_pool_t tpid, > odp_queue_t queue, > void *user_ptr) > { > + odp_timer_t hdl; > + > + if (odp_unlikely(tpid == ODP_TIMER_POOL_INVALID)) > + ODP_ABORT("Invalid timer pool.\n"); > Possibly it is helpful to check for invalid timer pool handle and die in a clean way. But we will not check a billion other invalid timer pool handles and those will likely generate undefined behaviour or (if we are lucky) a memory access violation (sigsegv). So there is not any guarantee that the use of invalid timer pool handle well be detected in any clean way. > if (odp_unlikely(queue == ODP_QUEUE_INVALID)) > ODP_ABORT("%s: Invalid queue handle\n", tpid->name); > The queue handle we want to verify now when it the context of the application. Later when a timer expires, we don't want to get errors in the timer background thread. That would likely be fatal and also difficult to debug. > /* We don't care about the validity of user_ptr because we will not > * attempt to dereference it */ > - odp_timer_t hdl = timer_alloc(tpid, queue, user_ptr); > - if (odp_likely(hdl != ODP_TIMER_INVALID)) { > - /* Success */ > - return hdl; > - } > + hdl = timer_alloc(tpid, queue, user_ptr); > + if (odp_likely(hdl != ODP_TIMER_INVALID)) > + return hdl; /* Success */ > As Bill wrote, this code can be simplified even more. Just call timer_alloc() and return its value directly. > /* errno set by timer_alloc() */ > return ODP_TIMER_INVALID; > } > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c index a6d3332..8696074 100644 --- a/platform/linux-generic/odp_timer.c +++ b/platform/linux-generic/odp_timer.c @@ -227,16 +227,15 @@ static inline odp_timer_t tp_idx_to_handle(struct odp_timer_pool_s *tp, static void itimer_init(odp_timer_pool *tp); static void itimer_fini(odp_timer_pool *tp); -static odp_timer_pool *odp_timer_pool_new( - const char *_name, - const odp_timer_pool_param_t *param) +static odp_timer_pool_t odp_timer_pool_new(const char *_name, + const odp_timer_pool_param_t *param) { uint32_t tp_idx = odp_atomic_fetch_add_u32(&num_timer_pools, 1); if (odp_unlikely(tp_idx >= MAX_TIMER_POOLS)) { /* Restore the previous value */ odp_atomic_sub_u32(&num_timer_pools, 1); __odp_errno = ENFILE; /* Table overflow */ - return NULL; + return ODP_TIMER_POOL_INVALID; } size_t sz0 = ODP_ALIGN_ROUNDUP(sizeof(odp_timer_pool), ODP_CACHE_LINE_SIZE); @@ -804,10 +803,9 @@ odp_timer_pool_create(const char *name, /* Verify that we have a valid (non-zero) timer resolution */ if (param->res_ns == 0) { __odp_errno = EINVAL; - return NULL; + return ODP_TIMER_POOL_INVALID; } - odp_timer_pool_t tp = odp_timer_pool_new(name, param); - return tp; + return odp_timer_pool_new(name, param); } void odp_timer_pool_start(void) @@ -855,15 +853,17 @@ odp_timer_t odp_timer_alloc(odp_timer_pool_t tpid, odp_queue_t queue, void *user_ptr) { + odp_timer_t hdl; + + if (odp_unlikely(tpid == ODP_TIMER_POOL_INVALID)) + ODP_ABORT("Invalid timer pool.\n"); if (odp_unlikely(queue == ODP_QUEUE_INVALID)) ODP_ABORT("%s: Invalid queue handle\n", tpid->name); /* We don't care about the validity of user_ptr because we will not * attempt to dereference it */ - odp_timer_t hdl = timer_alloc(tpid, queue, user_ptr); - if (odp_likely(hdl != ODP_TIMER_INVALID)) { - /* Success */ - return hdl; - } + hdl = timer_alloc(tpid, queue, user_ptr); + if (odp_likely(hdl != ODP_TIMER_INVALID)) + return hdl; /* Success */ /* errno set by timer_alloc() */ return ODP_TIMER_INVALID; }
Accodring to API return code for fail case is ODP_TIMER_POOL_INVALID and errno set event if it's defined to NULL. Also add check on pool alloc that input parameter is not invalid. https://bugs.linaro.org/show_bug.cgi?id=2139 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- v2: added missed ) on if statement. (looks like forgot to git ---ammend it ) platform/linux-generic/odp_timer.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)