[PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Giovanni Gherdovich]

The function g_queue_free_full is available only from GLib 2.32.
If BlueZ has to build against GLib 2.28, as stated in the configure.ac,
this patch replaces the calls to g_queue_free_full with its body,
taken from the sources of GLib 2.32.

Signed-off-by: Giovanni Gherdovich <g.gherdovich@gmail.com>
---
 profiles/audio/avctp.c |    3 ++-
 src/adapter.c          |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 013c587..745ced8 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -395,7 +395,8 @@ static void avctp_channel_destroy(struct avctp_channel *chan)
 		g_source_remove(chan->process_id);
 
 	g_free(chan->buffer);
-	g_queue_free_full(chan->queue, pending_destroy);
+	g_queue_foreach(chan->queue, (GFunc)pending_destroy, NULL);
+	g_queue_free(chan->queue);
 	g_slist_free_full(chan->processed, pending_destroy);
 	g_slist_free_full(chan->handlers, g_free);
 	g_free(chan);
diff --git a/src/adapter.c b/src/adapter.c
index e71cea8..a244ae2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1697,7 +1697,8 @@ static void adapter_free(gpointer user_data)
 	if (adapter->auth_idle_id)
 		g_source_remove(adapter->auth_idle_id);
 
-	g_queue_free_full(adapter->auths, g_free);
+	g_queue_foreach (adapter->auths, (GFunc)g_free, NULL);
+	g_queue_free (adapter->auths);
 
 	sdp_list_free(adapter->services, NULL);
 
-- 
1.7.4.1

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Anderson Lizardo]

Hi Giovanni,

A few coding style comments below.

On Sun, Dec 30, 2012 at 6:17 AM, Giovanni Gherdovich
<g.gherdovich@gmail.com> wrote:
> The function g_queue_free_full is available only from GLib 2.32.
> If BlueZ has to build against GLib 2.28, as stated in the configure.ac,
> this patch replaces the calls to g_queue_free_full with its body,
> taken from the sources of GLib 2.32.
>
> Signed-off-by: Giovanni Gherdovich <g.gherdovich@gmail.com>

BlueZ does not use Signed-off-by tag, so you must remove it.

> ---
>  profiles/audio/avctp.c |    3 ++-
>  src/adapter.c          |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
> index 013c587..745ced8 100644
> --- a/profiles/audio/avctp.c
> +++ b/profiles/audio/avctp.c
> @@ -395,7 +395,8 @@ static void avctp_channel_destroy(struct avctp_channel *chan)
>                 g_source_remove(chan->process_id);
>
>         g_free(chan->buffer);
> -       g_queue_free_full(chan->queue, pending_destroy);
> +       g_queue_foreach(chan->queue, (GFunc)pending_destroy, NULL);
> +       g_queue_free(chan->queue);

Where possible, try to avoid using casts for functions. In this case,
try removing "(GFunc)" and see if code still compiles cleanly with
"./bootstrap-configure && make".

>         g_slist_free_full(chan->processed, pending_destroy);
>         g_slist_free_full(chan->handlers, g_free);
>         g_free(chan);
> diff --git a/src/adapter.c b/src/adapter.c
> index e71cea8..a244ae2 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -1697,7 +1697,8 @@ static void adapter_free(gpointer user_data)
>         if (adapter->auth_idle_id)
>                 g_source_remove(adapter->auth_idle_id);
>
> -       g_queue_free_full(adapter->auths, g_free);
> +       g_queue_foreach (adapter->auths, (GFunc)g_free, NULL);
> +       g_queue_free (adapter->auths);

Same comment as above regarding "(GFunc)". Also remove the whitespace
before "(" for the two statements.

