[PATCH 1/3] Fix A2DP Sink/Source recognition

[PATCH 1/3] Fix A2DP Sink/Source recognition

[chanyeol.park]

From: Chan-yeol Park <chanyeol.park@samsung.com>

---
 audio/a2dp.c     |    8 ++++----
 audio/audio.conf |    2 +-
 audio/manager.c  |    4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index cc8f139..145dcf6 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1406,9 +1406,9 @@ int a2dp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
 		g_clear_error(&err);
 	} else {
 		if (strstr(str, "Sink"))
-			source = TRUE;
-		if (strstr(str, "Source"))
 			sink = TRUE;
+		if (strstr(str, "Source"))
+			source = TRUE;
 		if (strstr(str, "Socket"))
 			socket = TRUE;
 		g_free(str);
@@ -1421,9 +1421,9 @@ int a2dp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
 		g_clear_error(&err);
 	} else {
 		if (strstr(str, "Sink"))
-			source = FALSE;
-		if (strstr(str, "Source"))
 			sink = FALSE;
+		if (strstr(str, "Source"))
+			source = FALSE;
 		if (strstr(str, "Socket"))
 			socket = FALSE;
 		g_free(str);
diff --git a/audio/audio.conf b/audio/audio.conf
index fd6092a..1abdab9 100644
--- a/audio/audio.conf
+++ b/audio/audio.conf
@@ -9,7 +9,7 @@
 
 # If we want to disable support for specific services
 # Defaults to supporting all implemented services
-#Disable=Gateway,Source,Socket
+#Disable=Gateway,Sink,Socket
 
 # SCO routing. Either PCM or HCI (in which case audio is routed to/from ALSA)
 # Defaults to HCI
diff --git a/audio/manager.c b/audio/manager.c
index 9c7d0d3..9e5c71c 100644
--- a/audio/manager.c
+++ b/audio/manager.c
@@ -114,8 +114,8 @@ static struct enabled_interfaces enabled = {
 	.hfp		= TRUE,
 	.headset	= TRUE,
 	.gateway	= FALSE,
-	.sink		= TRUE,
-	.source		= FALSE,
+	.sink		= FALSE,
+	.source		= TRUE,
 	.control	= TRUE,
 	.socket		= FALSE,
 	.media		= TRUE,
-- 
1.7.9.5

[PATCH 2/3] Fix avrcp unregister potential crash

[chanyeol.park]

From: Chan-yeol Park <chanyeol.park@samsung.com>

---
 audio/avrcp.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index df39d04..2e946c1 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1272,8 +1272,11 @@ void avrcp_unregister(const bdaddr_t *src)
 
 	servers = g_slist_remove(servers, server);
 
-	remove_record_from_server(server->ct_record_id);
-	remove_record_from_server(server->tg_record_id);
+	if (server->ct_record_id)
+		remove_record_from_server(server->ct_record_id);
+
+	if (server->tg_record_id)
+		remove_record_from_server(server->tg_record_id);
 
 	avctp_unregister(&server->src);
 	g_free(server);
-- 
1.7.9.5

Re: [PATCH 2/3] Fix avrcp unregister potential crash

[Johan Hedberg]

Hi Chanyeol,

On Fri, May 18, 2012, chanyeol.park@samsung.com wrote:
> From: Chan-yeol Park <chanyeol.park@samsung.com>
> 
> ---
>  audio/avrcp.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index df39d04..2e946c1 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -1272,8 +1272,11 @@ void avrcp_unregister(const bdaddr_t *src)
>  
>  	servers = g_slist_remove(servers, server);
>  
> -	remove_record_from_server(server->ct_record_id);
> -	remove_record_from_server(server->tg_record_id);
> +	if (server->ct_record_id)
> +		remove_record_from_server(server->ct_record_id);
> +
> +	if (server->tg_record_id)
> +		remove_record_from_server(server->tg_record_id);
>  
>  	avctp_unregister(&server->src);
>  	g_free(server);

I don't think the commit message for this patch is truthful. If you look
at the code the remove_record_from_server will return ENOENT if you pass
it a non-existent handle. I.e. it will not crash. Please fix the commit
message to reflect what exactly is being fixed (i.e. an unnecessary call
to remove_record_from_server if there is no record handle).

Johan

Re: [PATCH 2/3] Fix avrcp unregister potential crash

[Lucas De Marchi]

On Fri, May 18, 2012 at 8:40 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Chanyeol,
>
> On Fri, May 18, 2012, chanyeol.park@samsung.com wrote:
>> From: Chan-yeol Park <chanyeol.park@samsung.com>
>>
>> ---
>>  audio/avrcp.c |    7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index df39d04..2e946c1 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -1272,8 +1272,11 @@ void avrcp_unregister(const bdaddr_t *src)
>>
>>       servers = g_slist_remove(servers, server);
>>
>> -     remove_record_from_server(server->ct_record_id);
>> -     remove_record_from_server(server->tg_record_id);
>> +     if (server->ct_record_id)
>> +             remove_record_from_server(server->ct_record_id);
>> +
>> +     if (server->tg_record_id)
>> +             remove_record_from_server(server->tg_record_id);
>>
>>       avctp_unregister(&server->src);
>>       g_free(server);
>
> I don't think the commit message for this patch is truthful. If you look
> at the code the remove_record_from_server will return ENOENT if you pass
> it a non-existent handle. I.e. it will not crash. Please fix the commit
> message to reflect what exactly is being fixed (i.e. an unnecessary call
> to remove_record_from_server if there is no record handle).

Then I don't see a reason: just let remove_record_from_server() fail ?

Chan, what are you fixing here?

Lucas De Marchi

Re: [PATCH 2/3] Fix avrcp unregister potential crash

[Johan Hedberg]

Hi Lucas,

On Fri, May 18, 2012, Lucas De Marchi wrote:
> >> --- a/audio/avrcp.c
> >> +++ b/audio/avrcp.c
> >> @@ -1272,8 +1272,11 @@ void avrcp_unregister(const bdaddr_t *src)
> >>
> >>       servers = g_slist_remove(servers, server);
> >>
> >> -     remove_record_from_server(server->ct_record_id);
> >> -     remove_record_from_server(server->tg_record_id);
> >> +     if (server->ct_record_id)
> >> +             remove_record_from_server(server->ct_record_id);
> >> +
> >> +     if (server->tg_record_id)
> >> +             remove_record_from_server(server->tg_record_id);
> >>
> >>       avctp_unregister(&server->src);
> >>       g_free(server);
> >
> > I don't think the commit message for this patch is truthful. If you look
> > at the code the remove_record_from_server will return ENOENT if you pass
> > it a non-existent handle. I.e. it will not crash. Please fix the commit
> > message to reflect what exactly is being fixed (i.e. an unnecessary call
> > to remove_record_from_server if there is no record handle).
> 
> Then I don't see a reason: just let remove_record_from_server() fail ?
> 
> Chan, what are you fixing here?

As discussed on IRC (and Chanyeol should really have explained this in
the commit message) handle 0 is actually the SDP server's own "master"
record. So it is plausible that weird behavior might occur if trying to
remove it. Since this is a SDP server internal detail and it doesn't
really make sense to remove it I'm thinking that
remove_record_from_server should return EINVAL if passed a 0 value.

Johan

Re: [PATCH 2/3] Fix avrcp unregister potential crash

[Johan Hedberg]

Hi,

On Fri, May 18, 2012, Johan Hedberg wrote:
> On Fri, May 18, 2012, Lucas De Marchi wrote:
> > >> --- a/audio/avrcp.c
> > >> +++ b/audio/avrcp.c
> > >> @@ -1272,8 +1272,11 @@ void avrcp_unregister(const bdaddr_t *src)
> > >>
> > >>       servers = g_slist_remove(servers, server);
> > >>
> > >> -     remove_record_from_server(server->ct_record_id);
> > >> -     remove_record_from_server(server->tg_record_id);
> > >> +     if (server->ct_record_id)
> > >> +             remove_record_from_server(server->ct_record_id);
> > >> +
> > >> +     if (server->tg_record_id)
> > >> +             remove_record_from_server(server->tg_record_id);
> > >>
> > >>       avctp_unregister(&server->src);
> > >>       g_free(server);
> > >
> > > I don't think the commit message for this patch is truthful. If you look
> > > at the code the remove_record_from_server will return ENOENT if you pass
> > > it a non-existent handle. I.e. it will not crash. Please fix the commit
> > > message to reflect what exactly is being fixed (i.e. an unnecessary call
> > > to remove_record_from_server if there is no record handle).
> > 
> > Then I don't see a reason: just let remove_record_from_server() fail ?
> > 
> > Chan, what are you fixing here?
> 
> As discussed on IRC (and Chanyeol should really have explained this in
> the commit message) handle 0 is actually the SDP server's own "master"
> record. So it is plausible that weird behavior might occur if trying to
> remove it. Since this is a SDP server internal detail and it doesn't
> really make sense to remove it I'm thinking that
> remove_record_from_server should return EINVAL if passed a 0 value.

I already pushed a patch for this so no need to work on it anymore.

Johan

[PATCH 3/3] Add support for AVRCP control,target role conf

[chanyeol.park]

From: Chan-yeol Park <chanyeol.park@samsung.com>

This patch adds logic for activating configured AVRCP role.
---
 audio/audio.conf |    2 +-
 audio/avrcp.c    |  107 ++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 73 insertions(+), 36 deletions(-)

diff --git a/audio/audio.conf b/audio/audio.conf
index 1abdab9..75dad56 100644
--- a/audio/audio.conf
+++ b/audio/audio.conf
@@ -9,7 +9,7 @@
 
 # If we want to disable support for specific services
 # Defaults to supporting all implemented services
-#Disable=Gateway,Sink,Socket
+#Disable=Gateway,Sink,Control,Socket
 
 # SCO routing. Either PCM or HCI (in which case audio is routed to/from ALSA)
 # Defaults to HCI
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 2e946c1..1b781a1 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1183,57 +1183,94 @@ void avrcp_disconnect(struct audio_device *dev)
 int avrcp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
 {
 	sdp_record_t *record;
-	gboolean tmp, master = TRUE;
+	gboolean tmp, master = TRUE, target = TRUE, control = FALSE;
+	char *str;
 	GError *err = NULL;
 	struct avrcp_server *server;
 
-	if (config) {
-		tmp = g_key_file_get_boolean(config, "General",
-							"Master", &err);
-		if (err) {
-			DBG("audio.conf: %s", err->message);
-			g_error_free(err);
-		} else
-			master = tmp;
+	if (!config)
+		goto proceed;
+
+	tmp = g_key_file_get_boolean(config, "General",
+			"Master", &err);
+	if (err) {
+		DBG("audio.conf: %s", err->message);
+		g_clear_error(&err);
+	} else
+		master = tmp;
+
+	str = g_key_file_get_string(config, "General", "Enable", &err);
+
+	if (err) {
+		DBG("audio.conf: %s", err->message);
+		g_clear_error(&err);
+	} else {
+		if (strstr(str, "Target"))
+			target = TRUE;
+		if (strstr(str, "Control"))
+			control = TRUE;
+		g_free(str);
 	}
 
+	str = g_key_file_get_string(config, "General", "Disable", &err);
+
+	if (err) {
+		DBG("audio.conf: %s", err->message);
+		g_clear_error(&err);
+	} else {
+		if (strstr(str, "Target"))
+			target = FALSE;
+		if (strstr(str, "Control"))
+			control = FALSE;
+		g_free(str);
+	}
+
+
+proceed:
 	server = g_new0(struct avrcp_server, 1);
 	if (!server)
 		return -ENOMEM;
 
-	record = avrcp_tg_record();
-	if (!record) {
-		error("Unable to allocate new service record");
-		g_free(server);
-		return -1;
-	}
+	if (target) {
+		record = avrcp_tg_record();
+		if (!record) {
+			error("Unable to allocate new service record");
+			g_free(server);
+			return -1;
+		}
 
-	if (add_record_to_server(src, record) < 0) {
-		error("Unable to register AVRCP target service record");
-		g_free(server);
-		sdp_record_free(record);
-		return -1;
-	}
-	server->tg_record_id = record->handle;
+		if (add_record_to_server(src, record) < 0) {
+			error("Unable to register AVRCP target service record");
+			g_free(server);
+			sdp_record_free(record);
+			return -1;
+		}
+		server->tg_record_id = record->handle;
 
-	record = avrcp_ct_record();
-	if (!record) {
-		error("Unable to allocate new service record");
-		g_free(server);
-		return -1;
 	}
 
-	if (add_record_to_server(src, record) < 0) {
-		error("Unable to register AVRCP service record");
-		sdp_record_free(record);
-		g_free(server);
-		return -1;
+	if (control) {
+		record = avrcp_ct_record();
+		if (!record) {
+			error("Unable to allocate new service record");
+			g_free(server);
+			return -1;
+		}
+
+		if (add_record_to_server(src, record) < 0) {
+			error("Unable to register AVRCP service record");
+			sdp_record_free(record);
+			g_free(server);
+			return -1;
+		}
+		server->ct_record_id = record->handle;
 	}
-	server->ct_record_id = record->handle;
 
 	if (avctp_register(src, master) < 0) {
-		remove_record_from_server(server->ct_record_id);
-		remove_record_from_server(server->tg_record_id);
+		if (control)
+			remove_record_from_server(server->ct_record_id);
+		if (target)
+			remove_record_from_server(server->tg_record_id);
 		g_free(server);
 		return -1;
 	}
-- 
1.7.9.5

Re: [PATCH 1/3] Fix A2DP Sink/Source recognition

[Johan Hedberg]

Hi Chanyeol,

On Fri, May 18, 2012, chanyeol.park@samsung.com wrote:
> From: Chan-yeol Park <chanyeol.park@samsung.com>
> 
> ---
>  audio/a2dp.c     |    8 ++++----
>  audio/audio.conf |    2 +-
>  audio/manager.c  |    4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/audio/a2dp.c b/audio/a2dp.c
> index cc8f139..145dcf6 100644
> --- a/audio/a2dp.c
> +++ b/audio/a2dp.c
> @@ -1406,9 +1406,9 @@ int a2dp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
>  		g_clear_error(&err);
>  	} else {
>  		if (strstr(str, "Sink"))
> -			source = TRUE;
> -		if (strstr(str, "Source"))
>  			sink = TRUE;
> +		if (strstr(str, "Source"))
> +			source = TRUE;
>  		if (strstr(str, "Socket"))
>  			socket = TRUE;
>  		g_free(str);
> @@ -1421,9 +1421,9 @@ int a2dp_register(DBusConnection *conn, const bdaddr_t *src, GKeyFile *config)
>  		g_clear_error(&err);
>  	} else {
>  		if (strstr(str, "Sink"))
> -			source = FALSE;
> -		if (strstr(str, "Source"))
>  			sink = FALSE;
> +		if (strstr(str, "Source"))
> +			source = FALSE;
>  		if (strstr(str, "Socket"))
>  			socket = FALSE;
>  		g_free(str);

This doesn't look right to me. The config file values have (for
historical reasons) been referring to the remote device object D-Bus
interface names whereas these gboolean variables are used to determine
what kind of local stream endpoints need to be registered. So having
e.g. support for "Source" on a remote device requires registering a
local sink stream endpoint.

Johan