[PATCH v3 00/10] Move adapter config file to ini-file format

[PATCH v3 00/10] Move adapter config file to ini-file format

[Frédéric Danis]

Adapter saved configuration will be saved to /var/lib/bluetooth/<adapter address>/settings
in ini-file format for BlueZ 5.
If this file does not exist, we try to convert legacy config file to it.

Access to variables during run-time is performed in adapter structure which is populated by loading saved configuration during initialization.

First 6 patches remove access to config file from run-time.
Last patch move to ini-file format style (load, save and convert).

Frédéric Danis (10):
  doc: Add settings storage documentation
  adapter: Read name in storage at init
  adaptername: Retrieve config name from adapter
  adapter: Read device class in storage at init
  adapter: Move pairable read to load_config()
  adapter: Read pairable timeout in storage at init
  adapter: Read discoverable timeout in storage at init
  adapter: Read mode in storage at init
  adapter: Move saved config to ini-file format
  TODO: Add entry to remove storage convertion function

 TODO                     |    6 +
 doc/settings-storage.txt |  102 ++++++++++++++
 plugins/adaptername.c    |    9 +-
 src/adapter.c            |  330 +++++++++++++++++++++++++++++++++++-----------
 src/adapter.h            |    1 +
 5 files changed, 370 insertions(+), 78 deletions(-)
 create mode 100644 doc/settings-storage.txt

-- 
1.7.9.5

[PATCH v3 01/10] doc: Add settings storage documentation

[Frédéric Danis]

---
 doc/settings-storage.txt |  102 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 doc/settings-storage.txt

diff --git a/doc/settings-storage.txt b/doc/settings-storage.txt
new file mode 100644
index 0000000..51d2220
--- /dev/null
+++ b/doc/settings-storage.txt
@@ -0,0 +1,102 @@
+Information related to local adapters and remote devices are stored in storage
+directory (default: /var/lib/bluetooth).
+Files are in ini-file format.
+
+There is one directory per adapter, named by its bluetooth address, which
+contains:
+ - a settings file for the local adapter
+ - an attribute_db file containing attributes of supported LE services
+ - names file containing names of all devices already seen
+ - one directory per remote device, named by remote device address, which
+   contains:
+    - a settings file
+    - a key file accessible only by root
+    - an attribute_db file containing attributes of remote LE services
+
+So the directory structure should be:
+    /var/lib/bluetooth/<adapter address>/
+        ./settings
+        ./attribute_db
+        ./names
+        ./<remote device address>/
+            ./settings
+            ./keys
+            ./attribute_db
+        ./<remote device address>/
+            ./settings
+            ./keys
+            ./attribute_db
+        ...
+
+Adapter directory files
+=======================
+
+Settings file contains 1 [General] group with adapter info:
+  [General]
+  Name=
+  Class=0x000000
+  Pairable=<true|false>
+  PairableTimeout=0
+  DiscoverableTimeout=0
+  Mode=<off|connectable|discoverable>
+  OnMode=<connectable|discoverable>
+
+The attribute_db file is a list of handles (group name) with UUID and Value as
+keys, for example:
+  [0x0001]
+  UUID=00002800-0000-1000-8000-00805f9b34fb
+  Value=0018
+
+  [0x0004]
+  UUID=00002803-0000-1000-8000-00805f9b34fb
+  Value=020600002A
+
+  [0x0006]
+  UUID=00002a00-0000-1000-8000-00805f9b34fb
+  Value=4578616D706C6520446576696365
+
+Names file contains 1 [Names] group with device address as key:
+  [Names]
+  <nn:nn:nn:nn:nn:nn>=device name a
+  <mm:mm:mm:mm:mm:mm>=device name b
+
+Device directory files
+======================
+
+Remote device settings file will include a [General] group with device infos
+and a [DeviceID] group with related infos:
+  [General]
+  Alias=
+  Class=0x000000
+  Manufacturer=0
+  LmpVersion=
+  LmpSubversion=
+  Features=0000000000000000
+  AddressType=<BR/EDR|LE>
+  LEAddressType=<public|random>
+  LastSeen=yyyy-mm-dd hh:mm:ss GMT
+  LastUsed=yyyy-mm-dd hh:mm:ss GMT
+  Trusted=<true|false>
+  Profiles=<128 bits UUID of profile 1>;<128 bits UUID of profile 2>;...
+
+  [DeviceID]
+  Assigner=0
+  Vendor=0
+  Product=0
+  Version=0
+
+Keys file includes informations related to link key or long term link key:
+  [LinkKey]
+  Key=
+  Type=0
+  PINLength=0
+
+  [LongTermKey]
+  Key=
+  Authenticated=<true|false>
+  EncSize=0
+  EDiv=0
+  Rand=0
+
+The attribute_db file is a list of handles (group name) with UUID and Value as
+keys (cf. attribute_db in adapter directory).
-- 
1.7.9.5

[PATCH v3 02/10] adapter: Read name in storage at init

[Frédéric Danis]

---
 src/adapter.c |   34 ++++++++++++++++++++++++++++++++++
 src/adapter.h |    1 +
 2 files changed, 35 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index ad8ee06..8b9af28 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -115,6 +115,10 @@ struct service_auth {
 	struct agent *agent;		/* NULL for queued auths */
 };
 
+struct btd_adapter_config {
+	char *name;
+};
+
 struct btd_adapter {
 	uint16_t dev_id;
 	gboolean up;
@@ -155,6 +159,8 @@ struct btd_adapter {
 	gboolean pairable;		/* pairable state */
 	gboolean initialized;
 
+	struct btd_adapter_config config;
+
 	gboolean off_requested;		/* DEVDOWN ioctl was called */
 
 	gint ref;
@@ -780,11 +786,24 @@ int adapter_set_name(struct btd_adapter *adapter, const char *name)
 		adapter->name = g_strdup(maxname);
 	}
 
+	g_free(adapter->config.name);
+	adapter->config.name = g_strdup(maxname);
+
 	write_local_name(&adapter->bdaddr, maxname);
 
 	return 0;
 }
 
+int adapter_get_config_name(struct btd_adapter *adapter, char **name)
+{
+	if (adapter->config.name == NULL)
+		return -EINVAL;
+
+	*name = adapter->config.name;
+
+	return 0;
+}
+
 static void set_name(struct btd_adapter *adapter, const char *name,
 						GDBusPendingPropertySet id)
 {
@@ -2665,6 +2684,18 @@ void btd_adapter_unref(struct btd_adapter *adapter)
 	g_free(path);
 }
 
+static void load_config(struct btd_adapter *adapter)
+{
+	char name[MAX_NAME_LENGTH + 1];
+
+	/* Get name */
+	if (read_local_name(&adapter->bdaddr, name) < 0)
+		adapter->config.name = NULL;
+	else
+		adapter->config.name = g_strdup(name);
+
+}
+
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
 {
 	adapter->up = up;
@@ -2684,6 +2715,7 @@ gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
 	if (main_opts.gatt_enabled)
 		btd_adapter_gatt_server_start(adapter);
 
+	load_config(adapter);
 	load_drivers(adapter);
 	btd_profile_foreach(probe_profile, adapter);
 	clear_blocked(adapter);
@@ -2752,6 +2784,8 @@ void adapter_remove(struct btd_adapter *adapter)
 	/* Return adapter to down state if it was not up on init */
 	if (!adapter->already_up && adapter->up)
 		mgmt_set_powered(adapter->dev_id, FALSE);
+
+	g_free(adapter->config.name);
 }
 
 uint16_t adapter_get_dev_id(struct btd_adapter *adapter)
diff --git a/src/adapter.h b/src/adapter.h
index b5bff10..b575c1b 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -133,6 +133,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter,
 void adapter_emit_device_found(struct btd_adapter *adapter,
 						struct remote_dev_info *dev);
 void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode);
+int adapter_get_config_name(struct btd_adapter *adapter, char **name);
 int adapter_set_name(struct btd_adapter *adapter, const char *name);
 void adapter_name_changed(struct btd_adapter *adapter, const char *name);
 void adapter_service_insert(struct btd_adapter *adapter, void *rec);
