2011-12-14 10:11:49

by Santiago Carot

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

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



2011-12-15 14:15:05

by Johan Hedberg

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

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

2011-12-15 12:57:13

by Santiago Carot

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

Hi,

2011/12/15 Johan Hedberg <[email protected]>:
> 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.

2011-12-15 11:59:42

by Johan Hedberg

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

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

2011-12-14 10:11:52

by Santiago Carot

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

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


2011-12-14 10:11:51

by Santiago Carot

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

---
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


2011-12-14 10:11:50

by Santiago Carot

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

---
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