[RFC BlueZ 0/6] LE Connection Establishment fixes

[RFC BlueZ 0/6] LE Connection Establishment fixes

[Vinicius Costa Gomes]

Hi,

Sending this as a RFC because I don't know if this is what you guys
had in mind.

Patches 1/6/ and 2/6 are simple enough fixes and should be considered
for inclusion, perhaps 6/6 as well, but the improvement is
non-functional.

The only problem with this aproach that I found while testing is the
problem exposed on the message of commit 5/6: If the remote device is
out of range during pairing any connection attempt will fail until the
Connection Complete event comes (with an error). This is the reason
that we put the devices in the connect list before pairing.

Is this reason enough to have the device put in the connect list
during pairing?


Cheers,


Vinicius Costa Gomes (6):
  device: Fix invalid memory access during Find Included
  device: Fix memory leak while storing GATT capable devices
  device: Clean up device_att_connect()
  device: Rename device_att_connect to device_connect_le
  core: Add a way to connect to LE devices
  gas: Move all the code to only one file

 Makefile.plugins        |   4 +-
 profiles/gatt/gas.c     |  46 +++++++++++++++++++++
 profiles/gatt/main.c    |  47 ---------------------
 profiles/gatt/manager.c |  79 -----------------------------------
 profiles/gatt/manager.h |  24 -----------
 src/adapter.c           |  22 +++++++++-
 src/adapter.h           |   3 ++
 src/device.c            | 108 +++++++++++++++++++++---------------------------
 src/device.h            |   3 +-
 9 files changed, 119 insertions(+), 217 deletions(-)
 delete mode 100644 profiles/gatt/main.c
 delete mode 100644 profiles/gatt/manager.c
 delete mode 100644 profiles/gatt/manager.h

-- 
1.8.1.1

[RFC BlueZ 1/6] device: Fix invalid memory access during Find Included

[Vinicius Costa Gomes]

When doing the Find Included Services GATT procedure, the status of the ATT
procedure was being ignored, and in the case of a timeout it is possible to
crash bluetooth with an invalid memory access.

Valgrind log:

==1755== Invalid read of size 8
==1755==    at 0x46971A: find_included_cb (device.c:2964)
==1755==    by 0x4465AE: isd_unref (gatt.c:92)
==1755==    by 0x446885: find_included_cb (gatt.c:425)
==1755==    by 0x448266: disconnect_timeout (gattrib.c:269)
==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x40A2EE: main (main.c:583)
==1755==  Address 0x69530a8 is 8 bytes inside a block of size 64 free'd
==1755==    at 0x4C2874F: free (vg_replace_malloc.c:446)
==1755==    by 0x40BFA6: service_filter (watch.c:486)
==1755==    by 0x40BC6A: message_filter (watch.c:554)
==1755==    by 0x5160A1D: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.7.2)
==1755==    by 0x40AAB7: message_dispatch (mainloop.c:76)
==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x40A2EE: main (main.c:583)
==1755==
==1755== Invalid read of size 8
==1755==    at 0x4486D5: g_attrib_get_buffer (gattrib.c:657)
==1755==    by 0x4467C5: find_included (gatt.c:363)
==1755==    by 0x4465AE: isd_unref (gatt.c:92)
==1755==    by 0x446885: find_included_cb (gatt.c:425)
==1755==    by 0x448266: disconnect_timeout (gattrib.c:269)
==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x40A2EE: main (main.c:583)
==1755==  Address 0x18 is not stack'd, malloc'd or (recently) free'd
==1755==
==1755==
==1755== Process terminating with default action of signal 11 (SIGSEGV)
==1755==  Access not within mapped region at address 0x18
==1755==    at 0x4486D5: g_attrib_get_buffer (gattrib.c:657)
==1755==    by 0x4467C5: find_included (gatt.c:363)
==1755==    by 0x4465AE: isd_unref (gatt.c:92)
==1755==    by 0x446885: find_included_cb (gatt.c:425)
==1755==    by 0x448266: disconnect_timeout (gattrib.c:269)
==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
==1755==    by 0x40A2EE: main (main.c:583)
---
 src/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index bb79b38..893e4bb 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2963,7 +2963,7 @@ static void find_included_cb(GSList *includes, uint8_t status,
 	struct gatt_primary *prim;
 	GSList *l;
 
-	if (includes == NULL)
+	if (includes == NULL || status != 0)
 		goto done;
 
 	for (l = includes; l; l = l->next) {
-- 
1.8.1.1

Re: [RFC BlueZ 1/6] device: Fix invalid memory access during Find Included

[Johan Hedberg]

Hi Vinicius,

On Fri, Jan 25, 2013, Vinicius Costa Gomes wrote:
> When doing the Find Included Services GATT procedure, the status of the ATT
> procedure was being ignored, and in the case of a timeout it is possible to
> crash bluetooth with an invalid memory access.
> 
> Valgrind log:
> 
> ==1755== Invalid read of size 8
> ==1755==    at 0x46971A: find_included_cb (device.c:2964)
> ==1755==    by 0x4465AE: isd_unref (gatt.c:92)
> ==1755==    by 0x446885: find_included_cb (gatt.c:425)
> ==1755==    by 0x448266: disconnect_timeout (gattrib.c:269)
> ==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x40A2EE: main (main.c:583)
> ==1755==  Address 0x69530a8 is 8 bytes inside a block of size 64 free'd
> ==1755==    at 0x4C2874F: free (vg_replace_malloc.c:446)
> ==1755==    by 0x40BFA6: service_filter (watch.c:486)
> ==1755==    by 0x40BC6A: message_filter (watch.c:554)
> ==1755==    by 0x5160A1D: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.7.2)
> ==1755==    by 0x40AAB7: message_dispatch (mainloop.c:76)
> ==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x40A2EE: main (main.c:583)
> ==1755==
> ==1755== Invalid read of size 8
> ==1755==    at 0x4486D5: g_attrib_get_buffer (gattrib.c:657)
> ==1755==    by 0x4467C5: find_included (gatt.c:363)
> ==1755==    by 0x4465AE: isd_unref (gatt.c:92)
> ==1755==    by 0x446885: find_included_cb (gatt.c:425)
> ==1755==    by 0x448266: disconnect_timeout (gattrib.c:269)
> ==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x40A2EE: main (main.c:583)
> ==1755==  Address 0x18 is not stack'd, malloc'd or (recently) free'd
> ==1755==
> ==1755==
> ==1755== Process terminating with default action of signal 11 (SIGSEGV)
> ==1755==  Access not within mapped region at address 0x18
> ==1755==    at 0x4486D5: g_attrib_get_buffer (gattrib.c:657)
> ==1755==    by 0x4467C5: find_included (gatt.c:363)
> ==1755==    by 0x4465AE: isd_unref (gatt.c:92)
> ==1755==    by 0x446885: find_included_cb (gatt.c:425)
> ==1755==    by 0x448266: disconnect_timeout (gattrib.c:269)
> ==1755==    by 0x4E76BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x4E76771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==1755==    by 0x40A2EE: main (main.c:583)
> ---
>  src/device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/device.c b/src/device.c
> index bb79b38..893e4bb 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2963,7 +2963,7 @@ static void find_included_cb(GSList *includes, uint8_t status,
>  	struct gatt_primary *prim;
>  	GSList *l;
>  
> -	if (includes == NULL)
> +	if (includes == NULL || status != 0)
>  		goto done;
>  
>  	for (l = includes; l; l = l->next) {

I think the bigger question is why is includes not NULL if status is not
zero? Does it contain invalid entries? Isn't that something that should
be fixed instead? Also, why isn't the code logging something in the case
of status != 0?

Johan

[RFC BlueZ 2/6] device: Fix memory leak while storing GATT capable devices

[Vinicius Costa Gomes]

bt_uuid2string() returns a newly allocated string, and so it should
be free'd.
---
 src/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/device.c b/src/device.c
index 893e4bb..6c4455b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2814,6 +2814,8 @@ static void store_services(struct btd_device *device)
 
 	sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
 	prim_uuid = bt_uuid2string(&uuid);
+	if (prim_uuid == NULL)
+		return;
 
 	ba2str(adapter_get_address(adapter), src_addr);
 	ba2str(&device->bdaddr, dst_addr);
@@ -2862,6 +2864,7 @@ static void store_services(struct btd_device *device)
 		g_file_set_contents(filename, data, length, NULL);
 	}
 
+	g_free(prim_uuid);
 	g_free(data);
 	g_key_file_free(key_file);
 }
-- 
1.8.1.1

Re: [RFC BlueZ 2/6] device: Fix memory leak while storing GATT capable devices

[Johan Hedberg]

Hi Vinicius,

On Fri, Jan 25, 2013, Vinicius Costa Gomes wrote:
> bt_uuid2string() returns a newly allocated string, and so it should
> be free'd.
> ---
>  src/device.c | 3 +++
>  1 file changed, 3 insertions(+)

This patch has been applied. Thanks.

Johan

[RFC BlueZ 3/6] device: Clean up device_att_connect()

[Vinicius Costa Gomes]

device_att_connect() is used to make an ATT connection to an LE device.

We clean it (and its associated callback) to have better error handling
and to respond the user with an error when it happens.

As it is only used for LE devices, we remove all the handling for non-LE
devices.
---
 src/device.c | 93 +++++++++++++++++++++++++-----------------------------------
 src/device.h |  3 +-
 2 files changed, 41 insertions(+), 55 deletions(-)

diff --git a/src/device.c b/src/device.c
index 6c4455b..89df161 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3045,7 +3045,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	struct btd_device *device = attcb->user_data;
 	uint8_t io_cap;
 	GAttrib *attrib;
-	int err;
+	int err = 0;
 
 	g_io_channel_unref(device->att_io);
 	device->att_io = NULL;
@@ -3056,6 +3056,7 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 		if (attcb->error)
 			attcb->error(gerr, user_data);
 
+		err = -ECONNABORTED;
 		goto done;
 	}
 
@@ -3079,10 +3080,10 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	else
 		io_cap = IO_CAPABILITY_NOINPUTNOOUTPUT;
 
-	/* this is a LE device during pairing */
 	err = adapter_create_bonding(device->adapter, &device->bdaddr,
 					device->bdaddr_type, io_cap);
-	if (err < 0) {
+done:
+	if (device->bonding && err < 0) {
 		DBusMessage *reply = btd_error_failed(device->bonding->msg,
 							strerror(-err));
 		g_dbus_send_message(dbus_conn, reply);
@@ -3090,7 +3091,6 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 		bonding_request_free(device->bonding);
 	}
 
-done:
 	g_free(attcb);
 }
 
@@ -3120,80 +3120,65 @@ static void att_success_cb(gpointer user_data)
 	g_slist_foreach(device->attios, attio_connected, device->attrib);
 }
 
-GIOChannel *device_att_connect(gpointer user_data)
+GIOChannel *device_att_connect(struct btd_device *dev)
 {
-	struct btd_device *device = user_data;
-	struct btd_adapter *adapter = device->adapter;
+	struct btd_adapter *adapter = dev->adapter;
 	struct att_callbacks *attcb;
+	BtIOSecLevel sec_level;
 	GIOChannel *io;
 	GError *gerr = NULL;
 	char addr[18];
 
-	ba2str(&device->bdaddr, addr);
+	/* There is one connection attempt going on */
+	if (dev->att_io)
+		return NULL;
+
+	ba2str(&dev->bdaddr, addr);
 
 	DBG("Connection attempt to: %s", addr);
 
 	attcb = g_new0(struct att_callbacks, 1);
 	attcb->error = att_error_cb;
 	attcb->success = att_success_cb;
-	attcb->user_data = device;
+	attcb->user_data = dev;
 
-	if (device_is_bredr(device)) {
-		io = bt_io_connect(att_connect_cb,
-					attcb, NULL, &gerr,
-					BT_IO_OPT_SOURCE_BDADDR,
-					adapter_get_address(adapter),
-					BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
-					BT_IO_OPT_PSM, ATT_PSM,
-					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
-					BT_IO_OPT_INVALID);
-	} else if (device->bonding) {
-		/* this is a LE device during pairing, using low sec level */
-		io = bt_io_connect(att_connect_cb,
-				attcb, NULL, &gerr,
-				BT_IO_OPT_SOURCE_BDADDR,
-				adapter_get_address(adapter),
-				BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
-				BT_IO_OPT_DEST_TYPE, device->bdaddr_type,
-				BT_IO_OPT_CID, ATT_CID,
-				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
-				BT_IO_OPT_INVALID);
-		if (io == NULL) {
+	if (dev->paired)
+		sec_level = BT_IO_SEC_MEDIUM;
+	else
+		sec_level = BT_IO_SEC_LOW;
+
+	/*
+	 * This connection will help us catch any PDUs that comes before
+	 * pairing finishes
+	 */
+	io = bt_io_connect(att_connect_cb, attcb, NULL, &gerr,
+			BT_IO_OPT_SOURCE_BDADDR, adapter_get_address(adapter),
+			BT_IO_OPT_DEST_BDADDR, &dev->bdaddr,
+			BT_IO_OPT_DEST_TYPE, dev->bdaddr_type,
+			BT_IO_OPT_CID, ATT_CID,
+			BT_IO_OPT_SEC_LEVEL, sec_level,
+			BT_IO_OPT_INVALID);
+
+	if (io == NULL) {
+		if (dev->bonding) {
 			DBusMessage *reply = btd_error_failed(
-					device->bonding->msg, gerr->message);
+					dev->bonding->msg, gerr->message);
+
 			g_dbus_send_message(dbus_conn, reply);
-			bonding_request_cancel(device->bonding);
-			bonding_request_free(device->bonding);
+			bonding_request_cancel(dev->bonding);
+			bonding_request_free(dev->bonding);
 		}
-	} else {
-		BtIOSecLevel sec_level;
 
-		if (device->paired)
-			sec_level = BT_IO_SEC_MEDIUM;
-		else
-			sec_level = BT_IO_SEC_LOW;
-
-		io = bt_io_connect(att_connect_cb,
-				attcb, NULL, &gerr,
-				BT_IO_OPT_SOURCE_BDADDR,
-				adapter_get_address(adapter),
-				BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
-				BT_IO_OPT_DEST_TYPE, device->bdaddr_type,
-				BT_IO_OPT_CID, ATT_CID,
-				BT_IO_OPT_SEC_LEVEL, sec_level,
-				BT_IO_OPT_INVALID);
-	}
-
-	if (io == NULL) {
 		error("ATT bt_io_connect(%s): %s", addr, gerr->message);
 		g_error_free(gerr);
 		g_free(attcb);
 		return NULL;
 	}
 
-	device->att_io = io;
+	/* Keep this, so we can cancel the connection */
+	dev->att_io = g_io_channel_ref(io);
 
-	return g_io_channel_ref(io);
+	return io;
 }
 
 static void att_browse_error_cb(const GError *gerr, gpointer user_data)
diff --git a/src/device.h b/src/device.h
index 6c35ff3..5438b0d 100644
--- a/src/device.h
+++ b/src/device.h
@@ -108,7 +108,8 @@ int device_unblock(struct btd_device *device, gboolean silent,
 							gboolean update_only);
 void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
 			uint16_t vendor, uint16_t product, uint16_t version);
