* [PATCH 2/2] AVCTP: Replace calls to g_queue_free_full function
From: Giovanni Gherdovich @ 2013-01-01 11:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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 in the AVTCP module
with its body, taken from the sources of GLib 2.32.
---
profiles/audio/avctp.c | 3 ++-
1 files changed, 2 insertions(+), 1 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);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 1/2] core: Replace calls to g_queue_free_full function
From: Giovanni Gherdovich @ 2013-01-01 11:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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 in the "core" BlueZ module
with its body, taken from the sources of GLib 2.32.
---
src/adapter.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index e71cea8..73c9e58 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
^ permalink raw reply related
* Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function
From: Giovanni Gherdovich @ 2013-01-01 10:19 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <CAJdJm_PpJOVALj5xJvCRA4mC6WAMyP5RsqHVruz6xaRBDhXg5A@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH] obexd: Fix crash while removing session
From: Marcin Zawiejski @ 2012-12-31 14:31 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1356963778-2135-1-git-send-email-dragmz@gmail.com>
On Mon, 2012-12-31 at 15:22 +0100, Marcin Zawiejski wrote:
> Crash occurs when removing a session with RemoveSession while another session has been created but not yet registered
>
Sorry for double post, please ignore this one.
Marcin.
^ permalink raw reply
* [PATCH] obexd: Fix crash while removing session
From: Marcin Zawiejski @ 2012-12-31 14:22 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcin Zawiejski
Crash occurs when removing a session with RemoveSession while another session has been created but not yet registered
Backtrace:
0 __strcmp_ssse3 () at ../sysdeps/i386/i686/multiarch/strcmp-ssse3.S:233
1 0xb758e7c3 in g_str_equal () from /lib/i386-linux-gnu/libglib-2.0.so.0
2 0x08073e56 in find_session (path=0x85c8504 "/org/bluez/obex/session0") at obexd/client/manager.c:146
3 remove_session (connection=0x85bc5e0, message=0x85bca98, user_data=0x0) at obexd/client/manager.c:216
4 0x08055f6f in process_message (connection=0x85bc5e0, message=<optimized out>, iface_user_data=0x0,
method=<optimized out>, method=<optimized out>) at gdbus/object.c:285
5 0xb7672666 in ?? () from /lib/i386-linux-gnu/libdbus-1.so.3
6 0xb76624d7 in dbus_connection_dispatch () from /lib/i386-linux-gnu/libdbus-1.so.3
7 0x080532f8 in message_dispatch (data=0x85bc5e0) at gdbus/mainloop.c:76
8 0xb759f6bf in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
9 0xb759e9e3 in g_main_context_dispatch () from /lib/i386-linux-gnu/libglib-2.0.so.0
10 0xb759ed80 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
11 0xb759f1db in g_main_loop_run () from /lib/i386-linux-gnu/libglib-2.0.so.0
12 0x08052d74 in main (argc=1, argv=0xbfb344e4) at obexd/src/main.c:323
---
obexd/client/manager.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..03e60a4 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -59,11 +59,16 @@ static GSList *sessions = NULL;
static void shutdown_session(struct obc_session *session)
{
- sessions = g_slist_remove(sessions, session);
obc_session_shutdown(session);
obc_session_unref(session);
}
+static void release_session(struct obc_session *session)
+{
+ sessions = g_slist_remove(sessions, session);
+ shutdown_session(session);
+}
+
static void unregister_session(void *data)
{
struct obc_session *session = data;
@@ -93,7 +98,16 @@ static void create_callback(struct obc_session *session,
path = obc_session_register(session, unregister_session);
+ if (path == NULL) {
+ DBusMessage *error = g_dbus_create_error(data->message,
+ ERROR_INTERFACE ".Failed",
+ NULL);
+ g_dbus_send_message(data->connection, error);
+ shutdown_session(session);
+ goto done;
+ }
+ sessions = g_slist_append(sessions, session);
g_dbus_send_reply(data->connection, data->message,
DBUS_TYPE_OBJECT_PATH, &path,
DBUS_TYPE_INVALID);
@@ -190,7 +204,6 @@ static DBusMessage *create_session(DBusConnection *connection,
dbus_message_get_sender(message),
create_callback, data);
if (session != NULL) {
- sessions = g_slist_append(sessions, session);
return NULL;
}
@@ -224,7 +237,7 @@ static DBusMessage *remove_session(DBusConnection *connection,
ERROR_INTERFACE ".NotAuthorized",
"Not Authorized");
- shutdown_session(session);
+ release_session(session);
return dbus_message_new_method_return(message);
}
--
1.8.0.3
^ permalink raw reply related
* [PATCH] obexd: Fix crash while removing session
From: Marcin Zawiejski @ 2012-12-31 14:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcin Zawiejski
Crash occurs when removing a session with RemoveSession while another session has been created but not yet registered
Backtrace:
0 __strcmp_ssse3 () at ../sysdeps/i386/i686/multiarch/strcmp-ssse3.S:233
1 0xb758e7c3 in g_str_equal () from /lib/i386-linux-gnu/libglib-2.0.so.0
2 0x08073e56 in find_session (path=0x85c8504 "/org/bluez/obex/session0") at obexd/client/manager.c:146
3 remove_session (connection=0x85bc5e0, message=0x85bca98, user_data=0x0) at obexd/client/manager.c:216
4 0x08055f6f in process_message (connection=0x85bc5e0, message=<optimized out>, iface_user_data=0x0,
method=<optimized out>, method=<optimized out>) at gdbus/object.c:285
5 0xb7672666 in ?? () from /lib/i386-linux-gnu/libdbus-1.so.3
6 0xb76624d7 in dbus_connection_dispatch () from /lib/i386-linux-gnu/libdbus-1.so.3
7 0x080532f8 in message_dispatch (data=0x85bc5e0) at gdbus/mainloop.c:76
8 0xb759f6bf in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
9 0xb759e9e3 in g_main_context_dispatch () from /lib/i386-linux-gnu/libglib-2.0.so.0
10 0xb759ed80 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
11 0xb759f1db in g_main_loop_run () from /lib/i386-linux-gnu/libglib-2.0.so.0
12 0x08052d74 in main (argc=1, argv=0xbfb344e4) at obexd/src/main.c:323
---
obexd/client/manager.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..03e60a4 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -59,11 +59,16 @@ static GSList *sessions = NULL;
static void shutdown_session(struct obc_session *session)
{
- sessions = g_slist_remove(sessions, session);
obc_session_shutdown(session);
obc_session_unref(session);
}
+static void release_session(struct obc_session *session)
+{
+ sessions = g_slist_remove(sessions, session);
+ shutdown_session(session);
+}
+
static void unregister_session(void *data)
{
struct obc_session *session = data;
@@ -93,7 +98,16 @@ static void create_callback(struct obc_session *session,
path = obc_session_register(session, unregister_session);
+ if (path == NULL) {
+ DBusMessage *error = g_dbus_create_error(data->message,
+ ERROR_INTERFACE ".Failed",
+ NULL);
+ g_dbus_send_message(data->connection, error);
+ shutdown_session(session);
+ goto done;
+ }
+ sessions = g_slist_append(sessions, session);
g_dbus_send_reply(data->connection, data->message,
DBUS_TYPE_OBJECT_PATH, &path,
DBUS_TYPE_INVALID);
@@ -190,7 +204,6 @@ static DBusMessage *create_session(DBusConnection *connection,
dbus_message_get_sender(message),
create_callback, data);
if (session != NULL) {
- sessions = g_slist_append(sessions, session);
return NULL;
}
@@ -224,7 +237,7 @@ static DBusMessage *remove_session(DBusConnection *connection,
ERROR_INTERFACE ".NotAuthorized",
"Not Authorized");
- shutdown_session(session);
+ release_session(session);
return dbus_message_new_method_return(message);
}
--
1.8.0.3
^ permalink raw reply related
* Re: SEGFAULT in obexd manager.c:find_session
From: Luiz Augusto von Dentz @ 2012-12-31 10:13 UTC (permalink / raw)
To: Marcin Zawiejski; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1356911904.7243.13.camel@lsdevbox>
Hi Marcin,
On Mon, Dec 31, 2012 at 1:58 AM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> ---
> diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> index 8f62a30..118dd48 100644
> --- a/obexd/client/manager.c
> +++ b/obexd/client/manager.c
> @@ -59,11 +59,16 @@ static GSList *sessions = NULL;
>
> static void shutdown_session(struct obc_session *session)
> {
> - sessions = g_slist_remove(sessions, session);
> obc_session_shutdown(session);
> obc_session_unref(session);
> }
>
> +static void release_session(struct obc_session *session)
> +{
> + sessions = g_slist_remove(sessions, session);
> + shutdown_session(session);
> +}
> +
> static void unregister_session(void *data)
> {
> struct obc_session *session = data;
> @@ -94,6 +99,16 @@ static void create_callback(struct obc_session
> *session,
>
> path = obc_session_register(session, unregister_session);
Normally we don't have empty lines between assigning and if statements
checking the values, also please follow the code style e.g.
if<space>(<condition>) it is quite similar to the one use in kernel.
> + if(path == NULL) {
> + DBusMessage *error = g_dbus_create_error(data->message,
> + ERROR_INTERFACE ".Failed",
> + NULL);
> + g_dbus_send_message(data->connection, error);
> + shutdown_session(session);
> + goto done;
> + }
> +
> + sessions = g_slist_append(sessions, session);
> g_dbus_send_reply(data->connection, data->message,
> DBUS_TYPE_OBJECT_PATH, &path,
> DBUS_TYPE_INVALID);
> @@ -190,7 +205,6 @@ static DBusMessage *create_session(DBusConnection
> *connection,
> dbus_message_get_sender(message),
> create_callback, data);
> if (session != NULL) {
> - sessions = g_slist_append(sessions, session);
> return NULL;
> }
>
> @@ -224,7 +238,7 @@ static DBusMessage *remove_session(DBusConnection
> *connection,
> ERROR_INTERFACE ".NotAuthorized",
> "Not Authorized");
>
> - shutdown_session(session);
> + release_session(session);
>
> return dbus_message_new_method_return(message);
> }
> ---
Besides that the patch looks good, once you fix the code style please
send this a proper patch e.g. git format-patch + git send-email and
please include the backtrace in the patch description.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: Specifying destination path in obexd/bluez-5
From: Luiz Augusto von Dentz @ 2012-12-31 10:01 UTC (permalink / raw)
To: Alex Fiestas; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <13320115.C4qltrvZED@monsterbad>
Hi Alex,
On Mon, Dec 31, 2012 at 10:56 AM, Alex Fiestas <afiestas@kde.org> wrote:
> Last time I tried to port KDE's obex support completely to obexd (we are
> already using obexd-client for opp sendfile) I couldn't complete it because
> there is no way of programatically specify the destination path of the
> incoming files, iirc using either ftp or opp.
>
> Now checking out BlueZ-5 I'm not sure I see a way of doing it either.
There is always the possibility of using links if the underline
filesystem support them. For OPP the agent can provide a different
path on the return of AuthorizePush.
> So, is there anyway of changing the destination path programatically without
> having to restart obexd-server? If not, can I add a wish somewhere (bugrack?)
> for it? Would you consider?
I guess this is about FTP, right? If I understood you want to be able
to change the folders being exported in run-time, that might be
possible but if we do that I think it would be better to let the agent
set its root folder once registered then you can fully control this on
the component implementing the agent interface (usually the UI)
leaving the command line switch only for systems without agents.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Specifying destination path in obexd/bluez-5
From: Alex Fiestas @ 2012-12-31 8:56 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
Last time I tried to port KDE's obex support completely to obexd (we are
already using obexd-client for opp sendfile) I couldn't complete it because
there is no way of programatically specify the destination path of the
incoming files, iirc using either ftp or opp.
Now checking out BlueZ-5 I'm not sure I see a way of doing it either.
So, is there anyway of changing the destination path programatically without
having to restart obexd-server? If not, can I add a wish somewhere (bugrack?)
for it? Would you consider?
Thanks and congratulations for BlueZ-5 :)!
^ permalink raw reply
* Re: SEGFAULT in obexd manager.c:find_session
From: Marcin Zawiejski @ 2012-12-31 0:01 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJTCf9eiVm10SoLRDQ870ayt00Eiz=szXEb1fhktWyHcQ@mail.gmail.com>
Hi Luiz,
On Sun, 2012-12-30 at 16:28 +0200, Luiz Augusto von Dentz wrote:
> Hi Marcin,
>
> On Sun, Dec 30, 2012 at 3:18 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> > Hi, I think there is a bug in obexd in manager.c:find_session function.
> >
> > What happens here is a segfault when manager.c:find_session calls
> > g_str_equal(obc_session_get_path(session), path). This is caused by the
> > sessions list having a session with a NULL path.
> >
> > Basically when I call manager.c:create_session, the session created there is
> > added to sessions list but it has a NULL path until the
> > manager.c:create_callback is called.
> >
> > However the manager.c:create_callback is not called at all if the remote
> > device refuses the connection. So when manager.c:find_session is called, it
> > actually calls the g_str_equal(NULL, path) causing the segfault.
> >
> > This might be simply fixed by modifying the manager.c:find_session to check
> > for a NULL session path before calling g_str_equal(...).
> >
> > The problem is reproducible by having two sessions, with one awaiting
> > connection and another one with an active file transfer. When the file
> > transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
> > obexd segfaults since the session awaiting connection has a NULL path.
> >
> > I'm not sure if the session with a NULL path should be on the sessions list
> > or not. If its okay, then here's a simple patch for the
> > manager.c:find_session function:
> >
> > ---
> > diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> > index 8f62a30..28b890c 100644
> > --- a/obexd/client/manager.c
> > +++ b/obexd/client/manager.c
> > @@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
> > *path)
> >
> > for (l = sessions; l; l = l->next) {
> > struct obc_session *session = l->data;
> > + const char *session_path = obc_session_get_path(session);
> >
> > - if (g_str_equal(obc_session_get_path(session), path) ==
> > TRUE)
> > + if (session_path != NULL && g_str_equal(session_path, path)
> > == TRUE)
> > return session;
> > }
> > ---
>
> You can use g_strcmp0 which checks for NULL, gonna take a look why the
> session path is NULL perhaps we should not even add to the session
> list until the connection completes and it is properly registered.
I think you are right - session with no path should not be placed on the
sessions list because it is not usable in such case anyway. Below is a patch
that works for me, here's a summary of the changes:
- move g_slist_append from create_session to create_callback so the
session is added to sessions list after it has been registered
- split shutdown_session into shutdown_session and release_session; the
shutdown_session performs the shutdown and unref on the session but does
not try to remove session from the list; the release_session removes the
session from the list and calls release_shutdown
- modify create_callback so it calls shutdown_session for failures
before the session has been registered (so it doesn't try to remove
session from the list since the session is not on the list)
- modify remove_session to call release_session in order to remove the
session from list and do shutdown
---
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..118dd48 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -59,11 +59,16 @@ static GSList *sessions = NULL;
static void shutdown_session(struct obc_session *session)
{
- sessions = g_slist_remove(sessions, session);
obc_session_shutdown(session);
obc_session_unref(session);
}
+static void release_session(struct obc_session *session)
+{
+ sessions = g_slist_remove(sessions, session);
+ shutdown_session(session);
+}
+
static void unregister_session(void *data)
{
struct obc_session *session = data;
@@ -94,6 +99,16 @@ static void create_callback(struct obc_session
*session,
path = obc_session_register(session, unregister_session);
+ if(path == NULL) {
+ DBusMessage *error = g_dbus_create_error(data->message,
+ ERROR_INTERFACE ".Failed",
+ NULL);
+ g_dbus_send_message(data->connection, error);
+ shutdown_session(session);
+ goto done;
+ }
+
+ sessions = g_slist_append(sessions, session);
g_dbus_send_reply(data->connection, data->message,
DBUS_TYPE_OBJECT_PATH, &path,
DBUS_TYPE_INVALID);
@@ -190,7 +205,6 @@ static DBusMessage *create_session(DBusConnection
*connection,
dbus_message_get_sender(message),
create_callback, data);
if (session != NULL) {
- sessions = g_slist_append(sessions, session);
return NULL;
}
@@ -224,7 +238,7 @@ static DBusMessage *remove_session(DBusConnection
*connection,
ERROR_INTERFACE ".NotAuthorized",
"Not Authorized");
- shutdown_session(session);
+ release_session(session);
return dbus_message_new_method_return(message);
}
---
Marcin.
^ permalink raw reply related
* Re: SEGFAULT in obexd manager.c:find_session
From: Marcin Zawiejski @ 2012-12-30 23:58 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJTCf9eiVm10SoLRDQ870ayt00Eiz=szXEb1fhktWyHcQ@mail.gmail.com>
Hi Luiz,
On Sun, 2012-12-30 at 16:28 +0200, Luiz Augusto von Dentz wrote:
> Hi Marcin,
>
> On Sun, Dec 30, 2012 at 3:18 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> > Hi, I think there is a bug in obexd in manager.c:find_session function.
> >
> > What happens here is a segfault when manager.c:find_session calls
> > g_str_equal(obc_session_get_path(session), path). This is caused by the
> > sessions list having a session with a NULL path.
> >
> > Basically when I call manager.c:create_session, the session created there is
> > added to sessions list but it has a NULL path until the
> > manager.c:create_callback is called.
> >
> > However the manager.c:create_callback is not called at all if the remote
> > device refuses the connection. So when manager.c:find_session is called, it
> > actually calls the g_str_equal(NULL, path) causing the segfault.
> >
> > This might be simply fixed by modifying the manager.c:find_session to check
> > for a NULL session path before calling g_str_equal(...).
> >
> > The problem is reproducible by having two sessions, with one awaiting
> > connection and another one with an active file transfer. When the file
> > transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
> > obexd segfaults since the session awaiting connection has a NULL path.
> >
> > I'm not sure if the session with a NULL path should be on the sessions list
> > or not. If its okay, then here's a simple patch for the
> > manager.c:find_session function:
> >
> > ---
> > diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> > index 8f62a30..28b890c 100644
> > --- a/obexd/client/manager.c
> > +++ b/obexd/client/manager.c
> > @@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
> > *path)
> >
> > for (l = sessions; l; l = l->next) {
> > struct obc_session *session = l->data;
> > + const char *session_path = obc_session_get_path(session);
> >
> > - if (g_str_equal(obc_session_get_path(session), path) ==
> > TRUE)
> > + if (session_path != NULL && g_str_equal(session_path, path)
> > == TRUE)
> > return session;
> > }
> > ---
>
> You can use g_strcmp0 which checks for NULL, gonna take a look why the
> session path is NULL perhaps we should not even add to the session
> list until the connection completes and it is properly registered.
I think you are right - session with no path should not be placed on the
sessions list because it is not usable in such case. Below is a patch
that works for me, here's a summary of the changes:
- move g_slist_append from create_session to create_callback so the
session is added to sessions list after it has been registered
- split shutdown_session into shutdown_session and release_session; the
shutdown_session performs the shutdown and unref on the session but does
not try to remove session from the list; the release_session removes the
session from the list and calls release_shutdown
- modify create_callback so it calls shutdown_session for failures
before the session has been registered (so it doesn't try to remove
session from the list since the session is not on the list)
- modify remove_session to call release_session in order to remove the
session from list and do shutdown
---
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..118dd48 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -59,11 +59,16 @@ static GSList *sessions = NULL;
static void shutdown_session(struct obc_session *session)
{
- sessions = g_slist_remove(sessions, session);
obc_session_shutdown(session);
obc_session_unref(session);
}
+static void release_session(struct obc_session *session)
+{
+ sessions = g_slist_remove(sessions, session);
+ shutdown_session(session);
+}
+
static void unregister_session(void *data)
{
struct obc_session *session = data;
@@ -94,6 +99,16 @@ static void create_callback(struct obc_session
*session,
path = obc_session_register(session, unregister_session);
+ if(path == NULL) {
+ DBusMessage *error = g_dbus_create_error(data->message,
+ ERROR_INTERFACE ".Failed",
+ NULL);
+ g_dbus_send_message(data->connection, error);
+ shutdown_session(session);
+ goto done;
+ }
+
+ sessions = g_slist_append(sessions, session);
g_dbus_send_reply(data->connection, data->message,
DBUS_TYPE_OBJECT_PATH, &path,
DBUS_TYPE_INVALID);
@@ -190,7 +205,6 @@ static DBusMessage *create_session(DBusConnection
*connection,
dbus_message_get_sender(message),
create_callback, data);
if (session != NULL) {
- sessions = g_slist_append(sessions, session);
return NULL;
}
@@ -224,7 +238,7 @@ static DBusMessage *remove_session(DBusConnection
*connection,
ERROR_INTERFACE ".NotAuthorized",
"Not Authorized");
- shutdown_session(session);
+ release_session(session);
return dbus_message_new_method_return(message);
}
---
Marcin.
^ permalink raw reply related
* Re: bluez-5.0 dropped --with-ouifile=PATH
From: Pacho Ramos @ 2012-12-30 20:12 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1356897940.19248.87.camel@aeonflux>
[-- Attachment #1: Type: text/plain, Size: 901 bytes --]
El dom, 30-12-2012 a las 12:05 -0800, Marcel Holtmann escribió:
> Hi Pacho,
>
> > > > That option is needed on Gentoo to let it catch oui.txt file present
> > > > on /usr/share/misc/oui.txt, could it be re-added to prevent us from
> > > > needing to sed src/oui.c looking for that file in our location?
> > >
> > > or you build against a recent version of udev that comes with hwdb
> > > support. Then it will actually start using the efficient version of
> > > reading the OUI values from this file.
> > >
> > > There is no plan to keep support for oui.txt for very long. It is still
> > > supported as transition period, but that is about it.
> >
> > And what is replacing oui.txt? udev directly?
>
> as I said above, udev's hwdb. Check udevadm hwdb for a command line
> example too. Also src/oui.c is pretty obvious in that regard.
>
> Regards
>
> Marcel
OK, thanks
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: bluez-5.0 dropped --with-ouifile=PATH
From: Marcel Holtmann @ 2012-12-30 20:05 UTC (permalink / raw)
To: Pacho Ramos; +Cc: linux-bluetooth
In-Reply-To: <1356897497.19380.0.camel@belkin4>
Hi Pacho,
> > > That option is needed on Gentoo to let it catch oui.txt file present
> > > on /usr/share/misc/oui.txt, could it be re-added to prevent us from
> > > needing to sed src/oui.c looking for that file in our location?
> >
> > or you build against a recent version of udev that comes with hwdb
> > support. Then it will actually start using the efficient version of
> > reading the OUI values from this file.
> >
> > There is no plan to keep support for oui.txt for very long. It is still
> > supported as transition period, but that is about it.
>
> And what is replacing oui.txt? udev directly?
as I said above, udev's hwdb. Check udevadm hwdb for a command line
example too. Also src/oui.c is pretty obvious in that regard.
Regards
Marcel
^ permalink raw reply
* Re: bluez-5.0 dropped --with-ouifile=PATH
From: Pacho Ramos @ 2012-12-30 19:58 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1356892983.19248.82.camel@aeonflux>
[-- Attachment #1: Type: text/plain, Size: 704 bytes --]
El dom, 30-12-2012 a las 10:43 -0800, Marcel Holtmann escribió:
> Hi Pacho,
>
> > That option is needed on Gentoo to let it catch oui.txt file present
> > on /usr/share/misc/oui.txt, could it be re-added to prevent us from
> > needing to sed src/oui.c looking for that file in our location?
>
> or you build against a recent version of udev that comes with hwdb
> support. Then it will actually start using the efficient version of
> reading the OUI values from this file.
>
> There is no plan to keep support for oui.txt for very long. It is still
> supported as transition period, but that is about it.
>
> Regards
>
> Marcel
>
And what is replacing oui.txt? udev directly?
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH BlueZ 00/11] Add experimental command line switch option
From: Marcel Holtmann @ 2012-12-30 19:02 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1356699069-5469-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
> These set of patches introduces g_dbus_set_flags along with experimental flags,
> g_dbus_set_flags is not per connection as originally suggested to avoid using
> dbus_connection_allocate_data_slot which requires calling
> dbus_connection_free_data_slot once done but currently we don't keep any data
> associated with connections, besides the flags comes from command line options
> which would end up setting the same flags for every connection anyway.
>
> In addition to experimental it also add support to enable deprecated via compat
> swith option, this makes any API marked as deprecated to be disabled if compat
> is not set. This is on purpose to expose applications using deprecated APIs.
besides the deprecated switch, all patches have been applied.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 06/11] gdbus: Introduce G_DBUS_FLAG_ENABLE_DEPRECATED
From: Marcel Holtmann @ 2012-12-30 18:52 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1356699069-5469-7-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
> This flag can be set using g_dbus_set_flags to enable deprecated
> interfaces, default is unset.
> ---
> gdbus/gdbus.h | 3 ++-
> gdbus/object.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+), 1 deletion(-)
I decided against this patch. At least for now. The whole point of the
interface versioning is to support two API versions. So having an option
to manually enable them makes no sense. We can revisit this once we
reach that point, but otherwise I leave this out for now.
Regards
Marcel
^ permalink raw reply
* Re: bluez-5.0 dropped --with-ouifile=PATH
From: Marcel Holtmann @ 2012-12-30 18:43 UTC (permalink / raw)
To: Pacho Ramos; +Cc: linux-bluetooth
In-Reply-To: <1356878136.18481.1.camel@belkin4>
Hi Pacho,
> That option is needed on Gentoo to let it catch oui.txt file present
> on /usr/share/misc/oui.txt, could it be re-added to prevent us from
> needing to sed src/oui.c looking for that file in our location?
or you build against a recent version of udev that comes with hwdb
support. Then it will actually start using the efficient version of
reading the OUI values from this file.
There is no plan to keep support for oui.txt for very long. It is still
supported as transition period, but that is about it.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function
From: Anderson Lizardo @ 2012-12-30 14:37 UTC (permalink / raw)
To: Giovanni Gherdovich; +Cc: linux-bluetooth
In-Reply-To: <1356862632-5938-1-git-send-email-g.gherdovich@gmail.com>
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
^ permalink raw reply
* bluez-5.0 dropped --with-ouifile=PATH
From: Pacho Ramos @ 2012-12-30 14:35 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 220 bytes --]
That option is needed on Gentoo to let it catch oui.txt file present
on /usr/share/misc/oui.txt, could it be re-added to prevent us from
needing to sed src/oui.c looking for that file in our location?
Thanks a lot
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: SEGFAULT in obexd manager.c:find_session
From: Luiz Augusto von Dentz @ 2012-12-30 14:28 UTC (permalink / raw)
To: Marcin Zawiejski; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <50E03F0D.6020605@gmail.com>
Hi Marcin,
On Sun, Dec 30, 2012 at 3:18 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> Hi, I think there is a bug in obexd in manager.c:find_session function.
>
> What happens here is a segfault when manager.c:find_session calls
> g_str_equal(obc_session_get_path(session), path). This is caused by the
> sessions list having a session with a NULL path.
>
> Basically when I call manager.c:create_session, the session created there is
> added to sessions list but it has a NULL path until the
> manager.c:create_callback is called.
>
> However the manager.c:create_callback is not called at all if the remote
> device refuses the connection. So when manager.c:find_session is called, it
> actually calls the g_str_equal(NULL, path) causing the segfault.
>
> This might be simply fixed by modifying the manager.c:find_session to check
> for a NULL session path before calling g_str_equal(...).
>
> The problem is reproducible by having two sessions, with one awaiting
> connection and another one with an active file transfer. When the file
> transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
> obexd segfaults since the session awaiting connection has a NULL path.
>
> I'm not sure if the session with a NULL path should be on the sessions list
> or not. If its okay, then here's a simple patch for the
> manager.c:find_session function:
>
> ---
> diff --git a/obexd/client/manager.c b/obexd/client/manager.c
> index 8f62a30..28b890c 100644
> --- a/obexd/client/manager.c
> +++ b/obexd/client/manager.c
> @@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
> *path)
>
> for (l = sessions; l; l = l->next) {
> struct obc_session *session = l->data;
> + const char *session_path = obc_session_get_path(session);
>
> - if (g_str_equal(obc_session_get_path(session), path) ==
> TRUE)
> + if (session_path != NULL && g_str_equal(session_path, path)
> == TRUE)
> return session;
> }
> ---
You can use g_strcmp0 which checks for NULL, gonna take a look why the
session path is NULL perhaps we should not even add to the session
list until the connection completes and it is properly registered.
--
Luiz Augusto von Dentz
^ permalink raw reply
* SEGFAULT in obexd manager.c:find_session
From: Marcin Zawiejski @ 2012-12-30 13:18 UTC (permalink / raw)
To: linux-bluetooth
Hi, I think there is a bug in obexd in manager.c:find_session function.
What happens here is a segfault when manager.c:find_session calls
g_str_equal(obc_session_get_path(session), path). This is caused by the
sessions list having a session with a NULL path.
Basically when I call manager.c:create_session, the session created
there is added to sessions list but it has a NULL path until the
manager.c:create_callback is called.
However the manager.c:create_callback is not called at all if the remote
device refuses the connection. So when manager.c:find_session is called,
it actually calls the g_str_equal(NULL, path) causing the segfault.
This might be simply fixed by modifying the manager.c:find_session to
check for a NULL session path before calling g_str_equal(...).
The problem is reproducible by having two sessions, with one awaiting
connection and another one with an active file transfer. When the file
transfer errors and I call org.bluez.obex.Client1 RemoveSession then the
obexd segfaults since the session awaiting connection has a NULL path.
I'm not sure if the session with a NULL path should be on the sessions
list or not. If its okay, then here's a simple patch for the
manager.c:find_session function:
---
diff --git a/obexd/client/manager.c b/obexd/client/manager.c
index 8f62a30..28b890c 100644
--- a/obexd/client/manager.c
+++ b/obexd/client/manager.c
@@ -142,8 +142,9 @@ static struct obc_session *find_session(const char
*path)
for (l = sessions; l; l = l->next) {
struct obc_session *session = l->data;
+ const char *session_path = obc_session_get_path(session);
- if (g_str_equal(obc_session_get_path(session), path) ==
TRUE)
+ if (session_path != NULL && g_str_equal(session_path,
path) == TRUE)
return session;
}
---
Marcin.
^ permalink raw reply related
* Re: inconsistency in configure.ac for BlueZ 5.0 : GLib 2.32 required
From: Giovanni Gherdovich @ 2012-12-30 10:25 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1356826885.19248.79.camel@aeonflux>
Hello Marcel,
2012/12/30 Marcel Holtmann <marcel@holtmann.org>:
>
> I prefer to keep it at GLib 2.28 for now. So please send a patch to
> workaround that issue for the missing g_queue_free_full.
OK, I just sent it.
Regards,
Giovanni
^ permalink raw reply
* [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function
From: Giovanni Gherdovich @ 2012-12-30 10:17 UTC (permalink / raw)
To: linux-bluetooth; +Cc: 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
^ permalink raw reply related
* BLE multiple connection - Recv() issue
From: Ajay @ 2012-12-30 1:25 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 695 bytes --]
hi,
I have 2 LE links A1 to B and A2 to C ( A1 and A2 are client
running at same device )
when i starts sending data from B to A1 its happening and A1 is
receiving properly . But the very next moment i started data transfer
from C to A2 , A1 stops recievng . I could see data from B to A1
reaches upto hci layer(hcidump) , and then routed to A2 l2cap recv() .
Here i used recv() and send() system calls for data transfer with l2cap
socket (cid 0x04 and psm 0x00) .Bind() is done at both client and server
side . Is it possible for a BLE device to recv data simultaneously from
other two devices .????? .
--
Thanks & Regards
AJAY KV
GlobalEdge software Ltd
8892753703
[-- Attachment #2: ajay_kv.vcf --]
[-- Type: text/x-vcard, Size: 74 bytes --]
begin:vcard
fn:AJAY KV
n:;AJAY
tel;cell:8892753703
version:2.1
end:vcard
^ permalink raw reply
* Re: inconsistency in configure.ac for BlueZ 5.0 : GLib 2.32 required
From: Marcel Holtmann @ 2012-12-30 0:21 UTC (permalink / raw)
To: Giovanni Gherdovich; +Cc: linux-bluetooth
In-Reply-To: <CAJN7YmO4th=T2u0s9ya6fEwjO5RdEiVZCfyhTHRLb94u9Q2KeA@mail.gmail.com>
Hi Giovanni,
> I believe there is a little inconsistency in configure.ac
> of BlueZ 5.0:
>
> it requires GLib 2.28, but actually the files
I prefer to keep it at GLib 2.28 for now. So please send a patch to
workaround that issue for the missing g_queue_free_full.
Regards
Marcel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox