[PATCH 1/2] serial: add Serial.ConnectFD()

[PATCH 1/2] serial: add Serial.ConnectFD()

[Gustavo F. Padovan]

From: "Gustavo F. Padovan" <padovan@profusion.mobi>

It's similar to Serial.Connect() but returns the actual RFCOMM file
descriptor instead of creating a device in /dev
---
 doc/serial-api.txt |   16 ++++++++++++++++
 serial/port.c      |   36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/doc/serial-api.txt b/doc/serial-api.txt
index 5f9bd5f..98b0ad4 100644
--- a/doc/serial-api.txt
+++ b/doc/serial-api.txt
@@ -26,6 +26,22 @@ Methods		string Connect(string pattern)
 					 org.bluez.Error.ConnectionAttemptFailed
 					 org.bluez.Error.NotSupported
 
+Methods		fd ConnectFD(string pattern) [experimental]
+
+			Connects to a specific RFCOMM based service on a
+			remote device and returns a file descriptor to talk
+			 with this device.
+
+			Possible patterns: UUID 128 bit as string
+					   Profile short names, e.g: spp, dun
+					   RFCOMM channel as string, 1-30
+
+			Possible errors: org.bluez.Error.InvalidArguments
+					 org.bluez.Error.InProgress
+					 org.bluez.Error.ConnectionAttemptFailed
+					 org.bluez.Error.NotSupported
+
+
 		void Disconnect(string device)
 
 			Disconnect a RFCOMM TTY device that has been
diff --git a/serial/port.c b/serial/port.c
index d011084..e359716 100644
--- a/serial/port.c
+++ b/serial/port.c
@@ -63,6 +63,10 @@
 #define MAX_OPEN_TRIES		5
 #define OPEN_WAIT		300	/* ms. udev node creation retry wait */
 
