gattrib: Fix a request/response command deadlock

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.

1 files changed, 87 insertions, 24 deletions

diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 769be36e..9886a920 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)
 {