[PATCH BlueZ v0 0/6] Exploit btd_service's userdata pointer

[PATCH BlueZ v0 0/6] Exploit btd_service's userdata pointer

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

With the adoption of btd_service, profile implementations can benefit from the provided userdata pointer which can replace the previously necessary lists to associate services with custom private types.

The proposed change not only removes boilerplate code but also data redundancy, presumably making the codebase more robust.

This patchset addresses the first examples which looked like more obvious to refactor. Note that some of the profiles were not tested due to lack of hardware.

Patch 2/6 fixes an issue that I came across during this work.

Mikel Astiz (6):
  network: Remove duplicated search
  network: Fix missing NULL check for given UUID
  network: Remove find_connection()
  deviceinfo: Use btd_service userdata pointer
  scanparam: Use btd_service userdata pointer
  input: Use btd_service userdata pointer

 profiles/deviceinfo/deviceinfo.c | 46 ++++++----------------------------------
 profiles/input/device.c          | 37 +++++++++-----------------------
 profiles/network/connection.c    | 38 ++++++++++++---------------------
 profiles/scanparam/scan.c        | 36 ++++++-------------------------
 4 files changed, 35 insertions(+), 122 deletions(-)

-- 
1.8.1.4

[PATCH BlueZ v0 1/6] network: Remove duplicated search

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Commit 0624791ea6e917d6c9ecb8e7e6e5a1327199448d seems to accidentally
have introduced this duplicated search as part of a non-trivial revert.
---
 profiles/network/connection.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/profiles/network/connection.c b/profiles/network/connection.c
