Re: [PATCH] gattrib: Fix a request/response command deadlock

[Vinicius Costa Gomes <vinicius.gomes@openbossa.org>]

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