We usually split the patches if they touch "core" files (in src/*) and
profile code in profiles/*. For simple patches it is no big deal, but
it also does not hurt if you do this always.

And finally, the commit summary should be in present tense (Replaced
-> Replace), e.g.:

core: Replace calls to g_queue_free_full function
AVCTP: Replace calls to g_queue_free_full function

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Giovanni Gherdovich]

Hi Anderson,

thank you for your review.
A few comments below.

2012/12/30 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> [...]
>> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
>> index 013c587..745ced8 100644
>> --- a/profiles/audio/avctp.c
>> +++ b/profiles/audio/avctp.c
>> @@ -395,7 +395,8 @@ static void avctp_channel_destroy(struct avctp_channel *chan)
>>                 g_source_remove(chan->process_id);
>>
>>         g_free(chan->buffer);
>> -       g_queue_free_full(chan->queue, pending_destroy);
>> +       g_queue_foreach(chan->queue, (GFunc)pending_destroy, NULL);
>> +       g_queue_free(chan->queue);
>
> Where possible, try to avoid using casts for functions. In this case,
> try removing "(GFunc)" and see if code still compiles cleanly with
> "./bootstrap-configure && make".

You rise a very good point here.
In this case, just removing the cast doesn't work:
the function "g_queue_foreach" is expecting an object of type

void (*) (void *, void *)

as second argument, while "pending_destroy" has type

void (*) (void *)

"GFunc" is a typedef to "void (*) (void *, void *)", and the
cast is required to make the code compile and run.

This brings us to the central issue: the patch I submitted,
as well as the GLib code from which I cut-and-pasted the body
of the function g_queue_free_full, strictly speaking
relies on "undefined behaviour", since you cast a function pointer
to another of incompatible type.
In this stackoverflow thread somebody offers an extract of
the C standard where this issue is discussed:
http://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type

I submitted a question to the GLib developers, asking them
why they do so and how they expect their code to work:
https://mail.gnome.org/archives/gtk-devel-list/2012-December/msg00061.html

I will quote here the answer I got, since it wasn't stored in their
mailing list archives, and the argument makes a lot of sense in my opinion:

"
In order to support varargs ('...', http://en.wikipedia.org/wiki/Stdarg.h)
C compilers put function call arguments backwards on the stack. This allows
functions that don't care about extra arguments to simply not offset
back far enough
on the stack to notice them. No modern C compiler excludes support
for varargs and glib relies on varargs anyway.
So its not really an issue."

Which is: you can cast a unary function to a binary function type;
the extra argument will just be ignored, even if the standard takes
a more safe approach and says "don't do that".

My understanding is that the real danger is if you do the opposite cast,
i.e. a binary function f casted to a unary function type:
you will then feed to f less arguments that it expects,
and it will then corrupt the stack looking for the missing input.

In order to solve this special issue in the BlueZ codebase in
a standard-compliant way, one would have to rewrite the function
"g_queue_foreach" so that it takes a unary function as second argument;
but as far as I understood, GLib code relies heavily on this kind
of "forbidden casts"; here another message from last week where a
developer was asking a question very similar to mine,
https://mail.gnome.org/archives/gtk-devel-list/2012-December/msg00032.html
and the answer is basically: "you found just one of the many instances
where we do that, and in practice it works just fine."

To summarize:
I am resubmitting my patch amended with all your requests apart from
the casting issue. If you still have strong objections against it,
I will re-submit again rewriting the function "g_queue_foreach"
with the right prototype, taking it out from GLib and putting it in
"Bluez space".

Regards,
Giovanni

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Luiz Augusto von Dentz]

Hi Giovanni,

On Tue, Jan 1, 2013 at 12:19 PM, Giovanni Gherdovich
<g.gherdovich@gmail.com> wrote:
> Hi Anderson,
>
> thank you for your review.
> A few comments below.
>
> 2012/12/30 Anderson Lizardo <anderson.lizardo@openbossa.org>:
>> [...]
>>> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
>>> index 013c587..745ced8 100644
>>> --- a/profiles/audio/avctp.c
>>> +++ b/profiles/audio/avctp.c
>>> @@ -395,7 +395,8 @@ static void avctp_channel_destroy(struct avctp_channel *chan)
>>>                 g_source_remove(chan->process_id);
>>>
>>>         g_free(chan->buffer);
>>> -       g_queue_free_full(chan->queue, pending_destroy);
>>> +       g_queue_foreach(chan->queue, (GFunc)pending_destroy, NULL);
>>> +       g_queue_free(chan->queue);
>>
>> Where possible, try to avoid using casts for functions. In this case,
>> try removing "(GFunc)" and see if code still compiles cleanly with
>> "./bootstrap-configure && make".
>
> You rise a very good point here.
> In this case, just removing the cast doesn't work:
> the function "g_queue_foreach" is expecting an object of type
>
> void (*) (void *, void *)
>
> as second argument, while "pending_destroy" has type
>
> void (*) (void *)
>
> "GFunc" is a typedef to "void (*) (void *, void *)", and the
> cast is required to make the code compile and run.
>
> This brings us to the central issue: the patch I submitted,
> as well as the GLib code from which I cut-and-pasted the body
> of the function g_queue_free_full, strictly speaking
> relies on "undefined behaviour", since you cast a function pointer
> to another of incompatible type.
> In this stackoverflow thread somebody offers an extract of
> the C standard where this issue is discussed:
> http://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type
>
> I submitted a question to the GLib developers, asking them
> why they do so and how they expect their code to work:
> https://mail.gnome.org/archives/gtk-devel-list/2012-December/msg00061.html
>
> I will quote here the answer I got, since it wasn't stored in their
> mailing list archives, and the argument makes a lot of sense in my opinion:
>
> "
> In order to support varargs ('...', http://en.wikipedia.org/wiki/Stdarg.h)
> C compilers put function call arguments backwards on the stack. This allows
> functions that don't care about extra arguments to simply not offset
> back far enough
> on the stack to notice them. No modern C compiler excludes support
> for varargs and glib relies on varargs anyway.
> So its not really an issue."
>
> Which is: you can cast a unary function to a binary function type;
> the extra argument will just be ignored, even if the standard takes
> a more safe approach and says "don't do that".
>
> My understanding is that the real danger is if you do the opposite cast,
> i.e. a binary function f casted to a unary function type:
> you will then feed to f less arguments that it expects,
> and it will then corrupt the stack looking for the missing input.
>
> In order to solve this special issue in the BlueZ codebase in
> a standard-compliant way, one would have to rewrite the function
> "g_queue_foreach" so that it takes a unary function as second argument;
> but as far as I understood, GLib code relies heavily on this kind
> of "forbidden casts"; here another message from last week where a
> developer was asking a question very similar to mine,
> https://mail.gnome.org/archives/gtk-devel-list/2012-December/msg00032.html
> and the answer is basically: "you found just one of the many instances
> where we do that, and in practice it works just fine."
>
> To summarize:
> I am resubmitting my patch amended with all your requests apart from
> the casting issue. If you still have strong objections against it,
> I will re-submit again rewriting the function "g_queue_foreach"
> with the right prototype, taking it out from GLib and putting it in
> "Bluez space".
>
> Regards,
> Giovanni
> --

There is something off here, in the past we did have an implementation
of g_slist_free_full to overcome this dependency problem but it was
removed in this commit:

commit 84156dadb25ec0973752d34d84fc9ffb3c720988
Author: Marcel Holtmann <marcel@holtmann.org>
Date:   Mon Apr 16 18:22:24 2012 +0200

    build: Remove glib-compat.h support

In that case I would just revert back this patch, but the
documentation actually say g_slist_free_full is available since 2.28
http://developer.gnome.org/glib/2.28/glib-Singly-Linked-Lists.html#g-slist-free-full
so I wonder what is going on.



--
Luiz Augusto von Dentz

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Vinicius Gomes]

Hi Luiz,

> In that case I would just revert back this patch, but the
> documentation actually say g_slist_free_full is available since 2.28
> http://developer.gnome.org/glib/2.28/glib-Singly-Linked-Lists.html#g-slist-free-full
> so I wonder what is going on.
>

The problem now is g_queue_free_full() not the g_slist_free_full().

>
>
> --
> Luiz Augusto von Dentz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
--
Vinicius

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Luiz Augusto von Dentz]

Hi Vinicius,

On Wed, Jan 2, 2013 at 10:23 PM, Vinicius Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi Luiz,
>
>> In that case I would just revert back this patch, but the
>> documentation actually say g_slist_free_full is available since 2.28
>> http://developer.gnome.org/glib/2.28/glib-Singly-Linked-Lists.html#g-slist-free-full
>> so I wonder what is going on.
>>
>
> The problem now is g_queue_free_full() not the g_slist_free_full().

Right, but it is quite the same situation and I don't get why we don't
just update, by the time distros start to package BlueZ 5 glib 2.32
wont be a problem, in fact it should not be a problem right now as it
is about a year old release:

commit 816554c62bf227498cb539924e6ee2050030b4b9
Author: Matthias Clasen <mclasen@redhat.com>
Date:   Sat Mar 24 11:28:35 2012 -0400

    2.32.0

--
Luiz Augusto von Dentz

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Vinicius Costa Gomes]

Hi Luiz,

> 
> Right, but it is quite the same situation and I don't get why we don't
> just update, by the time distros start to package BlueZ 5 glib 2.32
> wont be a problem, in fact it should not be a problem right now as it
> is about a year old release:

I agree with you here. And that was the suggestion that I gave to
Giovanni on IRC when he found the problem.

> 
> commit 816554c62bf227498cb539924e6ee2050030b4b9
> Author: Matthias Clasen <mclasen@redhat.com>
> Date:   Sat Mar 24 11:28:35 2012 -0400
> 
>     2.32.0
> 
> --
> Luiz Augusto von Dentz


Cheers,
-- 
Vinicius

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Giovanni Gherdovich]

Hi Vinicius and Luiz,

2013/1/2 Vinicius Costa Gomes <vinicius.gomes@openbossa.org>:
> [...]
>>
>> Right, but it is quite the same situation and I don't get why we don't
>> just update, by the time distros start to package BlueZ 5 glib 2.32
>> wont be a problem, in fact it should not be a problem right now as it
>> is about a year old release:
>
> I agree with you here. And that was the suggestion that I gave to
> Giovanni on IRC when he found the problem.

I just resubmitted an amended patch, since I had already prepared it.
BTW I will run a quick check on what GLib versions the various
major distros are packaging right now, just to have some data to add
to the poll.

Regards,
Giovanni

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Marcel Holtmann]

Hi Luiz,

> >> In that case I would just revert back this patch, but the
> >> documentation actually say g_slist_free_full is available since 2.28
> >> http://developer.gnome.org/glib/2.28/glib-Singly-Linked-Lists.html#g-slist-free-full
> >> so I wonder what is going on.
> >>
> >
> > The problem now is g_queue_free_full() not the g_slist_free_full().
> 
> Right, but it is quite the same situation and I don't get why we don't
> just update, by the time distros start to package BlueZ 5 glib 2.32
> wont be a problem, in fact it should not be a problem right now as it
> is about a year old release:

because every new GLib release drags in more dependencies. It is a bit
out of control. So requiring the 2.32 comes at a cost that I am not
willing to pay right now. We already have seen this with ConnMan where I
accidentally used a newer GLib function that was not present in a 2.28
and before. It is pretty hard for embedded system to do these kind of
upgrades when their dependencies and thus footprint and memory
consumption increases for just a simple convenience function.

Regards

Marcel

Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function

[Luiz Augusto von Dentz]

Hi Marcel,

On Thu, Jan 3, 2013 at 1:42 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>> >> In that case I would just revert back this patch, but the
>> >> documentation actually say g_slist_free_full is available since 2.28
>> >> http://developer.gnome.org/glib/2.28/glib-Singly-Linked-Lists.html#g-slist-free-full
>> >> so I wonder what is going on.
>> >>
>> >
>> > The problem now is g_queue_free_full() not the g_slist_free_full().
>>
>> Right, but it is quite the same situation and I don't get why we don't
>> just update, by the time distros start to package BlueZ 5 glib 2.32
>> wont be a problem, in fact it should not be a problem right now as it
>> is about a year old release:
>
> because every new GLib release drags in more dependencies. It is a bit
> out of control. So requiring the 2.32 comes at a cost that I am not
> willing to pay right now. We already have seen this with ConnMan where I
> accidentally used a newer GLib function that was not present in a 2.28
> and before. It is pretty hard for embedded system to do these kind of
> upgrades when their dependencies and thus footprint and memory
> consumption increases for just a simple convenience function.

It seems the mandatory glib dependencies are restricted to libffi,
pkg-config and Python:
http://www.linuxfromscratch.org/blfs/view/svn/general/glib2.html

Anyway regardless if we do update or not, I don't see why
g_queue_free_full is different than g_slist_free_full, so instead of
converting everything to g_queue_foreach + g_queue_free why we don't
bring back glib-compat and do this in one place as we did for
g_slist_free_full?

--
Luiz Augusto von Dentz