Re: [PATCH 2/3] handsfree-audio: Add support for initiating SCO connections

[Vinicius Costa Gomes <vinicius.gomes@openbossa.org>]

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

Hi Denis,

On 22:38 Tue 19 Mar, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 03/19/2013 07:10 PM, Vinicius Costa Gomes wrote:
> >When calling the card's .Connect() method, we should be able to
> >establish a SCO connection.
> >
> >Right now, we only have support for establishing the SCO connection
> >directly, this is what is expected from HFP 1.5 HF/AG devices.
> >---
> >  src/handsfree-audio.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 93 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> >index c7fa2fb..9eecb8e 100644
> >--- a/src/handsfree-audio.c
> >+++ b/src/handsfree-audio.c
> >@@ -53,6 +53,8 @@ struct ofono_handsfree_card {
> >  	char *remote;
> >  	char *local;
> >  	char *path;
> >+	DBusMessage *msg;
> >+	guint sco_watch;
> >  	const struct ofono_handsfree_card_driver *driver;
> >  	void *driver_data;
> >  };
> >@@ -235,10 +237,100 @@ static DBusMessage *card_get_properties(DBusConnection *conn,
> >  	return reply;
> >  }
> >
> >+static int card_connect_sco(struct ofono_handsfree_card *card)
> >+{
> >+	struct sockaddr_sco addr;
> >+	int sk, ret;
> >+
> >+	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
> >+								BTPROTO_SCO);
> >+	if (sk<  0)
> >+		return -1;
> >+
> >+	/* Bind to local address */
> >+	memset(&addr, 0, sizeof(addr));
> >+	addr.sco_family = AF_BLUETOOTH;
> >+	bt_str2ba(card->local,&addr.sco_bdaddr);
> >+
> >+	if (bind(sk, (struct sockaddr *)&addr, sizeof(addr))<  0) {
> >+		close(sk);
> >+		return -1;
> >+	}
> >+
> >+	/* Connect to remote device */
> >+	memset(&addr, 0, sizeof(addr));
> >+	addr.sco_family = AF_BLUETOOTH;
> >+	bt_str2ba(card->remote,&addr.sco_bdaddr);
> >+
> >+	ret = connect(sk, (struct sockaddr *)&addr, sizeof(addr));
> >+	if (ret<  0&&  !(errno == EAGAIN || errno == EINPROGRESS)) {
> 
> Why do we use EAGAIN here?  I thought only EINPROGRESS can be
> returned from a non-blocking connect.  Or are Bluetooth semantics
> different?

Just checked, you are right: only EINPROGRSS will be returned in this case.
Will fix.

> 
> >+		close(sk);
> >+		return -1;
> >+	}
> >+
> >+	return sk;
> >+}
> >+
> >+static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
> >+							gpointer user_data)
> >+
> >+{
> >+	struct ofono_handsfree_card *card = user_data;
> >+	DBusMessage *reply;
> >+	int sk;
> >+
> >+	if (cond&  (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
> >+		reply = __ofono_error_failed(card->msg);
> >+		goto done;
> >+	}
> >+
> >+	sk = g_io_channel_unix_get_fd(io);
> >+
> >+	close(sk);
> >+
> >+	reply = dbus_message_new_method_return(card->msg);
> >+
> 
> We probably should be paranoid and check whether we still have the agent.

For the method return message of the 'Connect()' call, I can't see any gains of
this check, but for sending the 'NewConnection()' message I can (and that is
what the next patch does). Could be that I am missing something.

One thing I just noticed that is missing is that there's no check that only 
the agent may call 'Connect()'. 

> 
> >+done:
> >+	g_dbus_send_message(ofono_dbus_get_connection(), reply);
> >+
> >+	return FALSE;
> >+}
> >+
> >+static void sco_watch_destroy(gpointer user_data)
> >+{
> >+	struct ofono_handsfree_card *card = user_data;
> >+
> >+	card->sco_watch = 0;
> >+	dbus_message_unref(card->msg);
> >+}
> >+
> >  static DBusMessage *card_connect(DBusConnection *conn,
> >  						DBusMessage *msg, void *data)
> >  {
> >-	return __ofono_error_not_implemented(msg);
> >+	struct ofono_handsfree_card *card = data;
> >+	GIOChannel *io;
> >+	int sk;
> >+
> >+	if (agent == NULL)
> >+		return __ofono_error_not_available(msg);
> >+
> >+	if (card->msg)
> >+		return __ofono_error_busy(msg);
> >+
> >+	sk = card_connect_sco(card);
> 
> I presume this fails if we already have a SCO link?

It will, but I will replace the check above (card->msg) with a check for
sco_watch, to make it clearer.

> 
> >+	if (sk<  0)
> >+		return __ofono_error_failed(msg);
> >+
> >+	io = g_io_channel_unix_new(sk);
> >+	card->sco_watch = g_io_add_watch_full(io, G_PRIORITY_DEFAULT,
> >+				G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> >+				sco_connect_cb, card, sco_watch_destroy);
> >+
> >+	g_io_channel_unref(io);
> >+
> >+	card->msg = dbus_message_ref(msg);
> >+
> >+	return NULL;
> >  }
> >
> >  static const GDBusMethodTable card_methods[] = {
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius