[PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This use dup to create a new fd to be used by AVDTP session leaving the
caller free to close the original fd. Note that even if the caller
decides to keep the original fd it will still be notified when
avdtp_shutdown is called since it uses shutdown.
---
 android/a2dp.c  | 1 -
 android/avdtp.c | 7 ++++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/android/a2dp.c b/android/a2dp.c
index 9087c62..b7bb8d2 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -150,7 +150,6 @@ static void signaling_connect_cb(GIOChannel *chan, GError *err,
 		return;
 	}
 
-	g_io_channel_set_close_on_unref(chan, FALSE);
 	fd = g_io_channel_unix_get_fd(chan);
 
 	/* FIXME: Add proper version */
diff --git a/android/avdtp.c b/android/avdtp.c
index 353316c..ec78cc6 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -2055,9 +2055,14 @@ struct avdtp *avdtp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
 {
 	struct avdtp *session;
 	GIOCondition cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;
+	int new_fd;
+
+	new_fd = dup(fd);
+	if (new_fd < 0)
+		return NULL;
 
 	session = g_new0(struct avdtp, 1);
-	session->io = g_io_channel_unix_new(fd);
+	session->io = g_io_channel_unix_new(new_fd);
 	session->version = version;
 	session->imtu = imtu;
 	session->omtu = omtu;
-- 
1.8.3.1

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Ravi kumar Veeramally]

Hi Luiz,

On 20.12.2013 11:56, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This use dup to create a new fd to be used by AVDTP session leaving the
> caller free to close the original fd. Note that even if the caller
> decides to keep the original fd it will still be notified when
> avdtp_shutdown is called since it uses shutdown.
> ---
>   android/a2dp.c  | 1 -
>   android/avdtp.c | 7 ++++++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/android/a2dp.c b/android/a2dp.c
> index 9087c62..b7bb8d2 100644
> --- a/android/a2dp.c
> +++ b/android/a2dp.c
> @@ -150,7 +150,6 @@ static void signaling_connect_cb(GIOChannel *chan, GError *err,
>   		return;
>   	}
>   
> -	g_io_channel_set_close_on_unref(chan, FALSE);
>   	fd = g_io_channel_unix_get_fd(chan);
>   
>   	/* FIXME: Add proper version */
> diff --git a/android/avdtp.c b/android/avdtp.c
> index 353316c..ec78cc6 100644
> --- a/android/avdtp.c
> +++ b/android/avdtp.c
> @@ -2055,9 +2055,14 @@ struct avdtp *avdtp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
>   {
>   	struct avdtp *session;
>   	GIOCondition cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;
> +	int new_fd;
> +
> +	new_fd = dup(fd);
> +	if (new_fd < 0)
> +		return NULL;
  You are returning NULL here, but not checking at
  static void signaling_connect_cb(...)
         dev->session = avdtp_new(fd, imtu, omtu, 0x0100);

>   
>   	session = g_new0(struct avdtp, 1);
> -	session->io = g_io_channel_unix_new(fd);
> +	session->io = g_io_channel_unix_new(new_fd);
>   	session->version = version;
>   	session->imtu = imtu;
>   	session->omtu = omtu;

Regards,
Ravi.

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Marcel Holtmann]

Hi Luiz,

> This use dup to create a new fd to be used by AVDTP session leaving the
> caller free to close the original fd. Note that even if the caller
> decides to keep the original fd it will still be notified when
> avdtp_shutdown is called since it uses shutdown.

I would be more curious on why this is needed.

Regards

Marcel

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Dec 20, 2013 at 3:16 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> This use dup to create a new fd to be used by AVDTP session leaving the
>> caller free to close the original fd. Note that even if the caller
>> decides to keep the original fd it will still be notified when
>> avdtp_shutdown is called since it uses shutdown.
>
> I would be more curious on why this is needed.

It is basically to make the caller able to release any resources, such
as GIOChannel managed by btio, without causing a disconnect.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Marcel Holtmann]

Hi Luiz,

>>> This use dup to create a new fd to be used by AVDTP session leaving the
>>> caller free to close the original fd. Note that even if the caller
>>> decides to keep the original fd it will still be notified when
>>> avdtp_shutdown is called since it uses shutdown.
>> 
>> I would be more curious on why this is needed.
> 
> It is basically to make the caller able to release any resources, such
> as GIOChannel managed by btio, without causing a disconnect.

you do not have to set close_on_unref with the GIOChannel. You can just unset that one. Then GIOChannel will not touch it and will not call close on it.

Regards