index 9481072..84f3dd6 100644
--- a/profiles/network/connection.c
+++ b/profiles/network/connection.c
@@ -433,10 +433,6 @@ static DBusMessage *local_connect(DBusConnection *conn,
 	if (err < 0)
 		return btd_error_failed(msg, strerror(-err));
 
-	nc = find_connection(peer->connections, id);
-	if (!nc)
-		return btd_error_failed(msg, strerror(-err));
-
 	nc->connect = dbus_message_ref(msg);
 
 	return NULL;
-- 
1.8.1.4

[PATCH BlueZ v0 2/6] network: Fix missing NULL check for given UUID

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The code dereferences a NULL pointer if find_connection() doesn't find
an existing connection, which will be the case if the input UUID is
invalid or not supported.
---
 profiles/network/connection.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/profiles/network/connection.c b/profiles/network/connection.c
index 84f3dd6..bac3b69 100644
--- a/profiles/network/connection.c
+++ b/profiles/network/connection.c
@@ -426,7 +426,10 @@ static DBusMessage *local_connect(DBusConnection *conn,
 	id = bnep_service_id(svc);
 
 	nc = find_connection(peer->connections, id);
-	if (nc && nc->connect)
+	if (nc == NULL)
+		return btd_error_invalid_args(msg);
+
+	if (nc->connect != NULL)
 		return btd_error_busy(msg);
 
 	err = connection_connect(nc->service);
-- 
1.8.1.4

[PATCH BlueZ v0 3/6] network: Remove find_connection()

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The userdata pointer in btd_service provides the necessary information
to find which service should be connected. This makes possible to remove
the restriction of having one single UUID instance per profile.
---
 profiles/network/connection.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/profiles/network/connection.c b/profiles/network/connection.c
index bac3b69..960a1fe 100644
--- a/profiles/network/connection.c
+++ b/profiles/network/connection.c
@@ -102,18 +102,6 @@ static struct network_peer *find_peer(GSList *list, struct btd_device *device)
 	return NULL;
 }
 
-static struct network_conn *find_connection(GSList *list, uint16_t id)
-{
-	for (; list; list = list->next) {
-		struct network_conn *nc = list->data;
-
-		if (nc->id == id)
-			return nc;
-	}
-
-	return NULL;
-}
-
 static struct network_conn *find_connection_by_state(GSList *list,
 							conn_state state)
 {
@@ -414,8 +402,10 @@ static DBusMessage *local_connect(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
 	struct network_peer *peer = data;
+	struct btd_service *service;
 	struct network_conn *nc;
 	const char *svc;
+	const char *uuid;
 	uint16_t id;
 	int err;
 
@@ -424,11 +414,17 @@ static DBusMessage *local_connect(DBusConnection *conn,
 		return btd_error_invalid_args(msg);
 
 	id = bnep_service_id(svc);
+	uuid = bnep_uuid(id);
 
-	nc = find_connection(peer->connections, id);
-	if (nc == NULL)
+	if (uuid == NULL)
 		return btd_error_invalid_args(msg);
 
+	service = btd_device_get_service(peer->device, uuid);
+	if (service == NULL)
+		return btd_error_not_supported(msg);
+
+	nc = btd_service_get_user_data(service);
+
 	if (nc->connect != NULL)
 		return btd_error_busy(msg);
 
@@ -694,13 +690,6 @@ int connection_register(struct btd_service *service)
 		peers = g_slist_append(peers, peer);
 	}
 
-	nc = find_connection(peer->connections, id);
-	if (nc) {
-		error("Device %s has multiple connection instances of %d",
-						device_get_path(device), id);
-		return -1;
-	}
-
 	nc = g_new0(struct network_conn, 1);
 	nc->id = id;
 	memset(nc->dev, 0, sizeof(nc->dev));
-- 
1.8.1.4

[PATCH BlueZ v0 4/6] deviceinfo: Use btd_service userdata pointer

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Avoid maintaining an internal list of probed deviceinfo instances by
making use of btd_service's userdata pointer.
---
 profiles/deviceinfo/deviceinfo.c | 46 ++++++----------------------------------
 1 file changed, 6 insertions(+), 40 deletions(-)

diff --git a/profiles/deviceinfo/deviceinfo.c b/profiles/deviceinfo/deviceinfo.c
index e8cb5f7..8343bb5 100644
--- a/profiles/deviceinfo/deviceinfo.c
+++ b/profiles/deviceinfo/deviceinfo.c
@@ -51,16 +51,14 @@ struct deviceinfo {
 	GSList			*chars;		/* Characteristics */
 };
 
-static GSList *servers = NULL;
-
 struct characteristic {
 	struct gatt_char	attr;	/* Characteristic */
 	struct deviceinfo	*d;	/* deviceinfo where the char belongs */
 };
 
-static void deviceinfo_free(gpointer user_data)
+static void deviceinfo_driver_remove(struct btd_service *service)
 {
-	struct deviceinfo *d = user_data;
+	struct deviceinfo *d = btd_service_get_user_data(service);
 
 	if (d->attioid > 0)
 		btd_device_remove_attio_callback(d->dev, d->attioid);
@@ -75,17 +73,6 @@ static void deviceinfo_free(gpointer user_data)
 	g_free(d);
 }
 
-static int cmp_device(gconstpointer a, gconstpointer b)
-{
-	const struct deviceinfo *d = a;
-	const struct btd_device *dev = b;
-
-	if (dev == d->dev)
-		return 0;
-
-	return -1;
-}
-
 static void read_pnpid_cb(guint8 status, const guint8 *pdu, guint16 len,
 							gpointer user_data)
 {
@@ -166,9 +153,10 @@ static void attio_disconnected_cb(gpointer user_data)
 	d->attrib = NULL;
 }
 
-static int deviceinfo_register(struct btd_device *device,
+static int deviceinfo_register(struct btd_service *service,
 						struct gatt_primary *prim)
 {
+	struct btd_device *device = btd_service_get_device(service);
 	struct deviceinfo *d;
 
 	d = g_new0(struct deviceinfo, 1);
@@ -177,28 +165,13 @@ static int deviceinfo_register(struct btd_device *device,
 	d->svc_range->start = prim->range.start;
 	d->svc_range->end = prim->range.end;
 
-	servers = g_slist_prepend(servers, d);
+	btd_service_set_user_data(service, d);
 
 	d->attioid = btd_device_add_attio_callback(device, attio_connected_cb,
 						attio_disconnected_cb, d);
 	return 0;
 }
 
-static void deviceinfo_unregister(struct btd_device *device)
-{
-	struct deviceinfo *d;
-	GSList *l;
-
-	l = g_slist_find_custom(servers, device, cmp_device);
-	if (l == NULL)
-		return;
-
-	d = l->data;
-	servers = g_slist_remove(servers, d);
-
-	deviceinfo_free(d);
-}
-
 static int deviceinfo_driver_probe(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
@@ -208,14 +181,7 @@ static int deviceinfo_driver_probe(struct btd_service *service)
 	if (prim == NULL)
 		return -EINVAL;
 
-	return deviceinfo_register(device, prim);
-}
-
-static void deviceinfo_driver_remove(struct btd_service *service)
-{
-	struct btd_device *device = btd_service_get_device(service);
-
-	deviceinfo_unregister(device);
+	return deviceinfo_register(service, prim);
 }
 
 static struct btd_profile deviceinfo_profile = {
-- 
1.8.1.4

[PATCH BlueZ v0 5/6] scanparam: Use btd_service userdata pointer

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Avoid maintaining an internal list of probed struct scan instances by
making use of btd_service's userdata pointer.
---
 profiles/scanparam/scan.c | 36 ++++++------------------------------
 1 file changed, 6 insertions(+), 30 deletions(-)

diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index a9baec8..fc4779f 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -64,16 +64,6 @@ struct scan {
 	uint16_t refresh_cb_id;
 };
 
-static GSList *servers = NULL;
-
-static int scan_device_cmp(gconstpointer a, gconstpointer b)
-{
-	const struct scan *scan = a;
-	const struct btd_device *device = b;
-
-	return (device == scan->device ? 0 : -1);
-}
-
 static void write_scan_params(GAttrib *attrib, uint16_t handle)
 {
 	uint8_t value[4];
@@ -227,8 +217,9 @@ static void attio_disconnected_cb(gpointer user_data)
 	scan->attrib = NULL;
 }
 
-static int scan_register(struct btd_device *device, struct gatt_primary *prim)
+static int scan_register(struct btd_service *service, struct gatt_primary *prim)
 {
+	struct btd_device *device = btd_service_get_device(service);
 	struct scan *scan;
 
 	scan = g_new0(struct scan, 1);
@@ -239,22 +230,14 @@ static int scan_register(struct btd_device *device, struct gatt_primary *prim)
 							attio_disconnected_cb,
 							scan);
 
-	servers = g_slist_prepend(servers, scan);
+	btd_service_set_user_data(service, scan);
 
 	return 0;
 }
 
-static void scan_unregister(struct btd_device *device)
+static void scan_param_remove(struct btd_service *service)
 {
-	struct scan *scan;
-	GSList *l;
-
-	l = g_slist_find_custom(servers, device, scan_device_cmp);
-	if (l == NULL)
-		return;
-
-	scan = l->data;
-	servers = g_slist_remove(servers, scan);
+	struct scan *scan = btd_service_get_user_data(service);
 
 	if (scan->refresh_cb_id) {
 		g_attrib_unregister(scan->attrib, scan->refresh_cb_id);
@@ -278,14 +261,7 @@ static int scan_param_probe(struct btd_service *service)
 	if (!prim)
 		return -EINVAL;
 
-	return scan_register(device, prim);
-}
-
-static void scan_param_remove(struct btd_service *service)
-{
-	struct btd_device *device = btd_service_get_device(service);
-
-	scan_unregister(device);
+	return scan_register(service, prim);
 }
 
 static struct btd_profile scan_profile = {
-- 
1.8.1.4

[PATCH BlueZ v0 6/6] input: Use btd_service userdata pointer

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Avoid maintaining an internal list of probed input_device instances by
making use of btd_service's userdata pointer.
---
 profiles/input/device.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 498c226..7c3130c 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -41,6 +41,7 @@
 
 #include "log.h"
 
+#include "lib/uuid.h"
 #include "../src/adapter.h"
 #include "../src/device.h"
 #include "../src/profile.h"
@@ -84,7 +85,6 @@ struct input_device {
 	uint32_t		reconnect_attempt;
 };
 
-static GSList *devices = NULL;
 static int idle_timeout = 0;
 
 void input_set_idle_timeout(int timeout)
@@ -94,18 +94,6 @@ void input_set_idle_timeout(int timeout)
 
 static void input_device_enter_reconnect_mode(struct input_device *idev);
 
-static struct input_device *find_device_by_path(GSList *list, const char *path)
-{
-	for (; list; list = list->next) {
-		struct input_device *idev = list->data;
-
-		if (!strcmp(idev->path, path))
-			return idev;
-	}
-
-	return NULL;
-}
-
 static void input_device_free(struct input_device *idev)
 {
 	if (idev->dc_id)
@@ -863,10 +851,6 @@ int input_device_register(struct btd_service *service)
 
 	DBG("%s", path);
 
-	idev = find_device_by_path(devices, path);
-	if (idev)
-		return -EEXIST;
-
 	idev = input_device_new(service);
 	if (!idev)
 		return -EINVAL;
@@ -883,24 +867,24 @@ int input_device_register(struct btd_service *service)
 
 	btd_service_set_user_data(service, idev);
 
-	devices = g_slist_append(devices, idev);
-
 	return 0;
 }
 
 static struct input_device *find_device(const bdaddr_t *src,
 					const bdaddr_t *dst)
 {
-	GSList *list;
+	struct btd_device *device;
+	struct btd_service *service;
 
-	for (list = devices; list != NULL; list = list->next) {
-		struct input_device *idev = list->data;
+	device = adapter_find_device(adapter_find(src), dst);
+	if (device == NULL)
+		return NULL;
 
-		if (!bacmp(&idev->src, src) && !bacmp(&idev->dst, dst))
-			return idev;
-	}
+	service = btd_device_get_service(device, HID_UUID);
+	if (service == NULL)
+		return NULL;
 
-	return NULL;
+	return btd_service_get_user_data(service);
 }
 
 void input_device_unregister(struct btd_service *service)
@@ -914,7 +898,6 @@ void input_device_unregister(struct btd_service *service)
 	g_dbus_unregister_interface(btd_get_dbus_connection(),
 						idev->path, INPUT_INTERFACE);
 
-	devices = g_slist_remove(devices, idev);
 	input_device_free(idev);
 }
 
-- 
1.8.1.4

Re: [PATCH BlueZ v0 0/6] Exploit btd_service's userdata pointer

[Johan Hedberg]

Hi Mikel,

On Mon, May 27, 2013, Mikel Astiz wrote:
> With the adoption of btd_service, profile implementations can benefit
> from the provided userdata pointer which can replace the previously
> necessary lists to associate services with custom private types.
> 
> The proposed change not only removes boilerplate code but also data
> redundancy, presumably making the codebase more robust.
> 
> This patchset addresses the first examples which looked like more
> obvious to refactor. Note that some of the profiles were not tested
> due to lack of hardware.
> 
> Patch 2/6 fixes an issue that I came across during this work.
> 
> Mikel Astiz (6):
>   network: Remove duplicated search
>   network: Fix missing NULL check for given UUID
>   network: Remove find_connection()
>   deviceinfo: Use btd_service userdata pointer
>   scanparam: Use btd_service userdata pointer
>   input: Use btd_service userdata pointer
> 
>  profiles/deviceinfo/deviceinfo.c | 46 ++++++----------------------------------
>  profiles/input/device.c          | 37 +++++++++-----------------------
>  profiles/network/connection.c    | 38 ++++++++++++---------------------
>  profiles/scanparam/scan.c        | 36 ++++++-------------------------
>  4 files changed, 35 insertions(+), 122 deletions(-)

Sorry for the delay in processing this set. It looks fine to me and all
patches in it have now been pushed upstream.

Johan