[PATCH v0 0/9] External HFP: Add SCO

[PATCH v0 0/9] External HFP: Add SCO

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

This patch series adds Bluetooth SCO socket declaration and the initial 
code to manage HF audio connections.

Some Bluetooth functions and structs are being declared locally, Bluetooth
headers dependency will be removed in the future.

Claudio Takahasi (9):
  bluez5: Add SCO socket declarations
  bluez5: Add bt_bacpy()
  hfp_hf_bluez5: Add SCO listen socket
  bluez5: Add bt_ba2str()
  bluez5: Add bt_getsockpeers()
  bluez5: Add RFCOMM socket address declaration
  hfp_hf_bluez5: Add rejecting SCO connection
  hfp_hf_bluez5: Reject SCO if source doesn't match
  hfp_hf_bluez5: Fix missing fd close

 plugins/bluez5.c        |  38 ++++++++++++
 plugins/bluez5.h        |  42 +++++++++++++
 plugins/hfp_hf_bluez5.c | 162 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 232 insertions(+), 10 deletions(-)

-- 
1.7.11.7

[PATCH v0 1/9] bluez5: Add SCO socket declarations

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

Adds local copy of SCO Bluetooth sockets declarations, since the
objective to avoid including BlueZ library headers.
---
 plugins/bluez5.c        |  3 +++
 plugins/bluez5.h        | 28 ++++++++++++++++++++++++++++
 plugins/hfp_hf_bluez5.c |  2 ++
 3 files changed, 33 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index 8d8b565..d471454 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -24,6 +24,9 @@
 #endif
 
 #include <errno.h>
+#include <stdint.h>
+#include <sys/socket.h>
+
 #include <glib.h>
 
 #define OFONO_API_SUBJECT_TO_CHANGE
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 3aa8ffe..fd0704e 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -28,6 +28,34 @@
 #define HFP_HS_UUID	"0000111e-0000-1000-8000-00805f9b34fb"
 #define HFP_AG_UUID	"0000111f-0000-1000-8000-00805f9b34fb"
 
+#ifndef AF_BLUETOOTH
+#define AF_BLUETOOTH		31
+#define PF_BLUETOOTH		AF_BLUETOOTH
+#endif
+
+#define BTPROTO_SCO		2
+
+#define SOL_SCO			17
+
+#ifndef SOL_BLUETOOTH
+#define SOL_BLUETOOTH		274
+#endif
+
+#define BT_DEFER_SETUP		7
+
+/* BD Address */
+typedef struct {
+	uint8_t b[6];
+} __attribute__((packed)) bdaddr_t;
+
+#define BDADDR_ANY   (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
+
+/* SCO socket address */
+struct sockaddr_sco {
+	sa_family_t	sco_family;
+	bdaddr_t	sco_bdaddr;
+};
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index f6ceb76..0e496ee 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -24,9 +24,11 @@
 #endif
 
 #include <errno.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <sys/socket.h>
 
 #include <glib.h>
 
-- 
1.7.11.7

[PATCH v0 2/9] bluez5: Add bt_bacpy()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Adds a copy of BlueZ bacpy() function.
---
 plugins/bluez5.c | 6 ++++++
 plugins/bluez5.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index d471454..876ad2d 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdint.h>
 #include <sys/socket.h>
+#include <string.h>
 
 #include <glib.h>
 
@@ -39,6 +40,11 @@
 
 #define BLUEZ_PROFILE_MGMT_INTERFACE   BLUEZ_SERVICE ".ProfileManager1"
 
+void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src)
+{
+	memcpy(dst, src, sizeof(bdaddr_t));
+}
+
 static void profile_register_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index fd0704e..3921c7b 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -56,6 +56,8 @@ struct sockaddr_sco {
 	bdaddr_t	sco_bdaddr;
 };
 
+void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src);
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
-- 
1.7.11.7

[PATCH v0 3/9] hfp_hf_bluez5: Add SCO listen socket

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

This patch adds the initial SCO server socket handling. BtIO and BlueZ
functions should not be used, with the new Profile API the objetive is
to get rid of these dependencies.
---
 plugins/hfp_hf_bluez5.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 0e496ee..398dee0 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -64,6 +64,7 @@ struct hfp {
 static GHashTable *modem_hash = NULL;
 static GHashTable *devices_proxies = NULL;
 static GDBusClient *bluez = NULL;
+static guint sco_watch = 0;
 
 static void hfp_debug(const char *str, void *user_data)
 {
@@ -378,6 +379,89 @@ static const GDBusMethodTable profile_methods[] = {
 	{ }
 };
 
+static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
+							gpointer user_data)
+{
+	struct sockaddr_sco saddr;
+	socklen_t optlen;
+	int sk, nsk;
+
+	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
+		return FALSE;
+
+	sk = g_io_channel_unix_get_fd(io);
+
+	memset(&saddr, 0, sizeof(saddr));
+	optlen = sizeof(saddr);
+
+	nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
+	if (nsk < 0)
+		return TRUE;
+
+	/* TODO: Verify if the device has a modem */
+
+	return TRUE;
+}
+
+static GIOChannel *sco_listen(int *err)
+{
+	struct sockaddr_sco addr;
+	GIOChannel *io;
+	int sk, defer_setup = 1;
+
+	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_SCO);
+	if (sk < 0) {
+		*err = -errno;
+		return NULL;
+	}
+
+	/* Bind to local address */
+	memset(&addr, 0, sizeof(addr));
+	addr.sco_family = AF_BLUETOOTH;
+	bt_bacpy(&addr.sco_bdaddr, BDADDR_ANY);
+
+	if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
+		close(sk);
+		*err = -errno;
+		return NULL;
+	}
+
+	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
+				&defer_setup, sizeof(defer_setup)) < 0) {
+		ofono_warn("Can't enable deferred setup: %s (%d)",
+						strerror(errno), errno);
+		*err = -errno;
+	}
+
+	io = g_io_channel_unix_new(sk);
+	g_io_channel_set_close_on_unref(io, TRUE);
+	g_io_channel_set_flags(io, G_IO_FLAG_NONBLOCK, NULL);
+
+	if (listen(sk, 5) < 0) {
+		g_io_channel_unref(io);
+		*err = -errno;
+		return NULL;
+	}
+
+	return io;
+}
+
+static int sco_init(void)
+{
+	GIOCondition cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;
+	GIOChannel *sco_io;
+	int err = 0;
+
+	sco_io = sco_listen(&err);
+	if (sco_io == NULL)
+		return err;
+
+	sco_watch = g_io_add_watch(sco_io, cond, sco_accept, NULL);
+	g_io_channel_unref(sco_io);
+
+	return 0;
+}
+
 static void connect_handler(DBusConnection *conn, void *user_data)
 {
 	DBG("Registering External Profile handler ...");
@@ -500,6 +584,12 @@ static int hfp_init(void)
 	if (DBUS_TYPE_UNIX_FD < 0)
 		return -EBADF;
 
+	err = sco_init();
+	if (err < 0) {
+		ofono_error("SCO: %s(%d)", strerror(-err), -err);
+		return err;
+	}
+
 	/* Registers External Profile handler */
 	if (!g_dbus_register_interface(conn, HFP_EXT_PROFILE_PATH,
 					BLUEZ_PROFILE_INTERFACE,
@@ -550,6 +640,9 @@ static void hfp_exit(void)
 
 	g_hash_table_destroy(modem_hash);
 	g_hash_table_destroy(devices_proxies);
+
+	if (sco_watch)
+		g_source_remove(sco_watch);
 }
 
 OFONO_PLUGIN_DEFINE(hfp_bluez5, "External Hands-Free Profile Plugin", VERSION,
-- 
1.7.11.7

[PATCH v0 4/9] bluez5: Add bt_ba2str()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

Adds a copy of BlueZ ba2str() function.
---
 plugins/bluez5.c        | 7 +++++++
 plugins/bluez5.h        | 2 ++
 plugins/hfp_hf_bluez5.c | 1 +
 3 files changed, 10 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index 876ad2d..d7e85f2 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -25,6 +25,7 @@
 
 #include <errno.h>
 #include <stdint.h>
+#include <stdio.h>
 #include <sys/socket.h>
 #include <string.h>
 
@@ -45,6 +46,12 @@ void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src)
 	memcpy(dst, src, sizeof(bdaddr_t));
 }
 
+int bt_ba2str(const bdaddr_t *ba, char *str)
+{
+	return sprintf(str, "%2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X",
+		ba->b[5], ba->b[4], ba->b[3], ba->b[2], ba->b[1], ba->b[0]);
+}
+
 static void profile_register_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 3921c7b..204c41d 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -58,6 +58,8 @@ struct sockaddr_sco {
 
 void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src);
 
+int bt_ba2str(const bdaddr_t *ba, char *str);
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 398dee0..a4a0894 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <unistd.h>
 #include <string.h>
 #include <sys/socket.h>
-- 
1.7.11.7

[PATCH v0 5/9] bluez5: Add bt_getsockpeers()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

This patch adds a generic Bluetooth helper function to allow getting
the adapter and device Bluetooth Address from Bluetooth sockets.
---
 plugins/bluez5.c | 22 ++++++++++++++++++++++
 plugins/bluez5.h |  3 +++
 2 files changed, 25 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index d7e85f2..06629e1 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -52,6 +52,28 @@ int bt_ba2str(const bdaddr_t *ba, char *str)
 		ba->b[5], ba->b[4], ba->b[3], ba->b[2], ba->b[1], ba->b[0]);
 }
 
+int bt_getsockpeers(int sock, struct sockaddr *src, struct sockaddr *dst,
+								socklen_t len)
+{
+	socklen_t olen;
+
+	if (src) {
+		memset(src, 0, len);
+		olen = len;
+		if (getsockname(sock, src, &olen) < 0)
+			return -errno;
+	}
+
+	if (dst) {
+		memset(dst, 0, len);
+		olen = len;
+		if (getpeername(sock, dst, &olen) < 0)
+			return -errno;
+	}
+
+	return 0;
+}
+
 static void profile_register_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 204c41d..2d70fbf 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -60,6 +60,9 @@ void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src);
 
 int bt_ba2str(const bdaddr_t *ba, char *str);
 
