Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201005041932.99783-1-inga.stotland@intel.com>
 <38633edb6423eabcd54c3b7559117e7ee64cb51b.camel@intel.com> <34814d0a7ad0f0ba19bb85e596f5a47843d2da39.camel@intel.com>
In-Reply-To: <34814d0a7ad0f0ba19bb85e596f5a47843d2da39.camel@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 19 Oct 2020 13:18:09 -0700
Message-ID: <CABBYNZ+4G-5CRLwcDX+BJa30K_umaJtoi7bBhXjbD=4Jpjj8nw@mail.gmail.com>
Subject: Re: [PATCH BlueZ] shared/timeout-ell: Fix timeout wrapper implementation
To:     "Stotland, Inga" <inga.stotland@intel.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Inga,

On Mon, Oct 19, 2020 at 1:00 PM Stotland, Inga <inga.stotland@intel.com> wrote:
>
> Another ping
>
> On Fri, 2020-10-09 at 21:03 -0700, Inga Stotland wrote:
>
> Gentle ping
>
> On Sun, 2020-10-04 at 21:19 -0700, Inga Stotland wrote:
>
> This fixes the following issues:
>
> - Correct user data is passed around to l_timeout_create():
>
>   locally allocated timeout data is a valid "user data" to
>
>   associate with a newly created timeout. Previously, user_data
>
>   passed as an argument to timeout_add() was incorrectly used as
>
>   an argument to l_timeout_create()
>
> - To maintain common API and work around the issue when the conversion
>
>   of a pointer to an unsigned int truncates the initial value, a queue
>
>   of active timeouts is maintained where pointer each l_timeout structure
>
>   is associate with a unique id. This id is returned when timeout_create()
>
>   API is called and can be subsequently used with timeout_remove().
>
> ---
>
>  src/shared/timeout-ell.c | 50 ++++++++++++++++++++++++++++++++++++----
>
>  1 file changed, 45 insertions(+), 5 deletions(-)
>
>
> diff --git a/src/shared/timeout-ell.c b/src/shared/timeout-ell.c
>
> index c2da387e2..023364069 100644
>
> --- a/src/shared/timeout-ell.c
>
> +++ b/src/shared/timeout-ell.c
>
> @@ -12,13 +12,23 @@
>
>
>
>  #include "timeout.h"
>
>
>
> +static struct l_queue *timeout_q;
>
> +
>
>  struct timeout_data {
>
>   timeout_func_t func;
>
>   timeout_destroy_func_t destroy;
>
> - unsigned int timeout;
>
>   void *user_data;
>
> + unsigned int timeout;
>
>  };
>
>
>
> +static bool match_id(const void *a, const void *b)
>
> +{
>
> + unsigned int to_id = L_PTR_TO_UINT(a);
>
> + unsigned int id = L_PTR_TO_UINT(b);
>
> +
>
> + return (to_id == id);
>
> +}
>
> +
>
>  static void timeout_callback(struct l_timeout *timeout, void *user_data)
>
>  {
>
>   struct timeout_data *data = user_data;
>
> @@ -43,7 +53,12 @@ unsigned int timeout_add(unsigned int timeout, timeout_func_t func,
>
>   void *user_data, timeout_destroy_func_t destroy)
>
>  {
>
>   struct timeout_data *data;
>
> - uint32_t id;
>
> + unsigned int id = 0;
>
> + struct l_timeout *to;
>
> + int tries = 0;
>
> +
>
> + if (!timeout_q)
>
> + timeout_q = l_queue_new();
>
>
>
>   data = l_new(struct timeout_data, 1);
>
>
>
> @@ -52,12 +67,37 @@ unsigned int timeout_add(unsigned int timeout, timeout_func_t func,
>
>   data->user_data = user_data;
>
>   data->timeout = timeout;
>
>
>
> - id = L_PTR_TO_UINT(l_timeout_create(timeout, timeout_callback,
>
> - user_data, timeout_destroy));
>
> + while (id == 0 && tries < 3) {
>
> + to = l_timeout_create(timeout, timeout_callback,
>
> + data, timeout_destroy);
>
> + if (!to)
>
> + break;
>
> +
>
> + tries++;
>
> + id = L_PTR_TO_UINT(to);
>
> +
>
> + if (id == 0 ||
>
> + l_queue_find(timeout_q, match_id, L_UINT_TO_PTR(id))) {
>
> +
>
> + l_timeout_remove(to);
>
> + continue;
>
> + }
>
> +
>
> + l_queue_push_tail(timeout_q, to);
>
> + }
>
> +
>
> + if (id == 0)
>
> + l_free(data);
>
> +
>
>   return id;
>
>  }
>
>
>
>  void timeout_remove(unsigned int id)
>
>  {
>
> - l_timeout_remove(L_UINT_TO_PTR(id));
>
> + struct l_timeout *to;
>
> +
>
> + to = l_queue_remove_if(timeout_q, match_id, L_UINT_TO_PTR(id));
>
> +
>
> + if (to)
>
> + l_timeout_remove(to);
>
>  }

Applied, thanks.

-- 
Luiz Augusto von Dentz