-GIOChannel *device_att_connect(gpointer user_data);
+
+GIOChannel *device_att_connect(struct btd_device *dev);
 
 struct btd_profile;
 
-- 
1.8.1.1

[RFC BlueZ 4/6] device: Rename device_att_connect to device_connect_le

[Vinicius Costa Gomes]

This makes it clearer that this (currently unused) function should
only be used with LE devices.
---
 src/device.c | 2 +-
 src/device.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/device.c b/src/device.c
index 89df161..c76fb2b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -3120,7 +3120,7 @@ static void att_success_cb(gpointer user_data)
 	g_slist_foreach(device->attios, attio_connected, device->attrib);
 }
 
-GIOChannel *device_att_connect(struct btd_device *dev)
+GIOChannel *device_connect_le(struct btd_device *dev)
 {
 	struct btd_adapter *adapter = dev->adapter;
 	struct att_callbacks *attcb;
diff --git a/src/device.h b/src/device.h
index 5438b0d..a86fd59 100644
--- a/src/device.h
+++ b/src/device.h
@@ -109,7 +109,7 @@ int device_unblock(struct btd_device *device, gboolean silent,
 void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
 			uint16_t vendor, uint16_t product, uint16_t version);
 
-GIOChannel *device_att_connect(struct btd_device *dev);
+GIOChannel *device_connect_le(struct btd_device *dev);
 
 struct btd_profile;
 
-- 
1.8.1.1

Re: [RFC BlueZ 4/6] device: Rename device_att_connect to device_connect_le

[Johan Hedberg]

Hi Vinicius,

On Fri, Jan 25, 2013, Vinicius Costa Gomes wrote:
> This makes it clearer that this (currently unused) function should
> only be used with LE devices.
> ---
>  src/device.c | 2 +-
>  src/device.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Patches 3 and 4 have been applied.

Johan

[RFC BlueZ 5/6] core: Add a way to connect to LE devices

[Vinicius Costa Gomes]

adapter_create_le_connection() implements the Direct Connection
Procedure, we will use it when the device is in range, and we want to
re-connect to it and also when we want to pair to devices.

When pairing and the device is out of range, any attempt of connection will
fail until the connection times out (the controller sends the Connection
Complete event with an error).
---
 src/adapter.c | 22 ++++++++++++++++++++--
 src/adapter.h |  3 +++
 src/device.c  | 10 ++++------
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 5fd5062..e5bcb15 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4054,8 +4054,8 @@ static void update_found_devices(struct btd_adapter *adapter,
 									dev);
 
 done:
-	if (device_is_le(dev) && g_slist_find(adapter->connect_list, dev)) {
-	}
+	if (device_is_le(dev) && g_slist_find(adapter->connect_list, dev))
+		adapter_create_le_connection(adapter, dev);
 }
 
 static void device_found_callback(uint16_t index, uint16_t length,
@@ -4710,6 +4710,24 @@ static void pin_code_request_callback(uint16_t index, uint16_t length,
 	}
 }
 
