* [PATCH] gattrib: Fix a request/response command deadlock
@ 2012-05-28 18:34 Ido Yariv
2012-05-28 19:45 ` Vinicius Costa Gomes
0 siblings, 1 reply; 8+ messages in thread
From: Ido Yariv @ 2012-05-28 18:34 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Ido Yariv
New requests and responses are never sent if a request was sent and the
response for it hasn't been received yet. As a result, if both end
points send requests at the same time, a deadlock could occur. This
could happen, for instance, if the client sends a read request and the
server sends an indication before responding to the read request.
Fix this by introducing an additional queue for responses. Responses may
be sent while there's still a pending request/indication.
---
attrib/gattrib.c | 86 +++++++++++++++++++++++++++++++++++++++++++----------
1 files changed, 69 insertions(+), 17 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 769be36..769d746 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -47,6 +47,7 @@ struct _GAttrib {
guint write_watch;
guint timeout_watch;
GQueue *queue;
+ GQueue *response_queue;
GSList *events;
guint next_cmd_id;
guint next_evt_id;
@@ -175,9 +176,13 @@ static void attrib_destroy(GAttrib *attrib)
while ((c = g_queue_pop_head(attrib->queue)))
command_destroy(c);
+ while ((c = g_queue_pop_head(attrib->response_queue)))
+ command_destroy(c);
g_queue_free(attrib->queue);
attrib->queue = NULL;
+ g_queue_free(attrib->response_queue);
+ attrib->response_queue = NULL;
for (l = attrib->events; l; l = l->next)
event_destroy(l->data);
@@ -259,21 +264,34 @@ static gboolean can_write_data(GIOChannel *io, GIOCondition cond,
GError *gerr = NULL;
gsize len;
GIOStatus iostat;
+ GQueue *queue;
if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
return FALSE;
- cmd = g_queue_peek_head(attrib->queue);
+ queue = attrib->response_queue;
+ cmd = g_queue_peek_head(queue);
+ if (cmd == NULL) {
+ queue = attrib->queue;
+ cmd = g_queue_peek_head(queue);
+ }
if (cmd == NULL)
return FALSE;
+ /*
+ * Verify that we didn't already send this command. This can only
+ * happen with elementes from attrib->queue.
+ */
+ if (cmd->sent)
+ return FALSE;
+
iostat = g_io_channel_write_chars(io, (gchar *) cmd->pdu, cmd->len,
&len, &gerr);
if (iostat != G_IO_STATUS_NORMAL)
return FALSE;
if (cmd->expected == 0) {
- g_queue_pop_head(attrib->queue);
+ g_queue_pop_head(queue);
command_destroy(cmd);
return TRUE;
@@ -315,7 +333,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
uint8_t buf[512], status;
gsize len;
GIOStatus iostat;
- gboolean qempty;
+ gboolean qempty, respqempty;
if (attrib->timeout_watch > 0) {
g_source_remove(attrib->timeout_watch);
@@ -369,6 +387,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
done:
qempty = attrib->queue == NULL || g_queue_is_empty(attrib->queue);
+ respqempty = attrib->response_queue == NULL ||
+ g_queue_is_empty(attrib->response_queue);
if (cmd) {
if (cmd->func)
@@ -377,7 +397,7 @@ done:
command_destroy(cmd);
}
- if (!qempty)
+ if (!qempty || !respqempty)
wake_up_sender(attrib);
return TRUE;
@@ -397,6 +417,7 @@ GAttrib *g_attrib_new(GIOChannel *io)
attrib->io = g_io_channel_ref(io);
attrib->queue = g_queue_new();
+ attrib->response_queue = g_queue_new();
attrib->read_watch = g_io_add_watch(attrib->io,
G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
@@ -421,6 +442,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
gpointer user_data, GDestroyNotify notify)
{
struct command *c;
+ GQueue *queue;
c = g_try_new0(struct command, 1);
if (c == NULL)
@@ -435,15 +457,25 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
c->user_data = user_data;
c->notify = notify;
+ if (is_response(opcode))
+ queue = attrib->response_queue;
+ else
+ queue = attrib->queue;
+
if (id) {
c->id = id;
- g_queue_push_head(attrib->queue, c);
+ g_queue_push_head(queue, c);
} else {
c->id = ++attrib->next_cmd_id;
- g_queue_push_tail(attrib->queue, c);
+ g_queue_push_tail(queue, c);
}
- if (g_queue_get_length(attrib->queue) == 1)
+ /*
+ * If a command was added to the queue and it was empty before, wake up
+ * the sender. If the sender was already woken up by the second queue,
+ * wake_up_sender will just return.
+ */
+ if (g_queue_get_length(queue) == 1)
wake_up_sender(attrib);
return c->id;
@@ -459,38 +491,49 @@ static gint command_cmp_by_id(gconstpointer a, gconstpointer b)
gboolean g_attrib_cancel(GAttrib *attrib, guint id)
{
- GList *l;
+ GList *l = NULL;
struct command *cmd;
+ GQueue *queue;
- if (attrib == NULL || attrib->queue == NULL)
+ if (attrib == NULL)
return FALSE;
- l = g_queue_find_custom(attrib->queue, GUINT_TO_POINTER(id),
- command_cmp_by_id);
+ queue = attrib->queue;
+ if (queue)
+ l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
+ command_cmp_by_id);
+ if (l == NULL) {
+ queue = attrib->response_queue;
+ if (!queue)
+ return FALSE;
+ l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
+ command_cmp_by_id);
+ }
+
if (l == NULL)
return FALSE;
cmd = l->data;
- if (cmd == g_queue_peek_head(attrib->queue) && cmd->sent)
+ if (cmd == g_queue_peek_head(queue) && cmd->sent)
cmd->func = NULL;
else {
- g_queue_remove(attrib->queue, cmd);
+ g_queue_remove(queue, cmd);
command_destroy(cmd);
}
return TRUE;
}
-gboolean g_attrib_cancel_all(GAttrib *attrib)
+static gboolean g_attrib_cancel_all_per_queue(GQueue *queue)
{
struct command *c, *head = NULL;
gboolean first = TRUE;
- if (attrib == NULL || attrib->queue == NULL)
+ if (queue == NULL)
return FALSE;
- while ((c = g_queue_pop_head(attrib->queue))) {
+ while ((c = g_queue_pop_head(queue))) {
if (first && c->sent) {
/* If the command was sent ignore its callback ... */
c->func = NULL;
@@ -504,12 +547,21 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
if (head) {
/* ... and put it back in the queue */
- g_queue_push_head(attrib->queue, head);
+ g_queue_push_head(queue, head);
}
return TRUE;
}
+gboolean g_attrib_cancel_all(GAttrib *attrib)
+{
+ if (attrib == NULL)
+ return FALSE;
+
+ return g_attrib_cancel_all_per_queue(attrib->queue) &&
+ g_attrib_cancel_all_per_queue(attrib->response_queue);
+}
+
gboolean g_attrib_set_debug(GAttrib *attrib,
GAttribDebugFunc func, gpointer user_data)
{
--
1.7.7.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] gattrib: Fix a request/response command deadlock
2012-05-28 18:34 [PATCH] gattrib: Fix a request/response command deadlock Ido Yariv
@ 2012-05-28 19:45 ` Vinicius Costa Gomes
2012-05-29 1:16 ` Ido Yariv
0 siblings, 1 reply; 8+ messages in thread
From: Vinicius Costa Gomes @ 2012-05-28 19:45 UTC (permalink / raw)
To: Ido Yariv; +Cc: linux-bluetooth
Hi Ido,
On 21:34 Mon 28 May, Ido Yariv wrote:
> New requests and responses are never sent if a request was sent and the
> response for it hasn't been received yet. As a result, if both end
> points send requests at the same time, a deadlock could occur. This
> could happen, for instance, if the client sends a read request and the
> server sends an indication before responding to the read request.
>
> Fix this by introducing an additional queue for responses. Responses may
> be sent while there's still a pending request/indication.
> ---
> attrib/gattrib.c | 86 +++++++++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 69 insertions(+), 17 deletions(-)
>
> diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> index 769be36..769d746 100644
> --- a/attrib/gattrib.c
> +++ b/attrib/gattrib.c
> @@ -47,6 +47,7 @@ struct _GAttrib {
> guint write_watch;
> guint timeout_watch;
> GQueue *queue;
> + GQueue *response_queue;
I would change the name of the other queue as well, to make it clear
that the two queues have different objectives.
> GSList *events;
> guint next_cmd_id;
> guint next_evt_id;
> @@ -175,9 +176,13 @@ static void attrib_destroy(GAttrib *attrib)
>
> while ((c = g_queue_pop_head(attrib->queue)))
> command_destroy(c);
I would add a empty line here.
> + while ((c = g_queue_pop_head(attrib->response_queue)))
> + command_destroy(c);
>
> g_queue_free(attrib->queue);
> attrib->queue = NULL;
And here.
> + g_queue_free(attrib->response_queue);
> + attrib->response_queue = NULL;
>
> for (l = attrib->events; l; l = l->next)
> event_destroy(l->data);
> @@ -259,21 +264,34 @@ static gboolean can_write_data(GIOChannel *io, GIOCondition cond,
> GError *gerr = NULL;
> gsize len;
> GIOStatus iostat;
> + GQueue *queue;
>
> if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
> return FALSE;
>
> - cmd = g_queue_peek_head(attrib->queue);
> + queue = attrib->response_queue;
> + cmd = g_queue_peek_head(queue);
> + if (cmd == NULL) {
> + queue = attrib->queue;
> + cmd = g_queue_peek_head(queue);
> + }
> if (cmd == NULL)
> return FALSE;
>
> + /*
> + * Verify that we didn't already send this command. This can only
> + * happen with elementes from attrib->queue.
> + */
> + if (cmd->sent)
> + return FALSE;
> +
> iostat = g_io_channel_write_chars(io, (gchar *) cmd->pdu, cmd->len,
> &len, &gerr);
> if (iostat != G_IO_STATUS_NORMAL)
> return FALSE;
>
> if (cmd->expected == 0) {
> - g_queue_pop_head(attrib->queue);
> + g_queue_pop_head(queue);
> command_destroy(cmd);
>
> return TRUE;
> @@ -315,7 +333,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> uint8_t buf[512], status;
> gsize len;
> GIOStatus iostat;
> - gboolean qempty;
> + gboolean qempty, respqempty;
>
> if (attrib->timeout_watch > 0) {
> g_source_remove(attrib->timeout_watch);
> @@ -369,6 +387,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
>
> done:
> qempty = attrib->queue == NULL || g_queue_is_empty(attrib->queue);
> + respqempty = attrib->response_queue == NULL ||
> + g_queue_is_empty(attrib->response_queue);
>
> if (cmd) {
> if (cmd->func)
> @@ -377,7 +397,7 @@ done:
> command_destroy(cmd);
> }
>
> - if (!qempty)
> + if (!qempty || !respqempty)
> wake_up_sender(attrib);
>
> return TRUE;
> @@ -397,6 +417,7 @@ GAttrib *g_attrib_new(GIOChannel *io)
>
> attrib->io = g_io_channel_ref(io);
> attrib->queue = g_queue_new();
> + attrib->response_queue = g_queue_new();
>
> attrib->read_watch = g_io_add_watch(attrib->io,
> G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> @@ -421,6 +442,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
> gpointer user_data, GDestroyNotify notify)
> {
> struct command *c;
> + GQueue *queue;
>
> c = g_try_new0(struct command, 1);
> if (c == NULL)
> @@ -435,15 +457,25 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
> c->user_data = user_data;
> c->notify = notify;
>
> + if (is_response(opcode))
> + queue = attrib->response_queue;
> + else
> + queue = attrib->queue;
> +
> if (id) {
> c->id = id;
> - g_queue_push_head(attrib->queue, c);
> + g_queue_push_head(queue, c);
I don't know if this is the right thing to do for responses. For
requests there's is no problem if I rearrange them, but if I rearrange
responses I guess that it would confuse the remote side.
> } else {
> c->id = ++attrib->next_cmd_id;
> - g_queue_push_tail(attrib->queue, c);
> + g_queue_push_tail(queue, c);
> }
>
> - if (g_queue_get_length(attrib->queue) == 1)
> + /*
> + * If a command was added to the queue and it was empty before, wake up
> + * the sender. If the sender was already woken up by the second queue,
> + * wake_up_sender will just return.
> + */
> + if (g_queue_get_length(queue) == 1)
> wake_up_sender(attrib);
>
> return c->id;
> @@ -459,38 +491,49 @@ static gint command_cmp_by_id(gconstpointer a, gconstpointer b)
>
> gboolean g_attrib_cancel(GAttrib *attrib, guint id)
> {
> - GList *l;
> + GList *l = NULL;
> struct command *cmd;
> + GQueue *queue;
>
> - if (attrib == NULL || attrib->queue == NULL)
> + if (attrib == NULL)
> return FALSE;
>
> - l = g_queue_find_custom(attrib->queue, GUINT_TO_POINTER(id),
> - command_cmp_by_id);
> + queue = attrib->queue;
> + if (queue)
> + l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
> + command_cmp_by_id);
> + if (l == NULL) {
> + queue = attrib->response_queue;
> + if (!queue)
> + return FALSE;
> + l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
> + command_cmp_by_id);
> + }
> +
> if (l == NULL)
> return FALSE;
>
> cmd = l->data;
>
> - if (cmd == g_queue_peek_head(attrib->queue) && cmd->sent)
> + if (cmd == g_queue_peek_head(queue) && cmd->sent)
> cmd->func = NULL;
> else {
> - g_queue_remove(attrib->queue, cmd);
> + g_queue_remove(queue, cmd);
> command_destroy(cmd);
> }
>
> return TRUE;
> }
>
> -gboolean g_attrib_cancel_all(GAttrib *attrib)
> +static gboolean g_attrib_cancel_all_per_queue(GQueue *queue)
I don't think that the g_attrib_ prefix here adds much, if that function
is not exported.
> {
> struct command *c, *head = NULL;
> gboolean first = TRUE;
>
> - if (attrib == NULL || attrib->queue == NULL)
> + if (queue == NULL)
> return FALSE;
>
> - while ((c = g_queue_pop_head(attrib->queue))) {
> + while ((c = g_queue_pop_head(queue))) {
> if (first && c->sent) {
> /* If the command was sent ignore its callback ... */
> c->func = NULL;
> @@ -504,12 +547,21 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
>
> if (head) {
> /* ... and put it back in the queue */
> - g_queue_push_head(attrib->queue, head);
> + g_queue_push_head(queue, head);
> }
>
> return TRUE;
> }
>
> +gboolean g_attrib_cancel_all(GAttrib *attrib)
> +{
> + if (attrib == NULL)
> + return FALSE;
> +
> + return g_attrib_cancel_all_per_queue(attrib->queue) &&
> + g_attrib_cancel_all_per_queue(attrib->response_queue);
> +}
> +
> gboolean g_attrib_set_debug(GAttrib *attrib,
> GAttribDebugFunc func, gpointer user_data)
> {
Other than that, patch looks good.
Just for information, some time ago I did something similar, but I never
felt sure enough about it, but I guess that I was not that far off, in case
you want to take a look:
http://git.infradead.org/users/vcgomes/bluez.git/commitdiff/refs/heads/two-queues
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gattrib: Fix a request/response command deadlock
2012-05-28 19:45 ` Vinicius Costa Gomes
@ 2012-05-29 1:16 ` Ido Yariv
2012-05-29 1:20 ` [PATCH v2] " Ido Yariv
0 siblings, 1 reply; 8+ messages in thread
From: Ido Yariv @ 2012-05-29 1:16 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth
Hi Vinicius,
On Mon, May 28, 2012 at 04:45:06PM -0300, Vinicius Costa Gomes wrote:
> Hi Ido,
>
> On 21:34 Mon 28 May, Ido Yariv wrote:
> > New requests and responses are never sent if a request was sent and the
> > response for it hasn't been received yet. As a result, if both end
> > points send requests at the same time, a deadlock could occur. This
> > could happen, for instance, if the client sends a read request and the
> > server sends an indication before responding to the read request.
> >
> > Fix this by introducing an additional queue for responses. Responses may
> > be sent while there's still a pending request/indication.
> > ---
> > attrib/gattrib.c | 86 +++++++++++++++++++++++++++++++++++++++++++----------
> > 1 files changed, 69 insertions(+), 17 deletions(-)
> >
> > diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> > index 769be36..769d746 100644
> > --- a/attrib/gattrib.c
> > +++ b/attrib/gattrib.c
> > @@ -47,6 +47,7 @@ struct _GAttrib {
> > guint write_watch;
> > guint timeout_watch;
> > GQueue *queue;
> > + GQueue *response_queue;
>
> I would change the name of the other queue as well, to make it clear
> that the two queues have different objectives.
Sure, why not.
>
> > GSList *events;
> > guint next_cmd_id;
> > guint next_evt_id;
> > @@ -175,9 +176,13 @@ static void attrib_destroy(GAttrib *attrib)
> >
> > while ((c = g_queue_pop_head(attrib->queue)))
> > command_destroy(c);
>
> I would add a empty line here.
Done.
>
> > + while ((c = g_queue_pop_head(attrib->response_queue)))
> > + command_destroy(c);
> >
> > g_queue_free(attrib->queue);
> > attrib->queue = NULL;
>
> And here.
Done.
>
> > + g_queue_free(attrib->response_queue);
> > + attrib->response_queue = NULL;
...
> > @@ -421,6 +442,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
> > gpointer user_data, GDestroyNotify notify)
> > {
> > struct command *c;
> > + GQueue *queue;
> >
> > c = g_try_new0(struct command, 1);
> > if (c == NULL)
> > @@ -435,15 +457,25 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
> > c->user_data = user_data;
> > c->notify = notify;
> >
> > + if (is_response(opcode))
> > + queue = attrib->response_queue;
> > + else
> > + queue = attrib->queue;
> > +
> > if (id) {
> > c->id = id;
> > - g_queue_push_head(attrib->queue, c);
> > + g_queue_push_head(queue, c);
>
> I don't know if this is the right thing to do for responses. For
> requests there's is no problem if I rearrange them, but if I rearrange
> responses I guess that it would confuse the remote side.
Good catch. Though I don't see how this could happen in practice, I'll
verify that we don't re-order responses to be on the safe side.
>
> > } else {
> > c->id = ++attrib->next_cmd_id;
> > - g_queue_push_tail(attrib->queue, c);
> > + g_queue_push_tail(queue, c);
> > }
...
> > -gboolean g_attrib_cancel_all(GAttrib *attrib)
> > +static gboolean g_attrib_cancel_all_per_queue(GQueue *queue)
>
> I don't think that the g_attrib_ prefix here adds much, if that function
> is not exported.
Sure, I'll change that.
...
> Other than that, patch looks good.
Thanks for reviewing it!
>
> Just for information, some time ago I did something similar, but I never
> felt sure enough about it, but I guess that I was not that far off, in case
> you want to take a look:
>
> http://git.infradead.org/users/vcgomes/bluez.git/commitdiff/refs/heads/two-queues
Nice, it looks very similar :)
Thanks,
Ido.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] gattrib: Fix a request/response command deadlock
2012-05-29 1:16 ` Ido Yariv
@ 2012-05-29 1:20 ` Ido Yariv
2012-05-29 2:40 ` Vinicius Costa Gomes
0 siblings, 1 reply; 8+ messages in thread
From: Ido Yariv @ 2012-05-29 1:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Vinicius Costa Gomes, Ido Yariv
New requests and responses are never sent if a request was sent and the
response for it hasn't been received yet. As a result, if both end
points send requests at the same time, a deadlock could occur. This
could happen, for instance, if the client sends a read request and the
server sends an indication before responding to the read request.
Fix this by introducing an additional queue for responses. Responses may
be sent while there's still a pending request/indication.
---
attrib/gattrib.c | 107 ++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 83 insertions(+), 24 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 769be36..71c94d8 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -46,7 +46,8 @@ struct _GAttrib {
guint read_watch;
guint write_watch;
guint timeout_watch;
- GQueue *queue;
+ GQueue *requests;
+ GQueue *responses;
GSList *events;
guint next_cmd_id;
guint next_evt_id;
@@ -173,11 +174,17 @@ static void attrib_destroy(GAttrib *attrib)
GSList *l;
struct command *c;
- while ((c = g_queue_pop_head(attrib->queue)))
+ while ((c = g_queue_pop_head(attrib->requests)))
+ command_destroy(c);
+
+ while ((c = g_queue_pop_head(attrib->responses)))
command_destroy(c);
- g_queue_free(attrib->queue);
- attrib->queue = NULL;
+ g_queue_free(attrib->requests);
+ attrib->requests = NULL;
+
+ g_queue_free(attrib->responses);
+ attrib->responses = NULL;
for (l = attrib->events; l; l = l->next)
event_destroy(l->data);
@@ -259,21 +266,34 @@ static gboolean can_write_data(GIOChannel *io, GIOCondition cond,
GError *gerr = NULL;
gsize len;
GIOStatus iostat;
+ GQueue *queue;
if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
return FALSE;
- cmd = g_queue_peek_head(attrib->queue);
+ queue = attrib->responses;
+ cmd = g_queue_peek_head(queue);
+ if (cmd == NULL) {
+ queue = attrib->requests;
+ cmd = g_queue_peek_head(queue);
+ }
if (cmd == NULL)
return FALSE;
+ /*
+ * Verify that we didn't already send this command. This can only
+ * happen with elementes from attrib->requests.
+ */
+ if (cmd->sent)
+ return FALSE;
+
iostat = g_io_channel_write_chars(io, (gchar *) cmd->pdu, cmd->len,
&len, &gerr);
if (iostat != G_IO_STATUS_NORMAL)
return FALSE;
if (cmd->expected == 0) {
- g_queue_pop_head(attrib->queue);
+ g_queue_pop_head(queue);
command_destroy(cmd);
return TRUE;
@@ -315,7 +335,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
uint8_t buf[512], status;
gsize len;
GIOStatus iostat;
- gboolean qempty;
+ gboolean norequests, noresponses;
if (attrib->timeout_watch > 0) {
g_source_remove(attrib->timeout_watch);
@@ -349,7 +369,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
if (is_response(buf[0]) == FALSE)
return TRUE;
- cmd = g_queue_pop_head(attrib->queue);
+ cmd = g_queue_pop_head(attrib->requests);
if (cmd == NULL) {
/* Keep the watch if we have events to report */
return attrib->events != NULL;
@@ -368,7 +388,10 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
status = 0;
done:
- qempty = attrib->queue == NULL || g_queue_is_empty(attrib->queue);
+ norequests = attrib->requests == NULL ||
+ g_queue_is_empty(attrib->requests);
+ noresponses = attrib->responses == NULL ||
+ g_queue_is_empty(attrib->responses);
if (cmd) {
if (cmd->func)
@@ -377,7 +400,7 @@ done:
command_destroy(cmd);
}
- if (!qempty)
+ if (!norequests || !noresponses)
wake_up_sender(attrib);
return TRUE;
@@ -396,7 +419,8 @@ GAttrib *g_attrib_new(GIOChannel *io)
return NULL;
attrib->io = g_io_channel_ref(io);
- attrib->queue = g_queue_new();
+ attrib->requests = g_queue_new();
+ attrib->responses = g_queue_new();
attrib->read_watch = g_io_add_watch(attrib->io,
G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
@@ -421,6 +445,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
gpointer user_data, GDestroyNotify notify)
{
struct command *c;
+ GQueue *queue;
c = g_try_new0(struct command, 1);
if (c == NULL)
@@ -435,15 +460,29 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
c->user_data = user_data;
c->notify = notify;
+ if (is_response(opcode))
+ queue = attrib->responses;
+ else
+ queue = attrib->requests;
+
if (id) {
c->id = id;
- g_queue_push_head(attrib->queue, c);
+ if (!is_response(opcode))
+ g_queue_push_head(queue, c);
+ else
+ /* Don't re-order responses even if an ID is given */
+ g_queue_push_tail(queue, c);
} else {
c->id = ++attrib->next_cmd_id;
- g_queue_push_tail(attrib->queue, c);
+ g_queue_push_tail(queue, c);
}
- if (g_queue_get_length(attrib->queue) == 1)
+ /*
+ * If a command was added to the queue and it was empty before, wake up
+ * the sender. If the sender was already woken up by the second queue,
+ * wake_up_sender will just return.
+ */
+ if (g_queue_get_length(queue) == 1)
wake_up_sender(attrib);
return c->id;
@@ -459,38 +498,49 @@ static gint command_cmp_by_id(gconstpointer a, gconstpointer b)
gboolean g_attrib_cancel(GAttrib *attrib, guint id)
{
- GList *l;
+ GList *l = NULL;
struct command *cmd;
+ GQueue *queue;
- if (attrib == NULL || attrib->queue == NULL)
+ if (attrib == NULL)
return FALSE;
- l = g_queue_find_custom(attrib->queue, GUINT_TO_POINTER(id),
- command_cmp_by_id);
+ queue = attrib->requests;
+ if (queue)
+ l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
+ command_cmp_by_id);
+ if (l == NULL) {
+ queue = attrib->responses;
+ if (!queue)
+ return FALSE;
+ l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
+ command_cmp_by_id);
+ }
+
if (l == NULL)
return FALSE;
cmd = l->data;
- if (cmd == g_queue_peek_head(attrib->queue) && cmd->sent)
+ if (cmd == g_queue_peek_head(queue) && cmd->sent)
cmd->func = NULL;
else {
- g_queue_remove(attrib->queue, cmd);
+ g_queue_remove(queue, cmd);
command_destroy(cmd);
}
return TRUE;
}
-gboolean g_attrib_cancel_all(GAttrib *attrib)
+static gboolean cancel_all_per_queue(GQueue *queue)
{
struct command *c, *head = NULL;
gboolean first = TRUE;
- if (attrib == NULL || attrib->queue == NULL)
+ if (queue == NULL)
return FALSE;
- while ((c = g_queue_pop_head(attrib->queue))) {
+ while ((c = g_queue_pop_head(queue))) {
if (first && c->sent) {
/* If the command was sent ignore its callback ... */
c->func = NULL;
@@ -504,12 +554,21 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
if (head) {
/* ... and put it back in the queue */
- g_queue_push_head(attrib->queue, head);
+ g_queue_push_head(queue, head);
}
return TRUE;
}
+gboolean g_attrib_cancel_all(GAttrib *attrib)
+{
+ if (attrib == NULL)
+ return FALSE;
+
+ return cancel_all_per_queue(attrib->requests) &&
+ cancel_all_per_queue(attrib->responses);
+}
+
gboolean g_attrib_set_debug(GAttrib *attrib,
GAttribDebugFunc func, gpointer user_data)
{
--
1.7.7.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] gattrib: Fix a request/response command deadlock
2012-05-29 1:20 ` [PATCH v2] " Ido Yariv
@ 2012-05-29 2:40 ` Vinicius Costa Gomes
2012-05-29 8:39 ` [PATCH v3] " Ido Yariv
0 siblings, 1 reply; 8+ messages in thread
From: Vinicius Costa Gomes @ 2012-05-29 2:40 UTC (permalink / raw)
To: Ido Yariv; +Cc: linux-bluetooth
Hi Ido,
On 04:20 Tue 29 May, Ido Yariv wrote:
> New requests and responses are never sent if a request was sent and the
> response for it hasn't been received yet. As a result, if both end
> points send requests at the same time, a deadlock could occur. This
> could happen, for instance, if the client sends a read request and the
> server sends an indication before responding to the read request.
>
> Fix this by introducing an additional queue for responses. Responses may
> be sent while there's still a pending request/indication.
> ---
> attrib/gattrib.c | 107 ++++++++++++++++++++++++++++++++++++++++++------------
> 1 files changed, 83 insertions(+), 24 deletions(-)
>
> diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> index 769be36..71c94d8 100644
> --- a/attrib/gattrib.c
> +++ b/attrib/gattrib.c
[snip]
> @@ -504,12 +554,21 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
>
> if (head) {
> /* ... and put it back in the queue */
> - g_queue_push_head(attrib->queue, head);
> + g_queue_push_head(queue, head);
> }
>
> return TRUE;
> }
>
> +gboolean g_attrib_cancel_all(GAttrib *attrib)
> +{
> + if (attrib == NULL)
> + return FALSE;
> +
> + return cancel_all_per_queue(attrib->requests) &&
> + cancel_all_per_queue(attrib->responses);
Sorry, but I just noticed something, even if it's very unlikely that it would
happen in practice: if attrib->requests is NULL, it wouldn't cancel the
responses.
> +}
> +
> gboolean g_attrib_set_debug(GAttrib *attrib,
> GAttribDebugFunc func, gpointer user_data)
> {
> --
> 1.7.7.6
>
Cheers,
--
Vinicius
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] gattrib: Fix a request/response command deadlock
2012-05-29 2:40 ` Vinicius Costa Gomes
@ 2012-05-29 8:39 ` Ido Yariv
2012-05-29 13:59 ` Vinicius Costa Gomes
2012-05-30 7:42 ` Johan Hedberg
0 siblings, 2 replies; 8+ messages in thread
From: Ido Yariv @ 2012-05-29 8:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Vinicius Costa Gomes, Ido Yariv
New requests and responses are never sent if a request was sent and the
response for it hasn't been received yet. As a result, if both end
points send requests at the same time, a deadlock could occur. This
could happen, for instance, if the client sends a read request and the
server sends an indication before responding to the read request.
Fix this by introducing an additional queue for responses. Responses may
be sent while there's still a pending request/indication.
---
Changes from v2:
- Cancel responses in g_attrib_cancel_all even if the requests queue is NULL
attrib/gattrib.c | 111 ++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 87 insertions(+), 24 deletions(-)
diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 769be36..9886a92 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -46,7 +46,8 @@ struct _GAttrib {
guint read_watch;
guint write_watch;
guint timeout_watch;
- GQueue *queue;
+ GQueue *requests;
+ GQueue *responses;
GSList *events;
guint next_cmd_id;
guint next_evt_id;
@@ -173,11 +174,17 @@ static void attrib_destroy(GAttrib *attrib)
GSList *l;
struct command *c;
- while ((c = g_queue_pop_head(attrib->queue)))
+ while ((c = g_queue_pop_head(attrib->requests)))
+ command_destroy(c);
+
+ while ((c = g_queue_pop_head(attrib->responses)))
command_destroy(c);
- g_queue_free(attrib->queue);
- attrib->queue = NULL;
+ g_queue_free(attrib->requests);
+ attrib->requests = NULL;
+
+ g_queue_free(attrib->responses);
+ attrib->responses = NULL;
for (l = attrib->events; l; l = l->next)
event_destroy(l->data);
@@ -259,21 +266,34 @@ static gboolean can_write_data(GIOChannel *io, GIOCondition cond,
GError *gerr = NULL;
gsize len;
GIOStatus iostat;
+ GQueue *queue;
if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
return FALSE;
- cmd = g_queue_peek_head(attrib->queue);
+ queue = attrib->responses;
+ cmd = g_queue_peek_head(queue);
+ if (cmd == NULL) {
+ queue = attrib->requests;
+ cmd = g_queue_peek_head(queue);
+ }
if (cmd == NULL)
return FALSE;
+ /*
+ * Verify that we didn't already send this command. This can only
+ * happen with elementes from attrib->requests.
+ */
+ if (cmd->sent)
+ return FALSE;
+
iostat = g_io_channel_write_chars(io, (gchar *) cmd->pdu, cmd->len,
&len, &gerr);
if (iostat != G_IO_STATUS_NORMAL)
return FALSE;
if (cmd->expected == 0) {
- g_queue_pop_head(attrib->queue);
+ g_queue_pop_head(queue);
command_destroy(cmd);
return TRUE;
@@ -315,7 +335,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
uint8_t buf[512], status;
gsize len;
GIOStatus iostat;
- gboolean qempty;
+ gboolean norequests, noresponses;
if (attrib->timeout_watch > 0) {
g_source_remove(attrib->timeout_watch);
@@ -349,7 +369,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
if (is_response(buf[0]) == FALSE)
return TRUE;
- cmd = g_queue_pop_head(attrib->queue);
+ cmd = g_queue_pop_head(attrib->requests);
if (cmd == NULL) {
/* Keep the watch if we have events to report */
return attrib->events != NULL;
@@ -368,7 +388,10 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
status = 0;
done:
- qempty = attrib->queue == NULL || g_queue_is_empty(attrib->queue);
+ norequests = attrib->requests == NULL ||
+ g_queue_is_empty(attrib->requests);
+ noresponses = attrib->responses == NULL ||
+ g_queue_is_empty(attrib->responses);
if (cmd) {
if (cmd->func)
@@ -377,7 +400,7 @@ done:
command_destroy(cmd);
}
- if (!qempty)
+ if (!norequests || !noresponses)
wake_up_sender(attrib);
return TRUE;
@@ -396,7 +419,8 @@ GAttrib *g_attrib_new(GIOChannel *io)
return NULL;
attrib->io = g_io_channel_ref(io);
- attrib->queue = g_queue_new();
+ attrib->requests = g_queue_new();
+ attrib->responses = g_queue_new();
attrib->read_watch = g_io_add_watch(attrib->io,
G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
@@ -421,6 +445,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
gpointer user_data, GDestroyNotify notify)
{
struct command *c;
+ GQueue *queue;
c = g_try_new0(struct command, 1);
if (c == NULL)
@@ -435,15 +460,29 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
c->user_data = user_data;
c->notify = notify;
+ if (is_response(opcode))
+ queue = attrib->responses;
+ else
+ queue = attrib->requests;
+
if (id) {
c->id = id;
- g_queue_push_head(attrib->queue, c);
+ if (!is_response(opcode))
+ g_queue_push_head(queue, c);
+ else
+ /* Don't re-order responses even if an ID is given */
+ g_queue_push_tail(queue, c);
} else {
c->id = ++attrib->next_cmd_id;
- g_queue_push_tail(attrib->queue, c);
+ g_queue_push_tail(queue, c);
}
- if (g_queue_get_length(attrib->queue) == 1)
+ /*
+ * If a command was added to the queue and it was empty before, wake up
+ * the sender. If the sender was already woken up by the second queue,
+ * wake_up_sender will just return.
+ */
+ if (g_queue_get_length(queue) == 1)
wake_up_sender(attrib);
return c->id;
@@ -459,38 +498,49 @@ static gint command_cmp_by_id(gconstpointer a, gconstpointer b)
gboolean g_attrib_cancel(GAttrib *attrib, guint id)
{
- GList *l;
+ GList *l = NULL;
struct command *cmd;
+ GQueue *queue;
- if (attrib == NULL || attrib->queue == NULL)
+ if (attrib == NULL)
return FALSE;
- l = g_queue_find_custom(attrib->queue, GUINT_TO_POINTER(id),
- command_cmp_by_id);
+ queue = attrib->requests;
+ if (queue)
+ l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
+ command_cmp_by_id);
+ if (l == NULL) {
+ queue = attrib->responses;
+ if (!queue)
+ return FALSE;
+ l = g_queue_find_custom(queue, GUINT_TO_POINTER(id),
+ command_cmp_by_id);
+ }
+
if (l == NULL)
return FALSE;
cmd = l->data;
- if (cmd == g_queue_peek_head(attrib->queue) && cmd->sent)
+ if (cmd == g_queue_peek_head(queue) && cmd->sent)
cmd->func = NULL;
else {
- g_queue_remove(attrib->queue, cmd);
+ g_queue_remove(queue, cmd);
command_destroy(cmd);
}
return TRUE;
}
-gboolean g_attrib_cancel_all(GAttrib *attrib)
+static gboolean cancel_all_per_queue(GQueue *queue)
{
struct command *c, *head = NULL;
gboolean first = TRUE;
- if (attrib == NULL || attrib->queue == NULL)
+ if (queue == NULL)
return FALSE;
- while ((c = g_queue_pop_head(attrib->queue))) {
+ while ((c = g_queue_pop_head(queue))) {
if (first && c->sent) {
/* If the command was sent ignore its callback ... */
c->func = NULL;
@@ -504,12 +554,25 @@ gboolean g_attrib_cancel_all(GAttrib *attrib)
if (head) {
/* ... and put it back in the queue */
- g_queue_push_head(attrib->queue, head);
+ g_queue_push_head(queue, head);
}
return TRUE;
}
+gboolean g_attrib_cancel_all(GAttrib *attrib)
+{
+ gboolean ret;
+
+ if (attrib == NULL)
+ return FALSE;
+
+ ret = cancel_all_per_queue(attrib->requests);
+ ret = cancel_all_per_queue(attrib->responses) && ret;
+
+ return ret;
+}
+
gboolean g_attrib_set_debug(GAttrib *attrib,
GAttribDebugFunc func, gpointer user_data)
{
--
1.7.7.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] gattrib: Fix a request/response command deadlock
2012-05-29 8:39 ` [PATCH v3] " Ido Yariv
@ 2012-05-29 13:59 ` Vinicius Costa Gomes
2012-05-30 7:42 ` Johan Hedberg
1 sibling, 0 replies; 8+ messages in thread
From: Vinicius Costa Gomes @ 2012-05-29 13:59 UTC (permalink / raw)
To: Ido Yariv; +Cc: linux-bluetooth
Hi Ido,
On 11:39 Tue 29 May, Ido Yariv wrote:
> New requests and responses are never sent if a request was sent and the
> response for it hasn't been received yet. As a result, if both end
> points send requests at the same time, a deadlock could occur. This
> could happen, for instance, if the client sends a read request and the
> server sends an indication before responding to the read request.
>
> Fix this by introducing an additional queue for responses. Responses may
> be sent while there's still a pending request/indication.
> ---
> Changes from v2:
> - Cancel responses in g_attrib_cancel_all even if the requests queue is NULL
>
[snip]
Thanks. Ack.
Cheers,
--
Vinicius
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] gattrib: Fix a request/response command deadlock
2012-05-29 8:39 ` [PATCH v3] " Ido Yariv
2012-05-29 13:59 ` Vinicius Costa Gomes
@ 2012-05-30 7:42 ` Johan Hedberg
1 sibling, 0 replies; 8+ messages in thread
From: Johan Hedberg @ 2012-05-30 7:42 UTC (permalink / raw)
To: Ido Yariv; +Cc: linux-bluetooth, Vinicius Costa Gomes
Hi Ido,
On Tue, May 29, 2012, Ido Yariv wrote:
> New requests and responses are never sent if a request was sent and the
> response for it hasn't been received yet. As a result, if both end
> points send requests at the same time, a deadlock could occur. This
> could happen, for instance, if the client sends a read request and the
> server sends an indication before responding to the read request.
>
> Fix this by introducing an additional queue for responses. Responses may
> be sent while there's still a pending request/indication.
> ---
> Changes from v2:
> - Cancel responses in g_attrib_cancel_all even if the requests queue is NULL
>
> attrib/gattrib.c | 111 ++++++++++++++++++++++++++++++++++++++++++------------
> 1 files changed, 87 insertions(+), 24 deletions(-)
Applied. Thanks.
Johan
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-05-30 7:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-28 18:34 [PATCH] gattrib: Fix a request/response command deadlock Ido Yariv
2012-05-28 19:45 ` Vinicius Costa Gomes
2012-05-29 1:16 ` Ido Yariv
2012-05-29 1:20 ` [PATCH v2] " Ido Yariv
2012-05-29 2:40 ` Vinicius Costa Gomes
2012-05-29 8:39 ` [PATCH v3] " Ido Yariv
2012-05-29 13:59 ` Vinicius Costa Gomes
2012-05-30 7:42 ` Johan Hedberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).