+#ifndef DBUS_TYPE_UNIX_FD
+#define DBUS_TYPE_UNIX_FD -1
+#endif
+
 struct serial_device {
 	DBusConnection	*conn;		/* for name listener handling */
 	bdaddr_t	src;		/* Source (local) address */
@@ -80,6 +84,7 @@ struct serial_port {
 	int		fd;		/* Opened file descriptor */
 	GIOChannel	*io;		/* BtIO channel */
 	guint		listener_id;
+	gboolean	fd_passing;
 	struct serial_device *device;
 };
 
@@ -329,6 +334,14 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *conn_err,
 	port->io = NULL;
 
 	sk = g_io_channel_unix_get_fd(chan);
+	if (port->fd_passing) {
+		reply = g_dbus_create_reply(port->msg,
+				DBUS_TYPE_UNIX_FD, &sk,
+				DBUS_TYPE_INVALID);
+		g_dbus_send_message(device->conn, reply);
+		return;
+	}
+
 	port->id = ioctl(sk, RFCOMMCREATEDEV, &req);
 	if (port->id < 0) {
 		int err = -errno;
@@ -468,8 +481,8 @@ static struct serial_port *create_port(struct serial_device *device,
 	return port;
 }
 
-static DBusMessage *port_connect(DBusConnection *conn,
-					DBusMessage *msg, void *user_data)
+static DBusMessage *_port_connect(DBusConnection *conn, DBusMessage *msg,
+					void *user_data, gboolean fd)
 {
 	struct serial_device *device = user_data;
 	struct serial_port *port;
@@ -501,6 +514,9 @@ static DBusMessage *port_connect(DBusConnection *conn,
 						NULL);
 	port->msg = dbus_message_ref(msg);
 
+	if (fd)
+		port->fd_passing = TRUE;
+
 	err = connect_port(port);
 	if (err < 0) {
 		error("%s", strerror(-err));
@@ -513,6 +529,21 @@ static DBusMessage *port_connect(DBusConnection *conn,
 	return NULL;
 }
 
+static DBusMessage *port_connect(DBusConnection *conn,
+					DBusMessage *msg, void *user_data)
+{
+	return _port_connect(conn, msg, user_data, FALSE);
+}
+
+static DBusMessage *port_connect_fd(DBusConnection *conn,
+					DBusMessage *msg, void *user_data)
+{
+	if (DBUS_TYPE_UNIX_FD < 0)
+		return btd_error_not_supported(msg);
+
+	return _port_connect(conn, msg, user_data, TRUE);
+}
+
 static DBusMessage *port_disconnect(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
@@ -546,6 +577,7 @@ static DBusMessage *port_disconnect(DBusConnection *conn,
 
 static GDBusMethodTable port_methods[] = {
 	{ "Connect",    "s", "s", port_connect, G_DBUS_METHOD_FLAG_ASYNC },
+	{ "ConnectFD",    "s", "h", port_connect_fd, G_DBUS_METHOD_FLAG_ASYNC },
 	{ "Disconnect", "s", "",  port_disconnect },
 	{ }
 };
-- 
1.7.6

[PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo F. Padovan]

From: "Gustavo F. Padovan" <padovan@profusion.mobi>

Disconnect can also be called for connections created with ConnectFD
---
 doc/serial-api.txt |    3 +++
 serial/port.c      |   14 ++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/doc/serial-api.txt b/doc/serial-api.txt
index 98b0ad4..09a4c0d 100644
--- a/doc/serial-api.txt
+++ b/doc/serial-api.txt
@@ -53,5 +53,8 @@ Methods		fd ConnectFD(string pattern) [experimental]
 			In that case one of patterns of the Connect method should
 			be suplied instead of the TTY device.
 
+			Connection created with Serial.ConnectFD only accept
+			as parameter the same parameters ConnectFD accepts.
+
 			Possible errors: org.bluez.Error.InvalidArguments
 					 org.bluez.Error.DoesNotExist
diff --git a/serial/port.c b/serial/port.c
index e359716..0c3d171 100644
--- a/serial/port.c
+++ b/serial/port.c
@@ -148,8 +148,11 @@ static int port_release(struct serial_port *port)
 	int rfcomm_ctl;
 	int err = 0;
 
-	if (port->id < 0) {
+	if (port->id < 0 || port->fd_passing == TRUE) {
 		if (port->io) {
+			int sk = g_io_channel_unix_get_fd(port->io);
+			shutdown(sk, SHUT_RDWR);
+
 			g_io_channel_shutdown(port->io, TRUE, NULL);
 			g_io_channel_unref(port->io);
 			port->io = NULL;
@@ -157,6 +160,9 @@ static int port_release(struct serial_port *port)
 			bt_cancel_discovery(&port->device->src,
 						&port->device->dst);
 
+		port->fd_passing = FALSE;
+		port->id = 1;
+
 		return 0;
 	}
 
@@ -330,9 +336,6 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *conn_err,
 	bacpy(&req.dst, &device->dst);
 	req.channel = port->channel;
 
-	g_io_channel_unref(port->io);
-	port->io = NULL;
-
 	sk = g_io_channel_unix_get_fd(chan);
 	if (port->fd_passing) {
 		reply = g_dbus_create_reply(port->msg,
@@ -342,6 +345,9 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *conn_err,
 		return;
 	}
 
+	g_io_channel_unref(port->io);
+	port->io = NULL;
+
 	port->id = ioctl(sk, RFCOMMCREATEDEV, &req);
 	if (port->id < 0) {
 		int err = -errno;
-- 
1.7.6

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Marcel Holtmann]

Hi Gustavo,

> Disconnect can also be called for connections created with ConnectFD
> ---
>  doc/serial-api.txt |    3 +++
>  serial/port.c      |   14 ++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/serial-api.txt b/doc/serial-api.txt
> index 98b0ad4..09a4c0d 100644
> --- a/doc/serial-api.txt
> +++ b/doc/serial-api.txt
> @@ -53,5 +53,8 @@ Methods		fd ConnectFD(string pattern) [experimental]
>  			In that case one of patterns of the Connect method should
>  			be suplied instead of the TTY device.
>  
> +			Connection created with Serial.ConnectFD only accept
> +			as parameter the same parameters ConnectFD accepts.
> +
>  			Possible errors: org.bluez.Error.InvalidArguments
>  					 org.bluez.Error.DoesNotExist

I really fail to see how this is all going to work. We already had this
problem with the PrivateNetwork API in ConnMan. We can not use the fd as
a unique reference.

Having the file descriptor instead of a TTY is a great idea, but
unfortunately it is not all this simple.

Regards

Marcel

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2011-08-23 09:18:13 -0700]:

> Hi Gustavo,
> 
> > Disconnect can also be called for connections created with ConnectFD
> > ---
> >  doc/serial-api.txt |    3 +++
> >  serial/port.c      |   14 ++++++++++----
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/doc/serial-api.txt b/doc/serial-api.txt
> > index 98b0ad4..09a4c0d 100644
> > --- a/doc/serial-api.txt
> > +++ b/doc/serial-api.txt
> > @@ -53,5 +53,8 @@ Methods		fd ConnectFD(string pattern) [experimental]
> >  			In that case one of patterns of the Connect method should
> >  			be suplied instead of the TTY device.
> >  
> > +			Connection created with Serial.ConnectFD only accept
> > +			as parameter the same parameters ConnectFD accepts.
> > +
> >  			Possible errors: org.bluez.Error.InvalidArguments
> >  					 org.bluez.Error.DoesNotExist
> 
> I really fail to see how this is all going to work. We already had this
> problem with the PrivateNetwork API in ConnMan. We can not use the fd as
> a unique reference.

I'm considering that UUID(or channel name, or a friendly name like "sap") is a
second reference. You do:

Serial.ConnectFD("sap")

and then

Serial.Disconnect("sap")

	Gustavo

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Marcel Holtmann]

Hi Gustavo,

> > > Disconnect can also be called for connections created with ConnectFD
> > > ---
> > >  doc/serial-api.txt |    3 +++
> > >  serial/port.c      |   14 ++++++++++----
> > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/doc/serial-api.txt b/doc/serial-api.txt
> > > index 98b0ad4..09a4c0d 100644
> > > --- a/doc/serial-api.txt
> > > +++ b/doc/serial-api.txt
> > > @@ -53,5 +53,8 @@ Methods		fd ConnectFD(string pattern) [experimental]
> > >  			In that case one of patterns of the Connect method should
> > >  			be suplied instead of the TTY device.
> > >  
> > > +			Connection created with Serial.ConnectFD only accept
> > > +			as parameter the same parameters ConnectFD accepts.
> > > +
> > >  			Possible errors: org.bluez.Error.InvalidArguments
> > >  					 org.bluez.Error.DoesNotExist
> > 
> > I really fail to see how this is all going to work. We already had this
> > problem with the PrivateNetwork API in ConnMan. We can not use the fd as
> > a unique reference.
> 
> I'm considering that UUID(or channel name, or a friendly name like "sap") is a
> second reference. You do:
> 
> Serial.ConnectFD("sap")
> 
> and then
> 
> Serial.Disconnect("sap")

does not really work nicely. Once you have two profiles or two of trhe
same UUIDs this will fail.

Regards

Marcel

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2011-08-23 09:30:04 -0700]:

> Hi Gustavo,
> 
> > > > Disconnect can also be called for connections created with ConnectFD
> > > > ---
> > > >  doc/serial-api.txt |    3 +++
> > > >  serial/port.c      |   14 ++++++++++----
> > > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/doc/serial-api.txt b/doc/serial-api.txt
> > > > index 98b0ad4..09a4c0d 100644
> > > > --- a/doc/serial-api.txt
> > > > +++ b/doc/serial-api.txt
> > > > @@ -53,5 +53,8 @@ Methods		fd ConnectFD(string pattern) [experimental]
> > > >  			In that case one of patterns of the Connect method should
> > > >  			be suplied instead of the TTY device.
> > > >  
> > > > +			Connection created with Serial.ConnectFD only accept
> > > > +			as parameter the same parameters ConnectFD accepts.
> > > > +
> > > >  			Possible errors: org.bluez.Error.InvalidArguments
> > > >  					 org.bluez.Error.DoesNotExist
> > > 
> > > I really fail to see how this is all going to work. We already had this
> > > problem with the PrivateNetwork API in ConnMan. We can not use the fd as
> > > a unique reference.
> > 
> > I'm considering that UUID(or channel name, or a friendly name like "sap") is a
> > second reference. You do:
> > 
> > Serial.ConnectFD("sap")
> > 
> > and then
> > 
> > Serial.Disconnect("sap")
> 
> does not really work nicely. Once you have two profiles or two of trhe
> same UUIDs this will fail.

This already fails today. Our code doesn't allow us to call twice the Connect
method, so we can't have two of the same UUID connected.

But what do you suggest? I think that create a unique ID for each connected
fd can fix this. Serial code would be responsible to assign the UID.

	Gustavo

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Luiz Augusto von Dentz]

Hi Marcel, Gustavo,

On Tue, Aug 23, 2011 at 7:45 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
>
> This already fails today. Our code doesn't allow us to call twice the Connect
> method, so we can't have two of the same UUID connected.

Well with RFCOMM you can't really connect multiple times to the same
channel/UUID and if we return the same fd clients will probably have
conflicts.

> But what do you suggest? I think that create a unique ID for each connected
> fd can fix this. Serial code would be responsible to assign the UID.

We can play with pipe/splice/tee, but I think that nobody will really
gonna need multiple clients per connection.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

Hi Luiz, Marcel,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-23 21:39:29 +0300]:

> Hi Marcel, Gustavo,
> 
> On Tue, Aug 23, 2011 at 7:45 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> >
> > This already fails today. Our code doesn't allow us to call twice the Connect
> > method, so we can't have two of the same UUID connected.
> 
> Well with RFCOMM you can't really connect multiple times to the same
> channel/UUID and if we return the same fd clients will probably have
> conflicts.

That said, I think this patch is ok, as it relies in UUID as a unique reference.

	Gustavo

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Marcel Holtmann]

Hi Luiz,

> > This already fails today. Our code doesn't allow us to call twice the Connect
> > method, so we can't have two of the same UUID connected.
> 
> Well with RFCOMM you can't really connect multiple times to the same
> channel/UUID and if we return the same fd clients will probably have
> conflicts.

you can not connect the same channel twice (except in the other
direction), that is true, but you can connect to a different channel
with a different UUID. That is the reason why we also allow connection
by handles. Or at least we should.

So even if we would make this limitation of 1 connection per UUID, the
API is a fully asymmetric then. You are suppose to disconnect with the
result of the connect call. I do not like that at all. It is bad API
design and we are trying to squeeze this in the wrong way.

Regards

Marcel

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, Aug 23, 2011 at 10:51 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> > This already fails today. Our code doesn't allow us to call twice the Connect
>> > method, so we can't have two of the same UUID connected.
>>
>> Well with RFCOMM you can't really connect multiple times to the same
>> channel/UUID and if we return the same fd clients will probably have
>> conflicts.
>
> you can not connect the same channel twice (except in the other
> direction), that is true, but you can connect to a different channel
> with a different UUID. That is the reason why we also allow connection
> by handles. Or at least we should.

You mean record handle? Currently we support connecting by UUID,
friendly name or channel.

> So even if we would make this limitation of 1 connection per UUID, the
> API is a fully asymmetric then. You are suppose to disconnect with the
> result of the connect call. I do not like that at all. It is bad API
> design and we are trying to squeeze this in the wrong way.

Currently we support disconnecting by UUID, friendly name, channel and
dev node. As you mentioned it doesn't really work for fd since it is
only unique per process, in the other hand the parameter is a pattern.

Perhaps what we should be doing is to return a object path in
Serial.Connect e.g. [variable
prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
just get it as parameter, the drawback is that this does not return
the tty/fd immediately so we need another round-trip or return
multiple values to Connect.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

Hi Luiz,

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-24 13:43:47 +0300]:

> Hi Marcel,
> 
> On Tue, Aug 23, 2011 at 10:51 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Luiz,
> >
> >> > This already fails today. Our code doesn't allow us to call twice the Connect
> >> > method, so we can't have two of the same UUID connected.
> >>
> >> Well with RFCOMM you can't really connect multiple times to the same
> >> channel/UUID and if we return the same fd clients will probably have
> >> conflicts.
> >
> > you can not connect the same channel twice (except in the other
> > direction), that is true, but you can connect to a different channel
> > with a different UUID. That is the reason why we also allow connection
> > by handles. Or at least we should.
> 
> You mean record handle? Currently we support connecting by UUID,
> friendly name or channel.
> 
> > So even if we would make this limitation of 1 connection per UUID, the
> > API is a fully asymmetric then. You are suppose to disconnect with the
> > result of the connect call. I do not like that at all. It is bad API
> > design and we are trying to squeeze this in the wrong way.
> 
> Currently we support disconnecting by UUID, friendly name, channel and
> dev node. As you mentioned it doesn't really work for fd since it is
> only unique per process, in the other hand the parameter is a pattern.
> 
> Perhaps what we should be doing is to return a object path in
> Serial.Connect e.g. [variable
> prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
> just get it as parameter, the drawback is that this does not return
> the tty/fd immediately so we need another round-trip or return
> multiple values to Connect.

Then we would need some sort of agent. Don't you think that add a agent and
one more round trip here is too much?
I would just add a handle to the Connect reply besides the fd, Disconnect
then use this handle as parameter to Disconnect.
If we want to disconnect before Connect replies we use the pattern as
parameter like we with the current API. Is that bad?

	Gustavo

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Luiz Augusto von Dentz]

Hi Gustavo,

On Thu, Aug 25, 2011 at 5:55 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-24 13:43:47 +0300]:
>
>> Hi Marcel,
>>
>> On Tue, Aug 23, 2011 at 10:51 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> > Hi Luiz,
>> >
>> >> > This already fails today. Our code doesn't allow us to call twice the Connect
>> >> > method, so we can't have two of the same UUID connected.
>> >>
>> >> Well with RFCOMM you can't really connect multiple times to the same
>> >> channel/UUID and if we return the same fd clients will probably have
>> >> conflicts.
>> >
>> > you can not connect the same channel twice (except in the other
>> > direction), that is true, but you can connect to a different channel
>> > with a different UUID. That is the reason why we also allow connection
>> > by handles. Or at least we should.
>>
>> You mean record handle? Currently we support connecting by UUID,
>> friendly name or channel.
>>
>> > So even if we would make this limitation of 1 connection per UUID, the
>> > API is a fully asymmetric then. You are suppose to disconnect with the
>> > result of the connect call. I do not like that at all. It is bad API
>> > design and we are trying to squeeze this in the wrong way.
>>
>> Currently we support disconnecting by UUID, friendly name, channel and
>> dev node. As you mentioned it doesn't really work for fd since it is
>> only unique per process, in the other hand the parameter is a pattern.
>>
>> Perhaps what we should be doing is to return a object path in
>> Serial.Connect e.g. [variable
>> prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
>> just get it as parameter, the drawback is that this does not return
>> the tty/fd immediately so we need another round-trip or return
>> multiple values to Connect.
>
> Then we would need some sort of agent. Don't you think that add a agent and
> one more round trip here is too much?

I wouldn't consider it an agent since the client doesn't have to
implement the object returned by ConnectFD, bluetoothd does. The the
extra round trips can be a problem though.

> I would just add a handle to the Connect reply besides the fd, Disconnect
> then use this handle as parameter to Disconnect.
> If we want to disconnect before Connect replies we use the pattern as
> parameter like we with the current API. Is that bad?

Well that doesn't change much my proposal does it, I mean returning an
object or a handle is kind of the same idea and has almost the same
problems, actually with handle is worse because it doesn't return
anything meaningful so in that case we must have multiple values in
the return e.g handle + fd.


-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-26 10:22:42 +0300]:

> Hi Gustavo,
> 
> On Thu, Aug 25, 2011 at 5:55 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> > Hi Luiz,
> >
> > * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-24 13:43:47 +0300]:
> >
> >> Hi Marcel,
> >>
> >> On Tue, Aug 23, 2011 at 10:51 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> >> > Hi Luiz,
> >> >
> >> >> > This already fails today. Our code doesn't allow us to call twice the Connect
> >> >> > method, so we can't have two of the same UUID connected.
> >> >>
> >> >> Well with RFCOMM you can't really connect multiple times to the same
> >> >> channel/UUID and if we return the same fd clients will probably have
> >> >> conflicts.
> >> >
> >> > you can not connect the same channel twice (except in the other
> >> > direction), that is true, but you can connect to a different channel
> >> > with a different UUID. That is the reason why we also allow connection
> >> > by handles. Or at least we should.
> >>
> >> You mean record handle? Currently we support connecting by UUID,
> >> friendly name or channel.
> >>
> >> > So even if we would make this limitation of 1 connection per UUID, the
> >> > API is a fully asymmetric then. You are suppose to disconnect with the
> >> > result of the connect call. I do not like that at all. It is bad API
> >> > design and we are trying to squeeze this in the wrong way.
> >>
> >> Currently we support disconnecting by UUID, friendly name, channel and
> >> dev node. As you mentioned it doesn't really work for fd since it is
> >> only unique per process, in the other hand the parameter is a pattern.
> >>
> >> Perhaps what we should be doing is to return a object path in
> >> Serial.Connect e.g. [variable
> >> prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
> >> just get it as parameter, the drawback is that this does not return
> >> the tty/fd immediately so we need another round-trip or return
> >> multiple values to Connect.
> >
> > Then we would need some sort of agent. Don't you think that add a agent and
> > one more round trip here is too much?
> 
> I wouldn't consider it an agent since the client doesn't have to
> implement the object returned by ConnectFD, bluetoothd does. The the
> extra round trips can be a problem though.

So how do wanna to implement this extra round trip? I thought it would a
NewConnection-like method to be called in the agent interface. That would be
my idea, but I think it's too much to simple Serial Connections process.

> 
> > I would just add a handle to the Connect reply besides the fd, Disconnect
> > then use this handle as parameter to Disconnect.
> > If we want to disconnect before Connect replies we use the pattern as
> > parameter like we with the current API. Is that bad?
> 
> Well that doesn't change much my proposal does it, I mean returning an
> object or a handle is kind of the same idea and has almost the same
> problems, actually with handle is worse because it doesn't return
> anything meaningful so in that case we must have multiple values in
> the return e.g handle + fd.

Yeah, you right. You proposal works for this case.

	Gustavo

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-24 13:43:47 +0300]:

> Hi Marcel,
> 
> On Tue, Aug 23, 2011 at 10:51 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Luiz,
> >
> >> > This already fails today. Our code doesn't allow us to call twice the Connect
> >> > method, so we can't have two of the same UUID connected.
> >>
> >> Well with RFCOMM you can't really connect multiple times to the same
> >> channel/UUID and if we return the same fd clients will probably have
> >> conflicts.
> >
> > you can not connect the same channel twice (except in the other
> > direction), that is true, but you can connect to a different channel
> > with a different UUID. That is the reason why we also allow connection
> > by handles. Or at least we should.
> 
> You mean record handle? Currently we support connecting by UUID,
> friendly name or channel.
> 
> > So even if we would make this limitation of 1 connection per UUID, the
> > API is a fully asymmetric then. You are suppose to disconnect with the
> > result of the connect call. I do not like that at all. It is bad API
> > design and we are trying to squeeze this in the wrong way.
> 
> Currently we support disconnecting by UUID, friendly name, channel and
> dev node. As you mentioned it doesn't really work for fd since it is
> only unique per process, in the other hand the parameter is a pattern.
> 
> Perhaps what we should be doing is to return a object path in
> Serial.Connect e.g. [variable
> prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
> just get it as parameter, the drawback is that this does not return
> the tty/fd immediately so we need another round-trip or return
> multiple values to Connect.

I've been discussing this with Johan and we came to two possible solutions.
The first one is the similar as this one, with three functions: 

	handle, fd ConnectFD("pattern")

	ConnectCancel(handle)

	Disconnect(handle)

And other with only ConnectFD() (and maybe ConnectCancel method). Disconnect
would be handled by the client with shutdown().

I'm tempted to go with this last approach, it's simpler and easier. What do
you think?

	Gustavo

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Luiz Augusto von Dentz]

Hi Gustavo,

On Tue, Sep 6, 2011 at 8:16 AM, Gustavo Padovan <padovan@profusion.mobi> wr=
ote:
> * Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-08-24 13:43:47 +030=
0]:
>
>> Hi Marcel,
>>
>> On Tue, Aug 23, 2011 at 10:51 PM, Marcel Holtmann <marcel@holtmann.org> =
wrote:
>> > Hi Luiz,
>> >
>> >> > This already fails today. Our code doesn't allow us to call twice t=
he Connect
>> >> > method, so we can't have two of the same UUID connected.
>> >>
>> >> Well with RFCOMM you can't really connect multiple times to the same
>> >> channel/UUID and if we return the same fd clients will probably have
>> >> conflicts.
>> >
>> > you can not connect the same channel twice (except in the other
>> > direction), that is true, but you can connect to a different channel
>> > with a different UUID. That is the reason why we also allow connection
>> > by handles. Or at least we should.
>>
>> You mean record handle? Currently we support connecting by UUID,
>> friendly name or channel.
>>
>> > So even if we would make this limitation of 1 connection per UUID, the
>> > API is a fully asymmetric then. You are suppose to disconnect with the
>> > result of the connect call. I do not like that at all. It is bad API
>> > design and we are trying to squeeze this in the wrong way.
>>
>> Currently we support disconnecting by UUID, friendly name, channel and
>> dev node. As you mentioned it doesn't really work for fd since it is
>> only unique per process, in the other hand the parameter is a pattern.
>>
>> Perhaps what we should be doing is to return a object path in
>> Serial.Connect e.g. [variable
>> prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
>> just get it as parameter, the drawback is that this does not return
>> the tty/fd immediately so we need another round-trip or return
>> multiple values to Connect.
>
> I've been discussing this with Johan and we came to two possible solution=
s.
> The first one is the similar as this one, with three functions:
>
> =A0 =A0 =A0 =A0handle, fd ConnectFD("pattern")
>
> =A0 =A0 =A0 =A0ConnectCancel(handle)
>
> =A0 =A0 =A0 =A0Disconnect(handle)
>
> And other with only ConnectFD() (and maybe ConnectCancel method). Disconn=
ect
> would be handled by the client with shutdown().
>
> I'm tempted to go with this last approach, it's simpler and easier. What =
do
> you think?

Yep, I guess the shutdown is the simplest one and I suppose we don't
have to keep any tracking/reference to the fd in bluetoothd it is up
to the client process to deal with the connection.


--=20
Luiz Augusto von Dentz

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Marcel Holtmann]

Hi Gustavo,

> > >> > This already fails today. Our code doesn't allow us to call twice the Connect
> > >> > method, so we can't have two of the same UUID connected.
> > >>
> > >> Well with RFCOMM you can't really connect multiple times to the same
> > >> channel/UUID and if we return the same fd clients will probably have
> > >> conflicts.
> > >
> > > you can not connect the same channel twice (except in the other
> > > direction), that is true, but you can connect to a different channel
> > > with a different UUID. That is the reason why we also allow connection
> > > by handles. Or at least we should.
> > 
> > You mean record handle? Currently we support connecting by UUID,
> > friendly name or channel.
> > 
> > > So even if we would make this limitation of 1 connection per UUID, the
> > > API is a fully asymmetric then. You are suppose to disconnect with the
> > > result of the connect call. I do not like that at all. It is bad API
> > > design and we are trying to squeeze this in the wrong way.
> > 
> > Currently we support disconnecting by UUID, friendly name, channel and
> > dev node. As you mentioned it doesn't really work for fd since it is
> > only unique per process, in the other hand the parameter is a pattern.
> > 
> > Perhaps what we should be doing is to return a object path in
> > Serial.Connect e.g. [variable
> > prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
> > just get it as parameter, the drawback is that this does not return
> > the tty/fd immediately so we need another round-trip or return
> > multiple values to Connect.
> 
> I've been discussing this with Johan and we came to two possible solutions.
> The first one is the similar as this one, with three functions: 
> 
> 	handle, fd ConnectFD("pattern")
> 
> 	ConnectCancel(handle)
> 
> 	Disconnect(handle)
> 
> And other with only ConnectFD() (and maybe ConnectCancel method). Disconnect
> would be handled by the client with shutdown().
> 
> I'm tempted to go with this last approach, it's simpler and easier. What do
> you think?

I thin the last approach sounds a lot better. It is simpler for the
client and not really intrusive on the API level.

If you want the ConnectCancel method, then it would be with pattern as
parameter. Since of course ConnectFD would only return once the
connection establishment is complete. Hence calling ConnectCancel with a
handle would be rather pointless. And btw. CancelConnect would most like
fit better with our existing API semantics.

As a long term approach, I think we need to redesign this anyway and
make this fully generic for L2CAP and RFCOMM. Since with fd passing we
can be having socket type like interaction with D-Bus and allow
applications to connect to remote systems the way they choose.

The real interesting part could be here if we represent SDP service
records as object path and then allow a connect method on top of these
that will return a fd and also allows proper monitoring of that
connection and addition options. And same could be done for a generic
server over D-Bus. But this is future talk :)

