Linux bluetooth development
 help / color / mirror / Atom feed
* Re: [PATCH] Bluetooth: Fix possible deadlock in SCO code
From: Chan-yeol Park @ 2012-11-28 14:35 UTC (permalink / raw)
  To: Gustavo Padovan; +Cc: linux-bluetooth
In-Reply-To: <1339738863-6407-1-git-send-email-gustavo@padovan.org>

Hi Gustavo

If we use the below patch, we face crash or circular locking dependency 
detected.
*It's very easily reproduced(about 100%)

I guess once sco_sock_shutdown() is called,"sk" would be destructed.
but due to response from remote side, sco_disconn_cfm(),sco_conn_del() 
would be called in order.
and finally in sco_conn_del() crash or circular locking dependency is 
happened.
because it access "sk" that is already destructed.

I think in sco_chan_del(), based on conn info, the relation between sk 
and conn should be cleaned
like the original code before you commit.

[  104.889622] Bluetooth: [sco_sock_shutdown] sock e8856000, sk eb695000
[  104.894666] Bluetooth: [sco_sock_clear_timer] sock eb695000 state 1
[  104.900869] Bluetooth: [__sco_sock_close] sk eb695000 state 1 socket 
e8856000
[  104.907976] Bluetooth: [sco_sock_set_timer] sock eb695000 state 8 
timeout 400
[  104.915106] Bluetooth: [sco_sock_release] sock e8856000, sk eb695000
[  104.921439] Bluetooth: [sco_sock_clear_timer] sock eb695000 state 8
[  104.927875] Bluetooth: [__sco_sock_close] sk eb695000 state 8 socket 
e8856000
[  104.938762] Bluetooth: [sco_chan_del] sk eb695000, conn ed38da60, err 104
[  104.956861] Bluetooth: [sco_sock_kill] sk eb695000 state 9
[  104.962321] Bluetooth: [sco_sock_destruct] sk eb695000
[  105.071125] Bluetooth: [sco_disconn_cfm] hcon ed376000 reason 22
[  105.075875] Bluetooth: [sco_conn_del] hcon ed376000 conn ed38da60, 
err 103
[  105.082848] Bluetooth: [sco_conn_del] before bh_lock_sock () sk eb695000

Could you give me your opinion?

regards
chanyeol

On 06/15/2012 02:41 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> sco_chan_del() only has conn != NULL when called from sco_conn_del() so
> just move the code from it that deal with conn to sco_conn_del().
>
> [  120.765529]
> [  120.765529] ======================================================
> [  120.766529] [ INFO: possible circular locking dependency detected ]
> [  120.766529] 3.5.0-rc1-10292-g3701f94-dirty #70 Tainted: G        W
> [  120.766529] -------------------------------------------------------
> [  120.766529] kworker/u:3/1497 is trying to acquire lock:
> [  120.766529]  (&(&conn->lock)->rlock#2){+.+...}, at:
> [<ffffffffa00b7ecc>] sco_chan_del+0x4c/0x170 [bluetooth]
> [  120.766529]
> [  120.766529] but task is already holding lock:
> [  120.766529]  (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at:
> [<ffffffffa00b8401>] sco_conn_del+0x61/0xe0 [bluetooth]
> [  120.766529]
> [  120.766529] which lock already depends on the new lock.
> [  120.766529]
> [  120.766529]
> [  120.766529] the existing dependency chain (in reverse order) is:
> [  120.766529]
> [  120.766529] -> #1 (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}:
> [  120.766529]        [<ffffffff8107980e>] lock_acquire+0x8e/0xb0
> [  120.766529]        [<ffffffff813c19e0>] _raw_spin_lock+0x40/0x80
> [  120.766529]        [<ffffffffa00b85e9>] sco_connect_cfm+0x79/0x300
> [bluetooth]
> [  120.766529]        [<ffffffffa0094b13>]
> hci_sync_conn_complete_evt.isra.90+0x343/0x400 [bluetooth]
> [  120.766529]        [<ffffffffa009d447>] hci_event_packet+0x317/0xfb0
> [bluetooth]
> [  120.766529]        [<ffffffffa008aa68>] hci_rx_work+0x2c8/0x890
> [bluetooth]
> [  120.766529]        [<ffffffff81047db7>] process_one_work+0x197/0x460
> [  120.766529]        [<ffffffff810489d6>] worker_thread+0x126/0x2d0
> [  120.766529]        [<ffffffff8104ee4d>] kthread+0x9d/0xb0
> [  120.766529]        [<ffffffff813c4294>] kernel_thread_helper+0x4/0x10
> [  120.766529]
> [  120.766529] -> #0 (&(&conn->lock)->rlock#2){+.+...}:
> [  120.766529]        [<ffffffff81078a8a>] __lock_acquire+0x154a/0x1d30
> [  120.766529]        [<ffffffff8107980e>] lock_acquire+0x8e/0xb0
> [  120.766529]        [<ffffffff813c19e0>] _raw_spin_lock+0x40/0x80
> [  120.766529]        [<ffffffffa00b7ecc>] sco_chan_del+0x4c/0x170
> [bluetooth]
> [  120.766529]        [<ffffffffa00b8414>] sco_conn_del+0x74/0xe0
> [bluetooth]
> [  120.766529]        [<ffffffffa00b88a2>] sco_disconn_cfm+0x32/0x60
> [bluetooth]
> [  120.766529]        [<ffffffffa0093a82>]
> hci_disconn_complete_evt.isra.53+0x242/0x390 [bluetooth]
> [  120.766529]        [<ffffffffa009d747>] hci_event_packet+0x617/0xfb0
> [bluetooth]
> [  120.766529]        [<ffffffffa008aa68>] hci_rx_work+0x2c8/0x890
> [bluetooth]
> [  120.766529]        [<ffffffff81047db7>] process_one_work+0x197/0x460
> [  120.766529]        [<ffffffff810489d6>] worker_thread+0x126/0x2d0
> [  120.766529]        [<ffffffff8104ee4d>] kthread+0x9d/0xb0
> [  120.766529]        [<ffffffff813c4294>] kernel_thread_helper+0x4/0x10
> [  120.766529]
> [  120.766529] other info that might help us debug this:
> [  120.766529]
> [  120.766529]  Possible unsafe locking scenario:
> [  120.766529]
> [  120.766529]        CPU0                    CPU1
> [  120.766529]        ----                    ----
> [  120.766529]   lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
> [  120.766529]
> lock(&(&conn->lock)->rlock#2);
> [  120.766529]
> lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
> [  120.766529]   lock(&(&conn->lock)->rlock#2);
> [  120.766529]
> [  120.766529]  *** DEADLOCK ***
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>   net/bluetooth/sco.c |   19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index cbdd313..596fdc8 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -149,6 +149,15 @@ static int sco_conn_del(struct hci_conn *hcon, int err)
>   		sco_sock_clear_timer(sk);
>   		sco_chan_del(sk, err);
>   		bh_unlock_sock(sk);
> +
> +		sco_conn_lock(conn);
> +		conn->sk = NULL;
> +		sco_pi(sk)->conn = NULL;
> +		sco_conn_unlock(conn);
> +
> +		if (conn->hcon)
> +			hci_conn_put(conn->hcon);
> +
>   		sco_sock_kill(sk);
>   	}
>   
> @@ -838,16 +847,6 @@ static void sco_chan_del(struct sock *sk, int err)
>   
>   	BT_DBG("sk %p, conn %p, err %d", sk, conn, err);
>   
> -	if (conn) {
> -		sco_conn_lock(conn);
> -		conn->sk = NULL;
> -		sco_pi(sk)->conn = NULL;
> -		sco_conn_unlock(conn);
> -
> -		if (conn->hcon)
> -			hci_conn_put(conn->hcon);
> -	}
> -
>   	sk->sk_state = BT_CLOSED;
>   	sk->sk_err   = err;
>   	sk->sk_state_change(sk);


^ permalink raw reply

* [PATCH v2 8/8] event: Store link key infos in new keys file
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/event.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/src/event.c b/src/event.c
index 7fc8f02..59da740 100644
--- a/src/event.c
+++ b/src/event.c
@@ -354,33 +354,68 @@ static int store_longtermkey(bdaddr_t *local, bdaddr_t *peer,
 	return err;
 }
 
+static void store_link_key(struct btd_adapter *adapter,
+				struct btd_device *device, uint8_t *key,
+				uint8_t type, uint8_t pin_length)
+{
+	char adapter_addr[18];
+	char device_addr[18];
+	char filename[PATH_MAX + 1];
+	GKeyFile *key_file;
+	char key_str[35];
+	char *str;
+	int i;
+	gsize length = 0;
+
+	ba2str(adapter_get_address(adapter), adapter_addr);
+	ba2str(device_get_address(device), device_addr);
+
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/keys", adapter_addr,
+								device_addr);
+	filename[PATH_MAX] = '\0';
+
+	key_file = g_key_file_new();
+	g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+	key_str[0] = '0';
+	key_str[1] = 'x';
+	for (i = 0; i < 16; i++)
+		sprintf(key_str + 2 + (i * 2), "%2.2X", key[i]);
+
+	g_key_file_set_string(key_file, "LinkKey", "Key", key_str);
+
+	g_key_file_set_integer(key_file, "LinkKey", "Type", type);
+	g_key_file_set_integer(key_file, "LinkKey", "PINLength", pin_length);
+
+	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);
+}
+
 int btd_event_link_key_notify(bdaddr_t *local, bdaddr_t *peer,
 				uint8_t *key, uint8_t key_type,
 				uint8_t pin_length)
 {
 	struct btd_adapter *adapter;
 	struct btd_device *device;
-	uint8_t peer_type;
-	int ret;
 
 	if (!get_adapter_and_device(local, peer, &adapter, &device, TRUE))
 		return -ENODEV;
 
 	DBG("storing link key of type 0x%02x", key_type);
 
-	peer_type = device_get_addr_type(device);
+	store_link_key(adapter, device, key, key_type, pin_length);
 
-	ret = write_link_key(local, peer, peer_type, key, key_type,
-								pin_length);
+	device_set_bonded(device, TRUE);
 
-	if (ret == 0) {
-		device_set_bonded(device, TRUE);
-
-		if (device_is_temporary(device))
-			device_set_temporary(device, FALSE);
-	}
+	if (device_is_temporary(device))
+		device_set_temporary(device, FALSE);
 
-	return ret;
+	return 0;
 }
 
 int btd_event_ltk_notify(bdaddr_t *local, bdaddr_t *peer, uint8_t bdaddr_type,
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 7/8] adapter: Upload link keys from new storage
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

Remove read_link_key() from device_create, this moves to load_devices.
---
 src/adapter.c |  109 +++++++++++++++++++++++++--------------------------------
 src/device.c  |    6 ----
 2 files changed, 48 insertions(+), 67 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 5bad0f9..a87eeda 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1777,31 +1777,35 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
 	return 0;
 }
 