-- 
1.7.9.5

[PATCH v3 03/10] adaptername: Retrieve config name from adapter

[Frédéric Danis]

Retrieve saved config name using adapter_get_config_name() instead
of reading it from storage file.
---
 plugins/adaptername.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/plugins/adaptername.c b/plugins/adaptername.c
index f58fb0f..46dbbe8 100644
--- a/plugins/adaptername.c
+++ b/plugins/adaptername.c
@@ -198,7 +198,8 @@ static void set_pretty_name(struct btd_adapter *adapter,
 static int adaptername_probe(struct btd_adapter *adapter)
 {
 	int current_id;
-	char name[MAX_NAME_LENGTH + 1];
+	char *name;
+	char str[MAX_NAME_LENGTH + 1];
 	char *pretty_hostname;
 
 	pretty_hostname = read_pretty_host_name();
@@ -211,8 +212,10 @@ static int adaptername_probe(struct btd_adapter *adapter)
 	adapter_set_allow_name_changes(adapter, TRUE);
 	current_id = adapter_get_dev_id(adapter);
 
-	if (read_local_name(adapter_get_address(adapter), name) < 0)
-		expand_name(name, MAX_NAME_LENGTH, main_opts.name, current_id);
+	if (adapter_get_config_name(adapter, &name) < 0) {
+		expand_name(str, MAX_NAME_LENGTH, main_opts.name, current_id);
+		name = str;
+	}
 
 	DBG("Setting name '%s' for device 'hci%d'", name, current_id);
 	adapter_set_name(adapter, name);
-- 
1.7.9.5

[PATCH v3 04/10] adapter: Read device class in storage at init

[Frédéric Danis]

---
 src/adapter.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 8b9af28..93dc0fb 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -117,6 +117,7 @@ struct service_auth {
 
 struct btd_adapter_config {
 	char *name;
+	uint8_t class[3];
 };
 
 struct btd_adapter {
@@ -729,6 +730,10 @@ void btd_adapter_class_changed(struct btd_adapter *adapter, uint8_t *new_class)
 	if (class == adapter->dev_class)
 		return;
 
+	adapter->config.class[0] = new_class[0];
+	adapter->config.class[1] = new_class[1];
+	adapter->config.class[2] = new_class[2];
+
 	write_local_class(&adapter->bdaddr, new_class);
 
 	adapter->dev_class = class;
@@ -2353,15 +2358,8 @@ void btd_adapter_get_mode(struct btd_adapter *adapter, uint8_t *mode,
 void btd_adapter_read_class(struct btd_adapter *adapter, uint8_t *major,
 								uint8_t *minor)
 {
-	uint8_t cls[3];
-
-	if (read_local_class(&adapter->bdaddr, cls) < 0) {
-		uint32_t class = htobl(main_opts.class);
-		memcpy(cls, &class, 3);
-	}
-
-	*major = cls[1];
-	*minor = cls[0];
+	*major = adapter->config.class[1];
+	*minor = adapter->config.class[0];
 }
 
 uint32_t btd_adapter_get_class(struct btd_adapter *adapter)
@@ -2694,6 +2692,11 @@ static void load_config(struct btd_adapter *adapter)
 	else
 		adapter->config.name = g_strdup(name);
 
+	/* Get class */
+	if (read_local_class(&adapter->bdaddr, adapter->config.class) < 0) {
+		uint32_t class = htobl(main_opts.class);
+		memcpy(adapter->config.class, &class, 3);
+	}
 }
 
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
-- 
1.7.9.5

[PATCH v3 05/10] adapter: Move pairable read to load_config()

[Frédéric Danis]

---
 src/adapter.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 93dc0fb..357b717 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2697,6 +2697,10 @@ static void load_config(struct btd_adapter *adapter)
 		uint32_t class = htobl(main_opts.class);
 		memcpy(adapter->config.class, &class, 3);
 	}
+
+	/* Get pairable mode */
+	if (read_device_pairable(&adapter->bdaddr, &adapter->pairable) < 0)
+		adapter->pairable = TRUE;
 }
 
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
@@ -2724,10 +2728,6 @@ gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
 	clear_blocked(adapter);
 	load_devices(adapter);
 
-	/* Set pairable mode */
-	if (read_device_pairable(&adapter->bdaddr, &adapter->pairable) < 0)
-		adapter->pairable = TRUE;
-
 	/* retrieve the active connections: address the scenario where
 	 * the are active connections before the daemon've started */
 	load_connections(adapter);
-- 
1.7.9.5

[PATCH v3 06/10] adapter: Read pairable timeout in storage at init

[Frédéric Danis]

---
 src/adapter.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 357b717..b869d81 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2278,16 +2278,6 @@ static int get_discoverable_timeout(const char *src)
 	return main_opts.discovto;
 }
 
-static int get_pairable_timeout(const char *src)
-{
-	int timeout;
-
-	if (read_pairable_timeout(src, &timeout) == 0)
-		return timeout;
-
-	return main_opts.pairto;
-}
-
 static void set_auto_connect(gpointer data, gpointer user_data)
 {
 	struct btd_device *device = data;
@@ -2432,7 +2422,6 @@ void btd_adapter_start(struct btd_adapter *adapter)
 	adapter->off_requested = FALSE;
 	adapter->up = TRUE;
 	adapter->discov_timeout = get_discoverable_timeout(address);
-	adapter->pairable_timeout = get_pairable_timeout(address);
 	adapter->off_timer = 0;
 
 	if (adapter->scan_mode & SCAN_INQUIRY)
@@ -2685,6 +2674,10 @@ void btd_adapter_unref(struct btd_adapter *adapter)
 static void load_config(struct btd_adapter *adapter)
 {
 	char name[MAX_NAME_LENGTH + 1];
+	char address[18];
+	int timeout;
+
+	ba2str(&adapter->bdaddr, address);
 
 	/* Get name */
 	if (read_local_name(&adapter->bdaddr, name) < 0)
@@ -2701,6 +2694,12 @@ static void load_config(struct btd_adapter *adapter)
 	/* Get pairable mode */
 	if (read_device_pairable(&adapter->bdaddr, &adapter->pairable) < 0)
 		adapter->pairable = TRUE;
+
+	/* Get pairable timeout */
+	if (read_pairable_timeout(address, &timeout) < 0)
+		adapter->pairable_timeout = main_opts.pairto;
+	else
+		adapter->pairable_timeout = timeout;
 }
 
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
-- 
1.7.9.5

[PATCH v3 07/10] adapter: Read discoverable timeout in storage at init

[Frédéric Danis]

---
 src/adapter.c |   19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index b869d81..40df4a1 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2268,16 +2268,6 @@ static void load_connections(struct btd_adapter *adapter)
 	g_slist_free_full(conns, g_free);
 }
 
-static int get_discoverable_timeout(const char *src)
-{
-	int timeout;
-
-	if (read_discoverable_timeout(src, &timeout) == 0)
-		return timeout;
-
-	return main_opts.discovto;
-}
-
 static void set_auto_connect(gpointer data, gpointer user_data)
 {
 	struct btd_device *device = data;
@@ -2339,7 +2329,7 @@ void btd_adapter_get_mode(struct btd_adapter *adapter, uint8_t *mode,
 		*on_mode = get_mode(&adapter->bdaddr, "on");
 
 	if (discoverable_timeout)
-		*discoverable_timeout = get_discoverable_timeout(address);
+		*discoverable_timeout = adapter->discov_timeout;
 
 	if (pairable)
 		*pairable = adapter->pairable;
@@ -2421,7 +2411,6 @@ void btd_adapter_start(struct btd_adapter *adapter)
 	adapter->dev_class = 0;
 	adapter->off_requested = FALSE;
 	adapter->up = TRUE;
-	adapter->discov_timeout = get_discoverable_timeout(address);
 	adapter->off_timer = 0;
 
 	if (adapter->scan_mode & SCAN_INQUIRY)
@@ -2700,6 +2689,12 @@ static void load_config(struct btd_adapter *adapter)
 		adapter->pairable_timeout = main_opts.pairto;
 	else
 		adapter->pairable_timeout = timeout;
+
+	/* Get discoverable timeout */
+	if (read_discoverable_timeout(address, &timeout) < 0)
+		adapter->discov_timeout = main_opts.discovto;
+	else
+		adapter->discov_timeout = timeout;
 }
 
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
-- 
1.7.9.5

[PATCH v3 08/10] adapter: Read mode in storage at init

[Frédéric Danis]

---
 src/adapter.c |   61 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 40df4a1..85c5da9 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -118,6 +118,8 @@ struct service_auth {
 struct btd_adapter_config {
 	char *name;
 	uint8_t class[3];
+	uint8_t mode;
+	uint8_t on_mode;
 };
 
 struct btd_adapter {
@@ -210,7 +212,7 @@ static const char *mode2str(uint8_t mode)
 	}
 }
 
-static uint8_t get_mode(const bdaddr_t *bdaddr, const char *mode)
+static uint8_t get_mode(const char *mode)
 {
 	if (strcasecmp("off", mode) == 0)
 		return MODE_OFF;
@@ -218,15 +220,7 @@ static uint8_t get_mode(const bdaddr_t *bdaddr, const char *mode)
 		return MODE_CONNECTABLE;
 	else if (strcasecmp("discoverable", mode) == 0)
 		return MODE_DISCOVERABLE;
-	else if (strcasecmp("on", mode) == 0) {
-		char onmode[14], srcaddr[18];
-
-		ba2str(bdaddr, srcaddr);
-		if (read_on_mode(srcaddr, onmode, sizeof(onmode)) < 0)
-			return MODE_CONNECTABLE;
-
-		return get_mode(bdaddr, onmode);
-	} else
+	else
 		return MODE_UNKNOWN;
 }
 
@@ -331,6 +325,10 @@ static int set_mode(struct btd_adapter *adapter, uint8_t new_mode)
 		return err;
 
 done:
+	adapter->config.mode = new_mode;
+	if (new_mode != MODE_OFF)
+		adapter->config.on_mode = new_mode;
+
 	modestr = mode2str(new_mode);
 	write_device_mode(&adapter->bdaddr, modestr);
 
@@ -390,7 +388,7 @@ static void set_powered(struct btd_adapter *adapter, gboolean powered,
 	int err;
 
 	if (powered) {
-		mode = get_mode(&adapter->bdaddr, "on");
+		mode = adapter->config.on_mode;
 		return set_discoverable(adapter, mode == MODE_DISCOVERABLE,
 									id);
 	}
@@ -1403,7 +1401,7 @@ static DBusMessage *request_session(DBusConnection *conn,
 	if (!adapter->mode_sessions)
 		adapter->global_mode = adapter->mode;
 
-	new_mode = get_mode(&adapter->bdaddr, "on");
+	new_mode = adapter->config.on_mode;
 
 	req = find_session(adapter->mode_sessions, sender);
 	if (req) {
@@ -2312,21 +2310,15 @@ void btd_adapter_get_mode(struct btd_adapter *adapter, uint8_t *mode,
 						uint16_t *discoverable_timeout,
 						gboolean *pairable)
 {
-	char str[14], address[18];
+	char address[18];
 
 	ba2str(&adapter->bdaddr, address);
 
-	if (mode) {
-		if (main_opts.remember_powered == FALSE)
-			*mode = main_opts.mode;
-		else if (read_device_mode(address, str, sizeof(str)) == 0)
-			*mode = get_mode(&adapter->bdaddr, str);
-		else
-			*mode = main_opts.mode;
-	}
+	if (mode)
+		*mode = adapter->config.mode;
 
 	if (on_mode)
-		*on_mode = get_mode(&adapter->bdaddr, "on");
+		*on_mode = adapter->config.on_mode;
 
 	if (discoverable_timeout)
 		*discoverable_timeout = adapter->discov_timeout;
@@ -2490,6 +2482,10 @@ static void set_mode_complete(struct btd_adapter *adapter)
 	const char *modestr;
 	int err;
 
+	adapter->config.mode = adapter->mode;
+	if (adapter->mode != MODE_OFF)
+		adapter->config.on_mode = adapter->mode;
+
 	modestr = mode2str(adapter->mode);
 	write_device_mode(&adapter->bdaddr, modestr);
 
@@ -2664,6 +2660,7 @@ static void load_config(struct btd_adapter *adapter)
 {
 	char name[MAX_NAME_LENGTH + 1];
 	char address[18];
+	char mode[14];
 	int timeout;
 
 	ba2str(&adapter->bdaddr, address);
@@ -2695,6 +2692,20 @@ static void load_config(struct btd_adapter *adapter)
 		adapter->discov_timeout = main_opts.discovto;
 	else
 		adapter->discov_timeout = timeout;
+
+	/* Get mode */
+	if (main_opts.remember_powered == FALSE)
+		adapter->config.mode = main_opts.mode;
+	else if (read_device_mode(address, mode, sizeof(mode)) == 0)
+		adapter->config.mode = get_mode(mode);
+	else
+		adapter->config.mode = main_opts.mode;
+
+	/* Get on mode */
+	if (read_on_mode(address, mode, sizeof(mode)) == 0)
+		adapter->config.on_mode = get_mode(mode);
+	else
+		adapter->config.on_mode = MODE_CONNECTABLE;
 }
 
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
@@ -3601,17 +3612,13 @@ gboolean adapter_powering_down(struct btd_adapter *adapter)
 
 int btd_adapter_restore_powered(struct btd_adapter *adapter)
 {
-	char mode[14], address[18];
-
 	if (!main_opts.remember_powered)
 		return -EINVAL;
 
 	if (adapter->up)
 		return 0;
 
-	ba2str(&adapter->bdaddr, address);
-	if (read_device_mode(address, mode, sizeof(mode)) == 0 &&
-						g_str_equal(mode, "off"))
+	if (adapter->config.mode == MODE_OFF)
 		return 0;
 
 	return mgmt_set_powered(adapter->dev_id, TRUE);
-- 
1.7.9.5

[PATCH v3 09/10] adapter: Move saved config to ini-file format

[Frédéric Danis]

Read and write config file in ini-file format.
If the file can not be loaded, try to convert legacy configuration.
---
 src/adapter.c |  206 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 174 insertions(+), 32 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 85c5da9..6b4197c 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -33,6 +33,7 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <sys/ioctl.h>
+#include <sys/file.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/uuid.h>
@@ -84,6 +85,8 @@
 
 #define OFF_TIMER 3
 
+#define SETTINGS_PATH STORAGEDIR "/%s/settings"
+
 static GSList *adapter_drivers = NULL;
 
 enum session_req_type {
@@ -224,6 +227,54 @@ static uint8_t get_mode(const char *mode)
 		return MODE_UNKNOWN;
 }
 
+static void write_config(struct btd_adapter *adapter)
+{
+	GKeyFile *key_file;
+	char filename[PATH_MAX + 1];
+	char address[18];
+	char *str;
+	gsize length = 0;
+
+	key_file = g_key_file_new();
+
+	g_key_file_set_string(key_file, "General", "Name",
+				adapter->config.name);
+
+	str = g_strdup_printf("0x%2.2x%2.2x%2.2x", adapter->config.class[2],
+				adapter->config.class[1],
+				adapter->config.class[0]);
+	g_key_file_set_string(key_file, "General", "Class", str);
+	g_free(str);
+
+	g_key_file_set_boolean(key_file, "General", "Pairable",
+				adapter->pairable);
+
+	if (adapter->pairable_timeout != main_opts.pairto)
+		g_key_file_set_integer(key_file, "General", "PairableTimeout",
+					adapter->pairable_timeout);
+
+	if (adapter->discov_timeout != main_opts.discovto)
+		g_key_file_set_integer(key_file, "General",
+					"DiscoverableTimeout",
+					adapter->discov_timeout);
+
+	g_key_file_set_string(key_file, "General", "Mode",
+				mode2str(adapter->config.mode));
+	g_key_file_set_string(key_file, "General", "OnMode",
+				mode2str(adapter->config.on_mode));
+
+	ba2str(&adapter->bdaddr, address);
+	snprintf(filename, PATH_MAX, SETTINGS_PATH, address);
+
+	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+
+	str = g_key_file_to_data(key_file, &length, NULL);
+	g_file_set_contents(filename, str, length, NULL);
+	g_free(str);
+
+	g_key_file_free(key_file);
+}
+
 static struct session_req *session_ref(struct session_req *req)
 {
 	req->refcount++;
@@ -293,7 +344,6 @@ static struct session_req *find_session_by_msg(GSList *list, const DBusMessage *
 static int set_mode(struct btd_adapter *adapter, uint8_t new_mode)
 {
 	int err;
-	const char *modestr;
 
 	if (adapter->pending_mode != NULL)
 		return -EALREADY;
@@ -329,10 +379,9 @@ done:
 	if (new_mode != MODE_OFF)
 		adapter->config.on_mode = new_mode;
 
-	modestr = mode2str(new_mode);
-	write_device_mode(&adapter->bdaddr, modestr);
+	write_config(adapter);
 
-	DBG("%s", modestr);
+	DBG("%s", mode2str(new_mode));
 
 	return 0;
 }
@@ -475,7 +524,7 @@ void btd_adapter_pairable_changed(struct btd_adapter *adapter,
 {
 	adapter->pairable = pairable;
 
-	write_device_pairable(&adapter->bdaddr, pairable);
+	write_config(adapter);
 
 	g_dbus_emit_property_changed(btd_get_dbus_connection(), adapter->path,
 					ADAPTER_INTERFACE, "Pairable");
@@ -692,7 +741,7 @@ static void set_discoverable_timeout(struct btd_adapter *adapter,
 
 	adapter->discov_timeout = timeout;
 
-	write_discoverable_timeout(&adapter->bdaddr, timeout);
+	write_config(adapter);
 
 	g_dbus_emit_property_changed(conn, adapter->path, ADAPTER_INTERFACE,
 						"DiscoverableTimeout");
@@ -712,7 +761,7 @@ static void set_pairable_timeout(struct btd_adapter *adapter,
 
 	adapter->pairable_timeout = timeout;
 
-	write_pairable_timeout(&adapter->bdaddr, timeout);
+	write_config(adapter);
 
 	g_dbus_emit_property_changed(conn, adapter->path, ADAPTER_INTERFACE,
 							"PairableTimeout");
@@ -732,7 +781,7 @@ void btd_adapter_class_changed(struct btd_adapter *adapter, uint8_t *new_class)
 	adapter->config.class[1] = new_class[1];
 	adapter->config.class[2] = new_class[2];
 
-	write_local_class(&adapter->bdaddr, new_class);
+	write_config(adapter);
 
 	adapter->dev_class = class;
 
@@ -792,7 +841,7 @@ int adapter_set_name(struct btd_adapter *adapter, const char *name)
 	g_free(adapter->config.name);
 	adapter->config.name = g_strdup(maxname);
 
-	write_local_name(&adapter->bdaddr, maxname);
+	write_config(adapter);
 
 	return 0;
 }
@@ -2479,17 +2528,15 @@ static void set_mode_complete(struct btd_adapter *adapter)
 {
 	DBusConnection *conn = btd_get_dbus_connection();
 	struct session_req *pending;
-	const char *modestr;
 	int err;
 
 	adapter->config.mode = adapter->mode;
 	if (adapter->mode != MODE_OFF)
 		adapter->config.on_mode = adapter->mode;
 
-	modestr = mode2str(adapter->mode);
-	write_device_mode(&adapter->bdaddr, modestr);
+	write_config(adapter);
 
-	DBG("%s", modestr);
+	DBG("%s", mode2str(adapter->mode));
 
 	if (adapter->mode == MODE_OFF) {
 		g_slist_free_full(adapter->mode_sessions, session_free);
@@ -2656,56 +2703,151 @@ void btd_adapter_unref(struct btd_adapter *adapter)
 	g_free(path);
 }
 
-static void load_config(struct btd_adapter *adapter)
+static void convert_config(struct btd_adapter *adapter, const char *filename,
+				GKeyFile *key_file)
 {
-	char name[MAX_NAME_LENGTH + 1];
 	char address[18];
-	char mode[14];
+	char str[MAX_NAME_LENGTH + 1];
+	char config_path[PATH_MAX + 1];
+	char *converted;
+	uint8_t class[3];
+	gboolean flag;
 	int timeout;
+	char *data;
+	gsize length = 0;
+
+	ba2str(&adapter->bdaddr, address);
+	snprintf(config_path, PATH_MAX, STORAGEDIR "/%s/config", address);
+
+	converted = textfile_get(config_path, "converted");
+	if (converted) {
+		if (strcmp(converted, "yes") == 0) {
+			DBG("Legacy config file already converted");
+			return;
+		}
+
+		g_free(converted);
+	}
+
+	if (read_local_name(&adapter->bdaddr, str) == 0)
+		g_key_file_set_string(key_file, "General", "Name", str);
+
+	if (read_local_class(&adapter->bdaddr, class) == 0) {
+		sprintf(str, "0x%2.2x%2.2x%2.2x", class[2], class[1], class[0]);
+		g_key_file_set_string(key_file, "General", "Class", str);
+	}
+
+	if (read_device_pairable(&adapter->bdaddr, &flag) == 0)
+		g_key_file_set_boolean(key_file, "General", "Pairable", flag);
+
+	if (read_pairable_timeout(address, &timeout) == 0)
+		g_key_file_set_integer(key_file, "General",
+						"PairableTimeout", timeout);
+
+	if (read_discoverable_timeout(address, &timeout) == 0)
+		g_key_file_set_integer(key_file, "General",
+						"DiscoverableTimeout", timeout);
+
+	if (read_device_mode(address, str, sizeof(str)) == 0)
+		g_key_file_set_string(key_file, "General", "Mode", str);
+
+	if (read_on_mode(address, str, sizeof(str)) == 0)
+		g_key_file_set_string(key_file, "General", "OnMode", str);
+
+	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+
+	data = g_key_file_to_data(key_file, &length, NULL);
+	g_file_set_contents(filename, data, length, NULL);
+	g_free(data);
+
+	textfile_put(config_path, "converted", "yes");
+}
+
+static void load_config(struct btd_adapter *adapter)
+{
+	GKeyFile *key_file;
+	char filename[PATH_MAX + 1];
+	char address[18];
+	char *str;
+	GError *gerr = NULL;
 
 	ba2str(&adapter->bdaddr, address);
 
+	key_file = g_key_file_new();
+
+	snprintf(filename, PATH_MAX, SETTINGS_PATH, address);
+
+	if (!g_key_file_load_from_file(key_file, filename, 0, NULL))
+		convert_config(adapter, filename, key_file);
+
 	/* Get name */
-	if (read_local_name(&adapter->bdaddr, name) < 0)
-		adapter->config.name = NULL;
-	else
-		adapter->config.name = g_strdup(name);
+	adapter->config.name = g_key_file_get_string(key_file, "General",
+								"Name", NULL);
 
 	/* Get class */
-	if (read_local_class(&adapter->bdaddr, adapter->config.class) < 0) {
+	str = g_key_file_get_string(key_file, "General", "Class", NULL);
+	if (str) {
+		char tmp[3];
+		int i;
+		uint8_t *class = adapter->config.class;
+
+		memset(tmp, 0, sizeof(tmp));
+		for (i = 0; i < 3; i++) {
+			memcpy(tmp, str + (i * 2) + 2, 2);
+			class[2 - i] = (uint8_t) strtol(tmp, NULL, 16);
+		}
+	} else {
 		uint32_t class = htobl(main_opts.class);
 		memcpy(adapter->config.class, &class, 3);
 	}
+	g_free(str);
 
 	/* Get pairable mode */
-	if (read_device_pairable(&adapter->bdaddr, &adapter->pairable) < 0)
+	adapter->pairable = g_key_file_get_boolean(key_file, "General",
+							"Pairable", &gerr);
+	if (gerr) {
 		adapter->pairable = TRUE;
+		g_error_free(gerr);
+		gerr = NULL;
+	}
 
 	/* Get pairable timeout */
-	if (read_pairable_timeout(address, &timeout) < 0)
+	adapter->pairable_timeout = g_key_file_get_integer(key_file, "General",
+						"PairableTimeout", &gerr);
+	if (gerr) {
 		adapter->pairable_timeout = main_opts.pairto;
-	else
-		adapter->pairable_timeout = timeout;
+		g_error_free(gerr);
+		gerr = NULL;
+	}
 
 	/* Get discoverable timeout */
-	if (read_discoverable_timeout(address, &timeout) < 0)
+	adapter->discov_timeout = g_key_file_get_integer(key_file, "General",
+						"DiscoverableTimeout", &gerr);
+	if (gerr) {
 		adapter->discov_timeout = main_opts.discovto;
-	else
-		adapter->discov_timeout = timeout;
+		g_error_free(gerr);
+		gerr = NULL;
+	}
 
 	/* Get mode */
+	str = g_key_file_get_string(key_file, "General", "Mode", NULL);
 	if (main_opts.remember_powered == FALSE)
 		adapter->config.mode = main_opts.mode;
-	else if (read_device_mode(address, mode, sizeof(mode)) == 0)
-		adapter->config.mode = get_mode(mode);
+	else if (str)
+		adapter->config.mode = get_mode(str);
 	else
 		adapter->config.mode = main_opts.mode;
+	g_free(str);
 
 	/* Get on mode */
-	if (read_on_mode(address, mode, sizeof(mode)) == 0)
-		adapter->config.on_mode = get_mode(mode);
+	str = g_key_file_get_string(key_file, "General", "OnMode", NULL);
+	if (str)
+		adapter->config.on_mode = get_mode(str);
 	else
 		adapter->config.on_mode = MODE_CONNECTABLE;
+	g_free(str);
+
+	g_key_file_free(key_file);
 }
 
 gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
-- 
1.7.9.5

[PATCH v3 10/10] TODO: Add entry to remove storage convertion function

[Frédéric Danis]

---
 TODO |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/TODO b/TODO
index 384d428..c6787cc 100644
--- a/TODO
+++ b/TODO
@@ -30,6 +30,12 @@ General
   Priority: Low
   Complexity: C1
 
+- Function in src/adapter.c to convert old storage files to new ini-file format
+  should be removed 6-8 months after first BlueZ 5 release.
+
+  Priority: Low
+  Complexity: C1
+
 BlueZ 5
 =======
 
-- 
1.7.9.5

Re: [PATCH v3 01/10] doc: Add settings storage documentation

[Marcel Holtmann]

Hi Fred,

>  doc/settings-storage.txt |  102 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 doc/settings-storage.txt
> 
> diff --git a/doc/settings-storage.txt b/doc/settings-storage.txt
> new file mode 100644
> index 0000000..51d2220
> --- /dev/null
> +++ b/doc/settings-storage.txt
> @@ -0,0 +1,102 @@
> +Information related to local adapters and remote devices are stored in storage
> +directory (default: /var/lib/bluetooth).
> +Files are in ini-file format.
> +
> +There is one directory per adapter, named by its bluetooth address, which
> +contains:
> + - a settings file for the local adapter
> + - an attribute_db file containing attributes of supported LE services
> + - names file containing names of all devices already seen
> + - one directory per remote device, named by remote device address, which
> +   contains:
> +    - a settings file
> +    - a key file accessible only by root
> +    - an attribute_db file containing attributes of remote LE services
> +
> +So the directory structure should be:
> +    /var/lib/bluetooth/<adapter address>/
> +        ./settings
> +        ./attribute_db

not sure this is a good name.

> +        ./names

Lets call this one "cache". Since we could throw it away and it will be
just rediscovered. And we can also stuff other cacheable details in
there.

> +        ./<remote device address>/
> +            ./settings

Besides the Alias, are they really settings. You can not configure much
anyway.

> +            ./keys

Not sure that I want the keys separated. That seems like a waste.

> +            ./attribute_db
> +        ./<remote device address>/
> +            ./settings
> +            ./keys
> +            ./attribute_db
> +        ...

Why did we want directories for remote devices again? Can we not just
put all in one big file with the name of the remote address?

> +
> +Adapter directory files
> +=======================
> +
> +Settings file contains 1 [General] group with adapter info:
> +  [General]
> +  Name=
> +  Class=0x000000
> +  Pairable=<true|false>
> +  PairableTimeout=0
> +  DiscoverableTimeout=0
> +  Mode=<off|connectable|discoverable>
> +  OnMode=<connectable|discoverable>

Actually with management interface now being used, we can be a bit
smarter here. The mode can be programmed even if the controller is not
powered.

So this might be better as Discoverable, Connectable, Pairable and
Powered boolean options.

Also Pairable has no timeout in the kernel API anymore. Do we still need
this?

> +The attribute_db file is a list of handles (group name) with UUID and Value as
> +keys, for example:
> +  [0x0001]
> +  UUID=00002800-0000-1000-8000-00805f9b34fb
> +  Value=0018
> +
> +  [0x0004]
> +  UUID=00002803-0000-1000-8000-00805f9b34fb
> +  Value=020600002A
> +
> +  [0x0006]
> +  UUID=00002a00-0000-1000-8000-00805f9b34fb
> +  Value=4578616D706C6520446576696365

Is this our primary key, the handle?

> +
> +Names file contains 1 [Names] group with device address as key:
> +  [Names]
> +  <nn:nn:nn:nn:nn:nn>=device name a
> +  <mm:mm:mm:mm:mm:mm>=device name b

I am not sure this is the best idea this way. Maybe just creating files
for each address we ever see is a better idea. Details like LastSeen
could be useful for unpaired devices.

> +
> +Device directory files
> +======================
> +
> +Remote device settings file will include a [General] group with device infos
> +and a [DeviceID] group with related infos:
> +  [General]
> +  Alias=
> +  Class=0x000000
> +  Manufacturer=0
> +  LmpVersion=
> +  LmpSubversion=

Take these 3 our. We were just storing them for statistic purposes.

> +  Features=0000000000000000
> +  AddressType=<BR/EDR|LE>
> +  LEAddressType=<public|random>

That is not needed. It is encoded in the address itself.

> +  LastSeen=yyyy-mm-dd hh:mm:ss GMT
> +  LastUsed=yyyy-mm-dd hh:mm:ss GMT
> +  Trusted=<true|false>
> +  Profiles=<128 bits UUID of profile 1>;<128 bits UUID of profile 2>;...
> +
> +  [DeviceID]
> +  Assigner=0

This is called Source.

> +  Vendor=0
> +  Product=0
> +  Version=0
> +
> +Keys file includes informations related to link key or long term link key:
> +  [LinkKey]
> +  Key=
> +  Type=0
> +  PINLength=0
> +
> +  [LongTermKey]
> +  Key=
> +  Authenticated=<true|false>
> +  EncSize=0
> +  EDiv=0
> +  Rand=0

Can be both in the same file about the device information.

> +
> +The attribute_db file is a list of handles (group name) with UUID and Value as
> +keys (cf. attribute_db in adapter directory).

Regards

Marcel

Re: [PATCH v3 02/10] adapter: Read name in storage at init

[Marcel Holtmann]

Hi Fred,

>  src/adapter.c |   34 ++++++++++++++++++++++++++++++++++
>  src/adapter.h |    1 +
>  2 files changed, 35 insertions(+)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index ad8ee06..8b9af28 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -115,6 +115,10 @@ struct service_auth {
>  	struct agent *agent;		/* NULL for queued auths */
>  };
>  
> +struct btd_adapter_config {
> +	char *name;
> +};
> +

why are we bothering we a separate config here? I think that should be
just put into btd_adapter.

>  struct btd_adapter {
>  	uint16_t dev_id;
>  	gboolean up;
> @@ -155,6 +159,8 @@ struct btd_adapter {
>  	gboolean pairable;		/* pairable state */
>  	gboolean initialized;
>  
> +	struct btd_adapter_config config;
> +
>  	gboolean off_requested;		/* DEVDOWN ioctl was called */
>  
>  	gint ref;
> @@ -780,11 +786,24 @@ int adapter_set_name(struct btd_adapter *adapter, const char *name)
>  		adapter->name = g_strdup(maxname);
>  	}
>  
> +	g_free(adapter->config.name);
> +	adapter->config.name = g_strdup(maxname);
> +
>  	write_local_name(&adapter->bdaddr, maxname);
>  
>  	return 0;
>  }
>  
> +int adapter_get_config_name(struct btd_adapter *adapter, char **name)
> +{
> +	if (adapter->config.name == NULL)
> +		return -EINVAL;
> +
> +	*name = adapter->config.name;
> +
> +	return 0;
> +}
> +

Who is the user of this. And why not just return const char *.

>  static void set_name(struct btd_adapter *adapter, const char *name,
>  						GDBusPendingPropertySet id)
>  {
> @@ -2665,6 +2684,18 @@ void btd_adapter_unref(struct btd_adapter *adapter)
>  	g_free(path);
>  }
>  
> +static void load_config(struct btd_adapter *adapter)
> +{
> +	char name[MAX_NAME_LENGTH + 1];
> +
> +	/* Get name */
> +	if (read_local_name(&adapter->bdaddr, name) < 0)
> +		adapter->config.name = NULL;
> +	else
> +		adapter->config.name = g_strdup(name);
> +
> +}
> +
>  gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
>  {
>  	adapter->up = up;
> @@ -2684,6 +2715,7 @@ gboolean adapter_init(struct btd_adapter *adapter, gboolean up)
>  	if (main_opts.gatt_enabled)
>  		btd_adapter_gatt_server_start(adapter);
>  
> +	load_config(adapter);
>  	load_drivers(adapter);
>  	btd_profile_foreach(probe_profile, adapter);
>  	clear_blocked(adapter);
> @@ -2752,6 +2784,8 @@ void adapter_remove(struct btd_adapter *adapter)
>  	/* Return adapter to down state if it was not up on init */
>  	if (!adapter->already_up && adapter->up)
>  		mgmt_set_powered(adapter->dev_id, FALSE);
> +
> +	g_free(adapter->config.name);
>  }
>  
>  uint16_t adapter_get_dev_id(struct btd_adapter *adapter)
> diff --git a/src/adapter.h b/src/adapter.h
> index b5bff10..b575c1b 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -133,6 +133,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter,
>  void adapter_emit_device_found(struct btd_adapter *adapter,
>  						struct remote_dev_info *dev);
>  void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode);
> +int adapter_get_config_name(struct btd_adapter *adapter, char **name);
>  int adapter_set_name(struct btd_adapter *adapter, const char *name);
>  void adapter_name_changed(struct btd_adapter *adapter, const char *name);
>  void adapter_service_insert(struct btd_adapter *adapter, void *rec);

Regards

Marcel

Re: [PATCH v3 03/10] adaptername: Retrieve config name from adapter

[Marcel Holtmann]

Hi Fred,

> Retrieve saved config name using adapter_get_config_name() instead
> of reading it from storage file.
> ---
>  plugins/adaptername.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/adaptername.c b/plugins/adaptername.c
> index f58fb0f..46dbbe8 100644
> --- a/plugins/adaptername.c
> +++ b/plugins/adaptername.c
> @@ -198,7 +198,8 @@ static void set_pretty_name(struct btd_adapter *adapter,
>  static int adaptername_probe(struct btd_adapter *adapter)
>  {
>  	int current_id;
> -	char name[MAX_NAME_LENGTH + 1];
> +	char *name;
> +	char str[MAX_NAME_LENGTH + 1];
>  	char *pretty_hostname;
>  
>  	pretty_hostname = read_pretty_host_name();
> @@ -211,8 +212,10 @@ static int adaptername_probe(struct btd_adapter *adapter)
>  	adapter_set_allow_name_changes(adapter, TRUE);
>  	current_id = adapter_get_dev_id(adapter);
>  
> -	if (read_local_name(adapter_get_address(adapter), name) < 0)
> -		expand_name(name, MAX_NAME_LENGTH, main_opts.name, current_id);
> +	if (adapter_get_config_name(adapter, &name) < 0) {
> +		expand_name(str, MAX_NAME_LENGTH, main_opts.name, current_id);
> +		name = str;
> +	}

fair enough, you are using it here. And even why you have this as a
separate config. However this is a bit messed up.

If we have a name configured we should use that. Only a not configured
system should allow a name change here. And reading a main_opts.name
from a plugin is also bad. This needs some further investigation.

Regards

Marcel

Re: [PATCH v3 04/10] adapter: Read device class in storage at init

[Marcel Holtmann]

Hi Fred,

>  src/adapter.c |   21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index 8b9af28..93dc0fb 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -117,6 +117,7 @@ struct service_auth {
>  
>  struct btd_adapter_config {
>  	char *name;
> +	uint8_t class[3];
>  };

same as the other one. Directly in btd_adapter if you ask me.

>  
>  struct btd_adapter {
> @@ -729,6 +730,10 @@ void btd_adapter_class_changed(struct btd_adapter *adapter, uint8_t *new_class)
>  	if (class == adapter->dev_class)
>  		return;
>  
> +	adapter->config.class[0] = new_class[0];
> +	adapter->config.class[1] = new_class[1];
> +	adapter->config.class[2] = new_class[2];
> +
>  	write_local_class(&adapter->bdaddr, new_class);

Is this still needed this way. Should not just say, store_adapter_info
or something alike.

>  
>  	adapter->dev_class = class;
> @@ -2353,15 +2358,8 @@ void btd_adapter_get_mode(struct btd_adapter *adapter, uint8_t *mode,
>  void btd_adapter_read_class(struct btd_adapter *adapter, uint8_t *major,
>  								uint8_t *minor)
>  {
> -	uint8_t cls[3];
> -
> -	if (read_local_class(&adapter->bdaddr, cls) < 0) {
> -		uint32_t class = htobl(main_opts.class);
> -		memcpy(cls, &class, 3);
> -	}
> -
> -	*major = cls[1];
> -	*minor = cls[0];
> +	*major = adapter->config.class[1];
> +	*minor = adapter->config.class[0];
>  }

Actually this is no longer read_class, this would be get_major_minor.

>  
>  uint32_t btd_adapter_get_class(struct btd_adapter *adapter)
> @@ -2694,6 +2692,11 @@ static void load_config(struct btd_adapter *adapter)
>  	else
>  		adapter->config.name = g_strdup(name);
>  
> +	/* Get class */
> +	if (read_local_class(&adapter->bdaddr, adapter->config.class) < 0) {
> +		uint32_t class = htobl(main_opts.class);
> +		memcpy(adapter->config.class, &class, 3);
> +	}
>  }

I really want to open the file once and read it all at once. Multiple
reads seem to be wrong.

Regards

Marcel

Re: [PATCH v3 05/10] adapter: Move pairable read to load_config()

[Marcel Holtmann]

Hi Fred,

>  src/adapter.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index 93dc0fb..357b717 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -2697,6 +2697,10 @@ static void load_config(struct btd_adapter *adapter)
>  		uint32_t class = htobl(main_opts.class);
>  		memcpy(adapter->config.class, &class, 3);
>  	}
> +
> +	/* Get pairable mode */
> +	if (read_device_pairable(&adapter->bdaddr, &adapter->pairable) < 0)
> +		adapter->pairable = TRUE;

maybe you change this later altogether, but the multiple reads here are
bad.

Regards

Marcel

Re: [PATCH v3 08/10] adapter: Read mode in storage at init

[Marcel Holtmann]

Hi Fred,

>  src/adapter.c |   61 ++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index 40df4a1..85c5da9 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -118,6 +118,8 @@ struct service_auth {
>  struct btd_adapter_config {
>  	char *name;
>  	uint8_t class[3];
> +	uint8_t mode;
> +	uint8_t on_mode;
>  };

here you have to do the real hard work to map this to proper modes
within management interface.

Regards

Marcel

Re: [PATCH v3 01/10] doc: Add settings storage documentation

[Johan Hedberg]

Hi Marcel,

On Wed, Oct 10, 2012, Marcel Holtmann wrote:
> > +            ./keys
> 
> Not sure that I want the keys separated. That seems like a waste.

Agreed. Same file seems good.

> > +            ./attribute_db
> > +        ./<remote device address>/
> > +            ./settings
> > +            ./keys
> > +            ./attribute_db
> > +        ...
> 
> Why did we want directories for remote devices again? Can we not just
> put all in one big file with the name of the remote address?

For profile-specific storage this could be convenient (when removing the
device we just remove the entire directory and the core daemon doesn't
need to care about profiles details). OTOH the core needs to either way
provide some API so the plugins know to write to the right location
(be it a single file for the entire device or a single directory for the
device).

> > +Adapter directory files
> > +=======================
> > +
> > +Settings file contains 1 [General] group with adapter info:
> > +  [General]
> > +  Name=
> > +  Class=0x000000
> > +  Pairable=<true|false>
> > +  PairableTimeout=0
> > +  DiscoverableTimeout=0
> > +  Mode=<off|connectable|discoverable>
> > +  OnMode=<connectable|discoverable>
> 
> Actually with management interface now being used, we can be a bit
> smarter here. The mode can be programmed even if the controller is not
> powered.
> 
> So this might be better as Discoverable, Connectable, Pairable and
> Powered boolean options.
> 
> Also Pairable has no timeout in the kernel API anymore. Do we still need
> this?

We do have it currently as a documented adapter D-Bus property so that'd
need to be removed too. I don't personally have a great need for this
but I could imagine some device manufacturers deciding to have something
like this (especially those with very simple UI's with a single button
or so).

> > +The attribute_db file is a list of handles (group name) with UUID and Value as
> > +keys, for example:
> > +  [0x0001]
> > +  UUID=00002800-0000-1000-8000-00805f9b34fb
> > +  Value=0018
> > +
> > +  [0x0004]
> > +  UUID=00002803-0000-1000-8000-00805f9b34fb
> > +  Value=020600002A
> > +
> > +  [0x0006]
> > +  UUID=00002a00-0000-1000-8000-00805f9b34fb
> > +  Value=4578616D706C6520446576696365
> 
> Is this our primary key, the handle?

Yep, the reasoning was that you can easily have multiple
profiles/services with the same UUID with GATT, so the UUID is no good
as a unique identifier (not that GLib even supports multiple groups with
the same name - it merges their content together to a single logical
group).

> > +Names file contains 1 [Names] group with device address as key:
> > +  [Names]
> > +  <nn:nn:nn:nn:nn:nn>=device name a
> > +  <mm:mm:mm:mm:mm:mm>=device name b
> 
> I am not sure this is the best idea this way. Maybe just creating files
> for each address we ever see is a better idea. Details like LastSeen
> could be useful for unpaired devices.

So instead of a cache file we'd have a cache directory? I'd be fine with
that.

> > +  Features=0000000000000000
> > +  AddressType=<BR/EDR|LE>
> > +  LEAddressType=<public|random>
> 
> That is not needed. It is encoded in the address itself.

Not quite true. You cannot distinguish between public and random. But
once you do know that it's random you *can* figure out what kind of
random it is from the two most significant bits of the address.

What we discussed on #bluez was that this could simply be a reference to
what address is indicated by the directory (or filename, if that's what
we go with) for the device storage, e.g. AddressType=<static|public> and
for private resolvable addresses (for which we'd have the public address
in storage) we'd need to check if we have any IRK's to know if we need
to look for a private address to connect to them. Additionally we could
have a SupportedTechnologies list like "BR/EDR,LE", "LE", etc (feel free
to suggest a better name if you want).

Johan

Re: [PATCH v3 01/10] doc: Add settings storage documentation

[Marcel Holtmann]

Hi Johan,

> > > +            ./attribute_db
> > > +        ./<remote device address>/
> > > +            ./settings
> > > +            ./keys
> > > +            ./attribute_db
> > > +        ...
> > 
> > Why did we want directories for remote devices again? Can we not just
> > put all in one big file with the name of the remote address?
> 
> For profile-specific storage this could be convenient (when removing the
> device we just remove the entire directory and the core daemon doesn't
> need to care about profiles details). OTOH the core needs to either way
> provide some API so the plugins know to write to the right location
> (be it a single file for the entire device or a single directory for the
> device).

fair enough. And a generic function to return the path for the device
specific storage location is a good idea anyway. So the profile does
need to care.

However we could also force the profile implementation to use a specific
file with such a function. And we could derive the name from the driver
name entry.

So we could return <device-bdaddr>-magic-profile for storage.

> > > +Adapter directory files
> > > +=======================
> > > +
> > > +Settings file contains 1 [General] group with adapter info:
> > > +  [General]
> > > +  Name=
> > > +  Class=0x000000
> > > +  Pairable=<true|false>
> > > +  PairableTimeout=0
> > > +  DiscoverableTimeout=0
> > > +  Mode=<off|connectable|discoverable>
> > > +  OnMode=<connectable|discoverable>
> > 
> > Actually with management interface now being used, we can be a bit
> > smarter here. The mode can be programmed even if the controller is not
> > powered.
> > 
> > So this might be better as Discoverable, Connectable, Pairable and
> > Powered boolean options.
> > 
> > Also Pairable has no timeout in the kernel API anymore. Do we still need
> > this?
> 
> We do have it currently as a documented adapter D-Bus property so that'd
> need to be removed too. I don't personally have a great need for this
> but I could imagine some device manufacturers deciding to have something
> like this (especially those with very simple UI's with a single button
> or so).

Fair enough. I was just asking if we still want the pairable timeout. I
have no problem with it.

> > > +The attribute_db file is a list of handles (group name) with UUID and Value as
> > > +keys, for example:
> > > +  [0x0001]
> > > +  UUID=00002800-0000-1000-8000-00805f9b34fb
> > > +  Value=0018
> > > +
> > > +  [0x0004]
> > > +  UUID=00002803-0000-1000-8000-00805f9b34fb
> > > +  Value=020600002A
> > > +
> > > +  [0x0006]
> > > +  UUID=00002a00-0000-1000-8000-00805f9b34fb
> > > +  Value=4578616D706C6520446576696365
> > 
> > Is this our primary key, the handle?
> 
> Yep, the reasoning was that you can easily have multiple
> profiles/services with the same UUID with GATT, so the UUID is no good
> as a unique identifier (not that GLib even supports multiple groups with
> the same name - it merges their content together to a single logical
> group).
> 
> > > +Names file contains 1 [Names] group with device address as key:
> > > +  [Names]
> > > +  <nn:nn:nn:nn:nn:nn>=device name a
> > > +  <mm:mm:mm:mm:mm:mm>=device name b
> > 
> > I am not sure this is the best idea this way. Maybe just creating files
> > for each address we ever see is a better idea. Details like LastSeen
> > could be useful for unpaired devices.
> 
> So instead of a cache file we'd have a cache directory? I'd be fine with
> that.

Something like that. Details need to be figured out, but I like to make
it bluntly clear that this data could be purged.

> 
> > > +  Features=0000000000000000
> > > +  AddressType=<BR/EDR|LE>
> > > +  LEAddressType=<public|random>
> > 
> > That is not needed. It is encoded in the address itself.
> 
> Not quite true. You cannot distinguish between public and random. But
> once you do know that it's random you *can* figure out what kind of
> random it is from the two most significant bits of the address.
> 
> What we discussed on #bluez was that this could simply be a reference to
> what address is indicated by the directory (or filename, if that's what
> we go with) for the device storage, e.g. AddressType=<static|public> and
> for private resolvable addresses (for which we'd have the public address
> in storage) we'd need to check if we have any IRK's to know if we need
> to look for a private address to connect to them. Additionally we could
> have a SupportedTechnologies list like "BR/EDR,LE", "LE", etc (feel free
> to suggest a better name if you want).

Since we will not store private random addresses, and we turn static
random addresses into a cache. So why do we still need this?

Regards

Marcel

Re: [PATCH v3 01/10] doc: Add settings storage documentation

[Johan Hedberg]

Hi Marcel,

On Thu, Oct 11, 2012, Marcel Holtmann wrote:
> > > > +  Features=0000000000000000
> > > > +  AddressType=<BR/EDR|LE>
> > > > +  LEAddressType=<public|random>
> > > 
> > > That is not needed. It is encoded in the address itself.
> > 
> > Not quite true. You cannot distinguish between public and random. But
> > once you do know that it's random you *can* figure out what kind of
> > random it is from the two most significant bits of the address.
> > 
> > What we discussed on #bluez was that this could simply be a reference to
> > what address is indicated by the directory (or filename, if that's what
> > we go with) for the device storage, e.g. AddressType=<static|public> and
> > for private resolvable addresses (for which we'd have the public address
> > in storage) we'd need to check if we have any IRK's to know if we need
> > to look for a private address to connect to them. Additionally we could
> > have a SupportedTechnologies list like "BR/EDR,LE", "LE", etc (feel free
> > to suggest a better name if you want).
> 
> Since we will not store private random addresses, and we turn static
> random addresses into a cache. So why do we still need this?

We will need to store exactly the same information for static random
addresses as for public ones, and not just the "core" data but also
profile-specific data. So why would you want to do it in two different
ways? Sounds like that just complicates the implementation for no
particular gain.

Johan

Re: [PATCH v3 01/10] doc: Add settings storage documentation

[Frederic Danis]

Hello Marcel,

On 10/10/2012 19:31, Marcel Holtmann wrote:
> Hi Fred,
>
>>   doc/settings-storage.txt |  102 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 102 insertions(+)
>>   create mode 100644 doc/settings-storage.txt
>>
>> diff --git a/doc/settings-storage.txt b/doc/settings-storage.txt
>> new file mode 100644
>> index 0000000..51d2220
>> --- /dev/null
>> +++ b/doc/settings-storage.txt
>> @@ -0,0 +1,102 @@
>> +Information related to local adapters and remote devices are stored in storage
>> +directory (default: /var/lib/bluetooth).
>> +Files are in ini-file format.
>> +
>> +There is one directory per adapter, named by its bluetooth address, which
>> +contains:
>> + - a settings file for the local adapter
>> + - an attribute_db file containing attributes of supported LE services
>> + - names file containing names of all devices already seen
>> + - one directory per remote device, named by remote device address, which
>> +   contains:
>> +    - a settings file
>> +    - a key file accessible only by root
>> +    - an attribute_db file containing attributes of remote LE services
>> +
>> +So the directory structure should be:
>> +    /var/lib/bluetooth/<adapter address>/
>> +        ./settings
>> +        ./attribute_db
>
> not sure this is a good name.

Are you OK with "attributes" name for this file?

>> +        ./names
>
> Lets call this one "cache". Since we could throw it away and it will be
> just rediscovered. And we can also stuff other cacheable details in
> there.

OK, I will replace it with cache directory, containing one file per 
remote device.

>> +        ./<remote device address>/
>> +            ./settings
>
> Besides the Alias, are they really settings. You can not configure much
> anyway.

Rename it "info"?

>> +            ./keys
>
> Not sure that I want the keys separated. That seems like a waste.

OK, I will merge it with previous file "info".

>> +            ./attribute_db
>> +        ./<remote device address>/
>> +            ./settings
>> +            ./keys
>> +            ./attribute_db
>> +        ...
>
> Why did we want directories for remote devices again? Can we not just
> put all in one big file with the name of the remote address?

Attribute_db (or whatever name we chose) stores the list of attributes 
advertised by the remote device.
In this file I use the handle value as group name (primary key) as we 
can get multiple entry with same UUID.

I think this should remain separated from "info" file, in order to parse 
it more easily (just need to parse all groups in it to retrieve complete 
attribute list).
That's why I think we should use one directory per remote device.

>> +
>> +Adapter directory files
>> +=======================
>> +
>> +Settings file contains 1 [General] group with adapter info:
>> +  [General]
>> +  Name=
>> +  Class=0x000000
>> +  Pairable=<true|false>
>> +  PairableTimeout=0
>> +  DiscoverableTimeout=0
>> +  Mode=<off|connectable|discoverable>
>> +  OnMode=<connectable|discoverable>
>
> Actually with management interface now being used, we can be a bit
> smarter here. The mode can be programmed even if the controller is not
> powered.
>
> So this might be better as Discoverable, Connectable, Pairable and
> Powered boolean options.

OK, I will replace Mode and OnMode keys by Discoverable, Connectable, 
Pairable and Powered keys.

> Also Pairable has no timeout in the kernel API anymore. Do we still need
> this?
>
>> +The attribute_db file is a list of handles (group name) with UUID and Value as
>> +keys, for example:
>> +  [0x0001]
>> +  UUID=00002800-0000-1000-8000-00805f9b34fb
>> +  Value=0018
>> +
>> +  [0x0004]
>> +  UUID=00002803-0000-1000-8000-00805f9b34fb
>> +  Value=020600002A
>> +
>> +  [0x0006]
>> +  UUID=00002a00-0000-1000-8000-00805f9b34fb
>> +  Value=4578616D706C6520446576696365
>
> Is this our primary key, the handle?
>
>> +
>> +Names file contains 1 [Names] group with device address as key:
>> +  [Names]
>> +  <nn:nn:nn:nn:nn:nn>=device name a
>> +  <mm:mm:mm:mm:mm:mm>=device name b
>
> I am not sure this is the best idea this way. Maybe just creating files
> for each address we ever see is a better idea. Details like LastSeen
> could be useful for unpaired devices.

OK
Regarding LastSeen key, is it interesting to keep it in device "info" 
file too?

So, we will open the corresponding cache/<device addr> file each time we 
need to resolve remote device name, right?

>
>> +
>> +Device directory files
>> +======================
>> +
>> +Remote device settings file will include a [General] group with device infos
>> +and a [DeviceID] group with related infos:
>> +  [General]
>> +  Alias=
>> +  Class=0x000000
>> +  Manufacturer=0
>> +  LmpVersion=
>> +  LmpSubversion=
>
> Take these 3 our. We were just storing them for statistic purposes.

Should I remove them?

>> +  Features=0000000000000000
>> +  AddressType=<BR/EDR|LE>
>> +  LEAddressType=<public|random>
>
> That is not needed. It is encoded in the address itself.
>
>> +  LastSeen=yyyy-mm-dd hh:mm:ss GMT
>> +  LastUsed=yyyy-mm-dd hh:mm:ss GMT
>> +  Trusted=<true|false>
>> +  Profiles=<128 bits UUID of profile 1>;<128 bits UUID of profile 2>;...
>> +
>> +  [DeviceID]
>> +  Assigner=0
>
> This is called Source.

OK, I will change this.

>> +  Vendor=0
>> +  Product=0
>> +  Version=0
>> +
>> +Keys file includes informations related to link key or long term link key:
>> +  [LinkKey]
>> +  Key=
>> +  Type=0
>> +  PINLength=0
>> +
>> +  [LongTermKey]
>> +  Key=
>> +  Authenticated=<true|false>
>> +  EncSize=0
>> +  EDiv=0
>> +  Rand=0
>
> Can be both in the same file about the device information.

OK, I will merge them.

Regards

Fred

-- 
Frederic Danis                            Open Source Technology Center
frederic.danis@intel.com                              Intel Corporation