Marcel

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Dec 20, 2013 at 3:52 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>>>> This use dup to create a new fd to be used by AVDTP session leaving the
>>>> caller free to close the original fd. Note that even if the caller
>>>> decides to keep the original fd it will still be notified when
>>>> avdtp_shutdown is called since it uses shutdown.
>>>
>>> I would be more curious on why this is needed.
>>
>> It is basically to make the caller able to release any resources, such
>> as GIOChannel managed by btio, without causing a disconnect.
>
> you do not have to set close_on_unref with the GIOChannel. You can just unset that one. Then GIOChannel will not touch it and will not call close on it.

That is what we used to do, but it is not very convenient since there
it is still possible to call g_io_channel_shutdown which would cause a
disconnect. Also dup turn out to be handful to check if the fd is
valid and free the user to do whatever it pleases with the original
fd, in the end what we are doing with the fd is very similar to what
we would do to a GIOChannel as dup just creates another reference the
underline socket is the same.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Marcel Holtmann]

Hi Luiz,

>>>>> This use dup to create a new fd to be used by AVDTP session leaving the
>>>>> caller free to close the original fd. Note that even if the caller
>>>>> decides to keep the original fd it will still be notified when
>>>>> avdtp_shutdown is called since it uses shutdown.
>>>> 
>>>> I would be more curious on why this is needed.
>>> 
>>> It is basically to make the caller able to release any resources, such
>>> as GIOChannel managed by btio, without causing a disconnect.
>> 
>> you do not have to set close_on_unref with the GIOChannel. You can just unset that one. Then GIOChannel will not touch it and will not call close on it.
> 
> That is what we used to do, but it is not very convenient since there
> it is still possible to call g_io_channel_shutdown which would cause a
> disconnect. Also dup turn out to be handful to check if the fd is
> valid and free the user to do whatever it pleases with the original
> fd, in the end what we are doing with the fd is very similar to what
> we would do to a GIOChannel as dup just creates another reference the
> underline socket is the same.

if the original caller calls g_io_channel_shutdown it will still cause a disconnect. No matter if you duped the fd or not. Not getting your point. If the caller is doing something stupid, then you have a problem not matter what.

This duped fd is different from everything else we have in src/shared/ in any of the projects. You hand over the fd and then it is owned by that part. After that you are not suppose to touch it anymore. That is the semantic that I want. And not another complicated concept of reference counting fd in multiple levels of the code.

Regards

Marcel

Re: [PATCH BlueZ] android/AVDTP: Duplicate fd passed to avdtp_new

[Luiz Augusto von Dentz]

Hi Marcel,

On Fri, Dec 20, 2013 at 4:39 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Luiz,
>
>>>>>> This use dup to create a new fd to be used by AVDTP session leaving =
the
>>>>>> caller free to close the original fd. Note that even if the caller
>>>>>> decides to keep the original fd it will still be notified when
>>>>>> avdtp_shutdown is called since it uses shutdown.
>>>>>
>>>>> I would be more curious on why this is needed.
>>>>
>>>> It is basically to make the caller able to release any resources, such
>>>> as GIOChannel managed by btio, without causing a disconnect.
>>>
>>> you do not have to set close_on_unref with the GIOChannel. You can just=
 unset that one. Then GIOChannel will not touch it and will not call close =
on it.
>>
>> That is what we used to do, but it is not very convenient since there
>> it is still possible to call g_io_channel_shutdown which would cause a
>> disconnect. Also dup turn out to be handful to check if the fd is
>> valid and free the user to do whatever it pleases with the original
>> fd, in the end what we are doing with the fd is very similar to what
>> we would do to a GIOChannel as dup just creates another reference the
>> underline socket is the same.
>
> if the original caller calls g_io_channel_shutdown it will still cause a =
disconnect. No matter if you duped the fd or not. Not getting your point. I=
f the caller is doing something stupid, then you have a problem not matter =
what.

Nope, g_io_channel_shutdown does not call shutdown, it only does close
and therefore do not affect in case of duped fd. The point is
convenience, having same code path regardless if the fd has been hand
over or not.

> This duped fd is different from everything else we have in src/shared/ in=
 any of the projects. You hand over the fd and then it is owned by that par=
t. After that you are not suppose to touch it anymore. That is the semantic=
 that I want. And not another complicated concept of reference counting fd =
in multiple levels of the code.

Well I don't think we had defined any semantic for fd ownership,
mgmt_new is the only one that actually have such a thing but it does
not work with GIOChannel/btio handling the connection setup, and if
you look at other examples like libdbus it does make use of dup when
passing fds around (and no this is not necessary with SCM_RIGTHS since
the kernel does take a reference while transferring the fd to another
process).

We could perhaps do the close_on_unref function similar to the mgmt,
which perhaps was inspired in the GIOChannel that also take ownership
and thus requires close_on_unref helper function to instrument how it
should deal with the fd, imo dup is probably less code since you can
always close on unref.

--=20
Luiz Augusto von Dentz