Regards

Marcel

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Marcel Holtmann]

Hi Luiz,

> >> >> > This already fails today. Our code doesn't allow us to call twice the Connect
> >> >> > method, so we can't have two of the same UUID connected.
> >> >>
> >> >> Well with RFCOMM you can't really connect multiple times to the same
> >> >> channel/UUID and if we return the same fd clients will probably have
> >> >> conflicts.
> >> >
> >> > you can not connect the same channel twice (except in the other
> >> > direction), that is true, but you can connect to a different channel
> >> > with a different UUID. That is the reason why we also allow connection
> >> > by handles. Or at least we should.
> >>
> >> You mean record handle? Currently we support connecting by UUID,
> >> friendly name or channel.
> >>
> >> > So even if we would make this limitation of 1 connection per UUID, the
> >> > API is a fully asymmetric then. You are suppose to disconnect with the
> >> > result of the connect call. I do not like that at all. It is bad API
> >> > design and we are trying to squeeze this in the wrong way.
> >>
> >> Currently we support disconnecting by UUID, friendly name, channel and
> >> dev node. As you mentioned it doesn't really work for fd since it is
> >> only unique per process, in the other hand the parameter is a pattern.
> >>
> >> Perhaps what we should be doing is to return a object path in
> >> Serial.Connect e.g. [variable
> >> prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serialXX then Disconnect
> >> just get it as parameter, the drawback is that this does not return
> >> the tty/fd immediately so we need another round-trip or return
> >> multiple values to Connect.
> >
> > I've been discussing this with Johan and we came to two possible solutions.
> > The first one is the similar as this one, with three functions:
> >
> >        handle, fd ConnectFD("pattern")
> >
> >        ConnectCancel(handle)
> >
> >        Disconnect(handle)
> >
> > And other with only ConnectFD() (and maybe ConnectCancel method). Disconnect
> > would be handled by the client with shutdown().
> >
> > I'm tempted to go with this last approach, it's simpler and easier. What do
> > you think?
> 
> Yep, I guess the shutdown is the simplest one and I suppose we don't
> have to keep any tracking/reference to the fd in bluetoothd it is up
> to the client process to deal with the connection.

yes, you do have to keep track of the client. You wanna do a proper
shutdown if the client exits unexpectedly. Or forgets to call shutdown.

Regards

Marcel

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Sep 7, 2011 at 2:56 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> yes, you do have to keep track of the client. You wanna do a proper
> shutdown if the client exits unexpectedly. Or forgets to call shutdown.

I guess the kernel would auto release the socket if the process exit
and nobody else has a reference to it, so if we close our fd after it
has been transferred the only one with reference is the client, iirc
this was a problem to tty because we have to release the devnode to
disconnect which involves ioctl but afaik that is not the case for
sockets and the client can basically call close and be done with it,
right?

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Marcel Holtmann]

