Skip to contentSkip to conversation

bluetooth: Add bluetooth server support

PATCH Superseded Frédéric Danis 3 participants 16 messages Jan 27, 2011 Kbuild

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Frédéric Danis

author
#0 lore raw

v2 cover bluetooth: Add bluetooth server support

It watches Bluetooth adapter property changes and adds SDP record to listen client connection request.
It supports multiple adapters, servers and client connections.

Use of GSList instead of GHashTable to store adapter path and address as we need to get address from adapter or vice-versa (during authorization phase).

Frédéric Danis (4):
bluetooth: only use bluetooth_refcount in
bluetooth_ref/bluetooth_unref
bluetooth: use GSList instead of GHashTable to store adapter
path/address
bluetooth: Add bluetooth server support
bluetooth: add Bluetooth service authorization support

 Makefile.am         |    1 +
 plugins/bluetooth.c |  558 +++++++++++++++++++++++++++++++++++++++++++++++++--
 plugins/bluetooth.h |    9 +
 3 files changed, 551 insertions(+), 17 deletions(-)
author
#1 lore raw

PATCH v2 1/4 bluetooth: only use bluetooth_refcount in bluetooth_ref/bluetooth_unref

---
 plugins/bluetooth.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
plugins/bluetooth.c +9 -5
index 93dd7a1..e59bd31 100644
@@ -505,7 +505,7 @@ static guint adapter_added_watch;
 static guint adapter_removed_watch;
 static guint property_watch;
 
-static void bluetooth_ref(void)
+static int bluetooth_ref(void)
 {
 	if (bluetooth_refcount > 0)
 		goto increment;

@@ -544,13 +544,15 @@ static void bluetooth_ref(void)
 increment:
 	g_atomic_int_inc(&bluetooth_refcount);
 
-	return;
+	return 0;
 
 remove:
 	g_dbus_remove_watch(connection, bluetooth_watch);
 	g_dbus_remove_watch(connection, adapter_added_watch);
 	g_dbus_remove_watch(connection, adapter_removed_watch);
 	g_dbus_remove_watch(connection, property_watch);
+
+	return -EIO;
 }
 
 static void bluetooth_unref(void)