-static struct link_key_info *get_key_info(const char *addr, const char *value)
+static struct link_key_info *get_key_info(const char *local,
+						const char *peer)
 {
-	struct link_key_info *info;
-	char tmp[3];
-	long int l;
+	struct link_key_info *info = NULL;
+	char filename[PATH_MAX + 1];
+	GKeyFile *key_file;
+	char *str;
 
-	if (strlen(value) < 36) {
-		error("Unexpectedly short (%zu) link key line", strlen(value));
-		return NULL;
-	}
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/keys", local, peer);
 
-	info = g_new0(struct link_key_info, 1);
+	key_file = g_key_file_new();
+	g_key_file_load_from_file(key_file, filename, 0, NULL);
 
-	str2ba(addr, &info->bdaddr);
+	str = g_key_file_get_string(key_file, "LinkKey", "Key", NULL);
+	if (!str || strlen(str) != 34)
+		goto failed;
 
-	str2buf(value, info->key, sizeof(info->key));
+	info = g_new0(struct link_key_info, 1);
+
+	str2ba(peer, &info->bdaddr);
+	str2buf(&str[2], info->key, sizeof(info->key));
 
-	memcpy(tmp, value + 33, 2);
-	info->type = (uint8_t) strtol(tmp, NULL, 10);
+	info->type = g_key_file_get_integer(key_file, "LinkKey", "Type", NULL);
+	info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
+						NULL);
 
-	memcpy(tmp, value + 35, 2);
-	l = strtol(tmp, NULL, 10);
-	if (l < 0)
-		l = 0;
-	info->pin_len = l;
+failed:
+	g_free(str);
+	g_key_file_free(key_file);
 
 	return info;
 }
@@ -1842,34 +1846,6 @@ static struct smp_ltk_info *get_ltk_info(const char *addr, uint8_t bdaddr_type,
 	return ltk;
 }
 
-static void create_stored_device_from_linkkeys(char *key, char *value,
-							void *user_data)
-{
-	char address[18];
-	uint8_t bdaddr_type;
-	struct adapter_keys *keys = user_data;
-	struct btd_adapter *adapter = keys->adapter;
-	struct btd_device *device;
-	struct link_key_info *info;
-
-	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) < 2)
-		bdaddr_type = BDADDR_BREDR;
-
-	info = get_key_info(address, value);
-	if (info)
-		keys->keys = g_slist_append(keys->keys, info);
-
-	if (g_slist_find_custom(adapter->devices, address,
-					(GCompareFunc) device_address_cmp))
-		return;
-
-	device = device_create(adapter, address, bdaddr_type);
-	if (device) {
-		device_set_temporary(device, FALSE);
-		adapter->devices = g_slist_append(adapter->devices, device);
-	}
-}
-
 static void create_stored_device_from_ltks(char *key, char *value,
 							void *user_data)
 {
@@ -2022,18 +1998,6 @@ static void load_devices(struct btd_adapter *adapter)
 	textfile_foreach(filename, create_stored_device_from_primaries,
 								adapter);
 
-	create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "linkkeys");
-	textfile_foreach(filename, create_stored_device_from_linkkeys, &keys);
-
-	err = mgmt_load_link_keys(adapter->dev_id, keys.keys,
-							main_opts.debug_keys);
-	if (err < 0)
-		error("Unable to load link keys: %s (%d)",
-							strerror(-err), -err);
-
-	g_slist_free_full(keys.keys, g_free);
-	keys.keys = NULL;
-
 	create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "longtermkeys");
 	textfile_foreach(filename, create_stored_device_from_ltks, &keys);
 
@@ -2058,13 +2022,22 @@ static void load_devices(struct btd_adapter *adapter)
 
 	while ((entry = readdir(dir)) != NULL) {
 		struct btd_device *device;
+		struct link_key_info *key_info;
+		GSList *l;
 
 		if (entry->d_type != DT_DIR || bachk(entry->d_name) < 0)
 			continue;
 
-		if (g_slist_find_custom(adapter->devices, entry->d_name,
-					(GCompareFunc) device_address_cmp))
-			continue;
+		key_info = get_key_info(srcaddr, entry->d_name);
+		if (key_info)
+			keys.keys = g_slist_append(keys.keys, key_info);
+
+		l = g_slist_find_custom(adapter->devices, entry->d_name,
+					(GCompareFunc) device_address_cmp);
+		if (l) {
+			device = l->data;
+			goto device_exist;
+		}
 
 		device = device_create(adapter, entry->d_name, BDADDR_BREDR);
 		if (!device)
@@ -2072,9 +2045,23 @@ static void load_devices(struct btd_adapter *adapter)
 
 		device_set_temporary(device, FALSE);
 		adapter->devices = g_slist_append(adapter->devices, device);
+
+device_exist:
+		if (key_info) {
+			device_set_paired(device, TRUE);
+			device_set_bonded(device, TRUE);
+		}
 	}
 
 	closedir(dir);
+
+	err = mgmt_load_link_keys(adapter->dev_id, keys.keys,
+							main_opts.debug_keys);
+	if (err < 0)
+		error("Unable to load link keys: %s (%d)",
+							strerror(-err), -err);
+
+	g_slist_free_full(keys.keys, g_free);
 }
 
 int btd_adapter_block_address(struct btd_adapter *adapter,
diff --git a/src/device.c b/src/device.c
index 9f05c1b..53d1220 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1905,12 +1905,6 @@ struct btd_device *device_create(struct btd_adapter *adapter,
 
 	load_info(device, srcaddr, address);
 
-	if (read_link_key(src, &device->bdaddr, device->bdaddr_type, NULL,
-								NULL) == 0) {
-		device_set_paired(device, TRUE);
-		device_set_bonded(device, TRUE);
-	}
-
 	if (device_is_le(device) && has_longtermkeys(src, &device->bdaddr,
 							device->bdaddr_type)) {
 		device_set_paired(device, TRUE);
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 6/8] device: Remove keys file in device_remove_stored()
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/device.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/device.c b/src/device.c
index f9927da..9f05c1b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2022,6 +2022,11 @@ static void device_remove_stored(struct btd_device *device)
 	filename[PATH_MAX] = '\0';
 	remove(filename);
 
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/keys", adapter_addr,
+			device_addr);
+	filename[PATH_MAX] = '\0';
+	remove(filename);
+
 	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s", adapter_addr,
 			device_addr);
 	filename[PATH_MAX] = '\0';
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 5/8] adapter: Convert storage linkkeys file
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/adapter.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index f9382fd..5bad0f9 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2644,6 +2644,34 @@ static void convert_did_entry(GKeyFile *key_file, void *value)
 	g_key_file_set_integer(key_file, "DeviceID", "Version", val);
 }
 
+static void convert_linkkey_entry(GKeyFile *key_file, void *value)
+{
+	char *type_str, *length_str, *str;
+	gint val;
+
+	type_str = strchr(value, ' ');
+	if (!type_str)
+		return;
+
+	*(type_str++) = 0;
+
+	length_str = strchr(type_str, ' ');
+	if (!length_str)
+		return;
+
+	*(length_str++) = 0;
+
+	str = g_strconcat("0x", value, NULL);
+	g_key_file_set_string(key_file, "LinkKey", "Key", str);
+	g_free(str);
+
+	val = strtol(type_str, NULL, 16);
+	g_key_file_set_integer(key_file, "LinkKey", "Type", val);
+
+	val = strtol(length_str, NULL, 16);
+	g_key_file_set_integer(key_file, "LinkKey", "PINLength", val);
+}
+
 static void convert_entry(char *key, char *value, void *user_data)
 {
 	struct device_converter *converter = user_data;
@@ -2744,6 +2772,9 @@ static void convert_device_storage(struct btd_adapter *adapter)
 
 	/* Convert device ids */
 	convert_file("did", address, "info", convert_did_entry);
+
+	/* Convert linkkeys */
+	convert_file("linkkeys", address, "keys", convert_linkkey_entry);
 }
 
 static void convert_config(struct btd_adapter *adapter, const char *filename,
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 4/8] adapter: Add destination file to convert_file()
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

In order to convert keys and longtermkeys, we will need to use
different target file names.
---
 src/adapter.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 1d6aa08..f9382fd 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2570,6 +2570,7 @@ static void convert_names_entry(char *key, char *value, void *user_data)
 struct device_converter {
 	char *address;
 	void (*cb)(GKeyFile *key_file, void *value);
+	char *filename;
 };
 
 static void convert_aliases_entry(GKeyFile *key_file, void *value)
@@ -2657,8 +2658,8 @@ static void convert_entry(char *key, char *value, void *user_data)
 	if (bachk(key) != 0)
 		return;
 
-	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/info",
-			converter->address, key);
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/%s",
+			converter->address, key, converter->filename);
 	filename[PATH_MAX] = '\0';
 	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
@@ -2673,7 +2674,7 @@ static void convert_entry(char *key, char *value, void *user_data)
 	g_key_file_free(key_file);
 }
 
