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: <20210611123042.21288-1-surban@surban.net>
In-Reply-To: <20210611123042.21288-1-surban@surban.net>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 11 Jun 2021 15:31:42 -0700
Message-ID: <CABBYNZ+Z-Z8e2XPpRxfdWoP2AP5QeMVFqhk7Go+2HMD7xpdc4g@mail.gmail.com>
Subject: Re: [PATCH BlueZ] gatt-database: No multiple calls to AcquireWrite
To:     Sebastian Urban <surban@surban.net>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Sebastian,

On Fri, Jun 11, 2021 at 5:32 AM Sebastian Urban <surban@surban.net> wrote:
>
> This checks if an outstanding call to AcquireWrite is already in
> progress. If so, the write request is placed into the queue, but
> AcquireWrite is not called again. When a response to AcquireWrite is
> received, acquire_write_reply sends all queued writes over the acquired
> socket.
>
> Making multiple simultaneous calls to AcquireWrite makes no sense,
> as this would open multiple socket pairs and only the last returned
> socket would be used for further writes.
> ---
>  src/gatt-database.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index be6dfb265..071f88583 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -2447,6 +2447,7 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
>  {
>         struct pending_op *op = user_data;
>         struct external_chrc *chrc;
> +       struct queue *resend;
>         DBusError err;
>         int fd;
>         uint16_t mtu;
> @@ -2488,18 +2489,35 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
>
>         chrc->write_io = sock_io_new(fd, chrc);
>
> -       if (sock_io_send(chrc->write_io, op->data.iov_base,
> -                               op->data.iov_len) < 0)
> -               goto retry;
> +       while ((op = queue_peek_head(chrc->pending_writes)) != NULL) {
> +               if (sock_io_send(chrc->write_io, op->data.iov_base,
> +                                       op->data.iov_len) < 0)
> +                       goto retry;
>
> -       gatt_db_attribute_write_result(op->attrib, op->id, 0);
> +               gatt_db_attribute_write_result(op->attrib, op->id, 0);
> +               pending_op_free(op);
> +       }
>
>         return;
>
>  retry:
> -       send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
> -                               op->data.iov_base, op->data.iov_len, 0,
> -                               op->link_type, false, false);
> +       /*
> +        * send_write pushes to chrc->pending_writes, so we need a
> +        * temporary queue to avoid an infinite loop.
> +        */
> +       resend = queue_new();
> +
> +       while ((op = queue_pop_head(chrc->pending_writes)) != NULL)
> +               queue_push_tail(resend, op);
> +
> +       while ((op = queue_pop_head(resend)) != NULL) {
> +               send_write(op->device, op->attrib, chrc->proxy, NULL, op->id,
> +                                       op->data.iov_base, op->data.iov_len, 0,
> +                                       op->link_type, false, false);
> +               pending_op_free(op);
> +       }

It might be better to have a separate function to flush the operation
on pending_writes since it just creating another pending_write_new we
could just call WriteValue directly since the original op can be
reused as we no longer free the op automatically.

> +       queue_destroy(resend, NULL);
>  }
>
>  static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
> @@ -2527,14 +2545,18 @@ static struct pending_op *acquire_write(struct external_chrc *chrc,
>                                         uint8_t link_type)
>  {
>         struct pending_op *op;
> +       bool acquiring = !queue_isempty(chrc->pending_writes);
>
>         op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
>                                 len, 0, link_type, false, false);
>
> +       if (acquiring)
> +               return op;
> +
>         if (g_dbus_proxy_method_call(chrc->proxy, "AcquireWrite",
>                                         acquire_write_setup,
>                                         acquire_write_reply,
> -                                       op, pending_op_free))
> +                                       op, NULL))
>                 return op;
>
>         pending_op_free(op);
> --
> 2.25.1
>


-- 
Luiz Augusto von Dentz