On 18 May 2016 at 16:24, Maxim Uvarov <maxim.uva...@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.uva...@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
>
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to