@@ -569,10 +571,12 @@ static void bluetooth_unref(void)
 
 int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile *profile)
 {
-	bluetooth_ref();
+	int err;
+
+	err = bluetooth_ref();
 
-	if (bluetooth_refcount == 0)
-		return -EIO;
+	if (err != 0)
+		return err;
 
 	g_hash_table_insert(uuid_hash, g_strdup(uuid), profile);
Signature · 1 line
1.7.1
#2 lore raw

Re: [PATCH 1/4] bluetooth: only use bluetooth_refcount in bluetooth_ref/bluetooth_unref

Hi Fred,

Quoted 8 lines
 plugins/bluetooth.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
plugins/bluetooth.c +0 -0
index 93dd7a1..e59bd31 100644

plugins/bluetooth.c static guint adapter_added_watch; · line 509

Quoted 6 lines
plugins/bluetooth.c +1 -1
@@ -505,7 +505,7 @@ static guint adapter_added_watch;
 static guint adapter_removed_watch;
 static guint property_watch;
 
-static void bluetooth_ref(void)
+static int bluetooth_ref(void)
 {

why are we now reverting something back that we already applied upstream. Something is wrong here.

Regards

Marcel

author
#3 lore raw

Re: [PATCH 1/4] bluetooth: only use bluetooth_refcount in bluetooth_ref/bluetooth_unref

Quoted 13 lines · Marcel Holtmann

Hi Fred,

Quoted 11 lines
  plugins/bluetooth.c |   14 +++++++++-----
  1 files changed, 9 insertions(+), 5 deletions(-)
plugins/bluetooth.c +0 -0
index 93dd7a1..e59bd31 100644
@@ -505,7 +505,7 @@ static guint adapter_added_watch;
  static guint adapter_removed_watch;
  static guint property_watch;
Quoted 6 lines

plugins/bluetooth.c static guint adapter_added_watch; · line 509

Quoted 3 lines
plugins/bluetooth.c +1 -1
-static void bluetooth_ref(void)
+static int bluetooth_ref(void)
  {

why are we now reverting something back that we already applied upstream. Something is wrong here.

I followed Padovan's comment on my first try for this patch :
"bluetooth_refcount can only be read/write by bluetooth_ref/unref" which seems good to me

Signature · 2 lines
Frederic Danis                            Open Source Technology Centre
frederic.danis(a)intel.com                              Intel Corporation
#4 lore raw

Re: [PATCH 1/4] bluetooth: only use bluetooth_refcount in bluetooth_ref/bluetooth_unref

Quoted 21 lines · Frederic Danis
Quoted 13 lines · Marcel Holtmann

Hi Fred,

Quoted 11 lines
  plugins/bluetooth.c |   14 +++++++++-----
  1 files changed, 9 insertions(+), 5 deletions(-)
plugins/bluetooth.c +0 -0
index 93dd7a1..e59bd31 100644
@@ -505,7 +505,7 @@ static guint adapter_added_watch;
  static guint adapter_removed_watch;
  static guint property_watch;
Quoted 6 lines

plugins/bluetooth.c static guint adapter_added_watch; · line 509

Quoted 3 lines
plugins/bluetooth.c +1 -1
-static void bluetooth_ref(void)
+static int bluetooth_ref(void)
  {

why are we now reverting something back that we already applied upstream. Something is wrong here.

Quoted 3 lines

I followed Padovan's comment on my first try for this patch :
"bluetooth_refcount can only be read/write by bluetooth_ref/unref" which seems good to me

Yeah, but I'm now wondering why we merged a code access bluetooth_refcount directly.

Signature · 2 lines
Gustavo F. Padovan
http://profusion.mobi
#5 lore raw

Re: [PATCH 1/4] bluetooth: only use bluetooth_refcount in bluetooth_ref/bluetooth_unref

Hi Gustavo,

Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Quoted 8 lines
  plugins/bluetooth.c |   14 +++++++++-----
  1 files changed, 9 insertions(+), 5 deletions(-)
plugins/bluetooth.c +0 -0
index 93dd7a1..e59bd31 100644

plugins/bluetooth.c static guint adapter_added_watch; · line 509

Quoted 6 lines
plugins/bluetooth.c +1 -1
@@ -505,7 +505,7 @@ static guint adapter_added_watch;
  static guint adapter_removed_watch;
  static guint property_watch;
 
-static void bluetooth_ref(void)
+static int bluetooth_ref(void)
  {

why are we now reverting something back that we already applied upstream. Something is wrong here.

Quoted 6 lines
Quoted 3 lines

I followed Padovan's comment on my first try for this patch :
"bluetooth_refcount can only be read/write by bluetooth_ref/unref" which seems good to me

Yeah, but I'm now wondering why we merged a code access bluetooth_refcount directly.

that part is just fine. Don't worry about that.

Regards

Marcel

author
#6 lore raw

PATCH v2 2/4 bluetooth: use GSList instead of GHashTable to store adapter path/address

In reply to Frédéric Danis
---
 plugins/bluetooth.c |   86 +++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 74 insertions(+), 12 deletions(-)
plugins/bluetooth.c +74 -12
index e59bd31..4da662a 100644
@@ -39,9 +39,14 @@
 
 static DBusConnection *connection;
 static GHashTable *uuid_hash = NULL;
-static GHashTable *adapter_address_hash = NULL;
+static GSList *adapter_list = NULL;
 static gint bluetooth_refcount;
 
+struct adapter_address {
+	char *adapter;
+	char *address;
+};
+
 void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
 				char *buf, int size)
 {

@@ -235,6 +240,52 @@ static void parse_string(DBusMessageIter *iter, gpointer user_data)
 	dbus_message_iter_get_basic(iter, str);
 }
 
+static gint adapter_compare(gconstpointer a, gconstpointer b)
+{
+	const struct adapter_address *entry = a;
+	const char *key = b;
+
+	return g_strcmp0(entry->adapter, key);
+}
+
+static GSList* adapter_address_add(GSList *list, const char *path,
+					const char *address)
+{
+	GSList *l;
+	struct adapter_address *entry;
+
+	l = g_slist_find_custom(adapter_list, path, adapter_compare);
+	if (l != NULL) {
+		entry = l->data;
+
+		g_free(entry->address);
+
+		entry->address = g_strdup(address);
+
+		return list;
+	}
+
+	entry = g_try_new0(struct adapter_address, 1);
+	if (entry == NULL) {
+		ofono_error("Unable to allocate adapter_address structure");
+		return list;
+	}
+
+	entry->adapter = g_strdup(path);
+	entry->address = g_strdup(address);
+
+	return g_slist_prepend(list, entry);
+}
+
+static GSList* adapter_address_remove(GSList *l, struct adapter_address *entry)
+{
+	g_free(entry->adapter);
+	g_free(entry->address);
+	g_free(entry);
+
+	return g_slist_remove(l, entry);
+}
+
 static void device_properties_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;

@@ -245,6 +296,7 @@ static void device_properties_cb(DBusPendingCall *call, gpointer user_data)
 	const char *device_addr = NULL;
 	const char *alias = NULL;
 	struct bluetooth_profile *profile;
+	GSList *entry;
 
 	reply = dbus_pending_call_steal_reply(call);
 

@@ -266,9 +318,15 @@ static void device_properties_cb(DBusPendingCall *call, gpointer user_data)
 				"Address", parse_string, &device_addr,
 				"Alias", parse_string, &alias, NULL);
 
-	if (adapter)
-		adapter_addr = g_hash_table_lookup(adapter_address_hash,
-							adapter);
+	if (adapter) {
+		entry = g_slist_find_custom(adapter_list, adapter,
+						adapter_compare);
+		if (entry != NULL) {
+			struct adapter_address *a = entry->data;
+
+			adapter_addr = a->address;
+		}
+	}
 
 	if ((have_uuid & HFP_AG) && device_addr && adapter_addr) {
 		profile = g_hash_table_lookup(uuid_hash, HFP_AG_UUID);

@@ -392,8 +450,7 @@ static void adapter_properties_cb(DBusPendingCall *call, gpointer user_data)
 					NULL);
 
 	DBG("Adapter Address: %s, Path: %s", addr, path);
-	g_hash_table_insert(adapter_address_hash,
-				g_strdup(path), g_strdup(addr));
+	adapter_list = adapter_address_add(adapter_list, path, addr);
 
 	for (l = device_list; l; l = l->next) {
 		const char *device = l->data;

@@ -429,10 +486,16 @@ static gboolean adapter_removed(DBusConnection *connection,
 				DBusMessage *message, void *user_data)
 {
 	const char *path;
+	GSList *l;
 
 	if (dbus_message_get_args(message, NULL, DBUS_TYPE_OBJECT_PATH, &path,
-				DBUS_TYPE_INVALID) == TRUE)
-		g_hash_table_remove(adapter_address_hash, path);
+				DBUS_TYPE_INVALID) == TRUE) {
+		l = g_slist_find_custom(adapter_list, path, adapter_compare);
+
+		if (l != NULL)
+			adapter_list = adapter_address_remove(adapter_list,
+								l->data);
+	}
 
 	return TRUE;
 }

@@ -538,9 +601,6 @@ static int bluetooth_ref(void)
 	uuid_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
 						g_free, NULL);
 
-	adapter_address_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
-						g_free, g_free);
-
 increment:
 	g_atomic_int_inc(&bluetooth_refcount);
 

