[PATCH 1/3] Fix handling of suspend response

Linux bluetooth development
 help / color / mirror / Atom feed

* [PATCH 1/3] Fix handling of suspend response
@ 2011-04-04  9:00 Luiz Augusto von Dentz
  2011-04-04  9:00 ` [PATCH 2/3] Refactore a2dp finalize_*_errno functions Luiz Augusto von Dentz
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2011-04-04  9:00 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

finalize_suspend is being called twice when a resume request is queue
after it or if start fails.
---
 audio/a2dp.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 9d9c61d..1208ad1 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1011,6 +1011,7 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	struct a2dp_sep *a2dp_sep = user_data;
 	struct a2dp_setup *setup;
 	gboolean start;
+	int perr;
 
 	if (a2dp_sep->type == AVDTP_SEP_TYPE_SINK)
 		DBG("Sink %p: Suspend_Cfm", sep);
@@ -1029,23 +1030,22 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (err) {
 		setup->stream = NULL;
 		setup->err = err;
-		finalize_suspend(setup);
 	}
-	else
-		finalize_suspend_errno(setup, 0);
+
+	finalize_suspend(setup);
 
 	if (!start)
 		return;
 
 	if (err) {
-		setup->err = err;
-		finalize_suspend(setup);
-	} else if (avdtp_start(session, a2dp_sep->stream) < 0) {
-		struct avdtp_error start_err;
-		error("avdtp_start failed");
-		avdtp_error_init(&start_err, AVDTP_ERRNO, EIO);
-		setup->err = err;
-		finalize_suspend(setup);
+		finalize_resume(setup);
+		return;
+	}
+
+	perr = avdtp_start(session, a2dp_sep->stream);
+	if (perr < 0) {
+		error("Error on avdtp_start %s (%d)", strerror(-perr), -perr);
+		finalize_resume_errno(setup, perr);
 	}
 }
 
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/3] Refactore a2dp finalize_*_errno functions
  2011-04-04  9:00 [PATCH 1/3] Fix handling of suspend response Luiz Augusto von Dentz
@ 2011-04-04  9:00 ` Luiz Augusto von Dentz
  2011-04-04  9:00 ` [PATCH 3/3] Fix not removing source when removing setup callback Luiz Augusto von Dentz
  2011-04-05  2:53 ` [PATCH 1/3] Fix handling of suspend response Johan Hedberg
  2 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2011-04-04  9:00 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

There were a lot of duplicate code in them so they are now replaced by
finalize_setup_errno which can deal with multiple callbacks reusing the
same error.
---
 audio/a2dp.c |   88 +++++++++++++++++++++++++++-------------------------------
 1 files changed, 41 insertions(+), 47 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 1208ad1..d51276d 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -202,8 +202,32 @@ static void setup_cb_free(struct a2dp_setup_cb *cb)
 	g_free(cb);
 }
 
-static gboolean finalize_config(struct a2dp_setup *s)
+static void finalize_setup_errno(struct a2dp_setup *s, int err,
+					GSourceFunc cb1, ...)
 {
+	GSourceFunc finalize;
+	va_list args;
+	struct avdtp_error avdtp_err;
+
+	if (err < 0) {
+		avdtp_error_init(&avdtp_err, AVDTP_ERRNO, -err);
+		s->err = &avdtp_err;
+	}
+
+	va_start(args, cb1);
+	finalize = cb1;
+	setup_ref(s);
+	while (finalize != NULL) {
+		finalize(s);
+		finalize = va_arg(args, GSourceFunc);
+	}
+	setup_unref(s);
+	va_end(args);
+}
+
+static gboolean finalize_config(gpointer data)
+{
+	struct a2dp_setup *s = data;
 	GSList *l;
 	struct avdtp_stream *stream = s->err ? NULL : s->stream;
 
@@ -223,18 +247,9 @@ static gboolean finalize_config(struct a2dp_setup *s)
 	return FALSE;
 }
 