+int adapter_create_le_connection(struct btd_adapter *adapter,
+						struct btd_device *dev)
+{
+	GIOChannel *io;
+
+	DBG("");
+
+	io = device_connect_le(dev);
+	if (io == NULL)
+		return -EBUSY;
+
+	g_io_channel_unref(io);
+
+	adapter_connect_list_remove(adapter, dev);
+
+	return 0;
+}
+
 int adapter_cancel_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
 							 uint8_t addr_type)
 {
diff --git a/src/adapter.h b/src/adapter.h
index 6e1ba9d..155e3f0 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -166,6 +166,9 @@ int btd_adapter_passkey_reply(struct btd_adapter *adapter,
 				const bdaddr_t *bdaddr, uint8_t bdaddr_type,
 				uint32_t passkey);
 
+int adapter_create_le_connection(struct btd_adapter *adapter,
+						struct btd_device *dev);
+
 int adapter_create_bonding(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
 					uint8_t addr_type, uint8_t io_cap);
 
diff --git a/src/device.c b/src/device.c
index c76fb2b..2bf1ad9 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1411,12 +1411,10 @@ static DBusMessage *pair_device(DBusConnection *conn, DBusMessage *msg,
 	device->bonding = bonding;
 	bonding->device = device;
 
-	if (device_is_le(device) && !device_is_connected(device)) {
-		adapter_connect_list_add(adapter, device);
-		return NULL;
-	}
-
-	err = adapter_create_bonding(adapter, &device->bdaddr,
+	if (device_is_le(device) && !device_is_connected(device))
+		err = adapter_create_le_connection(adapter, device);
+	else
+		err = adapter_create_bonding(adapter, &device->bdaddr,
 					device->bdaddr_type, io_cap);
 	if (err < 0)
 		return btd_error_failed(msg, strerror(-err));
-- 
1.8.1.1

[RFC BlueZ 6/6] gas: Move all the code to only one file

[Vinicius Costa Gomes]

Our Generic Attribute/Access Service plugin is small and simple enough
to be kept in only one file.
---
 Makefile.plugins        |  4 +--
 profiles/gatt/gas.c     | 46 ++++++++++++++++++++++++++++
 profiles/gatt/main.c    | 47 -----------------------------
 profiles/gatt/manager.c | 79 -------------------------------------------------
 profiles/gatt/manager.h | 24 ---------------
 5 files changed, 47 insertions(+), 153 deletions(-)
 delete mode 100644 profiles/gatt/main.c
 delete mode 100644 profiles/gatt/manager.c
 delete mode 100644 profiles/gatt/manager.h

diff --git a/Makefile.plugins b/Makefile.plugins
index faab011..f497782 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -69,9 +69,7 @@ builtin_sources += profiles/health/mcap_lib.h profiles/health/mcap_internal.h \
 endif
 
 builtin_modules += gatt
-builtin_sources += profiles/gatt/main.c profiles/gatt/manager.h \
-			profiles/gatt/manager.c profiles/gatt/gas.h \
-			profiles/gatt/gas.c
+builtin_sources += profiles/gatt/gas.c
 
 builtin_modules += scanparam
 builtin_sources += profiles/scanparam/scan.c
diff --git a/profiles/gatt/gas.c b/profiles/gatt/gas.c
index 429850b..31281b7 100644
--- a/profiles/gatt/gas.c
+++ b/profiles/gatt/gas.c
@@ -35,8 +35,10 @@
 #include <btio/btio.h>
 
 #include "lib/uuid.h"
+#include "plugin.h"
 #include "adapter.h"
 #include "device.h"
+#include "profile.h"
 #include "attrib/att.h"
 #include "attrib/gattrib.h"
 #include "attio.h"
@@ -405,3 +407,47 @@ void gas_unregister(struct btd_device *device)
 	devices = g_slist_remove(devices, gas);
 	gas_free(gas);
 }
+
+static int gatt_driver_probe(struct btd_profile *p, struct btd_device *device,
+								GSList *uuids)
+{
+	struct gatt_primary *gap, *gatt;
+
+	gap = btd_device_get_primary(device, GAP_UUID);
+	gatt = btd_device_get_primary(device, GATT_UUID);
+
+	if (gap == NULL || gatt == NULL) {
+		error("GAP and GATT are mandatory");
+		return -EINVAL;
+	}
+
+	return gas_register(device, &gap->range, &gatt->range);
+}
+
+static void gatt_driver_remove(struct btd_profile *p,
+						struct btd_device *device)
+{
+	gas_unregister(device);
+}
+
+static struct btd_profile gatt_profile = {
+	.name		= "gap-gatt-profile",
+	.remote_uuids	= BTD_UUIDS(GAP_UUID, GATT_UUID),
+	.device_probe	= gatt_driver_probe,
+	.device_remove	= gatt_driver_remove
+};
+
+static int gatt_init(void)
+{
+	btd_profile_register(&gatt_profile);
+
+	return 0;
+}
+
+static void gatt_exit(void)
+{
+	btd_profile_register(&gatt_profile);
+}
+
+BLUETOOTH_PLUGIN_DEFINE(gatt, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
+					gatt_init, gatt_exit)
diff --git a/profiles/gatt/main.c b/profiles/gatt/main.c
deleted file mode 100644
index ecd4455..0000000
--- a/profiles/gatt/main.c
+++ /dev/null
@@ -1,47 +0,0 @@
-/*
- *
- *  BlueZ - Bluetooth protocol stack for Linux
- *
- *  Copyright (C) 2012  Instituto Nokia de Tecnologia - INdT
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- *
- */
-
-#ifdef HAVE_CONFIG_H
-#include <config.h>
-#endif
-
-#include <stdint.h>
-#include <glib.h>
-#include <errno.h>
-
-#include "plugin.h"
-#include "manager.h"
-#include "hcid.h"
-#include "log.h"
-
-static int gatt_init(void)
-{
-	return gatt_manager_init();
-}
-
-static void gatt_exit(void)
-{
-	gatt_manager_exit();
-}
-
-BLUETOOTH_PLUGIN_DEFINE(gatt, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
-					gatt_init, gatt_exit)
diff --git a/profiles/gatt/manager.c b/profiles/gatt/manager.c
deleted file mode 100644
index 2f2bd14..0000000
--- a/profiles/gatt/manager.c
+++ /dev/null
@@ -1,79 +0,0 @@
-/*
- *
- *  BlueZ - Bluetooth protocol stack for Linux
- *
- *  Copyright (C) 2012  Instituto Nokia de Tecnologia - INdT
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- *
- */
-
-#ifdef HAVE_CONFIG_H
-#include "config.h"
-#endif
-
-#include <glib.h>
-#include <errno.h>
-#include <stdbool.h>
-
-#include "lib/uuid.h"
-#include "adapter.h"
-#include "device.h"
-#include "profile.h"
-#include "attrib/att.h"
-#include "attrib/gattrib.h"
-#include "attrib/gatt.h"
-#include "gas.h"
-#include "log.h"
-#include "manager.h"
-
-static int gatt_driver_probe(struct btd_profile *p, struct btd_device *device,
-								GSList *uuids)
-{
-	struct gatt_primary *gap, *gatt;
-
-	gap = btd_device_get_primary(device, GAP_UUID);
-	gatt = btd_device_get_primary(device, GATT_UUID);
-
-	if (gap == NULL || gatt == NULL) {
-		error("GAP and GATT are mandatory");
-		return -EINVAL;
-	}
-
-	return gas_register(device, &gap->range, &gatt->range);
-}
-
-static void gatt_driver_remove(struct btd_profile *p,
-						struct btd_device *device)
-{
-	gas_unregister(device);
-}
-
-static struct btd_profile gatt_profile = {
-	.name		= "gap-gatt-profile",
-	.remote_uuids	= BTD_UUIDS(GAP_UUID, GATT_UUID),
-	.device_probe	= gatt_driver_probe,
-	.device_remove	= gatt_driver_remove
-};
-
-int gatt_manager_init(void)
-{
-	return btd_profile_register(&gatt_profile);
-}
-
-void gatt_manager_exit(void)
-{
-	btd_profile_unregister(&gatt_profile);
-}
diff --git a/profiles/gatt/manager.h b/profiles/gatt/manager.h
deleted file mode 100644
index 502fceb..0000000
--- a/profiles/gatt/manager.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- *
- *  BlueZ - Bluetooth protocol stack for Linux
- *
- *  Copyright (C) 2012  Instituto Nokia de Tecnologia - INdT
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- *
- */
-
-int gatt_manager_init(void);
-void gatt_manager_exit(void);
-- 
1.8.1.1

Re: [RFC BlueZ 6/6] gas: Move all the code to only one file

[Johan Hedberg]

Hi Vinicius,

On Fri, Jan 25, 2013, Vinicius Costa Gomes wrote:
> Our Generic Attribute/Access Service plugin is small and simple enough
> to be kept in only one file.
> ---
>  Makefile.plugins        |  4 +--
>  profiles/gatt/gas.c     | 46 ++++++++++++++++++++++++++++
>  profiles/gatt/main.c    | 47 -----------------------------
>  profiles/gatt/manager.c | 79 -------------------------------------------------
>  profiles/gatt/manager.h | 24 ---------------
>  5 files changed, 47 insertions(+), 153 deletions(-)
>  delete mode 100644 profiles/gatt/main.c
>  delete mode 100644 profiles/gatt/manager.c
>  delete mode 100644 profiles/gatt/manager.h
> 
> diff --git a/Makefile.plugins b/Makefile.plugins
> index faab011..f497782 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -69,9 +69,7 @@ builtin_sources += profiles/health/mcap_lib.h profiles/health/mcap_internal.h \
>  endif
>  
>  builtin_modules += gatt
> -builtin_sources += profiles/gatt/main.c profiles/gatt/manager.h \
> -			profiles/gatt/manager.c profiles/gatt/gas.h \
> -			profiles/gatt/gas.c
> +builtin_sources += profiles/gatt/gas.c

You seem to have forgotten to remove gas.h in this patch. You remove it
from Makefile.plugins but it's still in the tree and gas.c includes it.
You'll also want to make gas_register/unregister static together with
removing the header file.

Johan

Re: [RFC BlueZ 6/6] gas: Move all the code to only one file

[Anderson Lizardo]

Hi Vinicius,

On Thu, Jan 24, 2013 at 11:07 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> +static int gatt_init(void)
> +{
> +       btd_profile_register(&gatt_profile);
> +
> +       return 0;
> +}
> +
> +static void gatt_exit(void)
> +{
> +       btd_profile_register(&gatt_profile);

It should be btd_profile_unregister() here.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [RFC BlueZ 0/6] LE Connection Establishment fixes

[Johan Hedberg]

Hi Vinicius,

On Fri, Jan 25, 2013, Vinicius Costa Gomes wrote:
> The only problem with this aproach that I found while testing is the
> problem exposed on the message of commit 5/6: If the remote device is
> out of range during pairing any connection attempt will fail until the
> Connection Complete event comes (with an error). This is the reason
> that we put the devices in the connect list before pairing.
> 
> Is this reason enough to have the device put in the connect list
> during pairing?

I don't think so. The mgmt_pair_device should have a timeout for LE
devices on the kernel side, but since this is not the case with current
kernel versions we'll need a workaround in user space. I.e. instead of
using scanning (I assume that's what putting it on the connect list will
do) you should just use a simple timeout (g_timeout_add_seconds) and
call mgmt_cancel_pair_device if it expires.

Johan

Re: [RFC BlueZ 0/6] LE Connection Establishment fixes

[Johan Hedberg]

Hi Vinicius,

On Fri, Jan 25, 2013, Johan Hedberg wrote:
> On Fri, Jan 25, 2013, Vinicius Costa Gomes wrote:
> > The only problem with this aproach that I found while testing is the
> > problem exposed on the message of commit 5/6: If the remote device is
> > out of range during pairing any connection attempt will fail until the
> > Connection Complete event comes (with an error). This is the reason
> > that we put the devices in the connect list before pairing.
> > 
> > Is this reason enough to have the device put in the connect list
> > during pairing?
> 
> I don't think so. The mgmt_pair_device should have a timeout for LE
> devices on the kernel side, but since this is not the case with current
> kernel versions we'll need a workaround in user space. I.e. instead of
> using scanning (I assume that's what putting it on the connect list will
> do) you should just use a simple timeout (g_timeout_add_seconds) and
> call mgmt_cancel_pair_device if it expires.

I realized that you were mainly talking about the socket timeout and not
mgmt_pair_device timeout. The socket does have a proper kernel side
timeout while mgmt_pair_device doesn't (yet) so the latter needs
protecting (checking for connection status before deciding what to do is
racy so the protection needs to be there).

Anyway, I went ahead and fixed the pairing part and that's all I have
time for today. If there's still something needing fixing with the
reconnections feel free to send further patches.

Johan

Re: [RFC BlueZ 0/6] LE Connection Establishment fixes

[Marcel Holtmann]

Hi Vinicius,

> Sending this as a RFC because I don't know if this is what you guys
> had in mind.
> 
> Patches 1/6/ and 2/6 are simple enough fixes and should be considered
> for inclusion, perhaps 6/6 as well, but the improvement is
> non-functional.
> 
> The only problem with this aproach that I found while testing is the
> problem exposed on the message of commit 5/6: If the remote device is
> out of range during pairing any connection attempt will fail until the
> Connection Complete event comes (with an error). This is the reason
> that we put the devices in the connect list before pairing.
> 
> Is this reason enough to have the device put in the connect list
> during pairing?

such a reasoning is pretty much wrong on all levels. An out of range
device might never come back in range and you end up with a device on
the connect list forever. And if it ever comes back, we end up with some
unexpected connection attempt to that device.

The only sane approach would have been to add a timeout and just abort
the pairing attempt after it. Everything else is just trying to fix a
symptom and making it actually worse while doing so.

Regards

Marcel