@@ -566,7 +626,9 @@ static void bluetooth_unref(void)
 	g_dbus_remove_watch(connection, property_watch);
 
 	g_hash_table_destroy(uuid_hash);
-	g_hash_table_destroy(adapter_address_hash);
+	while (adapter_list)
+		adapter_list = adapter_address_remove(adapter_list,
+							adapter_list->data);
 }
 
 int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile *profile)
Signature · 1 line
1.7.1
#7 lore raw

Re: [PATCH 2/4] bluetooth: use GSList instead of GHashTable to store adapter path/address

Hi Fred,

Quoted 10 lines
 plugins/bluetooth.c |   86 +++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 74 insertions(+), 12 deletions(-)
plugins/bluetooth.c +0 -0
index e59bd31..4da662a 100644
@@ -39,9 +39,14 @@
 
 static DBusConnection *connection;

plugins/bluetooth.c line 49

Quoted 10 lines
plugins/bluetooth.c +6 -1
 static GHashTable *uuid_hash = NULL;
-static GHashTable *adapter_address_hash = NULL;
+static GSList *adapter_list = NULL;
 static gint bluetooth_refcount;
 
+struct adapter_address {
+	char *adapter;
+	char *address;
+};
+

why are we doing this exactly with a list now instead of a hash table. I don't see the point here.

Regards

Marcel

author
#8 lore raw

Re: [PATCH 2/4] bluetooth: use GSList instead of GHashTable to store adapter path/address

Quoted 17 lines · Marcel Holtmann

Hi Fred,

Quoted 15 lines
  plugins/bluetooth.c |   86 +++++++++++++++++++++++++++++++++++++++++++-------
  1 files changed, 74 insertions(+), 12 deletions(-)
plugins/bluetooth.c +1 -1
index e59bd31..4da662a 100644
@@ -39,9 +39,14 @@
 
  static DBusConnection *connection;
  static GHashTable *uuid_hash = NULL;
-static GHashTable *adapter_address_hash = NULL;
+static GSList *adapter_list = NULL;
  static gint bluetooth_refcount;
Quoted 9 lines

plugins/bluetooth.c line 49

Quoted 5 lines
plugins/bluetooth.c +5 -0
+struct adapter_address {
+	char *adapter;
+	char *address;
+};
+

why are we doing this exactly with a list now instead of a hash table. I don't see the point here.

In patch 4, I need to find the adapter path (for the incoming connection) from its address to call the authorization method.

So, this is why I change to GSList.

Signature · 2 lines
Frederic Danis                            Open Source Technology Centre
frederic.danis(a)intel.com                              Intel Corporation
#9 lore raw

Re: [PATCH 2/4] bluetooth: use GSList instead of GHashTable to store adapter path/address

Hi Fred,

Quoted 25 lines
Quoted 15 lines
Quoted 15 lines
  plugins/bluetooth.c |   86 +++++++++++++++++++++++++++++++++++++++++++-------
  1 files changed, 74 insertions(+), 12 deletions(-)
plugins/bluetooth.c +1 -1
index e59bd31..4da662a 100644
@@ -39,9 +39,14 @@
 
  static DBusConnection *connection;
  static GHashTable *uuid_hash = NULL;
-static GHashTable *adapter_address_hash = NULL;
+static GSList *adapter_list = NULL;
  static gint bluetooth_refcount;
Quoted 9 lines

plugins/bluetooth.c line 49

Quoted 5 lines
plugins/bluetooth.c +5 -0
+struct adapter_address {
+	char *adapter;
+	char *address;
+};
+

why are we doing this exactly with a list now instead of a hash table. I don't see the point here.

Quoted 4 lines

In patch 4, I need to find the adapter path (for the incoming connection) from its address to call the authorization method.

So, this is why I change to GSList.

you can as easily walk a GHashTable than you can walk a GSList. So why not do that instead of moving everything to a list.

Regards

Marcel

author
#10 lore raw

Re: [PATCH 2/4] bluetooth: use GSList instead of GHashTable to store adapter path/address

Quoted 27 lines · Marcel Holtmann

Hi Fred,

Quoted 25 lines
Quoted 24 lines
Quoted 10 lines
   plugins/bluetooth.c |   86 +++++++++++++++++++++++++++++++++++++++++++-------
   1 files changed, 74 insertions(+), 12 deletions(-)
plugins/bluetooth.c +0 -0
index e59bd31..4da662a 100644
@@ -39,9 +39,14 @@
 
   static DBusConnection *connection;

plugins/bluetooth.c line 49

Quoted 10 lines
plugins/bluetooth.c +6 -1
   static GHashTable *uuid_hash = NULL;
-static GHashTable *adapter_address_hash = NULL;
+static GSList *adapter_list = NULL;
   static gint bluetooth_refcount;
 
+struct adapter_address {
+	char *adapter;
+	char *address;
+};
+

why are we doing this exactly with a list now instead of a hash table. I don't see the point here.

Quoted 7 lines
Quoted 4 lines

In patch 4, I need to find the adapter path (for the incoming connection) from its address to call the authorization method.

So, this is why I change to GSList.

you can as easily walk a GHashTable than you can walk a GSList. So why not do that instead of moving everything to a list.

I think that find a key from its value may use more cpu from HashTable than from GSList. If you want I can remove this and only use HashTable.

Signature · 2 lines
Frederic Danis                            Open Source Technology Centre
frederic.danis(a)intel.com                              Intel Corporation
#11 lore raw

