* Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function
From: Luiz Augusto von Dentz @ 2013-01-02 20:35 UTC (permalink / raw)
To: Vinicius Gomes
Cc: Giovanni Gherdovich, Anderson Lizardo,
linux-bluetooth@vger.kernel.org
In-Reply-To: <CAE6HaMmrc9BEW-cjmw3fkZptCzvr14M0GtKwoKXuqi8or1zeWg@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function
From: Vinicius Gomes @ 2013-01-02 20:23 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Giovanni Gherdovich, Anderson Lizardo,
linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJxF6H7V-e=6wMYgQSM4C7FfXW9FCrdRZ=B6U1YgLQSSQ@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 1/1] adapter, AVCTP: Replaced calls to g_queue_free_full function
From: Luiz Augusto von Dentz @ 2013-01-02 19:58 UTC (permalink / raw)
To: Giovanni Gherdovich; +Cc: Anderson Lizardo, linux-bluetooth@vger.kernel.org
In-Reply-To: <CAJN7YmNzo1ya34tdgyffcBf=SLt4RsJ=2QaSy3fD6meKx7wnsA@mail.gmail.com>
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
^ permalink raw reply
* FLUSHING ACL PACKETS
From: Ajay @ 2013-01-02 18:47 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 455 bytes --]
Hi ,
I wanted to moniter the acl flushed packet numbers. In my L2cap
client server code , i did the following steps
1. Enabled setsocket() option as BT_FLUSHABLE
2 . set automatic flush timeout as 50msec
On data transfer, i could see the flush occured event frequently on
hcidump . so how do i get the number of packets flushed???
kindly suggest me some solution...
--
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
* BLE multiple connection - Recv() issue
From: Ajay @ 2013-01-02 18:37 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <50DF97FB.7080700@globaledgesoft.com>
[-- Attachment #1: Type: text/plain, Size: 697 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: 81 bytes --]
begin:vcard
fn:AJAY KV
n:;AJAY
tel;cell:8892753703
version:2.1
end:vcard
^ permalink raw reply
* [RFC] Bluetooth: Allow getting SCO options for not connected sockets
From: Vinicius Costa Gomes @ 2013-01-02 14:47 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Vinicius Costa Gomes
Now, that we have proper support for deferred setup for SCO sockets it
is very convenient that we are able to get the socket options for
sockets that are not in the 'connected' state.
And makes the behaviour more consistent with what L2CAP does, for
example.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
Sending this as a RFC, because even though I couldn't think of any cases
that this new behaviour would surprise userspace applications, there
may be still be some.
net/bluetooth/sco.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 531a93d..271320c 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -730,11 +730,6 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname, char __user
switch (optname) {
case SCO_OPTIONS:
- if (sk->sk_state != BT_CONNECTED) {
- err = -ENOTCONN;
- break;
- }
-
opts.mtu = sco_pi(sk)->conn->mtu;
BT_DBG("mtu %d", opts.mtu);
--
1.8.0.2
^ permalink raw reply related
* Re: [PATCH] obexd: Fix crash while removing session
From: Luiz Augusto von Dentz @ 2013-01-02 14:27 UTC (permalink / raw)
To: Marcin Zawiejski; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1356964310.3001.1.camel@lsdevbox>
Hi Marcin,
On Mon, Dec 31, 2012 at 4:31 PM, Marcin Zawiejski <dragmz@gmail.com> wrote:
> 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.
Pushed, thanks
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix uuid output in debugfs
From: Johan Hedberg @ 2013-01-02 13:43 UTC (permalink / raw)
To: Bjørn Mork, Gustavo Padovan, linux-bluetooth
In-Reply-To: <20130102113122.GA22512@x220>
Hi,
On Wed, Jan 02, 2013, Johan Hedberg wrote:
> > > + u32 data0, data5;
> > > + u16 data1, data2, data3, data4;
> > > +
> > > + data5 = le32_to_cpu(*(__le32 *)uuid);
> > > + data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> > > + data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> > > + data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> > > + data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> > > + data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
> > > +
> > > + seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
> > > + data0, data1, data2, data3, data4, data5);
> > > }
> > >
> > > static int uuids_show(struct seq_file *f, void *p)
> >
> >
> > Why can't all this be replaced with
> >
> > static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> > {
> > seq_printf(f, "%pUl\n", uuid);
> > }
> >
> > ?
>
> I don't think there's any reason assuming that there are no unaligned
> access considerations (which I pointed out in my other reply). I wasn't
> aware of printk having such a nice extension to the usual format
> specifiers (and neither was Gustavo as it seems). Thanks for making us
> aware of it!
Actually I'm not getting the expected results with %pU. The following is
the output of seq_printf(f, "%pUl %pUb\n", uuid, uuid):
$ cat /sys/kernel/debug/bluetooth/hci0/uuids
5f9b34fb-0080-8000-0010-00000e110000 fb349b5f-8000-0080-0010-00000e110000
5f9b34fb-0080-8000-0010-00000c110000 fb349b5f-8000-0080-0010-00000c110000
5f9b34fb-0080-8000-0010-000004a00000 fb349b5f-8000-0080-0010-000004a00000
5f9b34fb-0080-8000-0010-000001180000 fb349b5f-8000-0080-0010-000001180000
5f9b34fb-0080-8000-0010-000000180000 fb349b5f-8000-0080-0010-000000180000
5f9b34fb-0080-8000-0010-000000120000 fb349b5f-8000-0080-0010-000000120000
None of those are of the correct format which I do get with Gustavo's
patch:
$ cat /sys/kernel/debug/bluetooth/hci0/uuids
0000110e-0000-1000-8000-00805f9b34fb
0000110c-0000-1000-8000-00805f9b34fb
0000a004-0000-1000-8000-00805f9b34fb
00001801-0000-1000-8000-00805f9b34fb
00001800-0000-1000-8000-00805f9b34fb
00001200-0000-1000-8000-00805f9b34fb
So it seems to me that the %pU specifier is expecting something quite
different from how we store the UUIDs. Looking at the difference of %pUb
and %pUl it seems it doesn't do a complete byte order swap for the whole
value but only for half of it, and even that half doesn't see a proper
64-bit byte order swap but just internal swaps for the one 32-bit and
two 16-bit parts.
So assuming that potential alignment issues get sorted out it seems that
Gustavos patch is the way to go forward.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix uuid output in debugfs
From: Bjørn Mork @ 2013-01-02 12:37 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: linux-bluetooth
In-Reply-To: <20130102113122.GA22512@x220>
Johan Hedberg <johan.hedberg@gmail.com> writes:
>> Why can't all this be replaced with
>>
>> static void print_bt_uuid(struct seq_file *f, u8 *uuid)
>> {
>> seq_printf(f, "%pUl\n", uuid);
>> }
>>
>> ?
>
> I don't think there's any reason assuming that there are no unaligned
> access considerations (which I pointed out in my other reply). I wasn't
> aware of printk having such a nice extension to the usual format
> specifiers (and neither was Gustavo as it seems). Thanks for making us
> aware of it!
lib/vsprintf.c:uuid_string() will only access "uuid" byte-by-byte so
alignment shouldn't be a problem.
Bjørn
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix uuid output in debugfs
From: Johan Hedberg @ 2013-01-02 11:31 UTC (permalink / raw)
To: Bjørn Mork; +Cc: Gustavo Padovan, linux-bluetooth
In-Reply-To: <87623g9chw.fsf@nemi.mork.no>
Hi Bjørn,
On Wed, Jan 02, 2013, Bjørn Mork wrote:
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 55cceee..05b78c7 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
> >
> > static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> > {
> > - __be32 data0, data4;
> > - __be16 data1, data2, data3, data5;
> > -
> > - memcpy(&data0, &uuid[0], 4);
> > - memcpy(&data1, &uuid[4], 2);
> > - memcpy(&data2, &uuid[6], 2);
> > - memcpy(&data3, &uuid[8], 2);
> > - memcpy(&data4, &uuid[10], 4);
> > - memcpy(&data5, &uuid[14], 2);
> > -
> > - seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
> > - ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
> > - ntohl(data4), ntohs(data5));
> > + u32 data0, data5;
> > + u16 data1, data2, data3, data4;
> > +
> > + data5 = le32_to_cpu(*(__le32 *)uuid);
> > + data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> > + data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> > + data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> > + data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> > + data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
> > +
> > + seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
> > + data0, data1, data2, data3, data4, data5);
> > }
> >
> > static int uuids_show(struct seq_file *f, void *p)
>
>
> Why can't all this be replaced with
>
> static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> {
> seq_printf(f, "%pUl\n", uuid);
> }
>
> ?
I don't think there's any reason assuming that there are no unaligned
access considerations (which I pointed out in my other reply). I wasn't
aware of printk having such a nice extension to the usual format
specifiers (and neither was Gustavo as it seems). Thanks for making us
aware of it!
Johan
^ permalink raw reply
* Re: Re: Specifying destination path in obexd/bluez-5
From: Alex Fiestas @ 2013-01-02 10:51 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZK6YAgZyxBa1a9kKFAHjy+dE7Td518CB393_9Y+RWPKkw@mail.gmail.com>
On Monday 31 December 2012 12:01:04 you wrote:
> 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.
Oh indeed, one thing less to worry then.
> > 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.
Only changing the root folder will be enough, we are already using links for
the rest.
With that, we should be able to port KDE bluetooth support to obexd-server
being the first step towards using BlueZ-5
Thanks !
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix uuid output in debugfs
From: Johan Hedberg @ 2013-01-02 9:27 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: linux-bluetooth, Gustavo Padovan
In-Reply-To: <1356130695-29342-1-git-send-email-gustavo@padovan.org>
Hi Gustavo,
On Fri, Dec 21, 2012, Gustavo Padovan wrote:
> The uuid should be printed in the CPU endianness and not in little-endian.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> net/bluetooth/hci_sysfs.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 55cceee..05b78c7 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
>
> static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> {
> - __be32 data0, data4;
> - __be16 data1, data2, data3, data5;
> -
> - memcpy(&data0, &uuid[0], 4);
> - memcpy(&data1, &uuid[4], 2);
> - memcpy(&data2, &uuid[6], 2);
> - memcpy(&data3, &uuid[8], 2);
> - memcpy(&data4, &uuid[10], 4);
> - memcpy(&data5, &uuid[14], 2);
> -
> - seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
> - ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
> - ntohl(data4), ntohs(data5));
> + u32 data0, data5;
> + u16 data1, data2, data3, data4;
> +
> + data5 = le32_to_cpu(*(__le32 *)uuid);
> + data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> + data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> + data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> + data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> + data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
This looks prone to unaligned access violations if the "u8 *uuid"
pointer doesn't start off with a nicely aligned address. The use of the
unaligned getter macros would also look nicer since you wouldn't have to
do explicit type casting.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix uuid output in debugfs
From: Bjørn Mork @ 2013-01-02 9:10 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: linux-bluetooth
In-Reply-To: <1356130624-29234-1-git-send-email-gustavo@padovan.org>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 55cceee..05b78c7 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -461,19 +461,18 @@ static const struct file_operations blacklist_fops = {
>
> static void print_bt_uuid(struct seq_file *f, u8 *uuid)
> {
> - __be32 data0, data4;
> - __be16 data1, data2, data3, data5;
> -
> - memcpy(&data0, &uuid[0], 4);
> - memcpy(&data1, &uuid[4], 2);
> - memcpy(&data2, &uuid[6], 2);
> - memcpy(&data3, &uuid[8], 2);
> - memcpy(&data4, &uuid[10], 4);
> - memcpy(&data5, &uuid[14], 2);
> -
> - seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x\n",
> - ntohl(data0), ntohs(data1), ntohs(data2), ntohs(data3),
> - ntohl(data4), ntohs(data5));
> + u32 data0, data5;
> + u16 data1, data2, data3, data4;
> +
> + data5 = le32_to_cpu(*(__le32 *)uuid);
> + data4 = le16_to_cpu(*(__le16 *)(uuid + 4));
> + data3 = le16_to_cpu(*(__le16 *)(uuid + 6));
> + data2 = le16_to_cpu(*(__le16 *)(uuid + 8));
> + data1 = le16_to_cpu(*(__le16 *)(uuid + 10));
> + data0 = le32_to_cpu(*(__le32 *)(uuid + 12));
> +
> + seq_printf(f, "%.8x-%.4x-%.4x-%.4x-%.4x%.8x\n",
> + data0, data1, data2, data3, data4, data5);
> }
>
> static int uuids_show(struct seq_file *f, void *p)
Why can't all this be replaced with
static void print_bt_uuid(struct seq_file *f, u8 *uuid)
{
seq_printf(f, "%pUl\n", uuid);
}
?
Bjørn
^ permalink raw reply
* Re: [PATCH 2/2] AVCTP: Replace calls to g_queue_free_full function
From: Marcel Holtmann @ 2013-01-02 1:19 UTC (permalink / raw)
To: Giovanni Gherdovich; +Cc: linux-bluetooth
In-Reply-To: <1357039308-8913-1-git-send-email-g.gherdovich@gmail.com>
Hi Giovanni,
> 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);On Tue, 2013-01-01 at 12:21 +0100, Giovanni Gherdovich wrote:
Same here. Provide a proper pending_destroy. The other places can just
call it with NULL as second parameter.
> + g_queue_free(chan->queue);
> g_slist_free_full(chan->processed, pending_destroy);
> g_slist_free_full(chan->handlers, g_free);
> g_free(chan);
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/2] core: Replace calls to g_queue_free_full function
From: Marcel Holtmann @ 2013-01-02 1:18 UTC (permalink / raw)
To: Giovanni Gherdovich; +Cc: linux-bluetooth
In-Reply-To: <1357039294-8878-1-git-send-email-g.gherdovich@gmail.com>
Hi Giovanni,
> 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);
I do not like the usage of (GFunc) casting here. Please add a function
that properly frees this and has the right signature.
> + g_queue_free(adapter->auths);
>
> sdp_list_free(adapter->services, NULL);
>
Regards
Marcel
^ permalink raw reply
* [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
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