Hi Luiz,

> > yes, you do have to keep track of the client. You wanna do a proper
> > shutdown if the client exits unexpectedly. Or forgets to call shutdown.
> 
> I guess the kernel would auto release the socket if the process exit
> and nobody else has a reference to it, so if we close our fd after it
> has been transferred the only one with reference is the client, iirc
> this was a problem to tty because we have to release the devnode to
> disconnect which involves ioctl but afaik that is not the case for
> sockets and the client can basically call close and be done with it,
> right?

does dbus-daemon holds the reference for the time of D-Bus message to be
in flight between bluetoothd and the client? If not, then we have a race
condition here, because we dropped the reference before it ended up on
the other side.

Regards

Marcel

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Sep 7, 2011 at 3:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> > yes, you do have to keep track of the client. You wanna do a proper
>> > shutdown if the client exits unexpectedly. Or forgets to call shutdown.
>>
>> I guess the kernel would auto release the socket if the process exit
>> and nobody else has a reference to it, so if we close our fd after it
>> has been transferred the only one with reference is the client, iirc
>> this was a problem to tty because we have to release the devnode to
>> disconnect which involves ioctl but afaik that is not the case for
>> sockets and the client can basically call close and be done with it,
>> right?
>
> does dbus-daemon holds the reference for the time of D-Bus message to be
> in flight between bluetoothd and the client? If not, then we have a race
> condition here, because we dropped the reference before it ended up on
> the other side.