+int bt_getsockpeers(int sock, struct sockaddr *src, struct sockaddr *dst,
+								socklen_t len);
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
-- 
1.7.11.7

[PATCH v0 6/9] bluez5: Add RFCOMM socket address declaration

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

This patch adds a copy of Bluetooth RFCOMM socket declaration.
---
 plugins/bluez5.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 2d70fbf..ba1b529 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -50,6 +50,13 @@ typedef struct {
 
 #define BDADDR_ANY   (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
 
+/* RFCOMM socket address */
+struct sockaddr_rc {
+	sa_family_t	rc_family;
+	bdaddr_t	rc_bdaddr;
+	uint8_t		rc_channel;
+};
+
 /* SCO socket address */
 struct sockaddr_sco {
 	sa_family_t	sco_family;
-- 
1.7.11.7

[PATCH v0 7/9] hfp_hf_bluez5: Add rejecting SCO connection

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

This patch rejects the SCO incoming connection if there isn't a modem
(service level connection) associated with the address. At the moment,
only the remote address is verified.
---
 plugins/hfp_hf_bluez5.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index a4a0894..d79647b 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -67,6 +67,16 @@ static GHashTable *devices_proxies = NULL;
 static GDBusClient *bluez = NULL;
 static guint sco_watch = 0;
 
+static gboolean modem_address_cmp(gpointer key, gpointer value,
+						gpointer user_data)
+{
+	const char *dst = user_data;
+	struct ofono_modem *modem = value;
+	const char *address = ofono_modem_get_string(modem, "Address");
+
+	return g_strcmp0(address, dst) != 0 ? FALSE : TRUE;
+}
+
 static void hfp_debug(const char *str, void *user_data)
 {
 	const char *prefix = user_data;
@@ -383,8 +393,10 @@ static const GDBusMethodTable profile_methods[] = {
 static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 							gpointer user_data)
 {
+	struct ofono_modem *modem;
 	struct sockaddr_sco saddr;
 	socklen_t optlen;
+	char dst[18];
 	int sk, nsk;
 
 	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
@@ -399,7 +411,13 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 	if (nsk < 0)
 		return TRUE;
 
-	/* TODO: Verify if the device has a modem */
+	bt_ba2str(&saddr.sco_bdaddr, dst);
+	modem = g_hash_table_find(modem_hash, modem_address_cmp, dst);
+	if (modem == NULL) {
+		ofono_error("Rejecting SCO: SLC connection missing!");
+		close(nsk);
+		return TRUE;
+	}
 
 	return TRUE;
 }
-- 
1.7.11.7

[PATCH v0 8/9] hfp_hf_bluez5: Reject SCO if source doesn't match

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 4402 bytes --]

This patch implements additional verification checking the Bluetooth
source address instead of the remote address only.
---
 plugins/hfp_hf_bluez5.c | 64 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index d79647b..504b0f3 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -60,6 +60,12 @@
 struct hfp {
 	struct hfp_slc_info info;
 	DBusMessage *msg;
+	char adapter[18];
+};
+
+struct bt_peer {
+	char adapter[18];
+	char device[18];
 };
 
 static GHashTable *modem_hash = NULL;
@@ -70,11 +76,18 @@ static guint sco_watch = 0;
 static gboolean modem_address_cmp(gpointer key, gpointer value,
 						gpointer user_data)
 {
-	const char *dst = user_data;
+	const struct bt_peer *bt_peer = user_data;
 	struct ofono_modem *modem = value;
-	const char *address = ofono_modem_get_string(modem, "Address");
+	const char *device = ofono_modem_get_string(modem, "Address");
+	struct hfp *hfp;
+	gboolean ret;
 
-	return g_strcmp0(address, dst) != 0 ? FALSE : TRUE;
+	ret = g_strcmp0(device, bt_peer->device);
+	if (ret != 0)
+		return FALSE;
+
+	hfp = ofono_modem_get_data(modem);
+	return g_strcmp0(hfp->adapter, bt_peer->adapter) != 0 ? FALSE : TRUE;
 }
 
 static void hfp_debug(const char *str, void *user_data)
@@ -284,14 +297,14 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 {
 	struct hfp *hfp;
 	struct ofono_modem *modem;
+	struct sockaddr_rc src, dst;
 	DBusMessageIter iter;
 	GDBusProxy *proxy;
 	DBusMessageIter entry;
-	const char *device, *alias, *address;
+	const char *device, *alias;
+	char device_address[18];
 	int fd, err;
 
-	DBG("Profile handler NewConnection");
-
 	if (dbus_message_iter_init(msg, &entry) == FALSE)
 		goto invalid;
 
@@ -310,11 +323,6 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 
 	dbus_message_iter_get_basic(&iter, &alias);
 
-	if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE)
-		goto invalid;
-
-	dbus_message_iter_get_basic(&iter, &address);
-
 	dbus_message_iter_next(&entry);
 	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
 		goto invalid;
@@ -323,7 +331,13 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	if (fd < 0)
 		goto invalid;
 
-	modem = modem_register(device, address, alias);
+	if (bt_getsockpeers(fd, (struct sockaddr *) &src,
+				(struct sockaddr *) &dst,
+				sizeof(struct sockaddr_rc)) < 0)
+		goto invalid;
+
+	bt_ba2str(&dst.rc_bdaddr, device_address);
+	modem = modem_register(device, device_address, alias);
 	if (modem == NULL) {
 		close(fd);
 		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
@@ -339,6 +353,9 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 
 	hfp = ofono_modem_get_data(modem);
 	hfp->msg = dbus_message_ref(msg);
+	bt_ba2str(&src.rc_bdaddr, hfp->adapter);
+
+	DBG("Profile handler NewConnection: %s", device_address);
 
 	return NULL;
 
@@ -394,9 +411,9 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 							gpointer user_data)
 {
 	struct ofono_modem *modem;
-	struct sockaddr_sco saddr;
+	struct sockaddr_sco src, dst;
+	struct bt_peer bt_peer;
 	socklen_t optlen;
-	char dst[18];
 	int sk, nsk;
 
 	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
@@ -404,15 +421,24 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 
 	sk = g_io_channel_unix_get_fd(io);
 
-	memset(&saddr, 0, sizeof(saddr));
-	optlen = sizeof(saddr);
+	memset(&dst, 0, sizeof(dst));
+	optlen = sizeof(dst);
 
-	nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
+	nsk = accept(sk, (struct sockaddr *) &dst, &optlen);
 	if (nsk < 0)
 		return TRUE;
 
-	bt_ba2str(&saddr.sco_bdaddr, dst);
-	modem = g_hash_table_find(modem_hash, modem_address_cmp, dst);
+	if (bt_getsockpeers(nsk, (struct sockaddr *) &src, NULL, optlen) < 0) {
+		close(nsk);
+		return TRUE;
+	}
+
+	bt_ba2str(&src.sco_bdaddr, bt_peer.adapter);
+	bt_ba2str(&dst.sco_bdaddr, bt_peer.device);
+
+	DBG("SCO: %s < %s", bt_peer.adapter, bt_peer.device);
+
+	modem = g_hash_table_find(modem_hash, modem_address_cmp, &bt_peer);
 	if (modem == NULL) {
 		ofono_error("Rejecting SCO: SLC connection missing!");
 		close(nsk);
-- 
1.7.11.7

[PATCH v0 9/9] hfp_hf_bluez5: Fix missing fd close

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

This patch fix an unusual scenario, service_level_connection() fails if
GIOChannel or GAtChat memory allocation fails.
---
 plugins/hfp_hf_bluez5.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 504b0f3..17c343b 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -346,10 +346,12 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	}
 
 	err = service_level_connection(modem, fd, HFP_VERSION_LATEST);
-	if (err < 0 && err != -EINPROGRESS)
+	if (err < 0 && err != -EINPROGRESS) {
+		close(fd);
 		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
 					".Rejected",
 					"Not enough resources");
+	}
 
 	hfp = ofono_modem_get_data(modem);
 	hfp->msg = dbus_message_ref(msg);
-- 
1.7.11.7

Re: [PATCH v0 5/9] bluez5: Add bt_getsockpeers()

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

Hi Claudio,

> This patch adds a generic Bluetooth helper function to allow getting
> the adapter and device Bluetooth Address from Bluetooth sockets.
> ---
>  plugins/bluez5.c | 22 ++++++++++++++++++++++
>  plugins/bluez5.h |  3 +++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/plugins/bluez5.c b/plugins/bluez5.c
> index d7e85f2..06629e1 100644
> --- a/plugins/bluez5.c
> +++ b/plugins/bluez5.c
> @@ -52,6 +52,28 @@ int bt_ba2str(const bdaddr_t *ba, char *str)
>  		ba->b[5], ba->b[4], ba->b[3], ba->b[2], ba->b[1], ba->b[0]);
>  }
>  
> +int bt_getsockpeers(int sock, struct sockaddr *src, struct sockaddr *dst,
> +								socklen_t len)
> +{

I really do not get why this is helpful at all. Except for making the
code less readable. So please do not do it. Call the socket functions
directly from where they are used.

In addition the name of this function is totally misleading. It does not
describe at all what it does. The peer is always the remote address. You
are not getting the src and dst peers here. That is not how the socket
nomenclature works. It is either socket name for the local address or
peer name for the remote address.

Regards

Marcel

Re: [PATCH v0 3/9] hfp_hf_bluez5: Add SCO listen socket

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 4298 bytes --]

Hi Claudio,

