[PATCH 0/5] Remote device cache support

[PATCH 0/5] Remote device cache support

[Szymon Janc]

Changes since RFC:
 - set cache limit to 300
 - update timestamp of cached device
 - rebased to master
 - other minor fixes

Szymon Janc (5):
  android/bluetooth: Split devices list to devices and bonded_devices
  android/bluetooth: Use defines for settings and devices files paths
  android/bluetooth: Always store device info
  android/bluetooth: Add support for caching remote device info
  android/bluetooth: Add support for loading caches devices from storage

 android/bluetooth.c | 202 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 149 insertions(+), 53 deletions(-)

-- 
1.8.3.2

[PATCH 1/5] android/bluetooth: Split devices list to devices and bonded_devices

[Szymon Janc]

From: Szymon Janc <szymon.janc@gmail.com>

Bonded devices are permament until unbondedn. Non-bonded devices will
be held in (size limited) cache based on timestamp property so split
list to ease separation.
---
 android/bluetooth.c | 47 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index 4849dab..8f08122 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -132,6 +132,8 @@ static const uint16_t uuid_list[] = {
 
 static uint16_t option_index = MGMT_INDEX_NONE;
 static struct mgmt *mgmt_if = NULL;
+
+static GSList *bonded_devices = NULL;
 static GSList *devices = NULL;
 
 /* This list contains addresses which are asked for records */
@@ -283,6 +285,10 @@ static struct device *find_device(const bdaddr_t *bdaddr)
 {
 	GSList *l;
 
+	l = g_slist_find_custom(bonded_devices, bdaddr, device_match);
+	if (l)
+		return l->data;
+
 	l = g_slist_find_custom(devices, bdaddr, device_match);
 	if (l)
 		return l->data;
@@ -559,12 +565,30 @@ static void set_device_bond_state(const bdaddr_t *addr, uint8_t status,
 	if (!dev)
 		return;
 
-	if (dev->bond_state != state) {
-		dev->bond_state = state;
-		send_bond_state_change(&dev->bdaddr, status, state);
+	if (dev->bond_state == state)
+		return;
 
-		store_device_info(dev);
+	switch (state) {
+	case HAL_BOND_STATE_NONE:
+		if (dev->bond_state == HAL_BOND_STATE_BONDED) {
+			bonded_devices = g_slist_remove(bonded_devices, dev);
+			devices = g_slist_prepend(devices, dev);
+		}
+		break;
+	case HAL_BOND_STATE_BONDED:
+		devices = g_slist_remove(devices, dev);
+		bonded_devices = g_slist_prepend(bonded_devices, dev);
+		break;
+	case HAL_BOND_STATE_BONDING:
+	default:
+		break;
 	}
+
+	dev->bond_state = state;
+
+	store_device_info(dev);
+
+	send_bond_state_change(&dev->bdaddr, status, state);
 }
 
 static  void send_device_property(const bdaddr_t *bdaddr, uint8_t type,
@@ -2128,18 +2152,15 @@ static uint8_t get_adapter_scan_mode(void)
 
 static uint8_t get_adapter_bonded_devices(void)
 {
-	uint8_t buf[sizeof(bdaddr_t) * g_slist_length(devices)];
+	uint8_t buf[sizeof(bdaddr_t) * g_slist_length(bonded_devices)];
 	int i = 0;
 	GSList *l;
 
 	DBG("");
 
-	for (l = devices; l; l = g_slist_next(l)) {
+	for (l = bonded_devices; l; l = g_slist_next(l)) {
 		struct device *dev = l->data;
 
-		if (dev->bond_state != HAL_BOND_STATE_BONDED)
-			continue;
-
 		bdaddr2android(&dev->bdaddr, buf + (i * sizeof(bdaddr_t)));
 		i++;
 	}
@@ -2691,11 +2712,10 @@ static void send_bonded_devices_props(void)
 {
 	GSList *l;
 
-	for (l = devices; l; l = g_slist_next(l)) {
+	for (l = bonded_devices; l; l = g_slist_next(l)) {
 		struct device *dev = l->data;
 
-		if (dev->bond_state == HAL_BOND_STATE_BONDED)
-			get_remote_device_props(dev);
+		get_remote_device_props(dev);
 	}
 }
 
@@ -3093,6 +3113,9 @@ void bt_bluetooth_unregister(void)
 {
 	DBG("");
 
+	g_slist_free_full(bonded_devices, (GDestroyNotify) free_device);
+	bonded_devices = NULL;
+
 	g_slist_free_full(devices, (GDestroyNotify) free_device);
 	devices = NULL;
 
-- 
1.8.3.2

[PATCH 2/5] android/bluetooth: Use defines for settings and devices files paths

[Szymon Janc]

From: Szymon Janc <szymon.janc@gmail.com>

---
 android/bluetooth.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index 8f08122..3314267 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -54,6 +54,9 @@
 
 #define DUT_MODE_FILE "/sys/kernel/debug/bluetooth/hci%u/dut_mode"
 
+#define SETTINGS_FILE ANDROID_STORAGEDIR"/settings"
+#define DEVICES_FILE ANDROID_STORAGEDIR"/devices"
+
 #define DEVICE_ID_SOURCE	0x0002	/* USB */
 #define DEVICE_ID_VENDOR	0x1d6b	/* Linux Foundation */
 #define DEVICE_ID_PRODUCT	0x0247	/* BlueZ for Android */
@@ -148,8 +151,7 @@ static void store_adapter_config(void)
 
 	key_file = g_key_file_new();
 
-	g_key_file_load_from_file(key_file, ANDROID_STORAGEDIR"/settings", 0,
-									NULL);
+	g_key_file_load_from_file(key_file, SETTINGS_FILE, 0, NULL);
 
 	ba2str(&adapter.bdaddr, addr);
 
@@ -160,7 +162,7 @@ static void store_adapter_config(void)
 
 	data = g_key_file_to_data(key_file, &length, NULL);
 
-	g_file_set_contents(ANDROID_STORAGEDIR"/settings", data, length, NULL);
+	g_file_set_contents(SETTINGS_FILE, data, length, NULL);
 
 	g_free(data);
 	g_key_file_free(key_file);
@@ -173,8 +175,7 @@ static void load_adapter_config(void)
 	char *str;
 
 	key_file = g_key_file_new();
-	g_key_file_load_from_file(key_file, ANDROID_STORAGEDIR"/settings", 0,
-									NULL);
+	g_key_file_load_from_file(key_file, SETTINGS_FILE, 0, NULL);
 
 	str = g_key_file_get_string(key_file, "General", "Address", NULL);
 	if (!str) {
@@ -216,8 +217,7 @@ static void store_device_info(struct device *dev)
 	ba2str(&dev->bdaddr, addr);
 
 	key_file = g_key_file_new();
-	g_key_file_load_from_file(key_file, ANDROID_STORAGEDIR"/devices", 0,
-									NULL);
+	g_key_file_load_from_file(key_file, DEVICES_FILE, 0, NULL);
 
 	if (dev->bond_state == HAL_BOND_STATE_NONE) {
 		g_key_file_remove_group(key_file, addr, NULL);
@@ -266,7 +266,7 @@ static void store_device_info(struct device *dev)
 
 done:
 	str = g_key_file_to_data(key_file, &length, NULL);
-	g_file_set_contents(ANDROID_STORAGEDIR"/devices", str, length, NULL);
+	g_file_set_contents(DEVICES_FILE, str, length, NULL);
 	g_free(str);
 
 	g_key_file_free(key_file);
@@ -521,8 +521,7 @@ static void store_link_key(const bdaddr_t *dst, const uint8_t *key,
 
 	key_file = g_key_file_new();
 
-	if (!g_key_file_load_from_file(key_file, ANDROID_STORAGEDIR"/devices",
-								0, NULL))
+	if (!g_key_file_load_from_file(key_file, DEVICES_FILE, 0, NULL))
 		return;
 
 	ba2str(dst, addr);
@@ -537,7 +536,7 @@ static void store_link_key(const bdaddr_t *dst, const uint8_t *key,
 	g_key_file_set_integer(key_file, addr, "LinkKeyPinLength", pin_length);
 
 	data = g_key_file_to_data(key_file, &length, NULL);
-	g_file_set_contents(ANDROID_STORAGEDIR"/devices", data, length, NULL);
+	g_file_set_contents(DEVICES_FILE, data, length, NULL);
 	g_free(data);
 
 	g_key_file_free(key_file);
@@ -1726,8 +1725,7 @@ static void load_devices_info(bt_bluetooth_ready cb)
 
 	key_file = g_key_file_new();
 
-	g_key_file_load_from_file(key_file, ANDROID_STORAGEDIR"/devices", 0,
-									NULL);
+	g_key_file_load_from_file(key_file, DEVICES_FILE, 0, NULL);
 
 	devs = g_key_file_get_groups(key_file, &len);
 
-- 
1.8.3.2

Re: [PATCH 2/5] android/bluetooth: Use defines for settings and devices files paths

[Marcel Holtmann]

Hi Szymon,

> ---
> android/bluetooth.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/android/bluetooth.c b/android/bluetooth.c
> index 8f08122..3314267 100644
> --- a/android/bluetooth.c
> +++ b/android/bluetooth.c
> @@ -54,6 +54,9 @@
> 
> #define DUT_MODE_FILE "/sys/kernel/debug/bluetooth/hci%u/dut_mode"
> 
> +#define SETTINGS_FILE ANDROID_STORAGEDIR"/settings"
> +#define DEVICES_FILE ANDROID_STORAGEDIR"/devices”

is there any good reason to keep cached devices together with the bonded devices? I get the feeling a cached device should be safe to throw away. So deleting the cache file should not cause any problems. Meaning it should be a separate file.

Regards

Marcel

Re: [PATCH 2/5] android/bluetooth: Use defines for settings and devices files paths

[Szymon Janc]

Hi Marcel,

On Tuesday 21 January 2014 08:54:35 Marcel Holtmann wrote:
> Hi Szymon,
>=20
> > ---
> > android/bluetooth.c | 24 +++++++++++-------------
> > 1 file changed, 11 insertions(+), 13 deletions(-)
> >=20
> > diff --git a/android/bluetooth.c b/android/bluetooth.c
> > index 8f08122..3314267 100644
> > --- a/android/bluetooth.c
> > +++ b/android/bluetooth.c
> > @@ -54,6 +54,9 @@
> >=20
> > #define DUT_MODE_FILE "/sys/kernel/debug/bluetooth/hci%u/dut_mode"
> >=20
> > +#define SETTINGS_FILE ANDROID_STORAGEDIR"/settings"
> > +#define DEVICES_FILE ANDROID_STORAGEDIR"/devices=E2=80=9D
>=20
> is there any good reason to keep cached devices together with the bon=
ded
> devices? I get the feeling a cached device should be safe to throw aw=
ay. So
> deleting the cache file should not cause any problems. Meaning it sho=
uld be
> a separate file.

I'm fine with having separate files too (even have this implemented on =
my=20
local branch), but wanted to avoid handling multiple files. Anyway, add=
/remove=20
bond doesn't happen that often so maybe this is not a problem.

So, is "device_cache" name OK? Or just "cache"?

--=20
Szymon K. Janc
szymon.janc@gmail.com

Re: [PATCH 2/5] android/bluetooth: Use defines for settings and devices files paths

[Marcel Holtmann]

Hi Szymon,

>>> ---
>>> android/bluetooth.c | 24 +++++++++++-------------
>>> 1 file changed, 11 insertions(+), 13 deletions(-)
>>> 
>>> diff --git a/android/bluetooth.c b/android/bluetooth.c
>>> index 8f08122..3314267 100644
>>> --- a/android/bluetooth.c
>>> +++ b/android/bluetooth.c
>>> @@ -54,6 +54,9 @@
>>> 
>>> #define DUT_MODE_FILE "/sys/kernel/debug/bluetooth/hci%u/dut_mode"
>>> 
>>> +#define SETTINGS_FILE ANDROID_STORAGEDIR"/settings"
>>> +#define DEVICES_FILE ANDROID_STORAGEDIR"/devices”
>> 
>> is there any good reason to keep cached devices together with the bonded
>> devices? I get the feeling a cached device should be safe to throw away. So
>> deleting the cache file should not cause any problems. Meaning it should be
>> a separate file.
> 
> I'm fine with having separate files too (even have this implemented on my 
> local branch), but wanted to avoid handling multiple files. Anyway, add/remove 
> bond doesn't happen that often so maybe this is not a problem.
> 
> So, is "device_cache" name OK? Or just "cache”?

I would just do “cache” and see where it takes us.

Regards

Marcel

[PATCH 3/5] android/bluetooth: Always store device info

[Szymon Janc]

This allows to cache remote device informations.
---
 android/bluetooth.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index 3314267..f32dd91 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -207,23 +207,11 @@ static void store_device_info(struct device *dev)
 	char **uuids = NULL;
 	char *str;
 
-	/* We only store bonded devices and need to modify the storage
-	 * if the state is either NONE or BONDED.
-	 */
-	if (dev->bond_state != HAL_BOND_STATE_BONDED &&
-					dev->bond_state != HAL_BOND_STATE_NONE)
-		return;
-
 	ba2str(&dev->bdaddr, addr);
 
 	key_file = g_key_file_new();
 	g_key_file_load_from_file(key_file, DEVICES_FILE, 0, NULL);
 
-	if (dev->bond_state == HAL_BOND_STATE_NONE) {
-		g_key_file_remove_group(key_file, addr, NULL);
-		goto done;
-	}
-
 	g_key_file_set_integer(key_file, addr, "Type", dev->bdaddr_type);
 
 	g_key_file_set_string(key_file, addr, "Name", dev->name);
@@ -264,7 +252,6 @@ static void store_device_info(struct device *dev)
 		g_key_file_remove_key(key_file, addr, "Services", NULL);
 	}
 
-done:
 	str = g_key_file_to_data(key_file, &length, NULL);
 	g_file_set_contents(DEVICES_FILE, str, length, NULL);
 	g_free(str);
@@ -542,6 +529,33 @@ static void store_link_key(const bdaddr_t *dst, const uint8_t *key,
 	g_key_file_free(key_file);
 }
 
+static void remove_stored_link_key(const bdaddr_t *dst)
+{
+	GKeyFile *key_file;
+	gsize length = 0;
+	char addr[18];
+	char *data;
+
+	key_file = g_key_file_new();
+
+	if (!g_key_file_load_from_file(key_file, DEVICES_FILE, 0, NULL))
+		return;
+
+	ba2str(dst, addr);
+
+	DBG("%s", addr);
+
+	g_key_file_remove_key(key_file, addr, "LinkKey", NULL);
+	g_key_file_remove_key(key_file, addr, "LinkKeyType", NULL);
+	g_key_file_remove_key(key_file, addr, "LinkKeyPinLength", NULL);
+
+	data = g_key_file_to_data(key_file, &length, NULL);
+	g_file_set_contents(DEVICES_FILE, data, length, NULL);
+	g_free(data);
+
+	g_key_file_free(key_file);
+}
+
 static void send_bond_state_change(const bdaddr_t *addr, uint8_t status,
 								uint8_t state)
 {
@@ -2443,6 +2457,8 @@ static void unpair_device_complete(uint8_t status, uint16_t length,
 	if (status != MGMT_STATUS_SUCCESS)
 		return;
 
+	remove_stored_link_key(&rp->addr.bdaddr);
+
 	set_device_bond_state(&rp->addr.bdaddr, HAL_STATUS_SUCCESS,
 							HAL_BOND_STATE_NONE);
 }
-- 
1.8.3.2

[PATCH 4/5] android/bluetooth: Add support for caching remote device info

[Szymon Janc]

Cache is limited to DEVICES_CACHE_MAX. Devices are sorted with
timestamp so if cache is full olderst device is removed.
---
 android/bluetooth.c | 72 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 16 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index f32dd91..6afde35 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -70,6 +70,8 @@
 /* Default discoverable timeout 120sec as in Android */
 #define DEFAULT_DISCOVERABLE_TIMEOUT 120
 
+#define DEVICES_CACHE_MAX 300
+
 #define BASELEN_PROP_CHANGED (sizeof(struct hal_ev_adapter_props_changed) \
 					+ (sizeof(struct hal_property)))
 
@@ -260,6 +262,27 @@ static void store_device_info(struct device *dev)
 	g_strfreev(uuids);
 }
 
+static void remove_device_info(struct device *dev)
+{
+	GKeyFile *key_file;
+	gsize length = 0;
+	char addr[18];
+	char *str;
+
+	ba2str(&dev->bdaddr, addr);
+
+	key_file = g_key_file_new();
+	g_key_file_load_from_file(key_file, DEVICES_FILE, 0, NULL);
+
+	g_key_file_remove_group(key_file, addr, NULL);
+
+	str = g_key_file_to_data(key_file, &length, NULL);
+	g_file_set_contents(DEVICES_FILE, str, length, NULL);
+	g_free(str);
+
+	g_key_file_free(key_file);
+}
+
 static int device_match(gconstpointer a, gconstpointer b)
 {
 	const struct device *dev = a;
@@ -283,6 +306,35 @@ static struct device *find_device(const bdaddr_t *bdaddr)
 	return NULL;
 }
 
+static void free_device(struct device *dev)
+{
+	g_free(dev->name);
+	g_free(dev->friendly_name);
+	g_slist_free_full(dev->uuids, g_free);
+	g_free(dev);
+}
+
+static void cache_device(struct device *new_dev)
+{
+	struct device *dev;
+	GSList *l;
+
+	if (g_slist_length(devices) < DEVICES_CACHE_MAX)
+		goto done;
+
+	l = g_slist_last(devices);
+	dev = l->data;
+
+	devices = g_slist_remove(bonded_devices, dev);
+	remove_device_info(dev);
+	free_device(dev);
+
+done:
+	new_dev->timestamp = time(NULL);
+	devices = g_slist_prepend(devices, new_dev);
+	store_device_info(new_dev);
+}
+
 static struct device *create_device(const bdaddr_t *bdaddr, uint8_t type)
 {
 	struct device *dev;
@@ -301,19 +353,10 @@ static struct device *create_device(const bdaddr_t *bdaddr, uint8_t type)
 	/* use address for name, will be change if one is present
 	 * eg. in EIR or set by set_property. */
 	dev->name = g_strdup(addr);
-	devices = g_slist_prepend(devices, dev);
 
 	return dev;
 }
 
-static void free_device(struct device *dev)
-{
-	g_free(dev->name);
-	g_free(dev->friendly_name);
-	g_slist_free_full(dev->uuids, g_free);
-	g_free(dev);
-}
-
 static struct device *get_device(const bdaddr_t *bdaddr, uint8_t type)
 {
 	struct device *dev;
@@ -585,7 +628,8 @@ static void set_device_bond_state(const bdaddr_t *addr, uint8_t status,
 	case HAL_BOND_STATE_NONE:
 		if (dev->bond_state == HAL_BOND_STATE_BONDED) {
 			bonded_devices = g_slist_remove(bonded_devices, dev);
-			devices = g_slist_prepend(devices, dev);
+			remove_stored_link_key(&dev->bdaddr);
+			cache_device(dev);
 		}
 		break;
 	case HAL_BOND_STATE_BONDED:
@@ -599,8 +643,6 @@ static void set_device_bond_state(const bdaddr_t *addr, uint8_t status,
 
 	dev->bond_state = state;
 
-	store_device_info(dev);
-
 	send_bond_state_change(&dev->bdaddr, status, state);
 }
 
@@ -1071,8 +1113,6 @@ static void update_found_device(const bdaddr_t *bdaddr, uint8_t bdaddr_type,
 
 		ev->status = HAL_STATUS_SUCCESS;
 		bdaddr2android(bdaddr, ev->bdaddr);
-
-		dev->timestamp = time(NULL);
 	}
 
 	if (eir.class) {
@@ -1100,6 +1140,8 @@ static void update_found_device(const bdaddr_t *bdaddr, uint8_t bdaddr_type,
 		(*num_prop)++;
 	}
 
+	cache_device(dev);
+
 	if (*num_prop)
 		ipc_send_notif(HAL_SERVICE_ID_BLUETOOTH, opcode, size, buf);
 
@@ -2457,8 +2499,6 @@ static void unpair_device_complete(uint8_t status, uint16_t length,
 	if (status != MGMT_STATUS_SUCCESS)
 		return;
 
-	remove_stored_link_key(&rp->addr.bdaddr);
-
 	set_device_bond_state(&rp->addr.bdaddr, HAL_STATUS_SUCCESS,
 							HAL_BOND_STATE_NONE);
 }
-- 
1.8.3.2

[PATCH 5/5] android/bluetooth: Add support for loading caches devices from storage

[Szymon Janc]

Info is now stored for all devices and bond state depends on linkkey
being stored. Based on that devices loaded from storage are put either
to cache or to bonded_devices list.
---
 android/bluetooth.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index 6afde35..80490f1 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -1688,7 +1688,8 @@ static void clear_uuids(void)
 					sizeof(cp), &cp, NULL, NULL, NULL);
 }
 
-static void create_device_from_info(GKeyFile *key_file, const char *peer)
+static struct device *create_device_from_info(GKeyFile *key_file,
+							const char *peer)
 {
 	struct device *dev;
 	uint8_t type;
@@ -1703,7 +1704,7 @@ static void create_device_from_info(GKeyFile *key_file, const char *peer)
 	str2ba(peer, &bdaddr);
 	dev = create_device(&bdaddr, type);
 
-	dev->bond_state = HAL_BOND_STATE_BONDED;
+	dev->bond_state = HAL_BOND_STATE_NONE;
 
 	str = g_key_file_get_string(key_file, peer, "Name", NULL);
 	if (str) {
@@ -1739,6 +1740,8 @@ static void create_device_from_info(GKeyFile *key_file, const char *peer)
 
 		g_strfreev(uuids);
 	}
+
+	return dev;
 }
 
 static struct mgmt_link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
@@ -1771,6 +1774,14 @@ failed:
 	return info;
 }
 
+static int device_timestamp_cmp(gconstpointer  a, gconstpointer  b)
+{
+	const struct device *deva = a;
+	const struct device *devb = b;
+
+	return deva->timestamp < devb->timestamp;
+}
+
 static void load_devices_info(bt_bluetooth_ready cb)
 {
 	GKeyFile *key_file;
@@ -1787,16 +1798,24 @@ static void load_devices_info(bt_bluetooth_ready cb)
 
 	for (i = 0; i < len; i++) {
 		struct mgmt_link_key_info *key_info;
+		struct device *dev;
 
-		create_device_from_info(key_file, devs[i]);
+		dev = create_device_from_info(key_file, devs[i]);
 
 		key_info = get_key_info(key_file, devs[i]);
-		if (key_info)
+		if (key_info) {
 			keys = g_slist_prepend(keys, key_info);
+			bonded_devices = g_slist_prepend(bonded_devices, dev);
+			dev->bond_state = HAL_BOND_STATE_BONDED;
+		} else {
+			devices = g_slist_prepend(devices, dev);
+		}
 
 		/* TODO ltk */
 	}
 
+	devices = g_slist_sort(devices, device_timestamp_cmp);
+
 	load_link_keys(keys, cb);
 	g_strfreev(devs);
 	g_slist_free_full(keys, g_free);
-- 
1.8.3.2