-static void convert_file(char *file, char *address,
+static void convert_file(char *file, char *address, char *dest_file,
 				void (*cb)(GKeyFile *key_file, void *value))
 {
 	char filename[PATH_MAX + 1];
@@ -2689,6 +2690,7 @@ static void convert_file(char *file, char *address,
 	} else {
 		converter.address = address;
 		converter.cb = cb;
+		converter.filename = dest_file;
 
 		textfile_foreach(filename, convert_entry, &converter);
 		textfile_put(filename, "converted", "yes");
@@ -2718,10 +2720,10 @@ static void convert_device_storage(struct btd_adapter *adapter)
 	free(str);
 
 	/* Convert aliases */
-	convert_file("aliases", address, convert_aliases_entry);
+	convert_file("aliases", address, "info", convert_aliases_entry);
 
 	/* Convert trusts */
-	convert_file("trusts", address, convert_trusts_entry);
+	convert_file("trusts", address, "info", convert_trusts_entry);
 
 	/* Convert classes */
 	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/classes", address);
@@ -2738,10 +2740,10 @@ static void convert_device_storage(struct btd_adapter *adapter)
 
 
 	/* Convert blocked */
-	convert_file("blocked", address, convert_blocked_entry);
+	convert_file("blocked", address, "info", convert_blocked_entry);
 
 	/* Convert device ids */
-	convert_file("did", address, convert_did_entry);
+	convert_file("did", address, "info", convert_did_entry);
 }
 
 static void convert_config(struct btd_adapter *adapter, const char *filename,
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 3/8] adapter: Load devices from new storage architecture
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/adapter.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 01cf22b..1d6aa08 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -34,6 +34,7 @@
 #include <stdbool.h>
 #include <sys/ioctl.h>
 #include <sys/file.h>
+#include <dirent.h>
 
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/uuid.h>
@@ -2008,6 +2009,8 @@ static void load_devices(struct btd_adapter *adapter)
 	char srcaddr[18];
 	struct adapter_keys keys = { adapter, NULL };
 	int err;
+	DIR *dir;
+	struct dirent *entry;
 
 	ba2str(&adapter->bdaddr, srcaddr);
 
@@ -2043,6 +2046,35 @@ static void load_devices(struct btd_adapter *adapter)
 
 	create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "blocked");
 	textfile_foreach(filename, create_stored_device_from_blocked, adapter);
+
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s", srcaddr);
+	filename[PATH_MAX] = '\0';
+
+	dir = opendir(filename);
+	if (!dir) {
+		error("Unable to open adapter storage directory: %s", filename);
+		return;
+	}
+
+	while ((entry = readdir(dir)) != NULL) {
+		struct btd_device *device;
+
+		if (entry->d_type != DT_DIR || bachk(entry->d_name) < 0)
+			continue;
+
+		if (g_slist_find_custom(adapter->devices, entry->d_name,
+					(GCompareFunc) device_address_cmp))
+			continue;
+
+		device = device_create(adapter, entry->d_name, BDADDR_BREDR);
+		if (!device)
+			continue;
+
+		device_set_temporary(device, FALSE);
+		adapter->devices = g_slist_append(adapter->devices, device);
+	}
+
+	closedir(dir);
 }
 
 int btd_adapter_block_address(struct btd_adapter *adapter,
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 2/8] adapter: Convert class to cached file
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354109967-6238-1-git-send-email-frederic.danis@linux.intel.com>

Instead of converting remote device's class directly in
<device storage dir>/info we should saved it in storage
cache directory.
Use 'converted Yes' instead of 'converted yes' to be able
to do conversion even if classes file was already converted
to device directory.

Class should be also saved in cache directory on device
discovery.

When a device is created it should try to retrieve class
from device info file.
If that fails fall back to the cache and save it to device
info file.
---
 src/adapter.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++------
 src/device.c  |   18 +++++++++++++-----
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 163360f..01cf22b 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -253,8 +253,8 @@ static void store_adapter_info(struct btd_adapter *adapter)
 	g_key_file_free(key_file);
 }
 
-static void store_cached_name(const bdaddr_t *local, const bdaddr_t *peer,
-				char *name)
+static void store_cached_string(const bdaddr_t *local, const bdaddr_t *peer,
+				const char *key, const char *string)
 {
 	char filename[PATH_MAX + 1];
 	char s_addr[18], d_addr[18];
@@ -270,7 +270,7 @@ static void store_cached_name(const bdaddr_t *local, const bdaddr_t *peer,
 
 	key_file = g_key_file_new();
 	g_key_file_load_from_file(key_file, filename, 0, NULL);
-	g_key_file_set_string(key_file, "General", "Name", name);
+	g_key_file_set_string(key_file, "General", key, string);
 
 	data = g_key_file_to_data(key_file, &length, NULL);
 	g_file_set_contents(filename, data, length, NULL);
@@ -279,6 +279,18 @@ static void store_cached_name(const bdaddr_t *local, const bdaddr_t *peer,
 	g_key_file_free(key_file);
 }
 
+static void store_cached_name(const bdaddr_t *local, const bdaddr_t *peer,
+				char *name)
+{
+	store_cached_string(local, peer, "Name", name);
+}
+
+static void store_cached_class(const bdaddr_t *local, const bdaddr_t *peer,
+				char *class)
+{
+	store_cached_string(local, peer, "Class", class);
+}
+
 static struct session_req *session_ref(struct session_req *req)
 {
 	req->refcount++;
@@ -2538,9 +2550,21 @@ static void convert_trusts_entry(GKeyFile *key_file, void *value)
 	g_key_file_set_boolean(key_file, "General", "Trusted", TRUE);
 }
 
-static void convert_classes_entry(GKeyFile *key_file, void *value)
+static void convert_classes_entry(char *key, char *value, void *user_data)
 {
-	g_key_file_set_string(key_file, "General", "Class", value);
+	char *address = user_data;
+	char *str = key;
+	bdaddr_t local, peer;
+
+	if (strchr(key, '#'))
+		str[17] = '\0';
+
+	if (bachk(str) != 0)
+		return;
+
+	str2ba(address, &local);
+	str2ba(str, &peer);
+	store_cached_class(&local, &peer, value);
 }
 
 static void convert_blocked_entry(GKeyFile *key_file, void *value)
@@ -2668,7 +2692,18 @@ static void convert_device_storage(struct btd_adapter *adapter)
 	convert_file("trusts", address, convert_trusts_entry);
 
 	/* Convert classes */
-	convert_file("classes", address, convert_classes_entry);
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/classes", address);
+	filename[PATH_MAX] = '\0';
+
+	str = textfile_get(filename, "converted");
+	if (str && strcmp(str, "Yes") == 0) {
+		DBG("Legacy classes file already converted");
+	} else {
+		textfile_foreach(filename, convert_classes_entry, address);
+		textfile_put(filename, "converted", "Yes");
+	}
+	free(str);
+
 
 	/* Convert blocked */
 	convert_file("blocked", address, convert_blocked_entry);
@@ -3120,6 +3155,13 @@ void adapter_update_found_devices(struct btd_adapter *adapter,
 	if (eir_data.name != NULL && eir_data.name_complete)
 		store_cached_name(&adapter->bdaddr, bdaddr, eir_data.name);
 
+	if (eir_data.class != 0) {
+		char class[9];
+
+		sprintf(class, "0x%06X", eir_data.class);
+		store_cached_class(&adapter->bdaddr, bdaddr, class);
+	}
+
 	/* Avoid creating LE device if it's not discoverable */
 	if (bdaddr_type != BDADDR_BREDR &&
 			!(eir_data.flags & (EIR_LIM_DISC | EIR_GEN_DISC))) {
diff --git a/src/device.c b/src/device.c
index a99ca34..f9927da 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1753,8 +1753,8 @@ static void device_set_version(struct btd_device *device, uint16_t value)
 						DEVICE_INTERFACE, "Version");
 }
 
-static char *load_cached_name(struct btd_device *device, const char *local,
-				const gchar *peer)
+static char *load_cached_string(struct btd_device *device, const char *local,
+				const gchar *peer, const char *key)
 {
 	char filename[PATH_MAX + 1];
 	GKeyFile *key_file;
@@ -1769,7 +1769,7 @@ static char *load_cached_name(struct btd_device *device, const char *local,
 	if (!g_key_file_load_from_file(key_file, filename, 0, NULL))
 		goto failed;
 
-	str = g_key_file_get_string(key_file, "General", "Name", NULL);
+	str = g_key_file_get_string(key_file, "General", key, NULL);
 	if (str) {
 		len = strlen(str);
 		if (len > HCI_MAX_NAME_LENGTH)
@@ -1803,7 +1803,7 @@ static void load_info(struct btd_device *device, const gchar *local,
 	 */
 	str = g_key_file_get_string(key_file, "General", "Name", NULL);
 	if (str == NULL) {
-		str = load_cached_name(device, local, peer);
+		str = load_cached_string(device, local, peer, "Name");
 		if (str)
 			store_needed = TRUE;
 	}
@@ -1817,8 +1817,16 @@ static void load_info(struct btd_device *device, const gchar *local,
 	device->alias = g_key_file_get_string(key_file, "General", "Alias",
 									NULL);
 
-	/* Load class */
+	/* Load device class from storage info file, if that fails fall back to
+	 * the cache.
+	 */
 	str = g_key_file_get_string(key_file, "General", "Class", NULL);
+	if (str == NULL) {
+		str = load_cached_string(device, local, peer, "Class");
+		if (str)
+			store_needed = TRUE;
+	}
+
 	if (str) {
 		uint32_t class;
 
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH v2 1/8] doc: Update settings-storage.txt
From: Frédéric Danis @ 2012-11-28 13:39 UTC (permalink / raw)
  To: linux-bluetooth

Class should also be saved in device file of cache directory.

Link key and Long term key for a device should be saved in
keys file under storage device directory
---
 doc/settings-storage.txt |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/doc/settings-storage.txt b/doc/settings-storage.txt
index 3fdcb03..db271a6 100644
--- a/doc/settings-storage.txt
+++ b/doc/settings-storage.txt
@@ -30,6 +30,7 @@ contains:
  - one directory per remote device, named by remote device address, which
    contains:
     - an info file
+    - a keys file
     - an attributes file containing attributes of remote LE services
 
 So the directory structure is:
@@ -42,9 +43,11 @@ So the directory structure is:
             ...
         ./<remote device address>/
             ./info
+            ./keys
             ./attributes
         ./<remote device address>/
             ./info
+            ./keys
             ./attributes
         ...
 
@@ -124,11 +127,13 @@ This general group contains:
 
   ShortName	String		Remote device shortened name
 
+  Class		String		Device class in hexadecimal, i.e. 0x000000
+
 Info file format
 ================
 
-Info file may includes multiple groups (General, Device ID, Link key and
-Long term key) related to a remote device.
+Info file may includes multiple groups (General and Device ID) related to
+a remote device.
 
 [General] group contains:
 
@@ -165,6 +170,12 @@ Long term key) related to a remote device.
   Version		Integer		Device version
 
 
+Keys file format
+================
+
+Keys file may includes multiple groups (Link key and Long term key) related
+to a remote device.
+
 [LinkKey] group contains:
 
   Key			String		Key in hexadecimal format
-- 
1.7.9.5


^ permalink raw reply related

* Re: [PATCH bluez] wiimote: add Wii-Remote-Plus ID and name detection
From: David Herrmann @ 2012-11-28 13:00 UTC (permalink / raw)
  To: David Herrmann, linux-bluetooth@vger.kernel.org, Peter Olson
In-Reply-To: <20121128125041.GA995@x220.ger.corp.intel.com>

Hi Johan

On Wed, Nov 28, 2012 at 1:50 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi David,
>
> On Mon, Oct 22, 2012, David Herrmann wrote:
>> The Nintendo Wii Remote Plus uses a new product ID and name. To detect
>> them properly, we need to add them to the wiimote-module.
>>
>> To avoid an overlong "if" statement, this converts the match-function to
>> walk over an array and check all VID/PID pairs and device-names. This
>> makes adding new devices much easier.
>> ---
>> Hi Johan
>>
>> I am actually not sure why Nintendo changed the VID/PID for the new revisions of
>> the WiimotePlus. I have a WiimotePlus which still uses the old numbers and works
>> here quite well. However, I have now got multiple requests from people with the
>> new device name and IDs. Unfortunately, I cannot test these so I'd like to have
>> a "Tested-by" by Peter (CC'ed) before this is applied.
>>
>> Thanks
>> David
>>
>>  plugins/wiimote.c | 35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>
> Well, the Tested-by never came, but since the patch looks ok to me I
> went ahead and applied it anyway (with a minor fix to use G_N_ELEMENTS()
> instead of your custom array length calculation).

Sorry for the long delay. But I don't have the device so it sometimes
takes a bit longer. Anyway, it doesn't do any harm if we apply it so
yes, thanks for not forgetting about it.

Cheers
David

^ permalink raw reply

* Re: [PATCH bluez] wiimote: add Wii-Remote-Plus ID and name detection
From: Johan Hedberg @ 2012-11-28 12:50 UTC (permalink / raw)
  To: David Herrmann; +Cc: linux-bluetooth, Peter Olson
In-Reply-To: <1350894680-3947-1-git-send-email-dh.herrmann@googlemail.com>

Hi David,

On Mon, Oct 22, 2012, David Herrmann wrote:
> The Nintendo Wii Remote Plus uses a new product ID and name. To detect
> them properly, we need to add them to the wiimote-module.
> 
> To avoid an overlong "if" statement, this converts the match-function to
> walk over an array and check all VID/PID pairs and device-names. This
> makes adding new devices much easier.
> ---
> Hi Johan
> 
> I am actually not sure why Nintendo changed the VID/PID for the new revisions of
> the WiimotePlus. I have a WiimotePlus which still uses the old numbers and works
> here quite well. However, I have now got multiple requests from people with the
> new device name and IDs. Unfortunately, I cannot test these so I'd like to have
> a "Tested-by" by Peter (CC'ed) before this is applied.
> 
> Thanks
> David
> 
>  plugins/wiimote.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)

Well, the Tested-by never came, but since the patch looks ok to me I
went ahead and applied it anyway (with a minor fix to use G_N_ELEMENTS()
instead of your custom array length calculation).

Johan

^ permalink raw reply

* Re: [PATCH BlueZ] gdbus: Fix crash error when calling g_dbus_remove_all_watches
From: Johan Hedberg @ 2012-11-28 12:23 UTC (permalink / raw)
  To: Tomasz Bursztyka; +Cc: linux-bluetooth
In-Reply-To: <1345802959-11774-1-git-send-email-tomasz.bursztyka@linux.intel.com>

Hi Thomasz,

On Fri, Aug 24, 2012, Tomasz Bursztyka wrote:
> ---
> Hi,
> 
> While using gdbus on some other code, I found out that bug around g_dbus_remove_all_watches() usage.
> 
> Tomasz
> 
>  gdbus/watch.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index d749176..968a38a 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -298,6 +298,9 @@ static void filter_data_call_and_free(struct filter_data *data)
>  		g_free(cb);
>  	}
>  
> +	g_slist_free(data->callbacks);
> +	data->callbacks = NULL;
> +
>  	filter_data_free(data);
>  }

It seems this patch never got applied. Is it so that no-one else has
seen the issue. Could someone (through basic static analysis) confirm if
the patch is correct? It'd be nice if we could also have a back trace of
the crash in the commit message.

Johan

^ permalink raw reply

* Re: eSCO latency configuration
From: Arnaud Mouiche @ 2012-11-28  9:17 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <20121127111241.GA24125@x220>

Hi Johan,

On 11/27/2012 12:12 PM, Johan Hedberg wrote:
> Hi Arnaud,
>
> On Mon, Sep 05, 2011, Arnaud Mouiche wrote:
>> What could be a way to add the feature without breaking any API
>> 1) use the MTU of the socket or hdev ?
>> 2) add hdev entry for sco latency (which can be configured with
>> hciconfig like voice settings)
>> 3) something else ...
>> or
>> 4) no one cares about latency...
>>
>> PS: the same way, the retransmit effort is not configurable today.
> I'm assuming this is for HFP or HSP? At least I'm not aware of other
> significant profiles using (e)SCO. Since the HFP specification defines a
> set of recommended parameter combinations I don't think we necessarily
> need any user-space facing interface for this (with the exception of the
> mSBC/CVSD codec selection which is needed for HFP 1.6).
>
> Instead, the kernel could simply start with the S3 settings and fall
> back to S2 and finally S1 if failures are encountered during the
> connection setup. For mSBC the starting point would be T2 with a
> fallback to T1 in case of failure. Do you agree that this would be an
> acceptable solution?
> Johan

yes, I'm concerned by the HFP with mSBC/CVSD case, with multiple 
concurrent HFP sessions. So there is a need to avoid race conditions of 
the configuration (ie no configuration at the adapter level).

I found the BT_DEFER_SETUP feature from Frédéric to be a nice solution, 
since it gives more flexibility to the userland for this kind of 
configuration.
(up to now for my tests, I'm doing ugly things like forcing the kernel 
to not respond the eSCO setup, and do the response from userland.... )

I'm pretty busy at this time, but my goal may be to propose a way to 
configure the eSCO response on top of the BT_DEFER_SETUP.
With BT_DEFER_SETUP, accepting is done on the first 'recvmsg'. May be we 
can configure the socket (setsockopt, ioctl...) just before this first 
'recvmsg' to tell the kernel the real purpose of the socket.
For example, userland can provide the expected set of configuration 
(latency, bandwith, retransmissions, packets, voice settings). But I 
don't have a clear of how handling fallback indeed.

I'll be also interested to be able to reject the connection_request for 
"Limited Resources" reasons. The scenario using this feature is when 
multiple active calls are already managed by the device, and
routing a new voice stream is simply not possible (for the hardware 
and/or for the user).
(note: one limitation of the HFP specs is that the routing is not really 
agreed before opening the link. only the codec selection is negociated, 
but specifications are clear that we can't reject the CVSD codec, so a 
real rejection of the route is not possible)


just a [raw] idea I just have:

Module   --------------- Kernel --------------------------------- Userland

setsockopt( DEFER )
listen()

                            <-- socket in listen mode with DEFER option ---

   -- Connection_request -->
                             --- socket accept available ----------------->

A) [ accept case ] 
.............................................................
accept()

