RE: [PATCH] gattrib: Fix command timeout handling

["Ganir, Chen" <chen.ganir@xxxxxx>]

Johan,
 
> -----Original Message-----
> From: linux-bluetooth-owner@xxxxxxxxxxxxxxx [mailto:linux-bluetooth-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Johan Hedberg
> Sent: Tuesday, June 05, 2012 3:17 PM
> To: linux-bluetooth@xxxxxxxxxxxxxxx
> Subject: [PATCH] gattrib: Fix command timeout handling
> 
> From: Johan Hedberg <johan.hedberg@xxxxxxxxx>
> 
> This patch fixes command timeout handling. Previously attrib_destroy
> was
> explicitly called which ignored any reference holders. This patch fixes
> the issue by first passing errors to command callbacks and after that
> marking the GAttrib object as stale so no further operations can be
> done.
> ---
> This is an untested patch which should hopefully fix bluetoothd crashes
> when we fail to receive a response to a command. I'm sending it to
> linux-bluetooth in case someone can spot some obvious problem with it.
> If there are no objections I'll complete testing of it during the next
> 24 hours and then apply it upstream.
> 
>  attrib/att.c     |    4 ++++
>  attrib/att.h     |    2 ++
>  attrib/gattrib.c |   29 ++++++++++++++++++++++++++++-
>  3 files changed, 34 insertions(+), 1 deletions(-)
> 
> diff --git a/attrib/att.c b/attrib/att.c
> index a3a8947..c8e2e1d 100644
> --- a/attrib/att.c
> +++ b/attrib/att.c
> @@ -72,6 +72,10 @@ const char *att_ecode2str(uint8_t status)
>  		return "Insufficient Resources to complete the request";
>  	case ATT_ECODE_IO:
>  		return "Internal application error: I/O";
> +	case ATT_ECODE_TIMEOUT:
> +		return "A timeout occured";
> +	case ATT_ECODE_ABORTED:
> +		return "The operation was aborted";
>  	default:
>  		return "Unexpected error code";
>  	}
> diff --git a/attrib/att.h b/attrib/att.h
> index d12a7f2..8979c95 100644
> --- a/attrib/att.h
> +++ b/attrib/att.h
> @@ -72,6 +72,8 @@
>  #define ATT_ECODE_INSUFF_RESOURCES		0x11
>  /* Application error */
>  #define ATT_ECODE_IO				0xFF
> +#define ATT_ECODE_TIMEOUT			0xFE
> +#define ATT_ECODE_ABORTED			0xFD
> 
>  /* Characteristic Property bit field */
>  #define ATT_CHAR_PROPER_BROADCAST		0x01
> diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> index 9886a92..29c3585 100644
> --- a/attrib/gattrib.c
> +++ b/attrib/gattrib.c
> @@ -53,6 +53,7 @@ struct _GAttrib {
>  	guint next_evt_id;
>  	GDestroyNotify destroy;
>  	gpointer destroy_user_data;
> +	gboolean stale;
>  };
> 
>  struct command {
> @@ -252,8 +253,25 @@ gboolean g_attrib_set_destroy_function(GAttrib
> *attrib,
>  static gboolean disconnect_timeout(gpointer data)
>  {
>  	struct _GAttrib *attrib = data;
> +	struct command *c;
> 
> -	attrib_destroy(attrib);
> +	c = g_queue_pop_head(attrib->requests);
> +	if (c == NULL)
> +		goto done;
> +
> +	if (c->func)
> +		c->func(ATT_ECODE_TIMEOUT, NULL, 0, c->user_data);
> +
> +	command_destroy(c);
> +
> +	while ((c = g_queue_pop_head(attrib->requests))) {
> +		if (c->func)
> +			c->func(ATT_ECODE_ABORTED, NULL, 0, c->user_data);
> +		command_destroy(c);
> +	}
> +
> +done:
> +	attrib->stale = TRUE;
> 
>  	return FALSE;
>  }
> @@ -268,6 +286,9 @@ static gboolean can_write_data(GIOChannel *io,
> GIOCondition cond,
>  	GIOStatus iostat;
>  	GQueue *queue;
> 
> +	if (attrib->stale)
> +		return FALSE;
> +
>  	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
>  		return FALSE;
> 
> @@ -337,6 +358,9 @@ static gboolean received_data(GIOChannel *io,
> GIOCondition cond, gpointer data)
>  	GIOStatus iostat;
>  	gboolean norequests, noresponses;
> 
> +	if (attrib->stale)
> +		return FALSE;
> +
>  	if (attrib->timeout_watch > 0) {
>  		g_source_remove(attrib->timeout_watch);
>  		attrib->timeout_watch = 0;
> @@ -447,6 +471,9 @@ guint g_attrib_send(GAttrib *attrib, guint id,
> guint8 opcode,
>  	struct command *c;
>  	GQueue *queue;
> 
> +	if (attrib->stale)
> +		return 0;
> +
>  	c = g_try_new0(struct command, 1);
>  	if (c == NULL)
>  		return 0;
> --
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> bluetooth" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Looks ok to me.

Chen Ganir.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html