[PATCH 1/4] hdp.c: Delay ChannelConnected signal

[PATCH 1/4] hdp.c: Delay ChannelConnected signal

[Santiago Carot-Nemesio]

This signal is sent before channel is connected as soon as MCAP accepts
the data channel creation request. At this moment, applications that
try to get the fd before the connection finishes will fail. This problem
can be solved delaying the ChannelConnection signal until the connection
operation finishes.
---
 health/hdp.c |   45 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/health/hdp.c b/health/hdp.c
index 6d53439..bcb6ef4 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -1626,6 +1626,25 @@ static void abort_and_del_mdl_cb(GError *err, gpointer data)
 	}
 }
 
+static void abort_mdl_connection_cb(GError *err, gpointer data)
+{
+	struct hdp_tmp_dc_data *hdp_conn = data;
+	struct hdp_channel *hdp_chann = hdp_conn->hdp_chann;
+
+	if (err != NULL)
+		error("Aborting error: %s", err->message);
+
+	/* Connection operation has failed but we have to */
+	/* notify the channel created at MCAP level */
+	if (hdp_chann->mdep != HDP_MDEP_ECHO)
+		g_dbus_emit_signal(hdp_conn->conn,
+					device_get_path(hdp_chann->dev->dev),
+					HEALTH_DEVICE,
+					"ChannelConnected",
+					DBUS_TYPE_OBJECT_PATH, &hdp_chann->path,
+					DBUS_TYPE_INVALID);
+}
+
 static void hdp_mdl_conn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
 {
 	struct hdp_tmp_dc_data *hdp_conn =  data;
@@ -1643,8 +1662,10 @@ static void hdp_mdl_conn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
 
 		/* Send abort request because remote side */
 		/* is now in PENDING state */
-		if (!mcap_mdl_abort(hdp_chann->mdl, abort_mdl_cb, NULL,
-								NULL, &gerr)) {
+		if (!mcap_mdl_abort(hdp_chann->mdl, abort_mdl_connection_cb,
+					hdp_tmp_dc_data_ref(hdp_conn),
+					hdp_tmp_dc_data_destroy, &gerr)) {
+			hdp_tmp_dc_data_unref(hdp_conn);
 			error("%s", gerr->message);
 			g_error_free(gerr);
 		}
@@ -1656,6 +1677,13 @@ static void hdp_mdl_conn_cb(struct mcap_mdl *mdl, GError *err, gpointer data)
 					DBUS_TYPE_INVALID);
 	g_dbus_send_message(hdp_conn->conn, reply);
 
+	g_dbus_emit_signal(hdp_conn->conn,
+					device_get_path(hdp_chann->dev->dev),
+					HEALTH_DEVICE,
+					"ChannelConnected",
+					DBUS_TYPE_OBJECT_PATH, &hdp_chann->path,
+					DBUS_TYPE_INVALID);
+
 	if (!check_channel_conf(hdp_chann)) {
 		close_mdl(hdp_chann);
 		return;
@@ -1710,14 +1738,6 @@ static void device_create_mdl_cb(struct mcap_mdl *mdl, uint8_t conf,
 	if (hdp_chan == NULL)
 		goto fail;
 
-	if (user_data->mdep != HDP_MDEP_ECHO)
-		g_dbus_emit_signal(user_data->conn,
-					device_get_path(hdp_chan->dev->dev),
-					HEALTH_DEVICE,
-					"ChannelConnected",
-					DBUS_TYPE_OBJECT_PATH, &hdp_chan->path,
-					DBUS_TYPE_INVALID);
-
 	hdp_conn = g_new0(struct hdp_tmp_dc_data, 1);
 	hdp_conn->msg = dbus_message_ref(user_data->msg);
 	hdp_conn->conn = dbus_connection_ref(user_data->conn);
@@ -1740,7 +1760,10 @@ static void device_create_mdl_cb(struct mcap_mdl *mdl, uint8_t conf,
 	hdp_tmp_dc_data_unref(hdp_conn);
 
 	/* Send abort request because remote side is now in PENDING state */
-	if (!mcap_mdl_abort(mdl, abort_mdl_cb, NULL, NULL, &gerr)) {
+	if (!mcap_mdl_abort(hdp_chan->mdl, abort_mdl_connection_cb,
+					hdp_tmp_dc_data_ref(hdp_conn),
+					hdp_tmp_dc_data_destroy, &gerr)) {
+		hdp_tmp_dc_data_unref(hdp_conn);
 		error("%s", gerr->message);
 		g_error_free(gerr);
 	}
-- 
1.7.8

[PATCH 2/4] mcap_sync.c: Fix include paths

[Santiago Carot-Nemesio]

---
 health/mcap_sync.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/health/mcap_sync.c b/health/mcap_sync.c
index 5545cea..e2d7715 100644
--- a/health/mcap_sync.c
+++ b/health/mcap_sync.c
@@ -28,8 +28,8 @@
 #include <stdlib.h>
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/l2cap.h>
-#include "../src/adapter.h"
-#include "../src/manager.h"
+#include <adapter.h>
+#include <manager.h>
 #include <sys/ioctl.h>
 
 #include "config.h"
-- 
1.7.8

[PATCH 3/4] hdp.c: Fix include paths

[Santiago Carot-Nemesio]

---
 health/hdp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/health/hdp.c b/health/hdp.c
index bcb6ef4..cf6ec76 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -36,7 +36,7 @@
 #include <mcap_lib.h>
 #include <bluetooth/l2cap.h>
 #include <sdpd.h>
-#include "../src/dbus-common.h"
+#include "dbus-common.h"
 #include <unistd.h>
 
 #ifndef DBUS_TYPE_UNIX_FD
-- 
1.7.8

[PATCH 4/4] hdp.c: Fix memory leak aborting data channel connections

[Santiago Carot-Nemesio]

Use GDestroyNotify function to decrease the reference counter of
the data channel provided in the callback when abort operation is
invoked in MCAP.
---
 health/hdp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/health/hdp.c b/health/hdp.c
index cf6ec76..db715f5 100644
--- a/health/hdp.c
+++ b/health/hdp.c
@@ -1780,8 +1780,8 @@ fail:
 	/* Send abort request because remote side is now in PENDING */
 	/* state. Then we have to delete it because we couldn't */
 	/* register the HealthChannel interface */
-	if (!mcap_mdl_abort(mdl, abort_and_del_mdl_cb, mcap_mdl_ref(mdl), NULL,
-								&gerr)) {
+	if (!mcap_mdl_abort(mdl, abort_and_del_mdl_cb, mcap_mdl_ref(mdl),
+				(GDestroyNotify) mcap_mdl_unref, &gerr)) {
 		error("%s", gerr->message);
 		g_error_free(gerr);
 		mcap_mdl_unref(mdl);
-- 
1.7.8

Re: [PATCH 2/4] mcap_sync.c: Fix include paths

[Johan Hedberg]

Hi Santiago,

On Wed, Dec 14, 2011, Santiago Carot-Nemesio wrote:
> ---
>  health/mcap_sync.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/health/mcap_sync.c b/health/mcap_sync.c
> index 5545cea..e2d7715 100644
> --- a/health/mcap_sync.c
> +++ b/health/mcap_sync.c
> @@ -28,8 +28,8 @@
>  #include <stdlib.h>
>  #include <bluetooth/bluetooth.h>
>  #include <bluetooth/l2cap.h>
> -#include "../src/adapter.h"
> -#include "../src/manager.h"
> +#include <adapter.h>
> +#include <manager.h>
>  #include <sys/ioctl.h>
>  
>  #include "config.h"

There's more to fix here than this. config.h should come before anything
else (and should be inside a #ifdef HAVE_CONFIG_H) and we sort the libc
includes first, then dbus,glib,etc and finally the bluez internal
includes.

Johan

Re: [PATCH 2/4] mcap_sync.c: Fix include paths

[Santiago Carot]

Hi,

2011/12/15 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi Santiago,
>
> On Wed, Dec 14, 2011, Santiago Carot-Nemesio wrote:
>> ---
>>  health/mcap_sync.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/health/mcap_sync.c b/health/mcap_sync.c
>> index 5545cea..e2d7715 100644
>> --- a/health/mcap_sync.c
>> +++ b/health/mcap_sync.c
>> @@ -28,8 +28,8 @@
>>  #include <stdlib.h>
>>  #include <bluetooth/bluetooth.h>
>>  #include <bluetooth/l2cap.h>
>> -#include "../src/adapter.h"
>> -#include "../src/manager.h"
>> +#include <adapter.h>
>> +#include <manager.h>
>>  #include <sys/ioctl.h>
>>
>>  #include "config.h"
>
> There's more to fix here than this. config.h should come before anything
> else (and should be inside a #ifdef HAVE_CONFIG_H) and we sort the libc
> includes first, then dbus,glib,etc and finally the bluez internal
> includes.
>

You are right, I didn't notice it. I'll send a new patch solving this issue.
Regards.

Re: [PATCH 1/4] hdp.c: Delay ChannelConnected signal

[Johan Hedberg]

Hi Santiago,

On Wed, Dec 14, 2011, Santiago Carot-Nemesio wrote:
> This signal is sent before channel is connected as soon as MCAP accepts
> the data channel creation request. At this moment, applications that
> try to get the fd before the connection finishes will fail. This problem
> can be solved delaying the ChannelConnection signal until the connection
> operation finishes.
> ---
>  health/hdp.c |   45 ++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 34 insertions(+), 11 deletions(-)

Thanks. The patches have now been pushed upstream. I had to do one extra
patch myself since I found quite many things wrong with the include
statement ordering for files under the health subdirectory.

Johan