Re: [PATCH 2/4] bluetooth: use GSList instead of GHashTable to store adapter path/address

Hi Frederic,

Quoted 31 lines · Frederic Danis
Quoted 30 lines · Marcel Holtmann

Hi Fred,

Quoted 28 lines
Quoted 15 lines
Quoted 15 lines
   plugins/bluetooth.c |   86 +++++++++++++++++++++++++++++++++++++++++++-------
   1 files changed, 74 insertions(+), 12 deletions(-)
plugins/bluetooth.c +1 -1
index e59bd31..4da662a 100644
@@ -39,9 +39,14 @@
 
   static DBusConnection *connection;
   static GHashTable *uuid_hash = NULL;
-static GHashTable *adapter_address_hash = NULL;
+static GSList *adapter_list = NULL;
   static gint bluetooth_refcount;
Quoted 9 lines

plugins/bluetooth.c line 49

Quoted 5 lines
plugins/bluetooth.c +5 -0
+struct adapter_address {
+	char *adapter;
+	char *address;
+};
+

why are we doing this exactly with a list now instead of a hash table. I don't see the point here.

In patch 4, I need to find the adapter path (for the incoming connection) from its address to call the authorization method.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

So, this is why I change to GSList.

you can as easily walk a GHashTable than you can walk a GSList. So why not do that instead of moving everything to a list.

I think that find a key from its value may use more cpu from HashTable than from GSList. If you want I can remove this and only use HashTable.

Please remove it and use the hashtable.

Signature · 2 lines
Gustavo F. Padovan
http://profusion.mobi
author
#12 lore raw

PATCH v2 3/4 bluetooth: Add bluetooth server support

In reply to Frédéric Danis
---
 Makefile.am         |    1 +
 plugins/bluetooth.c |  340 +++++++++++++++++++++++++++++++++++++++++++++++++++
 plugins/bluetooth.h |    9 ++
 3 files changed, 350 insertions(+), 0 deletions(-)
Makefile.am +1 -0
index 9b77e63..c0efe28 100644
@@ -318,6 +318,7 @@ builtin_sources += plugins/bluetooth.c plugins/bluetooth.h
 builtin_modules += hfp
 builtin_sources += plugins/hfp.c plugins/bluetooth.h
 
+builtin_sources += $(btio_sources)
 builtin_cflags += @BLUEZ_CFLAGS@
 builtin_libadd += @BLUEZ_LIBS@
 endif
plugins/bluetooth.c +340 -0

381 lines not on this page · raw message

plugins/bluetooth.h +9 -0
index 42b0d13..505d908 100644
@@ -23,6 +23,7 @@
 #define	BLUEZ_MANAGER_INTERFACE		BLUEZ_SERVICE ".Manager"
 #define	BLUEZ_ADAPTER_INTERFACE		BLUEZ_SERVICE ".Adapter"
 #define	BLUEZ_DEVICE_INTERFACE		BLUEZ_SERVICE ".Device"
+#define	BLUEZ_SERVICE_INTERFACE		BLUEZ_SERVICE ".Service"
 
 #define DBUS_TIMEOUT 15
 

@@ -39,10 +40,18 @@ struct bluetooth_profile {
 	void (*set_alias)(const char *device, const char *);
 };
 
+struct server;
+
+typedef void (*ConnectFunc)(GIOChannel *io, GError *err, gpointer user_data);
+
 int bluetooth_register_uuid(const char *uuid,
 				struct bluetooth_profile *profile);
 void bluetooth_unregister_uuid(const char *uuid);
 
+struct server *bluetooth_register_server(guint8 channel, const char *sdp_record,
+					ConnectFunc cb, gpointer user_data);
+void bluetooth_unregister_server(struct server *server);
+
 void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
 							char *buf, int size);
Signature · 1 line
1.7.1
#13 lore raw

Hi Frédéric,

Quoted 275 lines · Frédéric Danis
---
 Makefile.am         |    1 +
 plugins/bluetooth.c |  340 +++++++++++++++++++++++++++++++++++++++++++++++++++
 plugins/bluetooth.h |    9 ++
 3 files changed, 350 insertions(+), 0 deletions(-)
Makefile.am +1 -0
index 9b77e63..c0efe28 100644
@@ -318,6 +318,7 @@ builtin_sources += plugins/bluetooth.c plugins/bluetooth.h
 builtin_modules += hfp
 builtin_sources += plugins/hfp.c plugins/bluetooth.h
 
+builtin_sources += $(btio_sources)
 builtin_cflags += @BLUEZ_CFLAGS@
 builtin_libadd += @BLUEZ_LIBS@
 endif
plugins/bluetooth.c +230 -0
index 4da662a..86d4c25 100644
@@ -35,18 +35,36 @@
 
 #include <ofono/dbus.h>
 
+#include <btio.h>
 #include "bluetooth.h"
 
 static DBusConnection *connection;
 static GHashTable *uuid_hash = NULL;
 static GSList *adapter_list = NULL;
 static gint bluetooth_refcount;
+static GSList *server_list = NULL;
 
 struct adapter_address {
 	char *adapter;
 	char *address;
 };
 
+struct server {
+	guint8 channel;
+	char *sdp_record;
+	GIOChannel *io;
+	GHashTable *adapter_hash;
+	ConnectFunc connect_cb;
+	gpointer user_data;
+	GSList *client_list;
+};
+
+struct cb_data {
+	struct server *server;
+	char *path;
+	guint source;
+};
+
 void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
 				char *buf, int size)
 {

@@ -429,6 +447,277 @@ static gboolean property_changed(DBusConnection *connection, DBusMessage *msg,
 	return TRUE;
 }
 
+static void remove_record(char *path, guint handle, struct server *server)
+{
+	DBusMessage *msg;
+
+	msg = dbus_message_new_method_call(BLUEZ_SERVICE, path,
+					BLUEZ_SERVICE_INTERFACE,
+					"RemoveRecord");
+	if (msg == NULL) {
+		ofono_error("Unable to allocate D-Bus RemoveRecord message");
+		return;
+	}
+
+	dbus_message_append_args(msg, DBUS_TYPE_UINT32, &handle,
+					DBUS_TYPE_INVALID);
+	g_dbus_send_message(connection, msg);
+
+	ofono_info("Unregistered handle for %s, channel %d: 0x%x", path,
+			server->channel, handle);
+}
+
+static void server_stop(struct server *server)
+{
+	while (server->client_list) {
+		g_source_remove((guint) server->client_list->data);
+		server->client_list = g_slist_remove(server->client_list,
+						server->client_list->data);
+	}
+
+	g_hash_table_foreach_remove(server->adapter_hash,
+					(GHRFunc) remove_record, server);
+
+	if (server->io != NULL) {
+		g_io_channel_shutdown(server->io, TRUE, NULL);
+		g_io_channel_unref(server->io);
+		server->io = NULL;
+	}
+}
+
+static void cb_data_destroy(gpointer data)
+{
+	struct cb_data *cb_data = data;
+
+	if (cb_data->path != NULL)
+		g_free(cb_data->path);
+	g_free(cb_data);
+}
+
+static gboolean client_event(GIOChannel *chan, GIOCondition cond, gpointer data)
+{
+	struct cb_data *cb_data = data;
+	struct server *server = cb_data->server;
+
+	server->client_list = g_slist_remove(server->client_list,
+						(void *) cb_data->source);
+
+	cb_data_destroy(cb_data);
+
+	return FALSE;
+}
+
+static void confirm_event(GIOChannel *io, gpointer user_data)
+{
+	struct server *server = user_data;
+	struct cb_data *client_data;
+	GError *err = NULL;
+	char laddress[18], raddress[18];
+	guint8 channel;
+
+	bt_io_get(io, BT_IO_RFCOMM, &err, BT_IO_OPT_SOURCE, laddress,
+					BT_IO_OPT_DEST, raddress,
+					BT_IO_OPT_CHANNEL, &channel,
+					BT_IO_OPT_INVALID);
+	if (err) {
+		ofono_error("%s", err->message);
+		g_error_free(err);
+		return;
+	}
+
+	ofono_info("New connection for %s on  channel %u from: %s,", laddress,
+							channel, raddress);
+
+	if (!bt_io_accept(io, server->connect_cb, server->user_data,
+						NULL, &err)) {
+		ofono_error("%s", err->message);
+		g_error_free(err);
+		g_io_channel_unref(io);
+		return;
+	}
+
+	client_data = g_try_new0(struct cb_data, 1);
+	if (client_data == NULL) {
+		ofono_error("Unable to allocate client cb_data structure");
+		return;
+	}
+
+	client_data->server = server;
+	client_data->source = g_io_add_watch(io,
+					G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+					client_event, client_data);
+	server->client_list = g_slist_prepend(server->client_list,
+						(void *)client_data->source);
+}
+
+static void add_record_cb(DBusPendingCall *call, gpointer user_data)
+{
+	struct cb_data *cb_data = user_data;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+	DBusError derr;
+	guint32 handle;
+
+	dbus_error_init(&derr);
+
+	if (dbus_set_error_from_message(&derr, reply)) {
+		ofono_error("Replied with an error: %s, %s",
+					derr.name, derr.message);
+		dbus_error_free(&derr);
+		g_free(cb_data->path);
+		goto done;
+	}
+
+	dbus_message_get_args(reply, NULL, DBUS_TYPE_UINT32, &handle,
+					DBUS_TYPE_INVALID);
+
+	g_hash_table_insert(cb_data->server->adapter_hash, cb_data->path,
+				(void *) handle);
+
+	ofono_info("Registered handle for %s, channel %d: 0x%x", cb_data->path,
+			cb_data->server->channel, handle);
+
+done:
+	/* Do not free cb_data->path, it is used in adapter_hash */
+	g_free(cb_data);
+	dbus_message_unref(reply);
+}
+
+static void server_adapter_properties_cb(DBusPendingCall *call,
+						gpointer user_data)
+{
+	struct cb_data *cb_data = user_data;
+	const char *path = cb_data->path;
+	DBusMessage *reply;
+	const char *addr;
+	struct server *server = cb_data->server;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {
+		DBG("Bluetooth daemon is apparently not available.");
+		goto done;
+	}
+
+	bluetooth_parse_properties(reply, "Address", parse_string, &addr, NULL);
+
+	DBG("Adapter Address: %s, Path: %s", addr, path);
+	adapter_list = adapter_address_add(adapter_list, path, addr);
+
+	DBG("Add record on %s", path);
+
+	bluetooth_send_with_reply(path, BLUEZ_SERVICE_INTERFACE,
+				"AddRecord", add_record_cb,
+				cb_data, NULL, -1,
+				DBUS_TYPE_STRING, &server->sdp_record,
+				DBUS_TYPE_INVALID);
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void server_parse_adapters(DBusMessageIter *array, gpointer user_data)
+{
+	struct server *server = user_data;
+	DBusMessageIter value;
+	struct cb_data *cb_data;
+
+	if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY)
+		return;
+
+	dbus_message_iter_recurse(array, &value);
+
+	while (dbus_message_iter_get_arg_type(&value)
+			== DBUS_TYPE_OBJECT_PATH) {
+		const char *path;
+
+		dbus_message_iter_get_basic(&value, &path);
+
+		DBG("Calling GetProperties on %s", path);
+
+		cb_data = g_try_new0(struct cb_data, 1);
+		if (cb_data == NULL) {
+			ofono_error("Unable to allocate cb_data structure");
+			return;
+		}
+
+		cb_data->server = server;
+		cb_data->path = g_strdup(path);
+
+		bluetooth_send_with_reply(path, BLUEZ_ADAPTER_INTERFACE,
+				"GetProperties", server_adapter_properties_cb,
+				cb_data, NULL, -1, DBUS_TYPE_INVALID);
+
+		dbus_message_iter_next(&value);
+	}
+}
+
+static void server_manager_properties_cb(DBusPendingCall *call,
+						gpointer user_data)
+{
+	DBusMessage *reply;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {

plugins/bluetooth.c static gboolean property_changed(DBusConnection *connection, DBusMessage *msg, · line 671

Quoted 10 lines
plugins/bluetooth.c +10 -0
+		DBG("Bluetooth daemon is apparently not available.");
+		goto done;
+	}
+
+	bluetooth_parse_properties(reply, "Adapters", server_parse_adapters,
+						user_data, NULL);
+
+done:
+	dbus_message_unref(reply);
+}

What's going on here? You are duplicating many line of codes that we already have in bluetooth.c. Can't you just adapt the code we have there?

Signature · 2 lines
Gustavo F. Padovan
http://profusion.mobi
author
#14 lore raw
Quoted 284 lines · Gustavo F. Padovan

Hi Frédéric,

Quoted 280 lines · Frédéric Danis
---
  Makefile.am         |    1 +
  plugins/bluetooth.c |  340 +++++++++++++++++++++++++++++++++++++++++++++++++++
  plugins/bluetooth.h |    9 ++
  3 files changed, 350 insertions(+), 0 deletions(-)
Makefile.am +1 -0
index 9b77e63..c0efe28 100644
@@ -318,6 +318,7 @@ builtin_sources += plugins/bluetooth.c plugins/bluetooth.h
  builtin_modules += hfp
  builtin_sources += plugins/hfp.c plugins/bluetooth.h
 
+builtin_sources += $(btio_sources)
  builtin_cflags += @BLUEZ_CFLAGS@
  builtin_libadd += @BLUEZ_LIBS@
  endif
plugins/bluetooth.c +235 -0
index 4da662a..86d4c25 100644
@@ -35,18 +35,36 @@
 
  #include<ofono/dbus.h>
 
+#include<btio.h>
  #include "bluetooth.h"
 
  static DBusConnection *connection;
  static GHashTable *uuid_hash = NULL;
  static GSList *adapter_list = NULL;
  static gint bluetooth_refcount;
+static GSList *server_list = NULL;
 
  struct adapter_address {
  	char *adapter;
  	char *address;
  };
 
+struct server {
+	guint8 channel;
+	char *sdp_record;
+	GIOChannel *io;
+	GHashTable *adapter_hash;
+	ConnectFunc connect_cb;
+	gpointer user_data;
+	GSList *client_list;
+};
+
+struct cb_data {
+	struct server *server;
+	char *path;
+	guint source;
+};
+
  void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
  				char *buf, int size)
  {

@@ -429,6 +447,277 @@ static gboolean property_changed(DBusConnection *connection, DBusMessage *msg,
  	return TRUE;
  }
 
+static void remove_record(char *path, guint handle, struct server *server)
+{
+	DBusMessage *msg;
+
+	msg = dbus_message_new_method_call(BLUEZ_SERVICE, path,
+					BLUEZ_SERVICE_INTERFACE,
+					"RemoveRecord");
+	if (msg == NULL) {
+		ofono_error("Unable to allocate D-Bus RemoveRecord message");
+		return;
+	}
+
+	dbus_message_append_args(msg, DBUS_TYPE_UINT32,&handle,
+					DBUS_TYPE_INVALID);
+	g_dbus_send_message(connection, msg);
+
+	ofono_info("Unregistered handle for %s, channel %d: 0x%x", path,
+			server->channel, handle);
+}
+
+static void server_stop(struct server *server)
+{
+	while (server->client_list) {
+		g_source_remove((guint) server->client_list->data);
+		server->client_list = g_slist_remove(server->client_list,
+						server->client_list->data);
+	}
+
+	g_hash_table_foreach_remove(server->adapter_hash,
+					(GHRFunc) remove_record, server);
+
+	if (server->io != NULL) {
+		g_io_channel_shutdown(server->io, TRUE, NULL);
+		g_io_channel_unref(server->io);
+		server->io = NULL;
+	}
+}
+
+static void cb_data_destroy(gpointer data)
+{
+	struct cb_data *cb_data = data;
+
+	if (cb_data->path != NULL)
+		g_free(cb_data->path);
+	g_free(cb_data);
+}
+
+static gboolean client_event(GIOChannel *chan, GIOCondition cond, gpointer data)
+{
+	struct cb_data *cb_data = data;
+	struct server *server = cb_data->server;
+
+	server->client_list = g_slist_remove(server->client_list,
+						(void *) cb_data->source);
+
+	cb_data_destroy(cb_data);
+
+	return FALSE;
+}
+
+static void confirm_event(GIOChannel *io, gpointer user_data)
+{
+	struct server *server = user_data;
+	struct cb_data *client_data;
+	GError *err = NULL;
+	char laddress[18], raddress[18];
+	guint8 channel;
+
+	bt_io_get(io, BT_IO_RFCOMM,&err, BT_IO_OPT_SOURCE, laddress,
+					BT_IO_OPT_DEST, raddress,
+					BT_IO_OPT_CHANNEL,&channel,
+					BT_IO_OPT_INVALID);
+	if (err) {
+		ofono_error("%s", err->message);
+		g_error_free(err);
+		return;
+	}
+
+	ofono_info("New connection for %s on  channel %u from: %s,", laddress,
+							channel, raddress);
+
+	if (!bt_io_accept(io, server->connect_cb, server->user_data,
+						NULL,&err)) {
+		ofono_error("%s", err->message);
+		g_error_free(err);
+		g_io_channel_unref(io);
+		return;
+	}
+
+	client_data = g_try_new0(struct cb_data, 1);
+	if (client_data == NULL) {
+		ofono_error("Unable to allocate client cb_data structure");
+		return;
+	}
+
+	client_data->server = server;
+	client_data->source = g_io_add_watch(io,
+					G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+					client_event, client_data);
+	server->client_list = g_slist_prepend(server->client_list,
+						(void *)client_data->source);
+}
+
+static void add_record_cb(DBusPendingCall *call, gpointer user_data)
+{
+	struct cb_data *cb_data = user_data;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+	DBusError derr;
+	guint32 handle;
+
+	dbus_error_init(&derr);
+
+	if (dbus_set_error_from_message(&derr, reply)) {
+		ofono_error("Replied with an error: %s, %s",
+					derr.name, derr.message);
+		dbus_error_free(&derr);
+		g_free(cb_data->path);
+		goto done;
+	}
+
+	dbus_message_get_args(reply, NULL, DBUS_TYPE_UINT32,&handle,
+					DBUS_TYPE_INVALID);
+
+	g_hash_table_insert(cb_data->server->adapter_hash, cb_data->path,
+				(void *) handle);
+
+	ofono_info("Registered handle for %s, channel %d: 0x%x", cb_data->path,
+			cb_data->server->channel, handle);
+
+done:
+	/* Do not free cb_data->path, it is used in adapter_hash */
+	g_free(cb_data);
+	dbus_message_unref(reply);
+}
+
+static void server_adapter_properties_cb(DBusPendingCall *call,
+						gpointer user_data)
+{
+	struct cb_data *cb_data = user_data;
+	const char *path = cb_data->path;
+	DBusMessage *reply;
+	const char *addr;
+	struct server *server = cb_data->server;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {
+		DBG("Bluetooth daemon is apparently not available.");
+		goto done;
+	}
+
+	bluetooth_parse_properties(reply, "Address", parse_string,&addr, NULL);
+
+	DBG("Adapter Address: %s, Path: %s", addr, path);
+	adapter_list = adapter_address_add(adapter_list, path, addr);
+
+	DBG("Add record on %s", path);
+
+	bluetooth_send_with_reply(path, BLUEZ_SERVICE_INTERFACE,
+				"AddRecord", add_record_cb,
+				cb_data, NULL, -1,
+				DBUS_TYPE_STRING,&server->sdp_record,
+				DBUS_TYPE_INVALID);
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void server_parse_adapters(DBusMessageIter *array, gpointer user_data)
+{
+	struct server *server = user_data;
+	DBusMessageIter value;
+	struct cb_data *cb_data;
+
+	if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY)
+		return;
+
+	dbus_message_iter_recurse(array,&value);
+
+	while (dbus_message_iter_get_arg_type(&value)
+			== DBUS_TYPE_OBJECT_PATH) {
+		const char *path;
+
+		dbus_message_iter_get_basic(&value,&path);
+
+		DBG("Calling GetProperties on %s", path);
+
+		cb_data = g_try_new0(struct cb_data, 1);
+		if (cb_data == NULL) {
+			ofono_error("Unable to allocate cb_data structure");
+			return;
+		}
+
+		cb_data->server = server;
+		cb_data->path = g_strdup(path);
+
+		bluetooth_send_with_reply(path, BLUEZ_ADAPTER_INTERFACE,
+				"GetProperties", server_adapter_properties_cb,
+				cb_data, NULL, -1, DBUS_TYPE_INVALID);
+
+		dbus_message_iter_next(&value);
+	}
+}
+
+static void server_manager_properties_cb(DBusPendingCall *call,
+						gpointer user_data)
+{
+	DBusMessage *reply;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {
+		DBG("Bluetooth daemon is apparently not available.");
+		goto done;
+	}
+
+	bluetooth_parse_properties(reply, "Adapters", server_parse_adapters,
Quoted 9 lines

plugins/bluetooth.c static gboolean property_changed(DBusConnection *connection, DBusMessage *msg, · line 671

Quoted 5 lines
plugins/bluetooth.c +5 -0
+						user_data, NULL);
+
+done:
+	dbus_message_unref(reply);
+}

What's going on here? You are duplicating many line of codes that we already have in bluetooth.c. Can't you just adapt the code we have there?

Most of the code is same, but the end of callbacks do not finish in same way.
If this code is not duplicate, this will complicate the server and/or client code to manage that it is called form the other path. With this code the client and server path are isolated.

Signature · 2 lines
Frederic Danis                            Open Source Technology Centre
frederic.danis(a)intel.com                              Intel Corporation
author
#15 lore raw

PATCH v2 4/4 bluetooth: add Bluetooth service authorization support

In reply to Frédéric Danis
---
 plugins/bluetooth.c |  136 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 127 insertions(+), 9 deletions(-)
plugins/bluetooth.c +127 -9
index 86d4c25..7e4dbc1 100644
@@ -44,6 +44,8 @@ static GSList *adapter_list = NULL;
 static gint bluetooth_refcount;
 static GSList *server_list = NULL;
 
+#define TIMEOUT (60*1000) /* Timeout for user response (milliseconds) */
+
 struct adapter_address {
 	char *adapter;
 	char *address;

@@ -63,6 +65,7 @@ struct cb_data {
 	struct server *server;
 	char *path;
 	guint source;
+	GIOChannel *io;
 };
 
 void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,

@@ -266,6 +269,14 @@ static gint adapter_compare(gconstpointer a, gconstpointer b)
 	return g_strcmp0(entry->adapter, key);
 }
 
+static gint address_compare(gconstpointer a, gconstpointer b)
+{
+	const struct adapter_address *entry = a;
+	const char *key = b;
+
+	return g_strcmp0(entry->address, key);
+}
+
 static GSList* adapter_address_add(GSList *list, const char *path,
 					const char *address)
 {

@@ -507,13 +518,102 @@ static gboolean client_event(GIOChannel *chan, GIOCondition cond, gpointer data)
 	return FALSE;
 }
 
+static void cancel_authorization(struct cb_data *user_data)
+{
+	DBusMessage *msg;
+
+	if (user_data->path == NULL)
+		return;
+
+	msg = dbus_message_new_method_call(BLUEZ_SERVICE, user_data->path,
+						BLUEZ_SERVICE_INTERFACE,
+						"CancelAuthorization");
+
+	g_dbus_send_message(connection, msg);
+}
+
+static void auth_cb(DBusPendingCall *call, gpointer user_data)
+{
+	struct cb_data *cb_data = user_data;
+	struct server *server = cb_data->server;
+
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+	DBusError derr;
+	GError *err = NULL;
+
+	dbus_error_init(&derr);
+
+	if (dbus_set_error_from_message(&derr, reply)) {
+		ofono_error("RequestAuthorization error: %s, %s",
+				derr.name, derr.message);
+
+		if (dbus_error_has_name(&derr, DBUS_ERROR_NO_REPLY))
+			cancel_authorization(cb_data);
+
+		dbus_error_free(&derr);
+
+		dbus_message_unref(reply);
+
+		goto failed;
+	}
+
+	dbus_message_unref(reply);
+
+	ofono_info("RequestAuthorization succeeded");
+
+	if (!bt_io_accept(cb_data->io, server->connect_cb, server->user_data,
+						NULL, &err)) {
+		ofono_error("%s", err->message);
+		g_error_free(err);
+		goto failed;
+	}
+
+	g_source_remove(cb_data->source);
+	server->client_list = g_slist_remove(server->client_list,
+						(void *) cb_data->source);
+
+	cb_data->source = g_io_add_watch(cb_data->io,
+					G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+					client_event, cb_data);
+	server->client_list = g_slist_prepend(server->client_list,
+						(void *)cb_data->source);
+
+	return;
+
+failed:
+	g_source_remove(cb_data->source);
+	server->client_list = g_slist_remove(server->client_list,
+						(void *) cb_data->source);
+
+	cb_data_destroy(cb_data);
+}
+
+static gboolean auth_watch(GIOChannel *io, GIOCondition cond,
+						gpointer user_data)
+{
+	struct cb_data *cb_data = user_data;
+	struct server *server = cb_data->server;
+
+	cancel_authorization(cb_data);
+	server->client_list = g_slist_remove(server->client_list,
+						(void *) cb_data->source);
+
+	cb_data_destroy(cb_data);
+
+	return FALSE;
+}
+
 static void confirm_event(GIOChannel *io, gpointer user_data)
 {
 	struct server *server = user_data;
 	struct cb_data *client_data;
+	guint handle;
+	const char *addr;
+	int ret;
 	GError *err = NULL;
 	char laddress[18], raddress[18];
 	guint8 channel;
+	GSList *entry;
 
 	bt_io_get(io, BT_IO_RFCOMM, &err, BT_IO_OPT_SOURCE, laddress,
 					BT_IO_OPT_DEST, raddress,

@@ -528,14 +628,6 @@ static void confirm_event(GIOChannel *io, gpointer user_data)
 	ofono_info("New connection for %s on  channel %u from: %s,", laddress,
 							channel, raddress);
 
-	if (!bt_io_accept(io, server->connect_cb, server->user_data,
-						NULL, &err)) {
-		ofono_error("%s", err->message);
-		g_error_free(err);
-		g_io_channel_unref(io);
-		return;
-	}
-
 	client_data = g_try_new0(struct cb_data, 1);
 	if (client_data == NULL) {
 		ofono_error("Unable to allocate client cb_data structure");

@@ -543,9 +635,35 @@ static void confirm_event(GIOChannel *io, gpointer user_data)
 	}
 
 	client_data->server = server;
+	entry = g_slist_find_custom(adapter_list, laddress, address_compare);
+	if (entry != NULL) {
+		struct adapter_address *a = entry->data;
+
+		client_data->path = g_strdup(a->adapter);
+	}
+
+	client_data->io = io;
+
+	handle = (guint) g_hash_table_lookup(server->adapter_hash,
+						client_data->path);
+	addr = raddress;
+	ret = bluetooth_send_with_reply(client_data->path,
+					BLUEZ_SERVICE_INTERFACE,
+					"RequestAuthorization",
+					auth_cb, client_data, NULL, TIMEOUT,
+					DBUS_TYPE_STRING, &addr,
+					DBUS_TYPE_UINT32, &handle,
+					DBUS_TYPE_INVALID);
+	if (ret < 0) {
+		ofono_error("Request Bluetooth authorization failed");
+		return;
+	}
+
+	ofono_info("RequestAuthorization(%s, 0x%x)", raddress, handle);
+
 	client_data->source = g_io_add_watch(io,
 					G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-					client_event, client_data);
+					auth_watch, client_data);
 	server->client_list = g_slist_prepend(server->client_list,
 						(void *)client_data->source);
 }
Signature · 1 line
1.7.1