send() or
ioctl() or
setsockopt() or
better
                             <-------- provide the set of configuration 
items

recv()
  <-- Accept_synchronous_connection
-- Synchronous Connection Complete (OK or rjected) -->
-------------------------------> in case of failure
need to know the reason
for selecting a fallback
on next connection_request
attempt.




B) [ userland reject case ] 
.............................................................

accept()
close()

  <-- Reject_synchronous_connection (Limited Resources)





Note: Michael Knudsen seems to want to push also things concerning CSA2 
for codec configuration. I'm not really aware of what it imply and why 
it should be managed by the kernel. So I don't know how to put those 
things in the picture.


Arnaud


^ permalink raw reply

* [PATCH BlueZ 6/6] test: Convert simple-player to use DBus properties interface
From: Luiz Augusto von Dentz @ 2012-11-27 12:51 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354020685-17028-1-git-send-email-luiz.dentz@gmail.com>

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 test/simple-player | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/test/simple-player b/test/simple-player
index 9e72f3e..0037f3a 100755
--- a/test/simple-player
+++ b/test/simple-player
@@ -17,14 +17,16 @@ class Player(dbus.service.Object):
 			bus = dbus.SystemBus()
 			mp = dbus.Interface(bus.get_object("org.bluez", obj),
 						"org.bluez.MediaPlayer")
+			prop = dbus.Interface(bus.get_object("org.bluez", obj),
+						"org.freedesktop.DBus.Properties")
 
-			self.properties = mp.GetProperties()
+			self.properties = prop.GetAll("org.bluez.MediaPlayer")
 			self.metadata = mp.GetTrack()
 
