| Message ID | 20210611123042.21288-1-surban@surban.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ] gatt-database: No multiple calls to AcquireWrite | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=498861 ---Test result--- Test Summary: CheckPatch PASS 0.42 seconds GitLint PASS 0.13 seconds Prep - Setup ELL PASS 48.91 seconds Build - Prep PASS 0.12 seconds Build - Configure PASS 8.62 seconds Build - Make PASS 211.88 seconds Make Check PASS 9.74 seconds Make Distcheck PASS 251.91 seconds Build w/ext ELL - Configure PASS 8.66 seconds Build w/ext ELL - Make PASS 203.92 seconds Details ############################## Test: CheckPatch - PASS Desc: Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS Desc: Run gitlint with rule in .gitlint ############################## Test: Prep - Setup ELL - PASS Desc: Clone, build, and install ELL ############################## Test: Build - Prep - PASS Desc: Prepare environment for build ############################## Test: Build - Configure - PASS Desc: Configure the BlueZ source tree ############################## Test: Build - Make - PASS Desc: Build the BlueZ source tree ############################## Test: Make Check - PASS Desc: Run 'make check' ############################## Test: Make Distcheck - PASS Desc: Run distcheck to check the distribution ############################## Test: Build w/ext ELL - Configure - PASS Desc: Configure BlueZ source with '--enable-external-ell' configuration ############################## Test: Build w/ext ELL - Make - PASS Desc: Build BlueZ source with '--enable-external-ell' configuration --- Regards, Linux Bluetooth
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 >
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); + } + + 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);