Good question, iirc dbus-daemon does use dup but we better confirm
with Lennart how this is supposed to work. Anyway if it doesn't then I
guess it should be fixed in dbus-daemon to do it, otherwise as you
said it will be racy to transfer fd specially when fd is send in a
reply.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] serial: Add support to Disconnect fd passing connections

[Gustavo Padovan]

* Luiz Augusto von Dentz <luiz.dentz@gmail.com> [2011-09-07 18:05:08 +0300]:

> Hi Marcel,
> 
> On Wed, Sep 7, 2011 at 3:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Luiz,
> >
> >> > yes, you do have to keep track of the client. You wanna do a proper
> >> > shutdown if the client exits unexpectedly. Or forgets to call shutdown.
> >>
> >> I guess the kernel would auto release the socket if the process exit
> >> and nobody else has a reference to it, so if we close our fd after it
> >> has been transferred the only one with reference is the client, iirc
> >> this was a problem to tty because we have to release the devnode to
> >> disconnect which involves ioctl but afaik that is not the case for
> >> sockets and the client can basically call close and be done with it,
> >> right?
> >
> > does dbus-daemon holds the reference for the time of D-Bus message to be
> > in flight between bluetoothd and the client? If not, then we have a race
> > condition here, because we dropped the reference before it ended up on
> > the other side.
> 
> Good question, iirc dbus-daemon does use dup but we better confirm
> with Lennart how this is supposed to work. Anyway if it doesn't then I
> guess it should be fixed in dbus-daemon to do it, otherwise as you
> said it will be racy to transfer fd specially when fd is send in a
> reply.

It does use dup, I just checked on the source code.

	Gustavo