[Bluez-devel] Some Audio error handling fixes.

[Bluez-devel] Some Audio error handling fixes.

[Fabien Chevalier]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

Hi all,

While running tests on bluez-utils lately i found a number of big and
little error handling issues.
Attached patchset fixes all of them :-)

Can anyone have a look to it ?

While reading the latest code, i noticed common/error.{c, h} is gone...
There's a new one in src/, but with only one generic
error handling routine. I have the feeling this is a big step backwards,
as DBUS error names get duplicated into each service
again and again, paving the way for un unmanageable level of error
strings fragmentation :-( What do you guys think ?

Cheers,

Fabien


[-- Attachment #2: 0001-Error-code-fixes.patch --]
[-- Type: text/x-patch, Size: 1526 bytes --]

>>From a22fa4ea791b5555673cdb1729f8d72510fbb74f Mon Sep 17 00:00:00 2001
From: Fabien Chevalier <fabchevalier@free.fr>
Date: Sat, 13 Sep 2008 17:11:08 +0200
Subject: [PATCH] Error code fixes

In case of socket errors, we used to return a positive value
where the convention is to return negative values only
---
 audio/avdtp.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 592322f..ae56910 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1380,7 +1380,7 @@ static gboolean avdtp_parse_cmd(struct avdtp *session,
 		debug("Received DISCOVER_CMD");
 		return avdtp_discover_cmd(session, (void *) header, size);
 	case AVDTP_GET_CAPABILITIES:
-		debug("Received  GET_CAPABILITIES_CMD");
+		debug("Received	GET_CAPABILITIES_CMD");
 		return avdtp_getcap_cmd(session, (void *) header, size);
 	case AVDTP_SET_CONFIGURATION:
 		debug("Received SET_CONFIGURATION_CMD");
@@ -1559,9 +1559,9 @@ static void l2cap_connect_cb(GIOChannel *chan, int err, const bdaddr_t *src,
 	len = sizeof(l2o);
 	if (getsockopt(sk, SOL_L2CAP, L2CAP_OPTIONS, &l2o,
 				&len) < 0) {
-		err = errno;
-		error("getsockopt(L2CAP_OPTIONS): %s (%d)", strerror(err),
-				err);
+		err = -errno;
+		error("getsockopt(L2CAP_OPTIONS): %s (%d)", strerror(-err),
+				-err);
 		goto failed;
 	}
 
@@ -1598,7 +1598,7 @@ failed:
 					AVDTP_STATE_IDLE);
 		session->pending_open = NULL;
 	} else
-		connection_lost(session, -err);
+		connection_lost(session, err);
 
 	return;
 }
-- 
1.5.6.5



[-- Attachment #3: 0002-Connect-fix.patch --]
[-- Type: text/x-patch, Size: 960 bytes --]

>>From dbaed0dc30ebe2a4503a43d2e8e9fd69f1eced2d Mon Sep 17 00:00:00 2001
From: Fabien Chevalier <fabchevalier@free.fr>
Date: Sat, 13 Sep 2008 17:13:27 +0200
Subject: [PATCH] Connect() fix.

In case we are unable to connect the sink, we forget to send DBUS reply.
Caller is stuck forever...
---
 audio/sink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/sink.c b/audio/sink.c
index c6d2dc4..e79b9f1 100644
--- a/audio/sink.c
+++ b/audio/sink.c
@@ -381,11 +381,11 @@ static void discovery_complete(struct avdtp *session, GSList *seps, struct avdtp
 	return;
 
 failed:
-	pending_request_free(pending);
+	error_failed(pending->conn, pending->msg, "Stream setup failed");
+ 	pending_request_free(pending);
 	sink->connect = NULL;
 	avdtp_unref(sink->session);
 	sink->session = NULL;
-	error_failed(pending->conn, pending->msg, "Stream setup failed");
 }
 
 static DBusMessage *sink_connect(DBusConnection *conn,
-- 
1.5.6.5



[-- Attachment #4: 0003-Error-value-fixes.patch --]
[-- Type: text/x-patch, Size: 1233 bytes --]

>>From 6a3a4f1229728754e01f3bc7911c3d508348a56f Mon Sep 17 00:00:00 2001
From: Fabien Chevalier <fabchevalier@free.fr>
Date: Sat, 13 Sep 2008 17:14:34 +0200
Subject: [PATCH] Error value fixes.

In case the HSP/HFP headset is down, we return "not supported" : this is non-sense :
change that to connection attempt failed
---
 audio/headset.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/audio/headset.c b/audio/headset.c
index 102d0bc..45bcf34 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -1074,7 +1074,9 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
 	if (err < 0) {
 		error("Unable to get service record: %s (%d)", strerror(-err),
 			-err);
-		goto failed_not_supported;
+		error_connection_attempt_failed(dev->conn, p->msg, p->err);
+
+		goto failed;
 	}
 
 	if (!recs || !recs->data) {
@@ -1140,10 +1142,13 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
 failed_not_supported:
 	if (p->msg) {
 		error_not_supported(dev->conn, p->msg);
+	}
+failed:
+	if (p->msg) {
 		dbus_message_unref(p->msg);
 		p->msg = NULL;
 	}
-failed:
+
 	if (classes)
 		sdp_list_free(classes, free);
 	pending_connect_finalize(dev);
-- 
1.5.6.5



[-- Attachment #5: Type: text/plain, Size: 363 bytes --]

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[-- Attachment #6: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Luiz Augusto von Dentz]

Hi,

Looks good to me, can you configure a git tree to pull from? If you
don't have a host you can use http://gitorious.org/projects/bluez,
just create an account and clone repository.

-- =

Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great priz=
es
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=3D100&url=3D/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Fabien Chevalier]

Hi Luiz,

Thanks for the quick answer,

I'm gonna have a look to that right away...

Fabien

> Hi,
> 
> Looks good to me, can you configure a git tree to pull from? If you
> don't have a host you can use http://gitorious.org/projects/bluez,
> just create an account and clone repository.
> 


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Marcel Holtmann]

Hi Fabien,

> While running tests on bluez-utils lately i found a number of big and
> little error handling issues.
> Attached patchset fixes all of them :-)
> 
> Can anyone have a look to it ?

actually the convention is that if it is a return value, then we use a
negative error value. If it is a parameter, we use the positive one. So
the connection_lost change is wrong (it was wrong before though). Please
fix this too.

> While reading the latest code, i noticed common/error.{c, h} is gone...
> There's a new one in src/, but with only one generic
> error handling routine. I have the feeling this is a big step backwards,
> as DBUS error names get duplicated into each service
> again and again, paving the way for un unmanageable level of error
> strings fragmentation :-( What do you guys think ?

It is not a big step backwards since we are using libgdbus error
functions now and there are more simpler. There is a problem with the
strings returned for a specific error, but I don't really thing it is
that big of a problem.

In the long term we might get btd_error_* or alike functions since then
the plugins can use them. However right now every plugin has to handle
its own errors.

Regards

Marcel



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Fabien Chevalier]

Hi Marcel,

> 
> actually the convention is that if it is a return value, then we use a
> negative error value. If it is a parameter, we use the positive one. So
> the connection_lost change is wrong (it was wrong before though). Please
> fix this too.

Ok, that might be quite a huge patch though... it has been that way for 
so long... virtually every function will be impacted :-(

> 
>> While reading the latest code, i noticed common/error.{c, h} is gone...
>> There's a new one in src/, but with only one generic
>> error handling routine. I have the feeling this is a big step backwards,
>> as DBUS error names get duplicated into each service
>> again and again, paving the way for un unmanageable level of error
>> strings fragmentation :-( What do you guys think ?
> 
> It is not a big step backwards since we are using libgdbus error
> functions now and there are more simpler. There is a problem with the
> strings returned for a specific error, but I don't really thing it is
> that big of a problem.

Ok i see the thing now. By that i guess that all error handling outside 
of g_dbus_create_error are on their way out ?

I guess those ones should be replaced with g_dbus equivalents ?
./src/adapter.c:static DBusHandlerResult error_failed(DBusConnection *conn,

./src/adapter.c:static DBusHandlerResult 
error_connection_attempt_failed(DBusConnection *conn,


> 
> In the long term we might get btd_error_* or alike functions since then
> the plugins can use them. However right now every plugin has to handle
> its own errors.
> 
Thanks for the explanation,

Cheers,

Fabien

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Fabien Chevalier]

[-- Attachment #1: Type: text/plain, Size: 174 bytes --]

Hi all,

Updated patchset attached !

Luiz: I tried to find some doc to get started on gitorious but didn't 
found any... how to i create my remote clone ?

Regards,

Fabien

[-- Attachment #2: 0001-Error-code-fixes.patch --]
[-- Type: text/x-patch, Size: 2522 bytes --]

>>From 66048184e4d7c88afae449236a2e62ef352c4438 Mon Sep 17 00:00:00 2001
From: Fabien Chevalier <fabchevalier@free.fr>
Date: Sat, 13 Sep 2008 17:11:08 +0200
Subject: [PATCH] Error code fixes

In case of socket errors, we used to return a positive value
where the convention is to return negative values only : fix that
---
 audio/avdtp.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/audio/avdtp.c b/audio/avdtp.c
index 592322f..bf5c0d3 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -489,7 +489,7 @@ static gboolean disconnect_timeout(gpointer user_data)
 
 	session->dc_timer = 0;
 
-	connection_lost(session, -ETIMEDOUT);
+	connection_lost(session, ETIMEDOUT);
 
 	return FALSE;
 }
@@ -728,7 +728,7 @@ static void finalize_discovery(struct avdtp *session, int err)
 {
 	struct avdtp_error avdtp_err;
 
-	avdtp_error_init(&avdtp_err, AVDTP_ERROR_ERRNO, -err);
+	avdtp_error_init(&avdtp_err, AVDTP_ERROR_ERRNO, err);
 
 	if (!session->discov_cb)
 		return;
@@ -1380,7 +1380,7 @@ static gboolean avdtp_parse_cmd(struct avdtp *session,
 		debug("Received DISCOVER_CMD");
 		return avdtp_discover_cmd(session, (void *) header, size);
 	case AVDTP_GET_CAPABILITIES:
-		debug("Received  GET_CAPABILITIES_CMD");
+		debug("Received	GET_CAPABILITIES_CMD");
 		return avdtp_getcap_cmd(session, (void *) header, size);
 	case AVDTP_SET_CONFIGURATION:
 		debug("Received SET_CONFIGURATION_CMD");
@@ -1519,7 +1519,7 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 	return TRUE;
 
 failed:
-	connection_lost(session, -EIO);
+	connection_lost(session, EIO);
 
 	return FALSE;
 }
@@ -1559,9 +1559,9 @@ static void l2cap_connect_cb(GIOChannel *chan, int err, const bdaddr_t *src,
 	len = sizeof(l2o);
 	if (getsockopt(sk, SOL_L2CAP, L2CAP_OPTIONS, &l2o,
 				&len) < 0) {
-		err = errno;
-		error("getsockopt(L2CAP_OPTIONS): %s (%d)", strerror(err),
-				err);
+		err = -errno;
+		error("getsockopt(L2CAP_OPTIONS): %s (%d)", strerror(-err),
+				-err);
 		goto failed;
 	}
 
@@ -1710,7 +1710,7 @@ static gboolean request_timeout(gpointer user_data)
 	goto done;
 
 failed:
-	connection_lost(session, -ETIMEDOUT);
+	connection_lost(session, ETIMEDOUT);
 done:
 	pending_req_free(req);
 	return FALSE;
@@ -2716,7 +2716,7 @@ static void auth_cb(DBusError *derr, void *user_data)
 	if (derr && dbus_error_is_set(derr)) {
 		error("Access denied: %s", derr->message);
 
-		connection_lost(session, -EACCES);
+		connection_lost(session, EACCES);
 		return;
 	}
 
-- 
1.5.6.5


[-- Attachment #3: 0002-Connect-fix.patch --]
[-- Type: text/x-patch, Size: 959 bytes --]

>>From 8da5a48ab9667f13066d6e048100da9269d8a205 Mon Sep 17 00:00:00 2001
From: Fabien Chevalier <fabchevalier@free.fr>
Date: Sat, 13 Sep 2008 17:13:27 +0200
Subject: [PATCH] Connect() fix.

In case we are unable to connect the sink, we forget to send DBUS reply.
Caller is stuck forever...
---
 audio/sink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/sink.c b/audio/sink.c
index c6d2dc4..e79b9f1 100644
--- a/audio/sink.c
+++ b/audio/sink.c
@@ -381,11 +381,11 @@ static void discovery_complete(struct avdtp *session, GSList *seps, struct avdtp
 	return;
 
 failed:
-	pending_request_free(pending);
+	error_failed(pending->conn, pending->msg, "Stream setup failed");
+ 	pending_request_free(pending);
 	sink->connect = NULL;
 	avdtp_unref(sink->session);
 	sink->session = NULL;
-	error_failed(pending->conn, pending->msg, "Stream setup failed");
 }
 
 static DBusMessage *sink_connect(DBusConnection *conn,
-- 
1.5.6.5


[-- Attachment #4: 0003-Error-value-fixes.patch --]
[-- Type: text/x-patch, Size: 1232 bytes --]

>>From d79457b70e1480f0c590d043208dbe61ceca4026 Mon Sep 17 00:00:00 2001
From: Fabien Chevalier <fabchevalier@free.fr>
Date: Sat, 13 Sep 2008 17:14:34 +0200
Subject: [PATCH] Error value fixes.

In case the HSP/HFP headset is down, we return "not supported" : this is non-sense :
change that to connection attempt failed
---
 audio/headset.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/audio/headset.c b/audio/headset.c
index 102d0bc..45bcf34 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -1074,7 +1074,9 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
 	if (err < 0) {
 		error("Unable to get service record: %s (%d)", strerror(-err),
 			-err);
-		goto failed_not_supported;
+		error_connection_attempt_failed(dev->conn, p->msg, p->err);
+
+		goto failed;
 	}
 
 	if (!recs || !recs->data) {
@@ -1140,10 +1142,13 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
 failed_not_supported:
 	if (p->msg) {
 		error_not_supported(dev->conn, p->msg);
+	}
+failed:
+	if (p->msg) {
 		dbus_message_unref(p->msg);
 		p->msg = NULL;
 	}
-failed:
+
 	if (classes)
 		sdp_list_free(classes, free);
 	pending_connect_finalize(dev);
-- 
1.5.6.5


[-- Attachment #5: Type: text/plain, Size: 363 bytes --]

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[-- Attachment #6: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Marcel Holtmann]

Hi Fabien,

> Updated patchset attached !

does connection_lost() handles a positive error message properly?

> Luiz: I tried to find some doc to get started on gitorious but didn't 
> found any... how to i create my remote clone ?

I would prefer having a tree to pull from, because that makes my life a
lot easier.

Regards

Marcel



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] Some Audio error handling fixes.

[Fabien Chevalier]

Hi Marcel,

> Hi Fabien,
> 
>> Updated patchset attached !
> 
> does connection_lost() handles a positive error message properly?

Yes. This is the change line 30 of the patch.
Only thing connection_lost does is call finalize_discovery().

> 
>> Luiz: I tried to find some doc to get started on gitorious but didn't 
>> found any... how to i create my remote clone ?
> 
> I would prefer having a tree to pull from, because that makes my life a
> lot easier.

I'm on it... :-)

Regards,

Fabien

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel