Hi Giovanni, On Tue, Jan 1, 2013 at 12:19 PM, Giovanni Gherdovich <g.gherdovich@xxxxxxxxx> wrote: > Hi Anderson, > > thank you for your review. > A few comments below. > > 2012/12/30 Anderson Lizardo <anderson.lizardo@xxxxxxxxxxxxx>: >> [...] >>> 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@xxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