-			bus.add_signal_receiver(self.property_changed,
+			bus.add_signal_receiver(self.properties_changed,
 				path = obj,
-				dbus_interface = "org.bluez.MediaPlayer",
-				signal_name = "PropertyChanged")
+				dbus_interface = "org.freedesktop.DBus.Properties",
+				signal_name = "PropertiesChanged")
 
 			bus.add_signal_receiver(self.track_changed,
 				path = obj,
@@ -59,18 +61,20 @@ class Player(dbus.service.Object):
 		print("Release")
 		mainloop.quit()
 
-	@dbus.service.method("org.bluez.MediaPlayer",
-					in_signature="sv", out_signature="")
-	def SetProperty(self, key, value):
-		print("SetProperty (%s, %s)" % (key, value), file=sys.stderr)
+	@dbus.service.method("org.freedesktop.DBus.Properties",
+					in_signature="ssv", out_signature="")
+	def Set(self, interface, key, value):
+		print("Set (%s, %s)" % (key, value), file=sys.stderr)
 		return
 
-	@dbus.service.signal("org.bluez.MediaPlayer", signature="sv")
-	def PropertyChanged(self, setting, value):
-		"""PropertyChanged(setting, value)
+	@dbus.service.signal("org.freedesktop.DBus.Properties",
+							signature="sa{sv}as")
+	def PropertiesChanged(self, interface, properties,
+						invalidated = dbus.Array()):
+		"""PropertiesChanged(interface, properties, invalidated)
 
-		Send a PropertyChanged signal. 'setting' and 'value' are
-		string parameters as specified in doc/media-api.txt.
+		Send a PropertiesChanged signal. 'properties' is a dictionary
+		containing string parameters as specified in doc/media-api.txt.
 		"""
 		pass
 
@@ -86,10 +90,10 @@ class Player(dbus.service.Object):
 	def help(self, func):
 		help(self.__class__.__dict__[func])
 
-	def property_changed(self, setting, value):
-		print("property_changed(%s, %s)" % (setting, value))
+	def properties_changed(self, interface, properties, invalidated):
+		print("properties_changed(%s, %s)" % (properties, invalidated))
 
-		self.PropertyChanged(setting, value)
+		self.PropertiesChanged(interface, properties, invalidated)
 
 	def track_changed(self, metadata):
 		print("track_changed(%s)" % (metadata))
@@ -98,8 +102,8 @@ class Player(dbus.service.Object):
 
 class InputHandler:
 	commands = { 'TrackChanged': '(metadata)',
-					'PropertyChanged': '(key, value)',
-					'help': '(cmd)' }
+			'PropertiesChanged': '(interface, properties)',
+			'help': '(cmd)' }
 	def __init__(self, player):
 		self.player = player
 		print('\n\nAvailable commands:')
-- 
1.7.11.7


^ permalink raw reply related

* [PATCH BlueZ 5/6] test: Convert mpris-player to use DBus properties interface
From: Luiz Augusto von Dentz @ 2012-11-27 12:51 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354020685-17028-1-git-send-email-luiz.dentz@gmail.com>

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 test/mpris-player.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/test/mpris-player.c b/test/mpris-player.c
index 13f5c85..7985cdd 100644
--- a/test/mpris-player.c
+++ b/test/mpris-player.c
@@ -114,17 +114,18 @@ static void dict_append_entry(DBusMessageIter *dict, const char *key, int type,
 	dbus_message_iter_close_container(dict, &entry);
 }
 
-static dbus_bool_t emit_property_changed(DBusConnection *conn,
+static dbus_bool_t emit_properties_changed(DBusConnection *conn,
 					const char *path,
 					const char *interface,
 					const char *name,
 					int type, void *value)
 {
 	DBusMessage *signal;
-	DBusMessageIter iter;
+	DBusMessageIter iter, dict, entry, array;
 	dbus_bool_t result;
 
-	signal = dbus_message_new_signal(path, interface, "PropertyChanged");
+	signal = dbus_message_new_signal(path, DBUS_INTERFACE_PROPERTIES,
+							"PropertiesChanged");
 
 	if (!signal) {
 		fprintf(stderr, "Unable to allocate new %s.PropertyChanged"
@@ -133,10 +134,23 @@ static dbus_bool_t emit_property_changed(DBusConnection *conn,
 	}
 
 	dbus_message_iter_init_append(signal, &iter);
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &interface);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+			DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+			DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_VARIANT_AS_STRING
+			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
+
+	dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY, NULL,
+								&entry);
+	dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &name);
+	append_variant(&entry, type, value);
+	dbus_message_iter_close_container(&dict, &entry);
 
-	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &name);
+	dbus_message_iter_close_container(&iter, &dict);
 
-	append_variant(&iter, type, value);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+				DBUS_TYPE_STRING_AS_STRING, &array);
+	dbus_message_iter_close_container(&iter, &array);
 
 	result = dbus_connection_send(conn, signal, NULL);
 	dbus_message_unref(signal);
@@ -171,7 +185,7 @@ static int parse_property(DBusConnection *conn, const char *path,
 			dict_append_entry(properties, "Status",
 						DBUS_TYPE_STRING, &value);
 		else
-			emit_property_changed(sys, path,
+			emit_properties_changed(sys, path,
 					"org.bluez.MediaPlayer", "Status",
 					DBUS_TYPE_STRING, &value);
 	} else if (strcasecmp(key, "Position") == 0) {
@@ -188,7 +202,7 @@ static int parse_property(DBusConnection *conn, const char *path,
 			dict_append_entry(properties, "Position",
 						DBUS_TYPE_UINT32, &msec);
 		else
-			emit_property_changed(sys, path,
+			emit_properties_changed(sys, path,
 					"org.bluez.MediaPlayer", "Position",
 					DBUS_TYPE_UINT32, &msec);
 	} else if (strcasecmp(key, "Shuffle") == 0) {
@@ -206,9 +220,9 @@ static int parse_property(DBusConnection *conn, const char *path,
 			dict_append_entry(properties, "Shuffle",
 						DBUS_TYPE_STRING, &str);
 		else
-			emit_property_changed(sys, path,
+			emit_properties_changed(sys, path,
 					"org.bluez.MediaPlayer", "Shuffle",
-					DBUS_TYPE_UINT32, &str);
+					DBUS_TYPE_STRING, &str);
 	}
 
 	return 0;
-- 
1.7.11.7


^ permalink raw reply related

* [PATCH BlueZ 4/6] media-api: Update documentation of MediaPlayer interface
From: Luiz Augusto von Dentz @ 2012-11-27 12:51 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354020685-17028-1-git-send-email-luiz.dentz@gmail.com>

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 doc/media-api.txt | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/doc/media-api.txt b/doc/media-api.txt
index b4f2fc6..a814b60 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -133,24 +133,12 @@ Object path	freely definable (Target role)
 		[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/playerX
 		(Controller role)
 
-Methods		dict GetProperties()
-
-			Returns all properties for the interface. See the
-			properties section for available properties.
-
-		dict GetTrack()
+Methods		dict GetTrack()
 
 			Returns known metadata of the current track.
 
 			See TrackChanged for possible values.
 
-		void SetProperty(string property, variant value)
-
-			Changes the value of the specified property. Only
-			properties that are listed as read-write can be changed.
-
-			On success this will emit a PropertyChanged signal.
-
 		void Release()
 
 			This method gets called when the service daemon
@@ -159,12 +147,7 @@ Methods		dict GetProperties()
 			player, because when this method gets called it has
 			already been unregistered.
 
-Signals		PropertyChanged(string setting, variant value)
-
-			This signal indicates a changed value of the given
-			property.
-
-		TrackChanged(dict metadata)
+Signals		TrackChanged(dict metadata)
 
 			This signal indicates that current track has changed.
 			All available metadata for the new track shall be set
-- 
1.7.11.7


^ permalink raw reply related

* [PATCH BlueZ 3/6] media: Convert target MediaPlayer interface to use D-Bus Properties
From: Luiz Augusto von Dentz @ 2012-11-27 12:51 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354020685-17028-1-git-send-email-luiz.dentz@gmail.com>

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 profiles/audio/media.c | 100 ++++++++++++++++++++++---------------------------
 1 file changed, 45 insertions(+), 55 deletions(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 588d61e..f2fba7b 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -96,7 +96,7 @@ struct media_player {
 	GHashTable		*settings;	/* Player settings */
 	GHashTable		*track;		/* Player current track */
 	guint			watch;
-	guint			property_watch;
+	guint			properties_watch;
 	guint			track_watch;
 	char			*status;
 	uint32_t		position;
@@ -916,7 +916,7 @@ static void media_player_free(gpointer data)
 	}
 
 	g_dbus_remove_watch(conn, mp->watch);
-	g_dbus_remove_watch(conn, mp->property_watch);
+	g_dbus_remove_watch(conn, mp->properties_watch);
 	g_dbus_remove_watch(conn, mp->track_watch);
 
 	if (mp->track)
@@ -980,6 +980,7 @@ static const char *get_setting(const char *key, void *user_data)
 static int set_setting(const char *key, const char *value, void *user_data)
 {
 	struct media_player *mp = user_data;
+	const char *iface = MEDIA_PLAYER_INTERFACE;
 	DBusMessage *msg;
 	DBusMessageIter iter, var;
 
@@ -989,14 +990,14 @@ static int set_setting(const char *key, const char *value, void *user_data)
 		return -EINVAL;
 
 	msg = dbus_message_new_method_call(mp->sender, mp->path,
-						MEDIA_PLAYER_INTERFACE,
-						"SetProperty");
+					DBUS_INTERFACE_PROPERTIES, "Set");
 	if (msg == NULL) {
 		error("Couldn't allocate D-Bus message");
 		return -ENOMEM;
 	}
 
 	dbus_message_iter_init_append(msg, &iter);
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &iface);
 	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &key);
 
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_VARIANT,
@@ -1218,29 +1219,55 @@ static gboolean set_player_property(struct media_player *mp, const char *key,
 	return TRUE;
 }
 
-static gboolean property_changed(DBusConnection *connection, DBusMessage *msg,
+static gboolean parse_player_properties(struct media_player *mp,
+							DBusMessageIter *iter)
+{
+	DBusMessageIter dict;
+	int ctype;
+
+	ctype = dbus_message_iter_get_arg_type(iter);
+	if (ctype != DBUS_TYPE_ARRAY)
+		return FALSE;
+
+	dbus_message_iter_recurse(iter, &dict);
+
+	while ((ctype = dbus_message_iter_get_arg_type(&dict)) !=
+							DBUS_TYPE_INVALID) {
+		DBusMessageIter entry;
+		const char *key;
+
+		if (ctype != DBUS_TYPE_DICT_ENTRY)
+			return FALSE;
+
+		dbus_message_iter_recurse(&dict, &entry);
+		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
+			return FALSE;
+
+		dbus_message_iter_get_basic(&entry, &key);
+		dbus_message_iter_next(&entry);
+
+		if (set_player_property(mp, key, &entry) == FALSE)
+			return FALSE;
+
+		dbus_message_iter_next(&dict);
+	}
+
+	return TRUE;
+}
+
+static gboolean properties_changed(DBusConnection *connection, DBusMessage *msg,
 							void *user_data)
 {
 	struct media_player *mp = user_data;
 	DBusMessageIter iter;
-	const char *property;
 
 	DBG("sender=%s path=%s", mp->sender, mp->path);
 
 	dbus_message_iter_init(msg, &iter);
 
-	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING) {
-		error("Unexpected signature in %s.%s signal",
-					dbus_message_get_interface(msg),
-					dbus_message_get_member(msg));
-		return TRUE;
-	}
-
-	dbus_message_iter_get_basic(&iter, &property);
-
 	dbus_message_iter_next(&iter);
 
-	set_player_property(mp, property, &iter);
+	parse_player_properties(mp, &iter);
 
 	return TRUE;
 }
@@ -1407,10 +1434,9 @@ static struct media_player *media_player_create(struct media_adapter *adapter,
 	mp->watch = g_dbus_add_disconnect_watch(conn, sender,
 						media_player_exit, mp,
 						NULL);
-	mp->property_watch = g_dbus_add_signal_watch(conn, sender,
+	mp->properties_watch = g_dbus_add_properties_watch(conn, sender,
 						path, MEDIA_PLAYER_INTERFACE,
-						"PropertyChanged",
-						property_changed,
+						properties_changed,
 						mp, NULL);
 	mp->track_watch = g_dbus_add_signal_watch(conn, sender,
 						path, MEDIA_PLAYER_INTERFACE,
@@ -1439,42 +1465,6 @@ static struct media_player *media_player_create(struct media_adapter *adapter,
 	return mp;
 }
 
-static gboolean parse_player_properties(struct media_player *mp,
-							DBusMessageIter *iter)
-{
-	DBusMessageIter dict;
-	int ctype;
-
-	ctype = dbus_message_iter_get_arg_type(iter);
-	if (ctype != DBUS_TYPE_ARRAY)
-		return FALSE;
-
-	dbus_message_iter_recurse(iter, &dict);
-
-	while ((ctype = dbus_message_iter_get_arg_type(&dict)) !=
-							DBUS_TYPE_INVALID) {
-		DBusMessageIter entry;
-		const char *key;
-
-		if (ctype != DBUS_TYPE_DICT_ENTRY)
-			return FALSE;
-
-		dbus_message_iter_recurse(&dict, &entry);
-		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
-			return FALSE;
-
-		dbus_message_iter_get_basic(&entry, &key);
-		dbus_message_iter_next(&entry);
-
-		if (set_player_property(mp, key, &entry) == FALSE)
-			return FALSE;
-
-		dbus_message_iter_next(&dict);
-	}
-
-	return TRUE;
-}
-
 static DBusMessage *register_player(DBusConnection *conn, DBusMessage *msg,
 					void *data)
 {
-- 
1.7.11.7


^ permalink raw reply related

* [PATCH BlueZ 2/6] audio: Convert controller MediaPlayer interface to use D-Bus Properties
From: Luiz Augusto von Dentz @ 2012-11-27 12:51 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354020685-17028-1-git-send-email-luiz.dentz@gmail.com>

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 profiles/audio/player.c | 222 +++++++++++++++++++++++-------------------------
 1 file changed, 106 insertions(+), 116 deletions(-)

diff --git a/profiles/audio/player.c b/profiles/audio/player.c
index e83b761..34b1f20 100644
--- a/profiles/audio/player.c
+++ b/profiles/audio/player.c
@@ -50,7 +50,7 @@ struct player_callback {
 };
 
 struct pending_req {
-	DBusMessage *msg;
+	GDBusPendingPropertySet id;
 	const char *key;
 	const char *value;
 };
@@ -67,13 +67,6 @@ struct media_player {
 	GSList			*pending;
 };
 
-static void append_settings(void *key, void *value, void *user_data)
-{
-	DBusMessageIter *dict = user_data;
-
-	dict_append_entry(dict, key, DBUS_TYPE_STRING, &value);
-}
-
 static void append_metadata(void *key, void *value, void *user_data)
 {
 	DBusMessageIter *dict = user_data;
@@ -89,39 +82,6 @@ static void append_metadata(void *key, void *value, void *user_data)
 	dict_append_entry(dict, key, DBUS_TYPE_STRING, &value);
 }
 
-static DBusMessage *media_player_get_properties(DBusConnection *conn,
-						DBusMessage *msg, void *data)
-{
-	struct media_player *mp = data;
-	DBusMessage *reply;
-	DBusMessageIter iter, dict;
-	uint32_t position;
-
-	reply = dbus_message_new_method_return(msg);
-	if (!reply)
-		return NULL;
-
-	dbus_message_iter_init_append(reply, &iter);
-
-	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
-					DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
-					DBUS_TYPE_STRING_AS_STRING
-					DBUS_TYPE_VARIANT_AS_STRING
-					DBUS_DICT_ENTRY_END_CHAR_AS_STRING,
-					&dict);
-
-	position = media_player_get_position(mp);
-	dict_append_entry(&dict, "Position", DBUS_TYPE_UINT32, &position);
-
-	dict_append_entry(&dict, "Status", DBUS_TYPE_STRING, &mp->status);
-
-	g_hash_table_foreach(mp->settings, append_settings, &dict);
-
-	dbus_message_iter_close_container(&iter, &dict);
-
-	return reply;
-}
-
 static DBusMessage *media_player_get_track(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
@@ -164,111 +124,142 @@ static struct pending_req *find_pending(struct media_player *mp,
 	return NULL;
 }
 
-static struct pending_req *pending_new(DBusMessage *msg, const char *key,
-							const char *value)
+static struct pending_req *pending_new(GDBusPendingPropertySet id,
+					const char *key, const char *value)
 {
 	struct pending_req *p;
 
 	p = g_new0(struct pending_req, 1);
-	p->msg = dbus_message_ref(msg);
+	p->id = id;
 	p->key = key;
 	p->value = value;
 
 	return p;
 }
 
-static DBusMessage *player_set_setting(struct media_player *mp,
-					DBusMessage *msg, const char *key,
-					const char *value)
+static gboolean get_position(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
 {
-	struct player_callback *cb = mp->cb;
-	struct pending_req *p;
+	struct media_player *mp = data;
 
-	if (cb == NULL || cb->cbs->set_setting == NULL)
-		return btd_error_not_supported(msg);
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT32, &mp->position);
 
-	p = find_pending(mp, key);
-	if (p != NULL)
-		return btd_error_in_progress(msg);
+	return TRUE;
+}
 
-	if (!cb->cbs->set_setting(mp, key, value, cb->user_data))
-		return btd_error_invalid_args(msg);
+static gboolean status_exists(const GDBusPropertyTable *property, void *data)
+{
+	struct media_player *mp = data;
+
+	return mp->status != NULL;
+}
 
-	p = pending_new(msg, key, value);
+static gboolean get_status(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct media_player *mp = data;
 
-	mp->pending = g_slist_append(mp->pending, p);
+	if (mp->status == NULL)
+		return FALSE;
 
-	return NULL;
+	DBG("%s", mp->status);
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &mp->status);
+
+	return TRUE;
 }
 
-static DBusMessage *media_player_set_property(DBusConnection *conn,
-						DBusMessage *msg, void *data)
+static gboolean setting_exists(const GDBusPropertyTable *property, void *data)
 {
 	struct media_player *mp = data;
-	DBusMessageIter iter;
-	DBusMessageIter var;
-	const char *key, *value, *curval;
 
-	if (!dbus_message_iter_init(msg, &iter))
-		return btd_error_invalid_args(msg);
+	return g_hash_table_contains(mp->settings, property->name);
+}
 
-	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
-		return btd_error_invalid_args(msg);
+static gboolean get_setting(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct media_player *mp = data;
+	const char *value;
 
-	dbus_message_iter_get_basic(&iter, &key);
-	dbus_message_iter_next(&iter);
+	value = g_hash_table_lookup(mp->settings, property->name);
+	if (value == NULL)
+		return FALSE;
 
-	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
-		return btd_error_invalid_args(msg);
+	DBG("%s %s", property->name, value);
 
-	dbus_message_iter_recurse(&iter, &var);
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &value);
 
-	if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
-		return btd_error_invalid_args(msg);
+	return TRUE;
+}
 
-	dbus_message_iter_get_basic(&var, &value);
+static void player_set_setting(struct media_player *mp,
+					GDBusPendingPropertySet id,
+					const char *key, const char *value)
+{
+	struct player_callback *cb = mp->cb;
+	struct pending_req *p;
 
-	if (g_strcmp0(key, "Equalizer") != 0 &&
-				g_strcmp0(key, "Repeat") != 0 &&
-				g_strcmp0(key, "Shuffle") != 0 &&
-				g_strcmp0(key, "Scan") != 0)
-		return btd_error_invalid_args(msg);
+	if (cb == NULL || cb->cbs->set_setting == NULL)
+		return g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".NotSupported",
+					"Operation is not supported");
 
-	curval = g_hash_table_lookup(mp->settings, key);
-	if (g_strcmp0(curval, value) == 0)
-		return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+	p = find_pending(mp, key);
+	if (p != NULL)
+		return g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".InProgress",
+					"Operation already in progress");
 
-	return player_set_setting(mp, msg, key, value);
+	if (!cb->cbs->set_setting(mp, key, value, cb->user_data))
+		return g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".InvalidArguments",
+					"Invalid arguments in method call");
+
+	p = pending_new(id, key, value);
+
+	mp->pending = g_slist_append(mp->pending, p);
+}
+
+static void set_setting(const GDBusPropertyTable *property,
+			DBusMessageIter *iter, GDBusPendingPropertySet id,
+			void *data)
+{
+	struct media_player *mp = data;
+	const char *value;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+		return g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".InvalidArguments",
+					"Invalid arguments in method call");
+
+	dbus_message_iter_get_basic(iter, &value);
+
+	player_set_setting(mp, id, property->name, value);
 }
 
 static const GDBusMethodTable media_player_methods[] = {
-	{ GDBUS_METHOD("GetProperties",
-			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
-			media_player_get_properties) },
 	{ GDBUS_METHOD("GetTrack",
 			NULL, GDBUS_ARGS({ "metadata", "a{sv}" }),
 			media_player_get_track) },
-	{ GDBUS_ASYNC_METHOD("SetProperty",
-			GDBUS_ARGS({ "name", "s" }, { "value", "v" }),
-			NULL, media_player_set_property) },
 	{ }
 };
 
 static const GDBusSignalTable media_player_signals[] = {
-	{ GDBUS_SIGNAL("PropertyChanged",
-			GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
 	{ GDBUS_SIGNAL("TrackChanged",
 			GDBUS_ARGS({ "metadata", "a{sv}" })) },
 	{ }
 };
 
-static void pending_free(void *data)
-{
-	struct pending_req *p = data;
-
-	dbus_message_unref(p->msg);
-	g_free(p);
-}
+static const GDBusPropertyTable media_player_properties[] = {
+	{ "Position", "u", get_position },
+	{ "Status", "s", get_status, NULL, status_exists },
+	{ "Equalizer", "s", get_setting, set_setting, setting_exists },
+	{ "Repeat", "s", get_setting, set_setting, setting_exists },
+	{ "Shuffle", "s", get_setting, set_setting, setting_exists },
+	{ "Scan", "s", get_setting, set_setting, setting_exists },
+	{ }
+};
 
 void media_player_destroy(struct media_player *mp)
 {
@@ -286,7 +277,7 @@ void media_player_destroy(struct media_player *mp)
 	if (mp->process_id > 0)
 		g_source_remove(mp->process_id);
 
-	g_slist_free_full(mp->pending, pending_free);
+	g_slist_free_full(mp->pending, g_free);
 
 	g_timer_destroy(mp->progress);
 	g_free(mp->cb);
@@ -311,7 +302,7 @@ struct media_player *media_player_controller_create(const char *path)
 					mp->path, MEDIA_PLAYER_INTERFACE,
 					media_player_methods,
 					media_player_signals,
-					NULL, mp, NULL)) {
+					media_player_properties, mp, NULL)) {
 		error("D-Bus failed to register %s path", mp->path);
 		media_player_destroy(mp);
 		return NULL;
@@ -345,8 +336,8 @@ void media_player_set_position(struct media_player *mp, uint32_t position)
 	mp->position = position;
 	g_timer_start(mp->progress);
 
-	emit_property_changed(mp->path, MEDIA_PLAYER_INTERFACE, "Position",
-					DBUS_TYPE_UINT32, &mp->position);
+	g_dbus_emit_property_changed(btd_get_dbus_connection(), mp->path,
+					MEDIA_PLAYER_INTERFACE, "Position");
 }
 
 void media_player_set_setting(struct media_player *mp, const char *key,
@@ -354,7 +345,6 @@ void media_player_set_setting(struct media_player *mp, const char *key,
 {
 	char *curval;
 	struct pending_req *p;
-	DBusMessage *reply;
 
 	DBG("%s: %s", key, value);
 
@@ -363,7 +353,8 @@ void media_player_set_setting(struct media_player *mp, const char *key,
 		if (p == NULL)
 			return;
 
-		reply = btd_error_failed(p->msg, value);
+		g_dbus_pending_property_error(p->id, ERROR_INTERFACE ".Failed",
+									value);
 		goto send;
 	}
 
@@ -372,9 +363,8 @@ void media_player_set_setting(struct media_player *mp, const char *key,
 		goto done;
 
 	g_hash_table_replace(mp->settings, g_strdup(key), g_strdup(value));
-
-	emit_property_changed(mp->path, MEDIA_PLAYER_INTERFACE, key,
-					DBUS_TYPE_STRING, &value);
+	g_dbus_emit_property_changed(btd_get_dbus_connection(), mp->path,
+					MEDIA_PLAYER_INTERFACE, key);
 
 done:
 	p = find_pending(mp, key);
@@ -382,15 +372,15 @@ done:
 		return;
 
 	if (strcasecmp(value, p->value) == 0)
-		reply = g_dbus_create_reply(p->msg, DBUS_TYPE_INVALID);
+		g_dbus_pending_property_success(p->id);
 	else
-		reply = btd_error_not_supported(p->msg);
+		g_dbus_pending_property_error(p->id,
+					ERROR_INTERFACE ".NotSupported",
+					"Operation is not supported");
 
 send:
-	g_dbus_send_message(btd_get_dbus_connection(), reply);
-
 	mp->pending = g_slist_remove(mp->pending, p);
-	pending_free(p);
+	g_free(p);
 
 	return;
 }
@@ -410,8 +400,8 @@ void media_player_set_status(struct media_player *mp, const char *status)
 	g_free(mp->status);
 	mp->status = g_strdup(status);
 
-	emit_property_changed(mp->path, MEDIA_PLAYER_INTERFACE, "Status",
-					DBUS_TYPE_STRING, &status);
+	g_dbus_emit_property_changed(btd_get_dbus_connection(), mp->path,
+					MEDIA_PLAYER_INTERFACE, "Status");
 
 	mp->position = media_player_get_position(mp);
 	g_timer_start(mp->progress);
-- 
1.7.11.7


^ permalink raw reply related

* [PATCH BlueZ 1/6] gdbus: Add g_dbus_add_properties_watch function
From: Luiz Augusto von Dentz @ 2012-11-27 12:51 UTC (permalink / raw)
  To: linux-bluetooth

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Convenient function to create watches for D-Bus properties.
---
 gdbus/gdbus.h |  5 +++++
 gdbus/watch.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index ba49621..8b6dfe5 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -243,6 +243,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 				const char *interface, const char *member,
 				GDBusSignalFunction function, void *user_data,
 				GDBusDestroyFunction destroy);
+guint g_dbus_add_properties_watch(DBusConnection *connection,
+				const char *sender, const char *path,
+				const char *interface,
+				GDBusSignalFunction function, void *user_data,
+				GDBusDestroyFunction destroy);
 gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
 void g_dbus_remove_all_watches(DBusConnection *connection);
 
diff --git a/gdbus/watch.c b/gdbus/watch.c
index 1cd1211..9e4f994 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -752,6 +752,34 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
 	return cb->id;
 }
 
+guint g_dbus_add_properties_watch(DBusConnection *connection,
+				const char *sender, const char *path,
+				const char *interface,
+				GDBusSignalFunction function, void *user_data,
+				GDBusDestroyFunction destroy)
+{
+	struct filter_data *data;
+	struct filter_callback *cb;
+
+	data = filter_data_get(connection, signal_filter, sender, path,
+				DBUS_INTERFACE_PROPERTIES, "PropertiesChanged",
+				interface);
+	if (data == NULL)
+		return 0;
+
+	cb = filter_data_add_callback(data, NULL, NULL, function, destroy,
+					user_data);
+	if (cb == NULL)
+		return 0;
+
+	if (data->name != NULL && data->name_watch == 0)
+		data->name_watch = g_dbus_add_service_watch(connection,
+							data->name, NULL,
+							NULL, NULL, NULL);
+
+	return cb->id;
+}
+
 gboolean g_dbus_remove_watch(DBusConnection *connection, guint id)
 {
 	struct filter_data *data;
-- 
1.7.11.7


^ permalink raw reply related

* Re: eSCO latency configuration
From: Johan Hedberg @ 2012-11-27 11:12 UTC (permalink / raw)
  To: Arnaud Mouiche; +Cc: linux-bluetooth
In-Reply-To: <4E64AD1D.6000609@invoxia.com>

Hi Arnaud,

On Mon, Sep 05, 2011, Arnaud Mouiche wrote:
> What could be a way to add the feature without breaking any API
> 1) use the MTU of the socket or hdev ?
> 2) add hdev entry for sco latency (which can be configured with
> hciconfig like voice settings)
> 3) something else ...
> or
> 4) no one cares about latency...
> 
> PS: the same way, the retransmit effort is not configurable today.

I'm assuming this is for HFP or HSP? At least I'm not aware of other
significant profiles using (e)SCO. Since the HFP specification defines a
set of recommended parameter combinations I don't think we necessarily
need any user-space facing interface for this (with the exception of the
mSBC/CVSD codec selection which is needed for HFP 1.6).

Instead, the kernel could simply start with the S3 settings and fall
back to S2 and finally S1 if failures are encountered during the
connection setup. For mSBC the starting point would be T2 with a
fallback to T1 in case of failure. Do you agree that this would be an
acceptable solution?

Johan

^ permalink raw reply

* Re: [PATCH 7/7] event: Store link key infos in new keys file
From: Johan Hedberg @ 2012-11-27 11:03 UTC (permalink / raw)
  To: Frédéric Danis; +Cc: linux-bluetooth
In-Reply-To: <1354006193-7199-7-git-send-email-frederic.danis@linux.intel.com>

Hi Frederic,

On Tue, Nov 27, 2012, Frédéric Danis wrote:
> ---
>  src/event.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 47 insertions(+), 12 deletions(-)

After these patches there's the following leak:

==23252== 32,808 bytes in 1 blocks are definitely lost in loss record 210 of 210
==23252==    at 0x4A0883C: malloc (vg_replace_malloc.c:270)
==23252==    by 0x565E25F: __alloc_dir (opendir.c:199)
==23252==    by 0x175382: load_devices (adapter.c:2005)
==23252==    by 0x178E50: adapter_init (adapter.c:2896)
==23252==    by 0x17391E: btd_manager_register_adapter (manager.c:334)
==23252==    by 0x1883E1: mgmt_event.part.36 (mgmt.c:1081)
==23252==    by 0x4C78A74: g_main_context_dispatch (gmain.c:2715)
==23252==    by 0x4C78DA7: g_main_context_iterate.isra.24 (gmain.c:3290)
==23252==    by 0x4C791A1: g_main_loop_run (gmain.c:3484)
==23252==    by 0x121640: main (main.c:544)

That seems to be simple missing closedir() call. Didn't I already tell
you to always use valgrind?

Another problem with these is that I'm getting over a hundred unexpected
device objects which I didn't have before. It seems that this is because
you convert the classes file into the device storage instead of the
cache. It might be due to some other info as well though.

Johan

^ permalink raw reply

* [PATCH 7/7] event: Store link key infos in new keys file
From: Frédéric Danis @ 2012-11-27  8:49 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354006193-7199-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/event.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/src/event.c b/src/event.c
index 7fc8f02..59da740 100644
--- a/src/event.c
+++ b/src/event.c
@@ -354,33 +354,68 @@ static int store_longtermkey(bdaddr_t *local, bdaddr_t *peer,
 	return err;
 }
 
+static void store_link_key(struct btd_adapter *adapter,
+				struct btd_device *device, uint8_t *key,
+				uint8_t type, uint8_t pin_length)
+{
+	char adapter_addr[18];
+	char device_addr[18];
+	char filename[PATH_MAX + 1];
+	GKeyFile *key_file;
+	char key_str[35];
+	char *str;
+	int i;
+	gsize length = 0;
+
+	ba2str(adapter_get_address(adapter), adapter_addr);
+	ba2str(device_get_address(device), device_addr);
+
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/keys", adapter_addr,
+								device_addr);
+	filename[PATH_MAX] = '\0';
+
+	key_file = g_key_file_new();
+	g_key_file_load_from_file(key_file, filename, 0, NULL);
+
+	key_str[0] = '0';
+	key_str[1] = 'x';
+	for (i = 0; i < 16; i++)
+		sprintf(key_str + 2 + (i * 2), "%2.2X", key[i]);
+
+	g_key_file_set_string(key_file, "LinkKey", "Key", key_str);
+
+	g_key_file_set_integer(key_file, "LinkKey", "Type", type);
+	g_key_file_set_integer(key_file, "LinkKey", "PINLength", pin_length);
+
+	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);
+}
+
 int btd_event_link_key_notify(bdaddr_t *local, bdaddr_t *peer,
 				uint8_t *key, uint8_t key_type,
 				uint8_t pin_length)
 {
 	struct btd_adapter *adapter;
 	struct btd_device *device;
-	uint8_t peer_type;
-	int ret;
 
 	if (!get_adapter_and_device(local, peer, &adapter, &device, TRUE))
 		return -ENODEV;
 
 	DBG("storing link key of type 0x%02x", key_type);
 
-	peer_type = device_get_addr_type(device);
+	store_link_key(adapter, device, key, key_type, pin_length);
 
-	ret = write_link_key(local, peer, peer_type, key, key_type,
-								pin_length);
+	device_set_bonded(device, TRUE);
 
-	if (ret == 0) {
-		device_set_bonded(device, TRUE);
-
-		if (device_is_temporary(device))
-			device_set_temporary(device, FALSE);
-	}
+	if (device_is_temporary(device))
+		device_set_temporary(device, FALSE);
 
-	return ret;
+	return 0;
 }
 
 int btd_event_ltk_notify(bdaddr_t *local, bdaddr_t *peer, uint8_t bdaddr_type,
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH 6/7] adapter: Upload link keys from new storage
From: Frédéric Danis @ 2012-11-27  8:49 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354006193-7199-1-git-send-email-frederic.danis@linux.intel.com>

Remove read_link_key() from device_create, this moves to load_devices.
---
 src/adapter.c |  109 +++++++++++++++++++++++++--------------------------------
 src/device.c  |    6 ----
 2 files changed, 48 insertions(+), 67 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 32d3930..d473508 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1765,31 +1765,35 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
 	return 0;
 }
 
-static struct link_key_info *get_key_info(const char *addr, const char *value)
+static struct link_key_info *get_key_info(const char *local,
+						const char *peer)
 {
-	struct link_key_info *info;
-	char tmp[3];
-	long int l;
+	struct link_key_info *info = NULL;
+	char filename[PATH_MAX + 1];
+	GKeyFile *key_file;
+	char *str;
 
-	if (strlen(value) < 36) {
-		error("Unexpectedly short (%zu) link key line", strlen(value));
-		return NULL;
-	}
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/keys", local, peer);
 
-	info = g_new0(struct link_key_info, 1);
+	key_file = g_key_file_new();
+	g_key_file_load_from_file(key_file, filename, 0, NULL);
 
-	str2ba(addr, &info->bdaddr);
+	str = g_key_file_get_string(key_file, "LinkKey", "Key", NULL);
+	if (!str || strlen(str) != 34)
+		goto failed;
 
-	str2buf(value, info->key, sizeof(info->key));
+	info = g_new0(struct link_key_info, 1);
+
+	str2ba(peer, &info->bdaddr);
+	str2buf(&str[2], info->key, sizeof(info->key));
 
-	memcpy(tmp, value + 33, 2);
-	info->type = (uint8_t) strtol(tmp, NULL, 10);
+	info->type = g_key_file_get_integer(key_file, "LinkKey", "Type", NULL);
+	info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
+						NULL);
 
-	memcpy(tmp, value + 35, 2);
-	l = strtol(tmp, NULL, 10);
-	if (l < 0)
-		l = 0;
-	info->pin_len = l;
+failed:
+	g_free(str);
+	g_key_file_free(key_file);
 
 	return info;
 }
@@ -1830,34 +1834,6 @@ static struct smp_ltk_info *get_ltk_info(const char *addr, uint8_t bdaddr_type,
 	return ltk;
 }
 
-static void create_stored_device_from_linkkeys(char *key, char *value,
-							void *user_data)
-{
-	char address[18];
-	uint8_t bdaddr_type;
-	struct adapter_keys *keys = user_data;
-	struct btd_adapter *adapter = keys->adapter;
-	struct btd_device *device;
-	struct link_key_info *info;
-
-	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) < 2)
-		bdaddr_type = BDADDR_BREDR;
-
-	info = get_key_info(address, value);
-	if (info)
-		keys->keys = g_slist_append(keys->keys, info);
-
-	if (g_slist_find_custom(adapter->devices, address,
-					(GCompareFunc) device_address_cmp))
-		return;
-
-	device = device_create(adapter, address, bdaddr_type);
-	if (device) {
-		device_set_temporary(device, FALSE);
-		adapter->devices = g_slist_append(adapter->devices, device);
-	}
-}
-
 static void create_stored_device_from_ltks(char *key, char *value,
 							void *user_data)
 {
@@ -2010,18 +1986,6 @@ static void load_devices(struct btd_adapter *adapter)
 	textfile_foreach(filename, create_stored_device_from_primaries,
 								adapter);
 
-	create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "linkkeys");
-	textfile_foreach(filename, create_stored_device_from_linkkeys, &keys);
-
-	err = mgmt_load_link_keys(adapter->dev_id, keys.keys,
-							main_opts.debug_keys);
-	if (err < 0)
-		error("Unable to load link keys: %s (%d)",
-							strerror(-err), -err);
-
-	g_slist_free_full(keys.keys, g_free);
-	keys.keys = NULL;
-
 	create_name(filename, PATH_MAX, STORAGEDIR, srcaddr, "longtermkeys");
 	textfile_foreach(filename, create_stored_device_from_ltks, &keys);
 
@@ -2046,13 +2010,22 @@ static void load_devices(struct btd_adapter *adapter)
 
 	while ((entry = readdir(dir)) != NULL) {
 		struct btd_device *device;
+		struct link_key_info *key_info;
+		GSList *l;
 
 		if (entry->d_type != DT_DIR || bachk(entry->d_name) < 0)
 			continue;
 
-		if (g_slist_find_custom(adapter->devices, entry->d_name,
-					(GCompareFunc) device_address_cmp))
-			continue;
+		key_info = get_key_info(srcaddr, entry->d_name);
+		if (key_info)
+			keys.keys = g_slist_append(keys.keys, key_info);
+
+		l = g_slist_find_custom(adapter->devices, entry->d_name,
+					(GCompareFunc) device_address_cmp);
+		if (l) {
+			device = l->data;
+			goto device_exist;
+		}
 
 		device = device_create(adapter, entry->d_name, BDADDR_BREDR);
 		if (!device)
@@ -2060,7 +2033,21 @@ static void load_devices(struct btd_adapter *adapter)
 
 		device_set_temporary(device, FALSE);
 		adapter->devices = g_slist_append(adapter->devices, device);
+
+device_exist:
+		if (key_info) {
+			device_set_paired(device, TRUE);
+			device_set_bonded(device, TRUE);
+		}
 	}
+
+	err = mgmt_load_link_keys(adapter->dev_id, keys.keys,
+							main_opts.debug_keys);
+	if (err < 0)
+		error("Unable to load link keys: %s (%d)",
+							strerror(-err), -err);
+
+	g_slist_free_full(keys.keys, g_free);
 }
 
 int btd_adapter_block_address(struct btd_adapter *adapter,
diff --git a/src/device.c b/src/device.c
index 6b64c5d..d6780ee 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1897,12 +1897,6 @@ struct btd_device *device_create(struct btd_adapter *adapter,
 
 	load_info(device, srcaddr, address);
 
-	if (read_link_key(src, &device->bdaddr, device->bdaddr_type, NULL,
-								NULL) == 0) {
-		device_set_paired(device, TRUE);
-		device_set_bonded(device, TRUE);
-	}
-
 	if (device_is_le(device) && has_longtermkeys(src, &device->bdaddr,
 							device->bdaddr_type)) {
 		device_set_paired(device, TRUE);
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH 5/7] device: Remove keys file in device_remove_stored()
From: Frédéric Danis @ 2012-11-27  8:49 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354006193-7199-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/device.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/device.c b/src/device.c
index a99ca34..6b64c5d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2014,6 +2014,11 @@ static void device_remove_stored(struct btd_device *device)
 	filename[PATH_MAX] = '\0';
 	remove(filename);
 
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s/keys", adapter_addr,
+			device_addr);
+	filename[PATH_MAX] = '\0';
+	remove(filename);
+
 	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s", adapter_addr,
 			device_addr);
 	filename[PATH_MAX] = '\0';
-- 
1.7.9.5


^ permalink raw reply related

* [PATCH 4/7] adapter: Convert storage linkkeys file
From: Frédéric Danis @ 2012-11-27  8:49 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1354006193-7199-1-git-send-email-frederic.danis@linux.intel.com>

---
 src/adapter.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 9d6554a..32d3930 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2618,6 +2618,34 @@ static void convert_did_entry(GKeyFile *key_file, void *value)
 	g_key_file_set_integer(key_file, "DeviceID", "Version", val);
 }
 
+static void convert_linkkey_entry(GKeyFile *key_file, void *value)
+{
+	char *type_str, *length_str, *str;
+	gint val;
+
+	type_str = strchr(value, ' ');
+	if (!type_str)
+		return;
+
+	*(type_str++) = 0;
+
+	length_str = strchr(type_str, ' ');
+	if (!length_str)
+		return;
+
+	*(length_str++) = 0;
+
+	str = g_strconcat("0x", value, NULL);
+	g_key_file_set_string(key_file, "LinkKey", "Key", str);
+	g_free(str);
+
+	val = strtol(type_str, NULL, 16);
+	g_key_file_set_integer(key_file, "LinkKey", "Type", val);
+
+	val = strtol(length_str, NULL, 16);
+	g_key_file_set_integer(key_file, "LinkKey", "PINLength", val);
+}
+
 static void convert_entry(char *key, char *value, void *user_data)
 {
 	struct device_converter *converter = user_data;
@@ -2707,6 +2735,9 @@ static void convert_device_storage(struct btd_adapter *adapter)
 
 	/* Convert device ids */
 	convert_file("did", address, "info", convert_did_entry);
+
+	/* Convert linkkeys */
+	convert_file("linkkeys", address, "keys", convert_linkkey_entry);
 }
 
 static void convert_config(struct btd_adapter *adapter, const char *filename,
-- 
1.7.9.5


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox