From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============5074363296408119945=="
MIME-Version: 1.0
From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
Subject: Re: [PATCH 2/3] handsfree-audio: Add support for initiating SCO
 connections
Date: Wed, 20 Mar 2013 12:15:01 -0300
Message-ID: <20130320151501.GB6190@samus>
In-Reply-To: <51492F33.2080602@gmail.com>
List-Id: <ofono.ofono.org>
To: ofono@ofono.org

--===============5074363296408119945==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

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(DBusConne=
ction *conn,
> >  	return reply;
> >  }
> >
> >+static int card_connect_sco(struct ofono_handsfree_card *card)
> >+{
> >+	struct sockaddr_sco addr;
> >+	int sk, ret;
> >+
> >+	sk =3D 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 =3D 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 =3D AF_BLUETOOTH;
> >+	bt_str2ba(card->remote,&addr.sco_bdaddr);
> >+
> >+	ret =3D connect(sk, (struct sockaddr *)&addr, sizeof(addr));
> >+	if (ret<  0&&  !(errno =3D=3D EAGAIN || errno =3D=3D 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 =3D user_data;
> >+	DBusMessage *reply;
> >+	int sk;
> >+
> >+	if (cond&  (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
> >+		reply =3D __ofono_error_failed(card->msg);
> >+		goto done;
> >+	}
> >+
> >+	sk =3D g_io_channel_unix_get_fd(io);
> >+
> >+	close(sk);
> >+
> >+	reply =3D 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 gain=
s 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 =3D user_data;
> >+
> >+	card->sco_watch =3D 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 =3D data;
> >+	GIOChannel *io;
> >+	int sk;
> >+
> >+	if (agent =3D=3D NULL)
> >+		return __ofono_error_not_available(msg);
> >+
> >+	if (card->msg)
> >+		return __ofono_error_busy(msg);
> >+
> >+	sk =3D 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 =3D g_io_channel_unix_new(sk);
> >+	card->sco_watch =3D 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 =3D dbus_message_ref(msg);
> >+
> >+	return NULL;
> >  }
> >
> >  static const GDBusMethodTable card_methods[] =3D {
> =

> Regards,
> -Denis


Cheers,
-- =

Vinicius

--===============5074363296408119945==--