> This patch adds the initial SCO server socket handling. BtIO and BlueZ
> functions should not be used, with the new Profile API the objetive is
> to get rid of these dependencies.
> ---
>  plugins/hfp_hf_bluez5.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 0e496ee..398dee0 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -64,6 +64,7 @@ struct hfp {
>  static GHashTable *modem_hash = NULL;
>  static GHashTable *devices_proxies = NULL;
>  static GDBusClient *bluez = NULL;
> +static guint sco_watch = 0;
>  
>  static void hfp_debug(const char *str, void *user_data)
>  {
> @@ -378,6 +379,89 @@ static const GDBusMethodTable profile_methods[] = {
>  	{ }
>  };
>  
> +static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	struct sockaddr_sco saddr;
> +	socklen_t optlen;
> +	int sk, nsk;
> +
> +	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
> +		return FALSE;
> +
> +	sk = g_io_channel_unix_get_fd(io);
> +
> +	memset(&saddr, 0, sizeof(saddr));
> +	optlen = sizeof(saddr);
> +
> +	nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
> +	if (nsk < 0)
> +		return TRUE;
> +
> +	/* TODO: Verify if the device has a modem */
> +
> +	return TRUE;
> +}
> +
> +static GIOChannel *sco_listen(int *err)
> +{

I do not like this int *err. Why is this a separate function anyway.
Just do the whole SCO server socket setup with accept watch setup in one
function. There is no advantage in having two functions.

> +	struct sockaddr_sco addr;
> +	GIOChannel *io;
> +	int sk, defer_setup = 1;
> +
> +	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_SCO);
> +	if (sk < 0) {
> +		*err = -errno;
> +		return NULL;
> +	}
> +
> +	/* Bind to local address */
> +	memset(&addr, 0, sizeof(addr));
> +	addr.sco_family = AF_BLUETOOTH;
> +	bt_bacpy(&addr.sco_bdaddr, BDADDR_ANY);
> +
> +	if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
> +		close(sk);
> +		*err = -errno;
> +		return NULL;
> +	}
> +
> +	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
> +				&defer_setup, sizeof(defer_setup)) < 0) {
> +		ofono_warn("Can't enable deferred setup: %s (%d)",
> +						strerror(errno), errno);
> +		*err = -errno;

This is semantically incorrect. You can not return non-NULL and an
error.

Can we actually make HFP 1.6 work properly without defer setup enabled?

> +	}
> +
> +	io = g_io_channel_unix_new(sk);
> +	g_io_channel_set_close_on_unref(io, TRUE);
> +	g_io_channel_set_flags(io, G_IO_FLAG_NONBLOCK, NULL);

Can you just open the SOCK_NONBLOCK and SOCK_CLOEXEC as we should be
always doing.

> +
> +	if (listen(sk, 5) < 0) {
> +		g_io_channel_unref(io);
> +		*err = -errno;
> +		return NULL;
> +	}
> +
> +	return io;
> +}
> +
> +static int sco_init(void)
> +{
> +	GIOCondition cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;

What is this extra declaration good for? I rather have it inside the
call that is adding the watch.

> +	GIOChannel *sco_io;
> +	int err = 0;
> +
> +	sco_io = sco_listen(&err);
> +	if (sco_io == NULL)
> +		return err;
> +
> +	sco_watch = g_io_add_watch(sco_io, cond, sco_accept, NULL);
> +	g_io_channel_unref(sco_io);
> +
> +	return 0;
> +}
> +
>  static void connect_handler(DBusConnection *conn, void *user_data)
>  {
>  	DBG("Registering External Profile handler ...");
> @@ -500,6 +584,12 @@ static int hfp_init(void)
>  	if (DBUS_TYPE_UNIX_FD < 0)
>  		return -EBADF;
>  
> +	err = sco_init();
> +	if (err < 0) {
> +		ofono_error("SCO: %s(%d)", strerror(-err), -err);
> +		return err;
> +	}
> +
>  	/* Registers External Profile handler */
>  	if (!g_dbus_register_interface(conn, HFP_EXT_PROFILE_PATH,
>  					BLUEZ_PROFILE_INTERFACE,
> @@ -550,6 +640,9 @@ static void hfp_exit(void)
>  
>  	g_hash_table_destroy(modem_hash);
>  	g_hash_table_destroy(devices_proxies);
> +
> +	if (sco_watch)

Normally we do sco_watch > 0 for the GLib sources.

> +		g_source_remove(sco_watch);
>  }
>  
>  OFONO_PLUGIN_DEFINE(hfp_bluez5, "External Hands-Free Profile Plugin", VERSION,

Regards

Marcel

Re: [PATCH v0 8/9] hfp_hf_bluez5: Reject SCO if source doesn't match

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 4882 bytes --]

Hi Claudio,

> This patch implements additional verification checking the Bluetooth
> source address instead of the remote address only.
> ---
>  plugins/hfp_hf_bluez5.c | 64 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index d79647b..504b0f3 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -60,6 +60,12 @@
>  struct hfp {
>  	struct hfp_slc_info info;
>  	DBusMessage *msg;
> +	char adapter[18];
> +};
> +
> +struct bt_peer {
> +	char adapter[18];
> +	char device[18];
>  };

I do not get this. I am really not sure what this is all about and why
having Bluetooth addresses as strings here is a good idea.

>  
>  static GHashTable *modem_hash = NULL;
> @@ -70,11 +76,18 @@ static guint sco_watch = 0;
>  static gboolean modem_address_cmp(gpointer key, gpointer value,
>  						gpointer user_data)
>  {
> -	const char *dst = user_data;
> +	const struct bt_peer *bt_peer = user_data;
>  	struct ofono_modem *modem = value;
> -	const char *address = ofono_modem_get_string(modem, "Address");
> +	const char *device = ofono_modem_get_string(modem, "Address");
> +	struct hfp *hfp;
> +	gboolean ret;
>  
> -	return g_strcmp0(address, dst) != 0 ? FALSE : TRUE;
> +	ret = g_strcmp0(device, bt_peer->device);
> +	if (ret != 0)
> +		return FALSE;
> +
> +	hfp = ofono_modem_get_data(modem);
> +	return g_strcmp0(hfp->adapter, bt_peer->adapter) != 0 ? FALSE : TRUE;
>  }
>  
>  static void hfp_debug(const char *str, void *user_data)
> @@ -284,14 +297,14 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>  {
>  	struct hfp *hfp;
>  	struct ofono_modem *modem;
> +	struct sockaddr_rc src, dst;
>  	DBusMessageIter iter;
>  	GDBusProxy *proxy;
>  	DBusMessageIter entry;
> -	const char *device, *alias, *address;
> +	const char *device, *alias;
> +	char device_address[18];
>  	int fd, err;
>  
> -	DBG("Profile handler NewConnection");
> -
>  	if (dbus_message_iter_init(msg, &entry) == FALSE)
>  		goto invalid;
>  
> @@ -310,11 +323,6 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>  
>  	dbus_message_iter_get_basic(&iter, &alias);
>  
> -	if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE)
> -		goto invalid;
> -
> -	dbus_message_iter_get_basic(&iter, &address);
> -
>  	dbus_message_iter_next(&entry);
>  	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>  		goto invalid;
> @@ -323,7 +331,13 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>  	if (fd < 0)
>  		goto invalid;
>  
> -	modem = modem_register(device, address, alias);
> +	if (bt_getsockpeers(fd, (struct sockaddr *) &src,
> +				(struct sockaddr *) &dst,
> +				sizeof(struct sockaddr_rc)) < 0)
> +		goto invalid;
> +
> +	bt_ba2str(&dst.rc_bdaddr, device_address);
> +	modem = modem_register(device, device_address, alias);
>  	if (modem == NULL) {
>  		close(fd);
>  		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
> @@ -339,6 +353,9 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>  
>  	hfp = ofono_modem_get_data(modem);
>  	hfp->msg = dbus_message_ref(msg);
> +	bt_ba2str(&src.rc_bdaddr, hfp->adapter);
> +
> +	DBG("Profile handler NewConnection: %s", device_address);
>  
>  	return NULL;
>  
> @@ -394,9 +411,9 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>  							gpointer user_data)
>  {
>  	struct ofono_modem *modem;
> -	struct sockaddr_sco saddr;
> +	struct sockaddr_sco src, dst;
> +	struct bt_peer bt_peer;
>  	socklen_t optlen;
> -	char dst[18];
>  	int sk, nsk;
>  
>  	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
> @@ -404,15 +421,24 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>  
>  	sk = g_io_channel_unix_get_fd(io);
>  
> -	memset(&saddr, 0, sizeof(saddr));
> -	optlen = sizeof(saddr);
> +	memset(&dst, 0, sizeof(dst));
> +	optlen = sizeof(dst);
>  
> -	nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
> +	nsk = accept(sk, (struct sockaddr *) &dst, &optlen);
>  	if (nsk < 0)
>  		return TRUE;
>  
> -	bt_ba2str(&saddr.sco_bdaddr, dst);
> -	modem = g_hash_table_find(modem_hash, modem_address_cmp, dst);
> +	if (bt_getsockpeers(nsk, (struct sockaddr *) &src, NULL, optlen) < 0) {
> +		close(nsk);
> +		return TRUE;
> +	}
> +
> +	bt_ba2str(&src.sco_bdaddr, bt_peer.adapter);
> +	bt_ba2str(&dst.sco_bdaddr, bt_peer.device);
> +
> +	DBG("SCO: %s < %s", bt_peer.adapter, bt_peer.device);
> +
> +	modem = g_hash_table_find(modem_hash, modem_address_cmp, &bt_peer);

You do realize that using a hash table if you have to iterate it is
pretty inefficient. Have you compared your find vs lookup operations?

Regards

Marcel

Re: [PATCH v0 5/9] bluez5: Add bt_getsockpeers()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

Hi Marcel:

On Mon, Jan 28, 2013 at 12:32 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Claudio,
>
>> This patch adds a generic Bluetooth helper function to allow getting
>> the adapter and device Bluetooth Address from Bluetooth sockets.
>> ---
>>  plugins/bluez5.c | 22 ++++++++++++++++++++++
>>  plugins/bluez5.h |  3 +++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/plugins/bluez5.c b/plugins/bluez5.c
>> index d7e85f2..06629e1 100644
>> --- a/plugins/bluez5.c
>> +++ b/plugins/bluez5.c
>> @@ -52,6 +52,28 @@ int bt_ba2str(const bdaddr_t *ba, char *str)
>>               ba->b[5], ba->b[4], ba->b[3], ba->b[2], ba->b[1], ba->b[0]);
>>  }
>>
>> +int bt_getsockpeers(int sock, struct sockaddr *src, struct sockaddr *dst,
>> +                                                             socklen_t len)
>> +{
>
> I really do not get why this is helpful at all. Except for making the
> code less readable. So please do not do it. Call the socket functions
> directly from where they are used.
>
> In addition the name of this function is totally misleading. It does not
> describe at all what it does. The peer is always the remote address. You
> are not getting the src and dst peers here. That is not how the socket
> nomenclature works. It is either socket name for the local address or
> peer name for the remote address.
>
> Regards
>
> Marcel

Ok. I will remove this function. I added this function here because
other oFono Plugins will need a similar function.
We can add it later if we notice that it is really necessary.

Regards,
Claudio

Re: [PATCH v0 3/9] hfp_hf_bluez5: Add SCO listen socket

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 5609 bytes --]

Hi Marcel:

On Mon, Jan 28, 2013 at 12:59 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Claudio,
>
>> This patch adds the initial SCO server socket handling. BtIO and BlueZ
>> functions should not be used, with the new Profile API the objetive is
>> to get rid of these dependencies.
>> ---
>>  plugins/hfp_hf_bluez5.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 93 insertions(+)
>>
>> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
>> index 0e496ee..398dee0 100644
>> --- a/plugins/hfp_hf_bluez5.c
>> +++ b/plugins/hfp_hf_bluez5.c
>> @@ -64,6 +64,7 @@ struct hfp {
>>  static GHashTable *modem_hash = NULL;
>>  static GHashTable *devices_proxies = NULL;
>>  static GDBusClient *bluez = NULL;
>> +static guint sco_watch = 0;
>>
>>  static void hfp_debug(const char *str, void *user_data)
>>  {
>> @@ -378,6 +379,89 @@ static const GDBusMethodTable profile_methods[] = {
>>       { }
>>  };
>>
>> +static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>> +                                                     gpointer user_data)
>> +{
>> +     struct sockaddr_sco saddr;
>> +     socklen_t optlen;
>> +     int sk, nsk;
>> +
>> +     if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
>> +             return FALSE;
>> +
>> +     sk = g_io_channel_unix_get_fd(io);
>> +
>> +     memset(&saddr, 0, sizeof(saddr));
>> +     optlen = sizeof(saddr);
>> +
>> +     nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
>> +     if (nsk < 0)
>> +             return TRUE;
>> +
>> +     /* TODO: Verify if the device has a modem */
>> +
>> +     return TRUE;
>> +}
>> +
>> +static GIOChannel *sco_listen(int *err)
>> +{
>
> I do not like this int *err. Why is this a separate function anyway.
> Just do the whole SCO server socket setup with accept watch setup in one
> function. There is no advantage in having two functions.

OK. I will fix it.

>
>> +     struct sockaddr_sco addr;
>> +     GIOChannel *io;
>> +     int sk, defer_setup = 1;
>> +
>> +     sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_SCO);
>> +     if (sk < 0) {
>> +             *err = -errno;
>> +             return NULL;
>> +     }
>> +
>> +     /* Bind to local address */
>> +     memset(&addr, 0, sizeof(addr));
>> +     addr.sco_family = AF_BLUETOOTH;
>> +     bt_bacpy(&addr.sco_bdaddr, BDADDR_ANY);
>> +
>> +     if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
>> +             close(sk);
>> +             *err = -errno;
>> +             return NULL;
>> +     }
>> +
>> +     if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
>> +                             &defer_setup, sizeof(defer_setup)) < 0) {
>> +             ofono_warn("Can't enable deferred setup: %s (%d)",
>> +                                             strerror(errno), errno);
>> +             *err = -errno;
>
> This is semantically incorrect. You can not return non-NULL and an
> error.

It will be removed.

>
> Can we actually make HFP 1.6 work properly without defer setup enabled?

You can't make HFP1.6 work properly, but you need to "start" the
system to support 1.5, don't set the codec negotiation flag and export
the correct SDP record.

>
>> +     }
>> +
>> +     io = g_io_channel_unix_new(sk);
>> +     g_io_channel_set_close_on_unref(io, TRUE);
>> +     g_io_channel_set_flags(io, G_IO_FLAG_NONBLOCK, NULL);
>
> Can you just open the SOCK_NONBLOCK and SOCK_CLOEXEC as we should be
> always doing.

ok. It will be fixed.

>
>> +
>> +     if (listen(sk, 5) < 0) {
>> +             g_io_channel_unref(io);
>> +             *err = -errno;
>> +             return NULL;
>> +     }
>> +
>> +     return io;
>> +}
>> +
>> +static int sco_init(void)
>> +{
>> +     GIOCondition cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;
>
> What is this extra declaration good for? I rather have it inside the
> call that is adding the watch.

ok. I will fix it.
IMO it makes the code more readable. The compiler will optimize it anyway.

>
>> +     GIOChannel *sco_io;
>> +     int err = 0;
>> +
>> +     sco_io = sco_listen(&err);
>> +     if (sco_io == NULL)
>> +             return err;
>> +
>> +     sco_watch = g_io_add_watch(sco_io, cond, sco_accept, NULL);
>> +     g_io_channel_unref(sco_io);
>> +
>> +     return 0;
>> +}
>> +
>>  static void connect_handler(DBusConnection *conn, void *user_data)
>>  {
>>       DBG("Registering External Profile handler ...");
>> @@ -500,6 +584,12 @@ static int hfp_init(void)
>>       if (DBUS_TYPE_UNIX_FD < 0)
>>               return -EBADF;
>>
>> +     err = sco_init();
>> +     if (err < 0) {
>> +             ofono_error("SCO: %s(%d)", strerror(-err), -err);
>> +             return err;
>> +     }
>> +
>>       /* Registers External Profile handler */
>>       if (!g_dbus_register_interface(conn, HFP_EXT_PROFILE_PATH,
>>                                       BLUEZ_PROFILE_INTERFACE,
>> @@ -550,6 +640,9 @@ static void hfp_exit(void)
>>
>>       g_hash_table_destroy(modem_hash);
>>       g_hash_table_destroy(devices_proxies);
>> +
>> +     if (sco_watch)
>
> Normally we do sco_watch > 0 for the GLib sources.

ok. I will fix it.

Regards,
Claudio

>
>> +             g_source_remove(sco_watch);
>>  }
>>
>>  OFONO_PLUGIN_DEFINE(hfp_bluez5, "External Hands-Free Profile Plugin", VERSION,
>
> Regards
>
> Marcel
>
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono

Re: [PATCH v0 8/9] hfp_hf_bluez5: Reject SCO if source doesn't match

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 6406 bytes --]

Hi Marcel:

On Mon, Jan 28, 2013 at 1:04 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Claudio,
>
>> This patch implements additional verification checking the Bluetooth
>> source address instead of the remote address only.
>> ---
>>  plugins/hfp_hf_bluez5.c | 64 ++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
>> index d79647b..504b0f3 100644
>> --- a/plugins/hfp_hf_bluez5.c
>> +++ b/plugins/hfp_hf_bluez5.c
>> @@ -60,6 +60,12 @@
>>  struct hfp {
>>       struct hfp_slc_info info;
>>       DBusMessage *msg;
>> +     char adapter[18];
>> +};
>> +
>> +struct bt_peer {
>> +     char adapter[18];
>> +     char device[18];
>>  };
>
> I do not get this. I am really not sure what this is all about and why
> having Bluetooth addresses as strings here is a good idea.

Ok. I will use the bdaddr_t

I am trying to keep a standard: the device address is a string stored
in the modem "object".
hfp_hf_bluez4 is also using the same approach.

>
>>
>>  static GHashTable *modem_hash = NULL;
>> @@ -70,11 +76,18 @@ static guint sco_watch = 0;
>>  static gboolean modem_address_cmp(gpointer key, gpointer value,
>>                                               gpointer user_data)
>>  {
>> -     const char *dst = user_data;
>> +     const struct bt_peer *bt_peer = user_data;
>>       struct ofono_modem *modem = value;
>> -     const char *address = ofono_modem_get_string(modem, "Address");
>> +     const char *device = ofono_modem_get_string(modem, "Address");
>> +     struct hfp *hfp;
>> +     gboolean ret;
>>
>> -     return g_strcmp0(address, dst) != 0 ? FALSE : TRUE;
>> +     ret = g_strcmp0(device, bt_peer->device);
>> +     if (ret != 0)
>> +             return FALSE;
>> +
>> +     hfp = ofono_modem_get_data(modem);
>> +     return g_strcmp0(hfp->adapter, bt_peer->adapter) != 0 ? FALSE : TRUE;
>>  }
>>
>>  static void hfp_debug(const char *str, void *user_data)
>> @@ -284,14 +297,14 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>>  {
>>       struct hfp *hfp;
>>       struct ofono_modem *modem;
>> +     struct sockaddr_rc src, dst;
>>       DBusMessageIter iter;
>>       GDBusProxy *proxy;
>>       DBusMessageIter entry;
>> -     const char *device, *alias, *address;
>> +     const char *device, *alias;
>> +     char device_address[18];
>>       int fd, err;
>>
>> -     DBG("Profile handler NewConnection");
>> -
>>       if (dbus_message_iter_init(msg, &entry) == FALSE)
>>               goto invalid;
>>
>> @@ -310,11 +323,6 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>>
>>       dbus_message_iter_get_basic(&iter, &alias);
>>
>> -     if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE)
>> -             goto invalid;
>> -
>> -     dbus_message_iter_get_basic(&iter, &address);
>> -
>>       dbus_message_iter_next(&entry);
>>       if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>>               goto invalid;
>> @@ -323,7 +331,13 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>>       if (fd < 0)
>>               goto invalid;
>>
>> -     modem = modem_register(device, address, alias);
>> +     if (bt_getsockpeers(fd, (struct sockaddr *) &src,
>> +                             (struct sockaddr *) &dst,
>> +                             sizeof(struct sockaddr_rc)) < 0)
>> +             goto invalid;
>> +
>> +     bt_ba2str(&dst.rc_bdaddr, device_address);
>> +     modem = modem_register(device, device_address, alias);
>>       if (modem == NULL) {
>>               close(fd);
>>               return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
>> @@ -339,6 +353,9 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>>
>>       hfp = ofono_modem_get_data(modem);
>>       hfp->msg = dbus_message_ref(msg);
>> +     bt_ba2str(&src.rc_bdaddr, hfp->adapter);
>> +
>> +     DBG("Profile handler NewConnection: %s", device_address);
>>
>>       return NULL;
>>
>> @@ -394,9 +411,9 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>>                                                       gpointer user_data)
>>  {
>>       struct ofono_modem *modem;
>> -     struct sockaddr_sco saddr;
>> +     struct sockaddr_sco src, dst;
>> +     struct bt_peer bt_peer;
>>       socklen_t optlen;
>> -     char dst[18];
>>       int sk, nsk;
>>
>>       if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
>> @@ -404,15 +421,24 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>>
>>       sk = g_io_channel_unix_get_fd(io);
>>
>> -     memset(&saddr, 0, sizeof(saddr));
>> -     optlen = sizeof(saddr);
>> +     memset(&dst, 0, sizeof(dst));
>> +     optlen = sizeof(dst);
>>
>> -     nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
>> +     nsk = accept(sk, (struct sockaddr *) &dst, &optlen);
>>       if (nsk < 0)
>>               return TRUE;
>>
>> -     bt_ba2str(&saddr.sco_bdaddr, dst);
>> -     modem = g_hash_table_find(modem_hash, modem_address_cmp, dst);
>> +     if (bt_getsockpeers(nsk, (struct sockaddr *) &src, NULL, optlen) < 0) {
>> +             close(nsk);
>> +             return TRUE;
>> +     }
>> +
>> +     bt_ba2str(&src.sco_bdaddr, bt_peer.adapter);
>> +     bt_ba2str(&dst.sco_bdaddr, bt_peer.device);
>> +
>> +     DBG("SCO: %s < %s", bt_peer.adapter, bt_peer.device);
>> +
>> +     modem = g_hash_table_find(modem_hash, modem_address_cmp, &bt_peer);
>
> You do realize that using a hash table if you have to iterate it is
> pretty inefficient. Have you compared your find vs lookup operations?

I agree, but did you realize that the hash key is the device object
path(not the address)?
Remember that we need to support multiple adapters also. NewConnection
sends the device object, Device InterfaceAdded informs the device
object path, the device address and the adapter *object path*.
I could add an additional adapter proxy hash table to be able to map
adapter path to adapter Bluetooth Address, but it doesn't worth.

Regards,
Claudio.

>
> Regards
>
> Marcel
>
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono

[PATCH v1 0/8] External HFP: Add SCO

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

This patch series adds Bluetooth SCO socket declaration and the initial 
code to manage HF audio connections.

Bluetooth headers is being declared locally since the objective in the
future is to remove this dependency.

Changes between v0-v1:
* merged sco_init and sco_listen
* set sockt flags when the socket is created
* removed bt_getsockpeers
* other minor fixes

Pending:
* g_hash_table_find vs g_hash_table_lookup: It is not feasible
to use lookup. It will require to use another hash table key for the
modem or create an additional adapter hash table.

Claudio Takahasi (8):
  bluez5: Add SCO socket declarations
  bluez5: Add bt_bacpy()
  hfp_hf_bluez5: Add SCO listen socket
  bluez5: Add bt_ba2str()
  bluez5: Add bt_bacmp()
  bluez5: Add RFCOMM socket address declaration
  hfp_hf_bluez5: Add rejecting SCO connection
  hfp_hf_bluez5: Fix missing fd close

 plugins/bluez5.c        |  21 +++++++
 plugins/bluez5.h        |  41 +++++++++++++
 plugins/hfp_hf_bluez5.c | 156 ++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 208 insertions(+), 10 deletions(-)

-- 
1.7.11.7

[PATCH v1 1/8] bluez5: Add SCO socket declarations

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

Adds local copy of SCO Bluetooth sockets declarations, since the
objective to avoid including BlueZ library headers.
---
 plugins/bluez5.c        |  3 +++
 plugins/bluez5.h        | 28 ++++++++++++++++++++++++++++
 plugins/hfp_hf_bluez5.c |  2 ++
 3 files changed, 33 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index 8d8b565..d471454 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -24,6 +24,9 @@
 #endif
 
 #include <errno.h>
+#include <stdint.h>
+#include <sys/socket.h>
+
 #include <glib.h>
 
 #define OFONO_API_SUBJECT_TO_CHANGE
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 3aa8ffe..fd0704e 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -28,6 +28,34 @@
 #define HFP_HS_UUID	"0000111e-0000-1000-8000-00805f9b34fb"
 #define HFP_AG_UUID	"0000111f-0000-1000-8000-00805f9b34fb"
 
+#ifndef AF_BLUETOOTH
+#define AF_BLUETOOTH		31
+#define PF_BLUETOOTH		AF_BLUETOOTH
+#endif
+
+#define BTPROTO_SCO		2
+
+#define SOL_SCO			17
+
+#ifndef SOL_BLUETOOTH
+#define SOL_BLUETOOTH		274
+#endif
+
+#define BT_DEFER_SETUP		7
+
+/* BD Address */
+typedef struct {
+	uint8_t b[6];
+} __attribute__((packed)) bdaddr_t;
+
+#define BDADDR_ANY   (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
+
+/* SCO socket address */
+struct sockaddr_sco {
+	sa_family_t	sco_family;
+	bdaddr_t	sco_bdaddr;
+};
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index f6ceb76..0e496ee 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -24,9 +24,11 @@
 #endif
 
 #include <errno.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <sys/socket.h>
 
 #include <glib.h>
 
-- 
1.7.11.7

[PATCH v1 2/8] bluez5: Add bt_bacpy()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

Adds a copy of BlueZ bacpy() function.
---
 plugins/bluez5.c | 6 ++++++
 plugins/bluez5.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index d471454..876ad2d 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdint.h>
 #include <sys/socket.h>
+#include <string.h>
 
 #include <glib.h>
 
@@ -39,6 +40,11 @@
 
 #define BLUEZ_PROFILE_MGMT_INTERFACE   BLUEZ_SERVICE ".ProfileManager1"
 
+void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src)
+{
+	memcpy(dst, src, sizeof(bdaddr_t));
+}
+
 static void profile_register_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index fd0704e..3921c7b 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -56,6 +56,8 @@ struct sockaddr_sco {
 	bdaddr_t	sco_bdaddr;
 };
 
+void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src);
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
-- 
1.7.11.7

[PATCH v1 3/8] hfp_hf_bluez5: Add SCO listen socket

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

This patch adds the initial SCO server socket handling. BtIO and BlueZ
functions should not be used, with the new Profile API the objetive is
to get rid of these dependencies.
---
 plugins/hfp_hf_bluez5.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 0e496ee..e7d8717 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -28,6 +28,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <fcntl.h>
 #include <sys/socket.h>
 
 #include <glib.h>
@@ -64,6 +65,7 @@ struct hfp {
 static GHashTable *modem_hash = NULL;
 static GHashTable *devices_proxies = NULL;
 static GDBusClient *bluez = NULL;
+static guint sco_watch = 0;
 
 static void hfp_debug(const char *str, void *user_data)
 {
@@ -378,6 +380,72 @@ static const GDBusMethodTable profile_methods[] = {
 	{ }
 };
 
+static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
+							gpointer user_data)
+{
+	struct sockaddr_sco saddr;
+	socklen_t alen;
+	int sk, nsk;
+
+	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
+		return FALSE;
+
+	sk = g_io_channel_unix_get_fd(io);
+
+	memset(&saddr, 0, sizeof(saddr));
+	alen = sizeof(saddr);
+
+	nsk = accept(sk, (struct sockaddr *) &saddr, &alen);
+	if (nsk < 0)
+		return TRUE;
+
+	/* TODO: Verify if the device has a modem */
+
+	return TRUE;
+}
+
+static int sco_init(void)
+{
+	GIOChannel *sco_io;
+	struct sockaddr_sco saddr;
+	int sk, defer_setup = 1;
+
+	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
+								BTPROTO_SCO);
+	if (sk < 0)
+		return -errno;
+
+	/* Bind to local address */
+	memset(&saddr, 0, sizeof(saddr));
+	saddr.sco_family = AF_BLUETOOTH;
+	bt_bacpy(&saddr.sco_bdaddr, BDADDR_ANY);
+
+	if (bind(sk, (struct sockaddr *) &saddr, sizeof(saddr)) < 0) {
+		close(sk);
+		return -errno;
+	}
+
+	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
+				&defer_setup, sizeof(defer_setup)) < 0) {
+		ofono_warn("Can't enable deferred setup: %s (%d)",
+						strerror(errno), errno);
+	}
+
+	if (listen(sk, 5) < 0) {
+		close(sk);
+		return -errno;
+	}
+
+	sco_io = g_io_channel_unix_new(sk);
+	sco_watch = g_io_add_watch(sco_io,
+				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+				sco_accept, NULL);
+
+	g_io_channel_unref(sco_io);
+
+	return 0;
+}
+
 static void connect_handler(DBusConnection *conn, void *user_data)
 {
 	DBG("Registering External Profile handler ...");
@@ -500,6 +568,12 @@ static int hfp_init(void)
 	if (DBUS_TYPE_UNIX_FD < 0)
 		return -EBADF;
 
+	err = sco_init();
+	if (err < 0) {
+		ofono_error("SCO: %s(%d)", strerror(-err), -err);
+		return err;
+	}
+
 	/* Registers External Profile handler */
 	if (!g_dbus_register_interface(conn, HFP_EXT_PROFILE_PATH,
 					BLUEZ_PROFILE_INTERFACE,
@@ -550,6 +624,9 @@ static void hfp_exit(void)
 
 	g_hash_table_destroy(modem_hash);
 	g_hash_table_destroy(devices_proxies);
+
+	if (sco_watch > 0)
+		g_source_remove(sco_watch);
 }
 
 OFONO_PLUGIN_DEFINE(hfp_bluez5, "External Hands-Free Profile Plugin", VERSION,
-- 
1.7.11.7

[PATCH v1 4/8] bluez5: Add bt_ba2str()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

Adds a copy of BlueZ ba2str() function.
---
 plugins/bluez5.c        | 7 +++++++
 plugins/bluez5.h        | 2 ++
 plugins/hfp_hf_bluez5.c | 1 +
 3 files changed, 10 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index 876ad2d..d7e85f2 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -25,6 +25,7 @@
 
 #include <errno.h>
 #include <stdint.h>
+#include <stdio.h>
 #include <sys/socket.h>
 #include <string.h>
 
@@ -45,6 +46,12 @@ void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src)
 	memcpy(dst, src, sizeof(bdaddr_t));
 }
 
+int bt_ba2str(const bdaddr_t *ba, char *str)
+{
+	return sprintf(str, "%2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X",
+		ba->b[5], ba->b[4], ba->b[3], ba->b[2], ba->b[1], ba->b[0]);
+}
+
 static void profile_register_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 3921c7b..204c41d 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -58,6 +58,8 @@ struct sockaddr_sco {
 
 void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src);
 
+int bt_ba2str(const bdaddr_t *ba, char *str);
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index e7d8717..87ae5e3 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -26,6 +26,7 @@
 #include <errno.h>
 #include <stdint.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <unistd.h>
 #include <string.h>
 #include <fcntl.h>
-- 
1.7.11.7

[PATCH v1 5/8] bluez5: Add bt_bacmp()

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

Adds a copy of BlueZ bacmp() function.
---
 plugins/bluez5.c | 5 +++++
 plugins/bluez5.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/plugins/bluez5.c b/plugins/bluez5.c
index d7e85f2..d5c566e 100644
--- a/plugins/bluez5.c
+++ b/plugins/bluez5.c
@@ -52,6 +52,11 @@ int bt_ba2str(const bdaddr_t *ba, char *str)
 		ba->b[5], ba->b[4], ba->b[3], ba->b[2], ba->b[1], ba->b[0]);
 }
 
+int bt_bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
+{
+	return memcmp(ba1, ba2, sizeof(bdaddr_t));
+}
+
 static void profile_register_cb(DBusPendingCall *call, gpointer user_data)
 {
 	DBusMessage *reply;
diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 204c41d..95914c2 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -60,6 +60,8 @@ void bt_bacpy(bdaddr_t *dst, const bdaddr_t *src);
 
 int bt_ba2str(const bdaddr_t *ba, char *str);
 
+int bt_bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2);
+
 int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
 					const char *name, const char *object);
 
-- 
1.7.11.7

[PATCH v1 6/8] bluez5: Add RFCOMM socket address declaration

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

This patch adds a copy of Bluetooth RFCOMM socket declaration.
---
 plugins/bluez5.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/plugins/bluez5.h b/plugins/bluez5.h
index 95914c2..cdbfe72 100644
--- a/plugins/bluez5.h
+++ b/plugins/bluez5.h
@@ -50,6 +50,13 @@ typedef struct {
 
 #define BDADDR_ANY   (&(bdaddr_t) {{0, 0, 0, 0, 0, 0}})
 
+/* RFCOMM socket address */
+struct sockaddr_rc {
+	sa_family_t	rc_family;
+	bdaddr_t	rc_bdaddr;
+	uint8_t		rc_channel;
+};
+
 /* SCO socket address */
 struct sockaddr_sco {
 	sa_family_t	sco_family;
-- 
1.7.11.7

[PATCH v1 7/8] hfp_hf_bluez5: Add rejecting SCO connection

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 4472 bytes --]

This patch rejects the SCO incoming connection if there isn't a modem
(service level connection) associated with the address.
---
 plugins/hfp_hf_bluez5.c | 82 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 14 deletions(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 87ae5e3..871c720 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -61,6 +61,13 @@
 struct hfp {
 	struct hfp_slc_info info;
 	DBusMessage *msg;
+	bdaddr_t src;
+	bdaddr_t dst;
+};
+
+struct sock_bdaddr {
+	bdaddr_t src;
+	bdaddr_t dst;
 };
 
 static GHashTable *modem_hash = NULL;
@@ -68,6 +75,21 @@ static GHashTable *devices_proxies = NULL;
 static GDBusClient *bluez = NULL;
 static guint sco_watch = 0;
 
+static gboolean modem_bacmp(gpointer key, gpointer value,
+						gpointer user_data)
+{
+	const struct sock_bdaddr *bda = user_data;
+	struct ofono_modem *modem = value;
+	struct hfp *hfp = ofono_modem_get_data(modem);
+	int ret;
+
+	ret = bt_bacmp(&bda->dst, &hfp->dst);
+	if (ret != 0)
+		return FALSE;
+
+	return bt_bacmp(&bda->src, &hfp->src) != 0 ? FALSE : TRUE;
+}
+
 static void hfp_debug(const char *str, void *user_data)
 {
 	const char *prefix = user_data;
@@ -275,14 +297,15 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 {
 	struct hfp *hfp;
 	struct ofono_modem *modem;
+	struct sockaddr_rc src, dst;
+	socklen_t alen;
 	DBusMessageIter iter;
 	GDBusProxy *proxy;
 	DBusMessageIter entry;
-	const char *device, *alias, *address;
+	const char *device, *alias;
+	char device_address[18];
 	int fd, err;
 
-	DBG("Profile handler NewConnection");
-
 	if (dbus_message_iter_init(msg, &entry) == FALSE)
 		goto invalid;
 
@@ -301,11 +324,6 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 
 	dbus_message_iter_get_basic(&iter, &alias);
 
-	if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE)
-		goto invalid;
-
-	dbus_message_iter_get_basic(&iter, &address);
-
 	dbus_message_iter_next(&entry);
 	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
 		goto invalid;
@@ -314,7 +332,24 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	if (fd < 0)
 		goto invalid;
 
-	modem = modem_register(device, address, alias);
+	memset(&src, 0, sizeof(src));
+	alen = sizeof(src);
+	if (getsockname(fd, (struct sockaddr *) &src, &alen) < 0) {
+		close(fd);
+		goto invalid;
+	}
+
+	memset(&dst, 0, sizeof(dst));
+	alen = sizeof(dst);
+	if (getpeername(fd, (struct sockaddr *) &dst, &alen) < 0) {
+		close(fd);
+		goto invalid;
+	}
+
+	bt_ba2str(&dst.rc_bdaddr, device_address);
+	DBG("Profile handler NewConnection: %s", device_address);
+
+	modem = modem_register(device, device_address, alias);
 	if (modem == NULL) {
 		close(fd);
 		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
@@ -329,6 +364,8 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 					"Not enough resources");
 
 	hfp = ofono_modem_get_data(modem);
+	bt_bacpy(&hfp->src, &src.rc_bdaddr);
+	bt_bacpy(&hfp->dst, &dst.rc_bdaddr);
 	hfp->msg = dbus_message_ref(msg);
 
 	return NULL;
@@ -384,7 +421,9 @@ static const GDBusMethodTable profile_methods[] = {
 static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 							gpointer user_data)
 {
-	struct sockaddr_sco saddr;
+	struct ofono_modem *modem;
+	struct sockaddr_sco src, dst;
+	struct sock_bdaddr bda;
 	socklen_t alen;
 	int sk, nsk;
 
@@ -393,14 +432,29 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 
 	sk = g_io_channel_unix_get_fd(io);
 
-	memset(&saddr, 0, sizeof(saddr));
-	alen = sizeof(saddr);
+	memset(&dst, 0, sizeof(dst));
+	alen = sizeof(dst);
 
-	nsk = accept(sk, (struct sockaddr *) &saddr, &alen);
+	nsk = accept(sk, (struct sockaddr *) &dst, &alen);
 	if (nsk < 0)
 		return TRUE;
 
-	/* TODO: Verify if the device has a modem */
+	memset(&src, 0, sizeof(src));
+	alen = sizeof(src);
+	if (getsockname(nsk, (struct sockaddr *) &src, &alen) < 0) {
+		close(nsk);
+		return TRUE;
+	}
+
+	bt_bacpy(&bda.src, &src.sco_bdaddr);
+	bt_bacpy(&bda.dst, &dst.sco_bdaddr);
+
+	modem = g_hash_table_find(modem_hash, modem_bacmp, &bda);
+	if (modem == NULL) {
+		ofono_error("Rejecting SCO: SLC connection missing!");
+		close(nsk);
+		return TRUE;
+	}
 
 	return TRUE;
 }
-- 
1.7.11.7

[PATCH v1 8/8] hfp_hf_bluez5: Fix missing fd close

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

This patch fix an unusual scenario, service_level_connection() fails if
GIOChannel or GAtChat memory allocation fails.
---
 plugins/hfp_hf_bluez5.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 871c720..08c6fed 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -358,10 +358,12 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	}
 
 	err = service_level_connection(modem, fd, HFP_VERSION_LATEST);
-	if (err < 0 && err != -EINPROGRESS)
+	if (err < 0 && err != -EINPROGRESS) {
+		close(fd);
 		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
 					".Rejected",
 					"Not enough resources");
+	}
 
 	hfp = ofono_modem_get_data(modem);
 	bt_bacpy(&hfp->src, &src.rc_bdaddr);
-- 
1.7.11.7

Re: [PATCH v1 1/8] bluez5: Add SCO socket declarations

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> Adds local copy of SCO Bluetooth sockets declarations, since the
> objective to avoid including BlueZ library headers.
> ---
>   plugins/bluez5.c        |  3 +++
>   plugins/bluez5.h        | 28 ++++++++++++++++++++++++++++
>   plugins/hfp_hf_bluez5.c |  2 ++
>   3 files changed, 33 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v1 2/8] bluez5: Add bt_bacpy()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> Adds a copy of BlueZ bacpy() function.
> ---
>   plugins/bluez5.c | 6 ++++++
>   plugins/bluez5.h | 2 ++
>   2 files changed, 8 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v1 3/8] hfp_hf_bluez5: Add SCO listen socket

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> This patch adds the initial SCO server socket handling. BtIO and BlueZ
> functions should not be used, with the new Profile API the objetive is
> to get rid of these dependencies.
> ---
>   plugins/hfp_hf_bluez5.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 77 insertions(+)
>

Patch has been applied, however I had to amend:

> +	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
> +				&defer_setup, sizeof(defer_setup))<  0) {
> +		ofono_warn("Can't enable deferred setup: %s (%d)",
> +						strerror(errno), errno);
> +	}

These brackets are not necessary.  Also, in the future you might want to 
set a flag here indicating that only HFP 1.5 support is available.

Regards,
-Denis

Re: [PATCH v1 4/8] bluez5: Add bt_ba2str()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> Adds a copy of BlueZ ba2str() function.
> ---
>   plugins/bluez5.c        | 7 +++++++
>   plugins/bluez5.h        | 2 ++
>   plugins/hfp_hf_bluez5.c | 1 +
>   3 files changed, 10 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v1 5/8] bluez5: Add bt_bacmp()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> Adds a copy of BlueZ bacmp() function.
> ---
>   plugins/bluez5.c | 5 +++++
>   plugins/bluez5.h | 2 ++
>   2 files changed, 7 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v1 6/8] bluez5: Add RFCOMM socket address declaration

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> This patch adds a copy of Bluetooth RFCOMM socket declaration.
> ---
>   plugins/bluez5.h | 7 +++++++
>   1 file changed, 7 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v1 7/8] hfp_hf_bluez5: Add rejecting SCO connection

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> This patch rejects the SCO incoming connection if there isn't a modem
> (service level connection) associated with the address.

Why service level connection is in parenthesis?

> ---
>   plugins/hfp_hf_bluez5.c | 82 ++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 68 insertions(+), 14 deletions(-)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 87ae5e3..871c720 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -61,6 +61,13 @@
>   struct hfp {
>   	struct hfp_slc_info info;
>   	DBusMessage *msg;
> +	bdaddr_t src;
> +	bdaddr_t dst;
> +};

What is src and what is dst? This is completely context dependent and 
confusing.  Also, we are already storing the device address on the modem 
object, why do we need this yet again?

> +
> +struct sock_bdaddr {
> +	bdaddr_t src;
> +	bdaddr_t dst;
>   };

I'd really like to avoid this structure.  At the very least change the 
name into something that conveys its purpose.  e.g. bdaddr_pair, or 
something.  The way it is now is just silly.

>
>   static GHashTable *modem_hash = NULL;
> @@ -68,6 +75,21 @@ static GHashTable *devices_proxies = NULL;
>   static GDBusClient *bluez = NULL;
>   static guint sco_watch = 0;
>
> +static gboolean modem_bacmp(gpointer key, gpointer value,
> +						gpointer user_data)
> +{
> +	const struct sock_bdaddr *bda = user_data;
> +	struct ofono_modem *modem = value;
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +	int ret;
> +
> +	ret = bt_bacmp(&bda->dst,&hfp->dst);
> +	if (ret != 0)
> +		return FALSE;
> +
> +	return bt_bacmp(&bda->src,&hfp->src) != 0 ? FALSE : TRUE;
> +}
> +
>   static void hfp_debug(const char *str, void *user_data)
>   {
>   	const char *prefix = user_data;
> @@ -275,14 +297,15 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   {
>   	struct hfp *hfp;
>   	struct ofono_modem *modem;
> +	struct sockaddr_rc src, dst;
> +	socklen_t alen;
>   	DBusMessageIter iter;
>   	GDBusProxy *proxy;
>   	DBusMessageIter entry;
> -	const char *device, *alias, *address;
> +	const char *device, *alias;
> +	char device_address[18];
>   	int fd, err;
>
> -	DBG("Profile handler NewConnection");
> -
>   	if (dbus_message_iter_init(msg,&entry) == FALSE)
>   		goto invalid;
>
> @@ -301,11 +324,6 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>
>   	dbus_message_iter_get_basic(&iter,&alias);
>
> -	if (g_dbus_proxy_get_property(proxy, "Address",&iter) == FALSE)
> -		goto invalid;
> -
> -	dbus_message_iter_get_basic(&iter,&address);
> -
>   	dbus_message_iter_next(&entry);
>   	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>   		goto invalid;
> @@ -314,7 +332,24 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   	if (fd<  0)
>   		goto invalid;
>
> -	modem = modem_register(device, address, alias);
> +	memset(&src, 0, sizeof(src));
> +	alen = sizeof(src);
> +	if (getsockname(fd, (struct sockaddr *)&src,&alen)<  0) {
> +		close(fd);
> +		goto invalid;
> +	}
> +
> +	memset(&dst, 0, sizeof(dst));
> +	alen = sizeof(dst);
> +	if (getpeername(fd, (struct sockaddr *)&dst,&alen)<  0) {
> +		close(fd);
> +		goto invalid;
> +	}
> +
> +	bt_ba2str(&dst.rc_bdaddr, device_address);
> +	DBG("Profile handler NewConnection: %s", device_address);
> +
> +	modem = modem_register(device, device_address, alias);
>   	if (modem == NULL) {
>   		close(fd);
>   		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
> @@ -329,6 +364,8 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   					"Not enough resources");
>
>   	hfp = ofono_modem_get_data(modem);
> +	bt_bacpy(&hfp->src,&src.rc_bdaddr);
> +	bt_bacpy(&hfp->dst,&dst.rc_bdaddr);
>   	hfp->msg = dbus_message_ref(msg);
>

Fair enough, but this chunk really belongs in a separate patch.

>   	return NULL;
> @@ -384,7 +421,9 @@ static const GDBusMethodTable profile_methods[] = {
>   static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>   							gpointer user_data)
>   {
> -	struct sockaddr_sco saddr;
> +	struct ofono_modem *modem;
> +	struct sockaddr_sco src, dst;
> +	struct sock_bdaddr bda;
>   	socklen_t alen;
>   	int sk, nsk;
>
> @@ -393,14 +432,29 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>
>   	sk = g_io_channel_unix_get_fd(io);
>
> -	memset(&saddr, 0, sizeof(saddr));
> -	alen = sizeof(saddr);
> +	memset(&dst, 0, sizeof(dst));
> +	alen = sizeof(dst);
>
> -	nsk = accept(sk, (struct sockaddr *)&saddr,&alen);
> +	nsk = accept(sk, (struct sockaddr *)&dst,&alen);

When reading this patch I go 'what the...'

>   	if (nsk<  0)
>   		return TRUE;
>
> -	/* TODO: Verify if the device has a modem */
> +	memset(&src, 0, sizeof(src));
> +	alen = sizeof(src);
> +	if (getsockname(nsk, (struct sockaddr *)&src,&alen)<  0) {
> +		close(nsk);
> +		return TRUE;
> +	}
> +
> +	bt_bacpy(&bda.src,&src.sco_bdaddr);
> +	bt_bacpy(&bda.dst,&dst.sco_bdaddr);
> +
> +	modem = g_hash_table_find(modem_hash, modem_bacmp,&bda);
> +	if (modem == NULL) {
> +		ofono_error("Rejecting SCO: SLC connection missing!");
> +		close(nsk);
> +		return TRUE;
> +	}
>
>   	return TRUE;
>   }

Regards,
-Denis

Re: [PATCH v1 7/8] hfp_hf_bluez5: Add rejecting SCO connection

[Claudio Takahasi]

[-- Attachment #1: Type: text/plain, Size: 6696 bytes --]

Hi Denis:

On Tue, Jan 29, 2013 at 12:27 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Claudio,
>
>
> On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
>>
>> This patch rejects the SCO incoming connection if there isn't a modem
>> (service level connection) associated with the address.
>
>
> Why service level connection is in parenthesis?

I thought this additional information could be useful.
No problem, I gonna remove it.

>
>
>> ---
>>   plugins/hfp_hf_bluez5.c | 82
>> ++++++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 68 insertions(+), 14 deletions(-)
>>
>> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
>> index 87ae5e3..871c720 100644
>> --- a/plugins/hfp_hf_bluez5.c
>> +++ b/plugins/hfp_hf_bluez5.c
>> @@ -61,6 +61,13 @@
>>   struct hfp {
>>         struct hfp_slc_info info;
>>         DBusMessage *msg;
>> +       bdaddr_t src;
>> +       bdaddr_t dst;
>> +};
>
>
> What is src and what is dst? This is completely context dependent and
> confusing.  Also, we are already storing the device address on the modem
> object, why do we need this yet again?

We use this standard in BlueZ for source and destination. It could be
also sba and dba.
Do you have another suggestion?

Indeed, we already have the address stored in the modem, but it is the
opposite thing of what Marcel is asking: avoid BT address stored using
string format.
If I use string to store the BT source address I will be blamed also,
I did this for the PATCH v0. Please define how you want to store this
data.

>
>
>> +
>> +struct sock_bdaddr {
>> +       bdaddr_t src;
>> +       bdaddr_t dst;
>>   };
>
>
> I'd really like to avoid this structure.  At the very least change the name
> into something that conveys its purpose.  e.g. bdaddr_pair, or something.
> The way it is now is just silly.

If we get the Device address from the modem string this struct will
not be necessary.

>
>>
>>   static GHashTable *modem_hash = NULL;
>> @@ -68,6 +75,21 @@ static GHashTable *devices_proxies = NULL;
>>   static GDBusClient *bluez = NULL;
>>   static guint sco_watch = 0;
>>
>> +static gboolean modem_bacmp(gpointer key, gpointer value,
>> +                                               gpointer user_data)
>> +{
>> +       const struct sock_bdaddr *bda = user_data;
>> +       struct ofono_modem *modem = value;
>> +       struct hfp *hfp = ofono_modem_get_data(modem);
>> +       int ret;
>> +
>> +       ret = bt_bacmp(&bda->dst,&hfp->dst);
>>
>> +       if (ret != 0)
>> +               return FALSE;
>> +
>> +       return bt_bacmp(&bda->src,&hfp->src) != 0 ? FALSE : TRUE;
>>
>> +}
>> +
>>   static void hfp_debug(const char *str, void *user_data)
>>   {
>>         const char *prefix = user_data;
>> @@ -275,14 +297,15 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>>   {
>>         struct hfp *hfp;
>>         struct ofono_modem *modem;
>> +       struct sockaddr_rc src, dst;
>> +       socklen_t alen;
>>         DBusMessageIter iter;
>>         GDBusProxy *proxy;
>>         DBusMessageIter entry;
>> -       const char *device, *alias, *address;
>> +       const char *device, *alias;
>> +       char device_address[18];
>>         int fd, err;
>>
>> -       DBG("Profile handler NewConnection");
>> -
>>         if (dbus_message_iter_init(msg,&entry) == FALSE)
>>
>>                 goto invalid;
>>
>> @@ -301,11 +324,6 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>>
>>         dbus_message_iter_get_basic(&iter,&alias);
>>
>> -       if (g_dbus_proxy_get_property(proxy, "Address",&iter) == FALSE)
>>
>> -               goto invalid;
>> -
>> -       dbus_message_iter_get_basic(&iter,&address);
>> -
>>         dbus_message_iter_next(&entry);
>>         if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>>                 goto invalid;
>> @@ -314,7 +332,24 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>>         if (fd<  0)
>>                 goto invalid;
>>
>> -       modem = modem_register(device, address, alias);
>> +       memset(&src, 0, sizeof(src));
>> +       alen = sizeof(src);
>> +       if (getsockname(fd, (struct sockaddr *)&src,&alen)<  0) {
>>
>> +               close(fd);
>> +               goto invalid;
>> +       }
>> +
>> +       memset(&dst, 0, sizeof(dst));
>> +       alen = sizeof(dst);
>> +       if (getpeername(fd, (struct sockaddr *)&dst,&alen)<  0) {
>>
>> +               close(fd);
>> +               goto invalid;
>> +       }
>> +
>> +       bt_ba2str(&dst.rc_bdaddr, device_address);
>> +       DBG("Profile handler NewConnection: %s", device_address);
>> +
>> +       modem = modem_register(device, device_address, alias);
>>         if (modem == NULL) {
>>                 close(fd);
>>                 return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
>> @@ -329,6 +364,8 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>>                                         "Not enough resources");
>>
>>         hfp = ofono_modem_get_data(modem);
>> +       bt_bacpy(&hfp->src,&src.rc_bdaddr);
>> +       bt_bacpy(&hfp->dst,&dst.rc_bdaddr);
>>         hfp->msg = dbus_message_ref(msg);
>>
>
> Fair enough, but this chunk really belongs in a separate patch.

IMO it belongs to this patch since it is initializing a value that
will be used to compare the addresses when the sco connection is
accepted.
But no problem, I can split it.

>
>
>>         return NULL;
>> @@ -384,7 +421,9 @@ static const GDBusMethodTable profile_methods[] = {
>>   static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>>                                                         gpointer
>> user_data)
>>   {
>> -       struct sockaddr_sco saddr;
>> +       struct ofono_modem *modem;
>> +       struct sockaddr_sco src, dst;
>> +       struct sock_bdaddr bda;
>>         socklen_t alen;
>>         int sk, nsk;
>>
>> @@ -393,14 +432,29 @@ static gboolean sco_accept(GIOChannel *io,
>> GIOCondition cond,
>>
>>         sk = g_io_channel_unix_get_fd(io);
>>
>> -       memset(&saddr, 0, sizeof(saddr));
>> -       alen = sizeof(saddr);
>> +       memset(&dst, 0, sizeof(dst));
>> +       alen = sizeof(dst);
>>
>> -       nsk = accept(sk, (struct sockaddr *)&saddr,&alen);
>> +       nsk = accept(sk, (struct sockaddr *)&dst,&alen);
>
>
> When reading this patch I go 'what the...'

I am renaming it to follow a standard: src for source and dst for destination.
But I can revert it, no problem.

Regards,
Claudio

Re: [PATCH v1 7/8] hfp_hf_bluez5: Add rejecting SCO connection

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6884 bytes --]

Hi Claudio,

>>> This patch rejects the SCO incoming connection if there isn't a modem
>>> (service level connection) associated with the address.
>>
>>
>> Why service level connection is in parenthesis?
>
> I thought this additional information could be useful.
> No problem, I gonna remove it.
>

Actually the service level connection is important, the modem part is 
not.  Hence why I'm lost why it is in parenthesis.

<snip>

>>> +       bdaddr_t src;
>>> +       bdaddr_t dst;
>>> +};
>>
>>
>> What is src and what is dst? This is completely context dependent and
>> confusing.  Also, we are already storing the device address on the modem
>> object, why do we need this yet again?
>
> We use this standard in BlueZ for source and destination. It could be
> also sba and dba.
> Do you have another suggestion?

Then BlueZ is just plain weird.  Local and Remote would be way better names.

>
> Indeed, we already have the address stored in the modem, but it is the
> opposite thing of what Marcel is asking: avoid BT address stored using
> string format.
> If I use string to store the BT source address I will be blamed also,
> I did this for the PATCH v0. Please define how you want to store this
> data.
>

Whenever we store duplicate information, a question will be raised.  I 
see no point in doing it twice.

>>
>>
>>> +
>>> +struct sock_bdaddr {
>>> +       bdaddr_t src;
>>> +       bdaddr_t dst;
>>>    };
>>
>>
>> I'd really like to avoid this structure.  At the very least change the name
>> into something that conveys its purpose.  e.g. bdaddr_pair, or something.
>> The way it is now is just silly.
>
> If we get the Device address from the modem string this struct will
> not be necessary.
>
>>
>>>
>>>    static GHashTable *modem_hash = NULL;
>>> @@ -68,6 +75,21 @@ static GHashTable *devices_proxies = NULL;
>>>    static GDBusClient *bluez = NULL;
>>>    static guint sco_watch = 0;
>>>
>>> +static gboolean modem_bacmp(gpointer key, gpointer value,
>>> +                                               gpointer user_data)
>>> +{
>>> +       const struct sock_bdaddr *bda = user_data;
>>> +       struct ofono_modem *modem = value;
>>> +       struct hfp *hfp = ofono_modem_get_data(modem);
>>> +       int ret;
>>> +
>>> +       ret = bt_bacmp(&bda->dst,&hfp->dst);
>>>
>>> +       if (ret != 0)
>>> +               return FALSE;
>>> +
>>> +       return bt_bacmp(&bda->src,&hfp->src) != 0 ? FALSE : TRUE;
>>>
>>> +}
>>> +
>>>    static void hfp_debug(const char *str, void *user_data)
>>>    {
>>>          const char *prefix = user_data;
>>> @@ -275,14 +297,15 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>    {
>>>          struct hfp *hfp;
>>>          struct ofono_modem *modem;
>>> +       struct sockaddr_rc src, dst;
>>> +       socklen_t alen;
>>>          DBusMessageIter iter;
>>>          GDBusProxy *proxy;
>>>          DBusMessageIter entry;
>>> -       const char *device, *alias, *address;
>>> +       const char *device, *alias;
>>> +       char device_address[18];
>>>          int fd, err;
>>>
>>> -       DBG("Profile handler NewConnection");
>>> -
>>>          if (dbus_message_iter_init(msg,&entry) == FALSE)
>>>
>>>                  goto invalid;
>>>
>>> @@ -301,11 +324,6 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>
>>>          dbus_message_iter_get_basic(&iter,&alias);
>>>
>>> -       if (g_dbus_proxy_get_property(proxy, "Address",&iter) == FALSE)
>>>
>>> -               goto invalid;
>>> -
>>> -       dbus_message_iter_get_basic(&iter,&address);
>>> -
>>>          dbus_message_iter_next(&entry);
>>>          if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>>>                  goto invalid;
>>> @@ -314,7 +332,24 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>          if (fd<   0)
>>>                  goto invalid;
>>>
>>> -       modem = modem_register(device, address, alias);
>>> +       memset(&src, 0, sizeof(src));
>>> +       alen = sizeof(src);
>>> +       if (getsockname(fd, (struct sockaddr *)&src,&alen)<   0) {
>>>
>>> +               close(fd);
>>> +               goto invalid;
>>> +       }
>>> +
>>> +       memset(&dst, 0, sizeof(dst));
>>> +       alen = sizeof(dst);
>>> +       if (getpeername(fd, (struct sockaddr *)&dst,&alen)<   0) {
>>>
>>> +               close(fd);
>>> +               goto invalid;
>>> +       }
>>> +
>>> +       bt_ba2str(&dst.rc_bdaddr, device_address);
>>> +       DBG("Profile handler NewConnection: %s", device_address);
>>> +
>>> +       modem = modem_register(device, device_address, alias);
>>>          if (modem == NULL) {
>>>                  close(fd);
>>>                  return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
>>> @@ -329,6 +364,8 @@ static DBusMessage
>>> *profile_new_connection(DBusConnection *conn,
>>>                                          "Not enough resources");
>>>
>>>          hfp = ofono_modem_get_data(modem);
>>> +       bt_bacpy(&hfp->src,&src.rc_bdaddr);
>>> +       bt_bacpy(&hfp->dst,&dst.rc_bdaddr);
>>>          hfp->msg = dbus_message_ref(msg);
>>>
>>
>> Fair enough, but this chunk really belongs in a separate patch.
>
> IMO it belongs to this patch since it is initializing a value that
> will be used to compare the addresses when the sco connection is
> accepted.
> But no problem, I can split it.
>

I'd like to see the profile_new_connection changes as a separate patch, 
this part can be there as well, not a biggie.

>>
>>
>>>          return NULL;
>>> @@ -384,7 +421,9 @@ static const GDBusMethodTable profile_methods[] = {
>>>    static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>>>                                                          gpointer
>>> user_data)
>>>    {
>>> -       struct sockaddr_sco saddr;
>>> +       struct ofono_modem *modem;
>>> +       struct sockaddr_sco src, dst;
>>> +       struct sock_bdaddr bda;
>>>          socklen_t alen;
>>>          int sk, nsk;
>>>
>>> @@ -393,14 +432,29 @@ static gboolean sco_accept(GIOChannel *io,
>>> GIOCondition cond,
>>>
>>>          sk = g_io_channel_unix_get_fd(io);
>>>
>>> -       memset(&saddr, 0, sizeof(saddr));
>>> -       alen = sizeof(saddr);
>>> +       memset(&dst, 0, sizeof(dst));
>>> +       alen = sizeof(dst);
>>>
>>> -       nsk = accept(sk, (struct sockaddr *)&saddr,&alen);
>>> +       nsk = accept(sk, (struct sockaddr *)&dst,&alen);
>>
>>
>> When reading this patch I go 'what the...'
>
> I am renaming it to follow a standard: src for source and dst for destination.
> But I can revert it, no problem.
>

This is a dual-initiator profile, src and dst are really bad names for this.

Regards,
-Denis