-static gboolean finalize_config_errno(struct a2dp_setup *s, int err)
-{
-	struct avdtp_error avdtp_err;
-
-	avdtp_error_init(&avdtp_err, AVDTP_ERRNO, -err);
-	s->err = err ? &avdtp_err : NULL;
-
-	return finalize_config(s);
-}
-
-static gboolean finalize_resume(struct a2dp_setup *s)
+static gboolean finalize_resume(gpointer data)
 {
+	struct a2dp_setup *s = data;
 	GSList *l;
 
 	for (l = s->cb; l != NULL; ) {
@@ -252,18 +267,9 @@ static gboolean finalize_resume(struct a2dp_setup *s)
 	return FALSE;
 }
 
-static gboolean finalize_resume_errno(struct a2dp_setup *s, int err)
-{
-	struct avdtp_error avdtp_err;
-
-	avdtp_error_init(&avdtp_err, AVDTP_ERRNO, -err);
-	s->err = err ? &avdtp_err : NULL;
-
-	return finalize_resume(s);
-}
-
-static gboolean finalize_suspend(struct a2dp_setup *s)
+static gboolean finalize_suspend(gpointer data)
 {
+	struct a2dp_setup *s = data;
 	GSList *l;
 
 	for (l = s->cb; l != NULL; ) {
@@ -281,17 +287,7 @@ static gboolean finalize_suspend(struct a2dp_setup *s)
 	return FALSE;
 }
 
-static gboolean finalize_suspend_errno(struct a2dp_setup *s, int err)
-{
-	struct avdtp_error avdtp_err;
-
-	avdtp_error_init(&avdtp_err, AVDTP_ERRNO, -err);
-	s->err = err ? &avdtp_err : NULL;
-
-	return finalize_suspend(s);
-}
-
-static gboolean finalize_select(struct a2dp_setup *s)
+static void finalize_select(struct a2dp_setup *s)
 {
 	GSList *l;
 
@@ -306,8 +302,6 @@ static gboolean finalize_select(struct a2dp_setup *s)
 		cb->select_cb(s->session, s->sep, s->caps, cb->user_data);
 		setup_cb_free(cb);
 	}
-
-	return FALSE;
 }
 
 static struct a2dp_setup *find_setup_by_session(struct avdtp *session)
@@ -777,7 +771,7 @@ static void endpoint_open_cb(struct media_endpoint *endpoint, void *ret,
 
 	if (ret == NULL) {
 		setup->stream = NULL;
-		finalize_config_errno(setup, -EPERM);
+		finalize_setup_errno(setup, -EPERM, finalize_config);
 		return;
 	}
 
@@ -787,7 +781,7 @@ static void endpoint_open_cb(struct media_endpoint *endpoint, void *ret,
 
 	error("Error on avdtp_open %s (%d)", strerror(-err), -err);
 	setup->stream = NULL;
-	finalize_config_errno(setup, err);
+	finalize_setup_errno(setup, err, finalize_config);
 }
 
 static void setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
@@ -844,7 +838,7 @@ static void setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 			return;
 
 		setup->stream = NULL;
-		finalize_config_errno(setup, -EPERM);
+		finalize_setup_errno(setup, -EPERM, finalize_config);
 		return;
 	}
 
@@ -852,7 +846,7 @@ static void setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (ret < 0) {
 		error("Error on avdtp_open %s (%d)", strerror(-ret), -ret);
 		setup->stream = NULL;
-		finalize_config_errno(setup, ret);
+		finalize_setup_errno(setup, ret, finalize_config);
 	}
 }
 
@@ -1045,7 +1039,7 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	perr = avdtp_start(session, a2dp_sep->stream);
 	if (perr < 0) {
 		error("Error on avdtp_start %s (%d)", strerror(-perr), -perr);
-		finalize_resume_errno(setup, perr);
+		finalize_setup_errno(setup, -EIO, finalize_suspend);
 	}
 }
 
@@ -1065,8 +1059,8 @@ static gboolean close_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (!setup)
 		return TRUE;
 
-	finalize_suspend_errno(setup, -ECONNRESET);
-	finalize_resume_errno(setup, -ECONNRESET);
+	finalize_setup_errno(setup, -ECONNRESET, finalize_suspend,
+							finalize_resume);
 
 	return TRUE;
 }
@@ -1099,7 +1093,7 @@ static gboolean a2dp_reconfigure(gpointer data)
 	return FALSE;
 
 failed:
-	finalize_config_errno(setup, posix_err);
+	finalize_setup_errno(setup, posix_err, finalize_config);
 	return FALSE;
 }
 
