[PATCHv2 0/4] Add support for bonding callbacks and retrying

[PATCHv2 0/4] Add support for bonding callbacks and retrying

[Scott James Remnant]

Thanks for the comments on the autopair plugin patches, I'm still
working on updating that but I wanted to submit the underlying core
changes necessary while I do so.

This adds plugin support for a callback called when bonding completes,
either successfully or fails, or is cancelled. In the success or failure
cases the callback may return TRUE, in which case the bonding is retried
after a short backoff period.

The (to be submitted) autopair plugin will use this to retry bonding if
a fixed PIN it provides fails, and ignore the subsequent attempt so that
the ordinary PIN handling (user agent, keyboard auto-generation, etc.)
happens.

The existing wiimote plugin could use this to try both of the possible
PINs (host address and peer address), etc.

Scott James Remnant (4):
  Add support for retrying a bonding
  plugin: Add bonding callback support for plugins
  bonding: retry if callback returns TRUE
  bonding: call plugin callback on cancellation

 src/adapter.c |    2 +-
 src/device.c  |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h  |    9 ++++++
 3 files changed, 97 insertions(+), 1 deletions(-)

-- 
1.7.7.3

[PATCHv2 1/4] Add support for retrying a bonding

[Scott James Remnant]

In order to retry a bonding we need a timer that will perform the
retry, we need to stash the status and capability of the bonding
request so it can use them again, and in the case of a retrying
bonding attempt we need to not tear down the temporary D-Bus device
object on the adapter.
---
 src/adapter.c |    2 +-
 src/device.c  |   14 ++++++++++++++
 src/device.h  |    1 +
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index d5075db..a606174 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3072,7 +3072,7 @@ void adapter_remove_connection(struct btd_adapter *adapter,
 	if (device_is_authenticating(device))
 		device_cancel_authentication(device, TRUE);
 
-	if (device_is_temporary(device)) {
+	if (device_is_temporary(device) && !device_is_retrying(device)) {
 		const char *path = device_get_path(device);
 
 		DBG("Removing temporary device %s", path);
diff --git a/src/device.c b/src/device.c
index c19acd4..278ad4b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -86,6 +86,9 @@ struct bonding_req {
 	GIOChannel *io;
 	guint listener_id;
 	struct btd_device *device;
+	uint8_t capability;
+	uint8_t status;
+	guint retry_timer;
 };
 
 struct authentication_req {
@@ -2176,6 +2179,9 @@ static void bonding_request_free(struct bonding_req *bonding)
 	if (bonding->io)
 		g_io_channel_unref(bonding->io);
 
+	if (bonding->retry_timer)
+		g_source_remove(bonding->retry_timer);
+
 	device = bonding->device;
 	g_free(bonding);
 
@@ -2247,6 +2253,7 @@ proceed:
 
 	bonding->conn = dbus_connection_ref(conn);
 	bonding->msg = dbus_message_ref(msg);
+	bonding->capability = capability;
 
 	return bonding;
 }
@@ -2330,6 +2337,13 @@ static void device_auth_req_free(struct btd_device *device)
 	device->authr = NULL;
 }
 
+gboolean device_is_retrying(struct btd_device *device)
+{
+	struct bonding_req *bonding = device->bonding;
+
+	return bonding && bonding->retry_timer != 0;
+}
+
 void device_bonding_complete(struct btd_device *device, uint8_t status)
 {
 	struct bonding_req *bonding = device->bonding;
diff --git a/src/device.h b/src/device.h
index 13005ae..5b1152b 100644
--- a/src/device.h
+++ b/src/device.h
@@ -73,6 +73,7 @@ void device_set_temporary(struct btd_device *device, gboolean temporary);
 void device_set_bonded(struct btd_device *device, gboolean bonded);
 void device_set_auto_connect(struct btd_device *device, gboolean enable);
 gboolean device_is_connected(struct btd_device *device);
+gboolean device_is_retrying(struct btd_device *device);
 DBusMessage *device_create_bonding(struct btd_device *device,
 				DBusConnection *conn, DBusMessage *msg,
 				const char *agent_path, uint8_t capability);
-- 
1.7.7.3

[PATCHv2 2/4] plugin: Add bonding callback support for plugins

[Scott James Remnant]

Allow plugins to register a bonding callback on a device, this will be
called on completion or cancellation of a bonding attempt on that
device and allow retrying of the bonding attempt.

These callbacks will only be called once, in the case of retrying the
callback must be registered again separately from another callback
(e.g. the pincode callback).
---
 src/device.c |   17 +++++++++++++++++
 src/device.h |    8 ++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 278ad4b..4e11447 100644
--- a/src/device.c
+++ b/src/device.c
@@ -137,6 +137,7 @@ struct btd_device {
 	GSList		*primaries;		/* List of primary services */
 	GSList		*drivers;		/* List of device drivers */
 	GSList		*watches;		/* List of disconnect_data */
+	GSList          *bonding_callbacks;	/* List of bonding callbacks */
 	gboolean	temporary;
 	struct agent	*agent;
 	guint		disconn_timer;
@@ -235,6 +236,8 @@ static void device_free(gpointer user_data)
 	g_slist_free_full(device->attios, g_free);
 	g_slist_free_full(device->attios_offline, g_free);
 
+	g_slist_free(device->bonding_callbacks);
+
 	g_attrib_unref(device->attrib);
 
 	if (device->tmp_records)
@@ -2337,6 +2340,20 @@ static void device_auth_req_free(struct btd_device *device)
 	device->authr = NULL;
 }
 
+void btd_device_register_bonding_cb(struct btd_device *device,
+						btd_device_bonding_cb_t cb)
+{
+	device->bonding_callbacks = g_slist_prepend(
+						device->bonding_callbacks, cb);
+}
+
+void btd_device_unregister_bonding_cb(struct btd_device *device,
+						btd_device_bonding_cb_t cb)
+{
+	device->bonding_callbacks = g_slist_remove(
+						device->bonding_callbacks, cb);
+}
+
 gboolean device_is_retrying(struct btd_device *device)
 {
 	struct bonding_req *bonding = device->bonding;
diff --git a/src/device.h b/src/device.h
index 5b1152b..6dcbb5c 100644
--- a/src/device.h
+++ b/src/device.h
@@ -101,6 +101,14 @@ guint device_add_disconnect_watch(struct btd_device *device,
 void device_remove_disconnect_watch(struct btd_device *device, guint id);
 void device_set_class(struct btd_device *device, uint32_t value);
 
+typedef gboolean (*btd_device_bonding_cb_t) (struct btd_device *device,
+					gboolean complete, uint8_t status);
+
+void btd_device_register_bonding_cb(struct btd_device *dev,
+						btd_device_bonding_cb_t cb);
+void btd_device_unregister_bonding_cb(struct btd_device *dev,
+						btd_device_bonding_cb_t cb);
+
 #define BTD_UUIDS(args...) ((const char *[]) { args, NULL } )
 
 struct btd_device_driver {
-- 
1.7.7.3

[PATCHv2 3/4] bonding: retry if callback returns TRUE

[Scott James Remnant]

When a bonding completes, pass the status to any plugin bonding
callbacks; if any return TRUE than set a timer to retry the bonding
after an appropriate backoff period.
---
 src/device.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index 4e11447..c36173d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2354,6 +2354,44 @@ void btd_device_unregister_bonding_cb(struct btd_device *device,
 						device->bonding_callbacks, cb);
 }
 
+static gboolean device_bonding_retry(gpointer data)
+{
+	struct btd_device *device = data;
+	struct btd_adapter *adapter = device_get_adapter(device);
+	struct bonding_req *bonding = device->bonding;
+	int err;
+
+	if (!bonding)
+		return FALSE;
+
+	DBG("retrying bonding");
+	err = adapter_create_bonding(adapter, &device->bdaddr,
+							bonding->capability);
+	if (err < 0)
+		device_bonding_complete(device, bonding->status);
+
+	bonding->retry_timer = 0;
+	return FALSE;
+}
+
+static gboolean device_bonding_get_retry(struct btd_device *device,
+								uint8_t status)
+{
+	GSList *l;
+	btd_device_bonding_cb_t cb;
+	gboolean retry = FALSE;
+
+	for (l = device->bonding_callbacks; l != NULL; l = g_slist_next(l)) {
+		cb = l->data;
+		retry |= cb(device, TRUE, status);
+	}
+
+	g_slist_free(device->bonding_callbacks);
+	device->bonding_callbacks = NULL;
+
+	return retry;
+}
+
 gboolean device_is_retrying(struct btd_device *device)
 {
 	struct bonding_req *bonding = device->bonding;
@@ -2368,6 +2406,14 @@ void device_bonding_complete(struct btd_device *device, uint8_t status)
 
 	DBG("bonding %p status 0x%02x", bonding, status);
 
+	if (device_bonding_get_retry(device, status) && status) {
+		DBG("backing off and retrying");
+		bonding->status = status;
+		bonding->retry_timer = g_timeout_add(3000,
+						device_bonding_retry, device);
+		return;
+	}
+
 	if (auth && auth->type == AUTH_TYPE_NOTIFY && auth->agent)
 		agent_cancel(auth->agent);
 
-- 
1.7.7.3

[PATCHv2 4/4] bonding: call plugin callback on cancellation

[Scott James Remnant]

Call the plugin callbacks when a bonding request is cancelled.
---
 src/device.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/src/device.c b/src/device.c
index c36173d..67537fc 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2504,6 +2504,8 @@ void device_cancel_bonding(struct btd_device *device, uint8_t status)
 	struct bonding_req *bonding = device->bonding;
 	DBusMessage *reply;
 	char addr[18];
+	GSList *l;
+	btd_device_bonding_cb_t cb;
 
 	if (!bonding)
 		return;
@@ -2511,6 +2513,14 @@ void device_cancel_bonding(struct btd_device *device, uint8_t status)
 	ba2str(&device->bdaddr, addr);
 	DBG("Canceling bonding request for %s", addr);
 
+	for (l = device->bonding_callbacks; l != NULL; l = g_slist_next(l)) {
+		cb = l->data;
+		cb(device, FALSE, 0);
+	}
+
+	g_slist_free(device->bonding_callbacks);
+	device->bonding_callbacks = NULL;
+
 	if (device->authr)
 		device_cancel_authentication(device, FALSE);
 
-- 
1.7.7.3

Re: [PATCHv2 0/4] Add support for bonding callbacks and retrying

[Marcel Holtmann]

Hi Scott,

> Thanks for the comments on the autopair plugin patches, I'm still
> working on updating that but I wanted to submit the underlying core
> changes necessary while I do so.
> 
> This adds plugin support for a callback called when bonding completes,
> either successfully or fails, or is cancelled. In the success or failure
> cases the callback may return TRUE, in which case the bonding is retried
> after a short backoff period.
> 
> The (to be submitted) autopair plugin will use this to retry bonding if
> a fixed PIN it provides fails, and ignore the subsequent attempt so that
> the ordinary PIN handling (user agent, keyboard auto-generation, etc.)
> happens.

yesterday Johan and talked about this a little bit and I just wanna
quickly iterate some small comments here.

So we should only allow retrying when we initiated the bonding. If the
other side started the pairing, then retrying should not even be
considered.

We have to also ensure that we do not disconnect the ACL in between the
retry attempts. Otherwise some car kits might cancel their pairing
procedure and you have to have user interaction to get it back into
pairing mode. So if the ACL gets disconnect, then we should just fail
and cancel the bonding.

For some extra credit we might need to retry the pairing in the kernel
if we happen to run into an LMP collision due to role switch changes or
similar issues. And hide that from the user if this happens.

Regards

Marcel

Re: [PATCHv2 0/4] Add support for bonding callbacks and retrying

[Scott James Remnant]

On Mon, Jan 30, 2012 at 11:41 AM, Marcel Holtmann <marcel@holtmann.org> wro=
te:

> Hi Scott,
>
Hey! How's it going?

> > This adds plugin support for a callback called when bonding completes,
> > either successfully or fails, or is cancelled. In the success or failur=
e
> > cases the callback may return TRUE, in which case the bonding is retrie=
d
> > after a short backoff period.
> >
> yesterday Johan and talked about this a little bit and I just wanna
> quickly iterate some small comments here.
>
Great!

> So we should only allow retrying when we initiated the bonding. If the
> other side started the pairing, then retrying should not even be
> considered.
>
I thought I put a check in for that, but I've just realized that after
I separated out the autopair and the keyboard pairing stuff, that
check ended up only in the keyboard pairing side.

My intent was this entire path should be only when the host initiated
the bonding, not the client.

> We have to also ensure that we do not disconnect the ACL in between the
> retry attempts. Otherwise some car kits might cancel their pairing
> procedure and you have to have user interaction to get it back into
> pairing mode. So if the ACL gets disconnect, then we should just fail
> and cancel the bonding.
>
In my testing, OS X/iOS drop the ACL between retry attempts - so we
wouldn't be any worse than they are. I'm not sure whether it's even
possible in bluez to avoid dropping the ACL?

Scott
--
Have you ever, ever felt like this?
Had strange things happen? =A0Are you going round the twist?

Re: [PATCHv2 0/4] Add support for bonding callbacks and retrying

[Marcel Holtmann]

Hi Scott,

> > > This adds plugin support for a callback called when bonding completes,
> > > either successfully or fails, or is cancelled. In the success or failure
> > > cases the callback may return TRUE, in which case the bonding is retried
> > > after a short backoff period.
> > >
> > yesterday Johan and talked about this a little bit and I just wanna
> > quickly iterate some small comments here.
> >
> Great!
> 
> > So we should only allow retrying when we initiated the bonding. If the
> > other side started the pairing, then retrying should not even be
> > considered.
> >
> I thought I put a check in for that, but I've just realized that after
> I separated out the autopair and the keyboard pairing stuff, that
> check ended up only in the keyboard pairing side.
> 
> My intent was this entire path should be only when the host initiated
> the bonding, not the client.

to be honest, I did not get further into actually checking the code
itself. I just wanted to ensure we are on the same and expecting how
this is suppose to work ;)

> > We have to also ensure that we do not disconnect the ACL in between the
> > retry attempts. Otherwise some car kits might cancel their pairing
> > procedure and you have to have user interaction to get it back into
> > pairing mode. So if the ACL gets disconnect, then we should just fail
> > and cancel the bonding.
> >
> In my testing, OS X/iOS drop the ACL between retry attempts - so we
> wouldn't be any worse than they are. I'm not sure whether it's even
> possible in bluez to avoid dropping the ACL?

Is Apple doing retried pairing (or auto-pairing) as well?

Strictly speaking the ACL disconnect is host stack triggered. So you
have HCI_Authentication_Requested and you can just execute that again
with the same link. We also do have a ACL disconnect timeout handling of
2 seconds that will allow sockets to re-use the same ACL link. Can you
provide some hcidump traces from your test cases?

Regards

Marcel

Re: [PATCHv2 0/4] Add support for bonding callbacks and retrying

[Scott James Remnant]

On Mon, Jan 30, 2012 at 1:51 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:

>> > We have to also ensure that we do not disconnect the ACL in between th=
e
>> > retry attempts. Otherwise some car kits might cancel their pairing
>> > procedure and you have to have user interaction to get it back into
>> > pairing mode. So if the ACL gets disconnect, then we should just fail
>> > and cancel the bonding.
>> >
>> In my testing, OS X/iOS drop the ACL between retry attempts - so we
>> wouldn't be any worse than they are. I'm not sure whether it's even
>> possible in bluez to avoid dropping the ACL?
>
> Is Apple doing retried pairing (or auto-pairing) as well?
>
They are; they send 0000 to the device, and then if it fails, it
retries after about 3s.

Android does the same (from the bluez Agent, ick)

> Strictly speaking the ACL disconnect is host stack triggered. So you
> have HCI_Authentication_Requested and you can just execute that again
> with the same link. We also do have a ACL disconnect timeout handling of
> 2 seconds that will allow sockets to re-use the same ACL link. Can you
> provide some hcidump traces from your test cases?
>
Sure, probably will be a few days though. I may have a play and see if
I can get it to avoid dropping the ACL link too.

Scott
--=20
Have you ever, ever felt like this?
Had strange things happen? =A0Are you going round the twist?

Re: [PATCHv2 0/4] Add support for bonding callbacks and retrying

[Marcel Holtmann]

Hi Scott,

> >> > We have to also ensure that we do not disconnect the ACL in between the
> >> > retry attempts. Otherwise some car kits might cancel their pairing
> >> > procedure and you have to have user interaction to get it back into
> >> > pairing mode. So if the ACL gets disconnect, then we should just fail
> >> > and cancel the bonding.
> >> >
> >> In my testing, OS X/iOS drop the ACL between retry attempts - so we
> >> wouldn't be any worse than they are. I'm not sure whether it's even
> >> possible in bluez to avoid dropping the ACL?
> >
> > Is Apple doing retried pairing (or auto-pairing) as well?
> >
> They are; they send 0000 to the device, and then if it fails, it
> retries after about 3s.

if possible get us some hcidump traces so we can see which side is
actually dropping the link. I am curious about the timing.

> Android does the same (from the bluez Agent, ick)
> 
> > Strictly speaking the ACL disconnect is host stack triggered. So you
> > have HCI_Authentication_Requested and you can just execute that again
> > with the same link. We also do have a ACL disconnect timeout handling of
> > 2 seconds that will allow sockets to re-use the same ACL link. Can you
> > provide some hcidump traces from your test cases?
> >
> Sure, probably will be a few days though. I may have a play and see if
> I can get it to avoid dropping the ACL link too.

If everybody else is fine with dropping the ACL link in between, then so
should we. We just might need a blacklist for some stupid carkits.

Regards

Marcel