@@ -2126,7 +2120,7 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
 	case AVDTP_STATE_STREAMING:
 		if (avdtp_stream_has_capabilities(setup->stream, caps)) {
 			DBG("Configuration match: resuming");
-			g_idle_add((GSourceFunc) finalize_config, setup);
+			g_idle_add(finalize_config, setup);
 		} else if (!setup->reconfigure) {
 			setup->reconfigure = TRUE;
 			if (avdtp_close(session, sep->stream, FALSE) < 0) {
@@ -2184,7 +2178,7 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
 		if (sep->suspending)
 			setup->start = TRUE;
 		else
-			g_idle_add((GSourceFunc) finalize_resume, setup);
+			g_idle_add(finalize_resume, setup);
 		break;
 	default:
 		error("SEP in bad state for resume");
@@ -2221,7 +2215,7 @@ unsigned int a2dp_suspend(struct avdtp *session, struct a2dp_sep *sep,
 		goto failed;
 		break;
 	case AVDTP_STATE_OPEN:
-		g_idle_add((GSourceFunc) finalize_suspend, setup);
+		g_idle_add(finalize_suspend, setup);
 		break;
 	case AVDTP_STATE_STREAMING:
 		if (avdtp_suspend(session, sep->stream) < 0) {
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 3/3] Fix not removing source when removing setup callback
  2011-04-04  9:00 [PATCH 1/3] Fix handling of suspend response Luiz Augusto von Dentz
  2011-04-04  9:00 ` [PATCH 2/3] Refactore a2dp finalize_*_errno functions Luiz Augusto von Dentz
@ 2011-04-04  9:00 ` Luiz Augusto von Dentz
  2011-04-05  2:53 ` [PATCH 1/3] Fix handling of suspend response Johan Hedberg
  2 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2011-04-04  9:00 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

In rare situation this may lead to access invalid memory since setup can
be freed before idle callback is called.
---
 audio/a2dp.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index d51276d..1f3bc99 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -82,6 +82,7 @@ struct a2dp_setup_cb {
 	a2dp_config_cb_t config_cb;
 	a2dp_stream_cb_t resume_cb;
 	a2dp_stream_cb_t suspend_cb;
+	guint source_id;
 	void *user_data;
 	unsigned int id;
 };
@@ -197,6 +198,9 @@ static void setup_cb_free(struct a2dp_setup_cb *cb)
 {
 	struct a2dp_setup *setup = cb->setup;
 
+	if (cb->source_id)
+		g_source_remove(cb->source_id);
+
 	setup->cb = g_slist_remove(setup->cb, cb);
 	setup_unref(cb->setup);
 	g_free(cb);
@@ -2120,7 +2124,8 @@ unsigned int a2dp_config(struct avdtp *session, struct a2dp_sep *sep,
 	case AVDTP_STATE_STREAMING:
 		if (avdtp_stream_has_capabilities(setup->stream, caps)) {
 			DBG("Configuration match: resuming");
-			g_idle_add(finalize_config, setup);
+			cb_data->source_id = g_idle_add(finalize_config,
+								setup);
 		} else if (!setup->reconfigure) {
 			setup->reconfigure = TRUE;
 			if (avdtp_close(session, sep->stream, FALSE) < 0) {
@@ -2178,7 +2183,8 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
 		if (sep->suspending)
 			setup->start = TRUE;
 		else
-			g_idle_add(finalize_resume, setup);
+			cb_data->source_id = g_idle_add(finalize_resume,
+								setup);
 		break;
 	default:
 		error("SEP in bad state for resume");
@@ -2215,7 +2221,7 @@ unsigned int a2dp_suspend(struct avdtp *session, struct a2dp_sep *sep,
 		goto failed;
 		break;
 	case AVDTP_STATE_OPEN:
-		g_idle_add(finalize_suspend, setup);
+		cb_data->source_id = g_idle_add(finalize_suspend, setup);
 		break;
 	case AVDTP_STATE_STREAMING:
 		if (avdtp_suspend(session, sep->stream) < 0) {
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/3] Fix handling of suspend response
  2011-04-04  9:00 [PATCH 1/3] Fix handling of suspend response Luiz Augusto von Dentz
  2011-04-04  9:00 ` [PATCH 2/3] Refactore a2dp finalize_*_errno functions Luiz Augusto von Dentz
  2011-04-04  9:00 ` [PATCH 3/3] Fix not removing source when removing setup callback Luiz Augusto von Dentz
@ 2011-04-05  2:53 ` Johan Hedberg
  2 siblings, 0 replies; 4+ messages in thread
From: Johan Hedberg @ 2011-04-05  2:53 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

On Mon, Apr 04, 2011, Luiz Augusto von Dentz wrote:
> finalize_suspend is being called twice when a resume request is queue
> after it or if start fails.
> ---
>  audio/a2dp.c |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)

All three patches have been pushed upstream. Thanks.

Johan

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-04-05  2:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-04  9:00 [PATCH 1/3] Fix handling of suspend response Luiz Augusto von Dentz
2011-04-04  9:00 ` [PATCH 2/3] Refactore a2dp finalize_*_errno functions Luiz Augusto von Dentz
2011-04-04  9:00 ` [PATCH 3/3] Fix not removing source when removing setup callback Luiz Augusto von Dentz
2011-04-05  2:53 ` [PATCH 1/3] Fix handling of suspend response Johan Hedberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox