* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 21:15 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131216205858.GA20119@sottospazio.it>
[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]
On Mon, Dec 16, 2013 at 09:58:58PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
> > On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> > > On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> > > >
> > > > This solution is acceptable to me, but I think the comment should briefly
> > > > explain why this fix is necessary, and the changelog should explain why in detail.
> > > >
> > > > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > > > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > > > conditionally release on RFCOMM_RELEASE_ONHUP.
> > > >
> > > > Because then:
> > > > 1) this fix would not be necessary.
> > > > 2) the release in rfcomm_tty_hangup() would not be necessary
> > > > 3) the second release in rfcomm_release_dev would not be necessary
> > > > 4) the RFCOMM_TTY_RELEASED bit could be removed
> > > >
> > > >
> > > > Regards,
> > > > Peter Hurley
> > >
> > > Taking over the refcount in the install method would certainly look better. I'm
> > > going to test it ASAP :D
> > >
> > > But why getting rid of the release in in rfcomm_tty_hangup()?
> > > We could lose the bluetooth connection at any time and the dlc callback
> > > would have to hangup the tty (and release the port if necessary).
> > >
> > > Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> > > created without the RFCOMM_RELEASE_ONHUP flag.
> > >
> > > Besides any process could release the port behind us (with the command rfcomm
> > > release rfcomm1 for example).
> > >
> > > Gianluca
> >
> > Nevermind I figured it out the reason...
>
> I'm testing the attached patch ATM, which does what you described. It works
> very well.
>
> It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
> that bit.
ok, replying to myself again (sorry for that). RFCOMM_TTY_RELEASE cannot go
away. We have still to manage the case where the port is opened without
RFCOMM_RELEASE_ONHUP.
This last patch does release the port in that situation.
Tested with:
# rfcomm bind 1 <addr>
# rfcomm release 1
and with
# rfcomm connect 1 <addr>
Thanks,
Gianluca
[-- Attachment #2: rfc3.patch --]
[-- Type: text/x-diff, Size: 1319 bytes --]
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..0357dcf 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,7 +437,8 @@ static int rfcomm_release_dev(void __user *arg)
tty_kref_put(tty);
}
- if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+ if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
+ !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
tty_port_put(&dev->port);
tty_port_put(&dev->port);
@@ -673,6 +674,14 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
if (err)
rfcomm_tty_cleanup(tty);
+ /* take over the tty_port reference if it was created with the
+ * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+ * when the last process closes the tty. This behaviour is expected by
+ * userspace.
+ */
+ if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+ tty_port_put(&dev->port);
+
return err;
}
@@ -1010,10 +1019,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
BT_DBG("tty %p dev %p", tty, dev);
tty_port_hangup(&dev->port);
-
- if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
- !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
}
static int rfcomm_tty_tiocmget(struct tty_struct *tty)
^ permalink raw reply related
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 20:58 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131216202720.GB19746@sottospazio.it>
[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]
On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> > On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> > >
> > > This solution is acceptable to me, but I think the comment should briefly
> > > explain why this fix is necessary, and the changelog should explain why in detail.
> > >
> > > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > > conditionally release on RFCOMM_RELEASE_ONHUP.
> > >
> > > Because then:
> > > 1) this fix would not be necessary.
> > > 2) the release in rfcomm_tty_hangup() would not be necessary
> > > 3) the second release in rfcomm_release_dev would not be necessary
> > > 4) the RFCOMM_TTY_RELEASED bit could be removed
> > >
> > >
> > > Regards,
> > > Peter Hurley
> >
> > Taking over the refcount in the install method would certainly look better. I'm
> > going to test it ASAP :D
> >
> > But why getting rid of the release in in rfcomm_tty_hangup()?
> > We could lose the bluetooth connection at any time and the dlc callback
> > would have to hangup the tty (and release the port if necessary).
> >
> > Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> > created without the RFCOMM_RELEASE_ONHUP flag.
> >
> > Besides any process could release the port behind us (with the command rfcomm
> > release rfcomm1 for example).
> >
> > Gianluca
>
> Nevermind I figured it out the reason...
I'm testing the attached patch ATM, which does what you described. It works
very well.
It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
that bit.
Does it look better?
Thanks,
Gianluca
[-- Attachment #2: rfc2.patch --]
[-- Type: text/x-diff, Size: 1221 bytes --]
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..f455a22 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,9 +437,6 @@ static int rfcomm_release_dev(void __user *arg)
tty_kref_put(tty);
}
- if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
-
tty_port_put(&dev->port);
return 0;
}
@@ -673,6 +670,14 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
if (err)
rfcomm_tty_cleanup(tty);
+ /* take over the tty_port reference if it was created with the
+ * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+ * when the last process closes the tty. This behaviour is expected by
+ * userspace.
+ */
+ if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+ tty_port_put(&dev->port);
+
return err;
}
@@ -1010,10 +1015,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
BT_DBG("tty %p dev %p", tty, dev);
tty_port_hangup(&dev->port);
-
- if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
- !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
}
static int rfcomm_tty_tiocmget(struct tty_struct *tty)
^ permalink raw reply related
* Re: [patch] Bluetooth: fix ->alloc_skb() error checking
From: Dan Carpenter @ 2013-12-16 20:57 UTC (permalink / raw)
To: Anderson Lizardo
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, David S. Miller,
BlueZ development, netdev, kernel-janitors
In-Reply-To: <CAJdJm_O9q3YgpjTQexbLvMm8ViViu+MrmRCqPwQDDUzaxVQYUg@mail.gmail.com>
On Mon, Dec 16, 2013 at 04:35:25PM -0400, Anderson Lizardo wrote:
> Hi Dan,
>
> On Mon, Dec 16, 2013 at 4:28 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > @@ -2413,8 +2413,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
> >
> > skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> > msg->msg_flags & MSG_DONTWAIT);
> > - if (IS_ERR(skb))
> > - return skb;
> > + if (skb)
> > + return ERR_PTR(-ENOMEM);
>
> It should be "!skb" above right?
>
Gar.... I'm so sorry about that.
regards,
dan carpenter
^ permalink raw reply
* Re: [patch] Bluetooth: fix ->alloc_skb() error checking
From: Anderson Lizardo @ 2013-12-16 20:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, David S. Miller,
BlueZ development, netdev, kernel-janitors
In-Reply-To: <20131216202857.GB19601@elgon.mountain>
Hi Dan,
On Mon, Dec 16, 2013 at 4:28 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> @@ -2413,8 +2413,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
>
> skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
> msg->msg_flags & MSG_DONTWAIT);
> - if (IS_ERR(skb))
> - return skb;
> + if (skb)
> + return ERR_PTR(-ENOMEM);
It should be "!skb" above right?
>
> skb->priority = priority;
>
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Lukasz Rymanowski @ 2013-12-16 20:33 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZ+d6c1ohwkj4Bqoa_=fN+aCT34LwJR1eE53xix_woPYow@mail.gmail.com>
Hi Luiz,
On Mon, Dec 16, 2013 at 7:31 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lukasz,
>
> On Mon, Dec 16, 2013 at 6:27 PM, Lukasz Rymanowski
> <lukasz.rymanowski@gmail.com> wrote:
>> Hi Luiz,
>>
>>
>> On Mon, Dec 16, 2013 at 2:08 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> This IPC is used to communicate Android BlueZ daemon and AudioFlinger
>>> plugin.
>>> ---
>>> v2: Rework IPC commands to match Android Audio HAL
>>>
>>> android/Makefile.am | 3 +-
>>> android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 87 insertions(+), 1 deletion(-)
>>> create mode 100644 android/audio-ipc-api.txt
>>>
>>> diff --git a/android/Makefile.am b/android/Makefile.am
>>> index 79f30d7..ac00bb2 100644
>>> --- a/android/Makefile.am
>>> +++ b/android/Makefile.am
>>> @@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
>>> android/pixit-gap.txt android/pixit-hid.txt \
>>> android/pixit-opp.txt android/pixit-pan.txt \
>>> android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
>>> - android/pts-opp.txt android/pts-pbap.txt
>>> + android/pts-opp.txt android/pts-pbap.txt \
>>> + android/audio-ipc-api.txt
>>> diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
>>> new file mode 100644
>>> index 0000000..e9a2136
>>> --- /dev/null
>>> +++ b/android/audio-ipc-api.txt
>>> @@ -0,0 +1,85 @@
>>> +Bluetooth Audio Plugin
>>> +======================
>>> +
>>> +The audio plugin happen to be in a different socket but all the rules for
>>> +HAL socket apply here as well, the abstract socket name is
>>> +"\0bluez_audio_socket" (tentative):
>>> +
>>> + .--Android--. .---Audio---.
>>> + | daemon | | Plugin |
>>> + | | Command | |
>>> + | | <-------------------------- | |
>>> + | | | |
>>> + | | --------------------------> | |
>>> + | | Response | |
>>> + | | | |
>>> + | | | |
>>> + | | | |
>>> + '-----------' '-----------'
>>> +
>>> +
>>> + Audio HAL Daemon
>>> + ----------------------------------------------------
>>> +
>>> + call dev->open() --> command 0x01
>>> + return dev->open() <-- response 0x01
>>> +
>>> + call dev->open_output_stream() --> command 0x03
>>> + return dev->open_output_stream() <-- response 0x03
>>> +
>>> + call stream_in->read() --> command 0x05
>>> + return stream_in->read() <-- response 0x05
>>> +
>> I think it should be stream_out->write() here.
>>
>>> + call stream_in->common.standby() --> command 0x06
>>> + return stream_in->common.standby() <-- response 0x06
>>> +
>> Also here: stream_out->common.standby()
>
> I guess it should work both ways, but perhaps Android is never meant
> to sink role nevertheless I would not limit the IPC to just source as
> it could be useful and it doesn't need that much to work.
>
I agree that it should work for stream in as well, and actually I can
imagine product based on Android acting as SINK on A2DP.
Anyway, maybe it would be better to point it out directly and keep HAL
calls example for stream out.
/Łukasz
> --
> Luiz Augusto von Dentz
^ permalink raw reply
* [patch] Bluetooth: fix ->alloc_skb() error checking
From: Dan Carpenter @ 2013-12-16 20:28 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo Padovan, Johan Hedberg, David S. Miller, linux-bluetooth,
netdev, kernel-janitors
There are two functions that implement ->alloc_skb().
a2mp_cahan_alloc_skb_cb() returns NULL.
l2cap_sock_alloc_skb_cb() returns an ERR_PTR.
The callers all check for ERR_PTRs and don't check for NULL. On the
other hand bt_skb_alloc() and the core net alloc_skb() return NULL so
returning an error pointer is inconsistent. This confusion between some
alloc_skb() functions returning ERR_PTR and some returning NULL has been
a source of bugs such as 787949039fcd ('Bluetooth: fix return value
check'). This patch makes ->alloc_skb() return NULL and changes the
callers to match.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b0ad2c752d73..84c3ce04cb35 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2341,8 +2341,8 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
tmp = chan->ops->alloc_skb(chan, count,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(tmp))
- return PTR_ERR(tmp);
+ if (!tmp)
+ return -ENOMEM;
*frag = tmp;
@@ -2379,8 +2379,8 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + hlen,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
skb->priority = priority;
@@ -2413,8 +2413,8 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + L2CAP_HDR_SIZE,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (skb)
+ return ERR_PTR(-ENOMEM);
skb->priority = priority;
@@ -2457,8 +2457,8 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + hlen,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
/* Create L2CAP header */
lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
@@ -2578,8 +2578,8 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan,
skb = chan->ops->alloc_skb(chan, count + hlen,
msg->msg_flags & MSG_DONTWAIT);
- if (IS_ERR(skb))
- return skb;
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
/* Create L2CAP header */
lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
^ permalink raw reply related
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 20:27 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131216202044.GA19746@sottospazio.it>
On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> >
> > This solution is acceptable to me, but I think the comment should briefly
> > explain why this fix is necessary, and the changelog should explain why in detail.
> >
> > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > conditionally release on RFCOMM_RELEASE_ONHUP.
> >
> > Because then:
> > 1) this fix would not be necessary.
> > 2) the release in rfcomm_tty_hangup() would not be necessary
> > 3) the second release in rfcomm_release_dev would not be necessary
> > 4) the RFCOMM_TTY_RELEASED bit could be removed
> >
> >
> > Regards,
> > Peter Hurley
>
> Taking over the refcount in the install method would certainly look better. I'm
> going to test it ASAP :D
>
> But why getting rid of the release in in rfcomm_tty_hangup()?
> We could lose the bluetooth connection at any time and the dlc callback
> would have to hangup the tty (and release the port if necessary).
>
> Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> created without the RFCOMM_RELEASE_ONHUP flag.
>
> Besides any process could release the port behind us (with the command rfcomm
> release rfcomm1 for example).
>
> Gianluca
Nevermind I figured it out the reason...
^ permalink raw reply
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Gianluca Anzolin @ 2013-12-16 20:20 UTC (permalink / raw)
To: Peter Hurley
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <52AF55B4.6000303@hurleysoftware.com>
On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
>
> This solution is acceptable to me, but I think the comment should briefly
> explain why this fix is necessary, and the changelog should explain why in detail.
>
> Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> conditionally release on RFCOMM_RELEASE_ONHUP.
>
> Because then:
> 1) this fix would not be necessary.
> 2) the release in rfcomm_tty_hangup() would not be necessary
> 3) the second release in rfcomm_release_dev would not be necessary
> 4) the RFCOMM_TTY_RELEASED bit could be removed
>
>
> Regards,
> Peter Hurley
Taking over the refcount in the install method would certainly look better. I'm
going to test it ASAP :D
But why getting rid of the release in in rfcomm_tty_hangup()?
We could lose the bluetooth connection at any time and the dlc callback
would have to hangup the tty (and release the port if necessary).
Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
created without the RFCOMM_RELEASE_ONHUP flag.
Besides any process could release the port behind us (with the command rfcomm
release rfcomm1 for example).
Gianluca
^ permalink raw reply
* Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
From: Peter Hurley @ 2013-12-16 19:34 UTC (permalink / raw)
To: Gianluca Anzolin
Cc: Alexander Holler, marcel, Gustavo Padovan, linux-bluetooth,
gregkh, jslaby, linux-kernel
In-Reply-To: <20131215150847.GA10288@sottospazio.it>
On 12/15/2013 10:08 AM, Gianluca Anzolin wrote:
> On Sun, Dec 15, 2013 at 09:03:35AM -0500, Peter Hurley wrote:
>> On 12/15/2013 06:24 AM, Gianluca Anzolin wrote:
>>> On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote:
>>>> Am 12.12.2013 21:36, schrieb Peter Hurley:
>>>>
>>>>>> What currently happens is that when one kills rfcomm (and any other
>>>>>> terminal which might use that tty), the entry in /dev doesn't
>>>>>> disappear. That means the same call to refcomm with the same device
>>>>>> (e.g. [/dev/]rfcomm1 doesn't work.
>>>>>
>>>>> Thanks for the report, Alexander.
>>>>>
>>>>> Point 4 above details a different situation; something else is
>>>>> happening.
>>>>>
>>>>> Would you please detail the necessary steps to reproduce this regression?
>>>>> (How do you 'kill' rfcomm? etc. Shell command lines would be best.)
>>>>
>>>> Just call
>>>>
>>>> rfcomm connect rfcomm9 01:23:45:67:89:ab
>>>>
>>>> wait until the connection happened (a message will appear) and then
>>>> press ctrl-c. This still terminates the bluetooth connection, but the
>>>> device in /dev is now left.
>>>
>>> Yes I'm able to reproduce the regression which is indeed caused by that
>>> commit.
>>>
>>> However I'm puzzled. Surely there is a fifth case I didn't cover because
>>> when rfcomm_dev_state_change() is called, the tty_port is there but the tty is
>>> not, and therefore I cannot get a reference to it and send the HUP.
>>
>> There is a fifth case, but it's crazy.
>>
>> The tty has been properly shutdown and destroyed because the tty file handle
>> was closed, not hungup. The rfcomm device reference was properly put
>> when the tty was released.
>>
>> But when the remote hangs up (and sends disc), then rfcomm_dev_state_change()
>> is called -- to kill the port reference (thus the rfcomm device) that was
>> instantiated locally! Ridiculous. Doubly ridiculous because it's the local
>> port shutdown that closes the dlc locally that sends the disconnect (and sets
>> the local dlc state) that triggers the received rfcomm_dev_state_change()!
>>
>> If this behavior is desirable (or necessary because it's been exposed to
>> userspace), then why was the design ever reference-counted to begin with?
>>
>> Regards,
>> Peter Hurley
>
> The attached patch fixes the regression by releasing the tty_port in the
> shutdown method(). This way we can avoid strange games in the dlc callback
> where we are constrained by the dlc lock.
>
> If this kind of approach is acceptable I will submit the patch for inclusion in
> a separate email.
This solution is acceptable to me, but I think the comment should briefly
explain why this fix is necessary, and the changelog should explain why in detail.
Perhaps with a fixme comment that rfcomm_tty_install() should just take over
the port reference (instead of adding one) and rfcomm_tty_cleanup() should
conditionally release on RFCOMM_RELEASE_ONHUP.
Because then:
1) this fix would not be necessary.
2) the release in rfcomm_tty_hangup() would not be necessary
3) the second release in rfcomm_release_dev would not be necessary
4) the RFCOMM_TTY_RELEASED bit could be removed
Regards,
Peter Hurley
^ permalink raw reply
* Re: [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Luiz Augusto von Dentz @ 2013-12-16 18:31 UTC (permalink / raw)
To: Lukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CAN_7+YaOQLno_kZoi7sAk2f8Ky_natHGa1Zi-J9vErfZ5WW8ug@mail.gmail.com>
Hi Lukasz,
On Mon, Dec 16, 2013 at 6:27 PM, Lukasz Rymanowski
<lukasz.rymanowski@gmail.com> wrote:
> Hi Luiz,
>
>
> On Mon, Dec 16, 2013 at 2:08 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> This IPC is used to communicate Android BlueZ daemon and AudioFlinger
>> plugin.
>> ---
>> v2: Rework IPC commands to match Android Audio HAL
>>
>> android/Makefile.am | 3 +-
>> android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 87 insertions(+), 1 deletion(-)
>> create mode 100644 android/audio-ipc-api.txt
>>
>> diff --git a/android/Makefile.am b/android/Makefile.am
>> index 79f30d7..ac00bb2 100644
>> --- a/android/Makefile.am
>> +++ b/android/Makefile.am
>> @@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
>> android/pixit-gap.txt android/pixit-hid.txt \
>> android/pixit-opp.txt android/pixit-pan.txt \
>> android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
>> - android/pts-opp.txt android/pts-pbap.txt
>> + android/pts-opp.txt android/pts-pbap.txt \
>> + android/audio-ipc-api.txt
>> diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
>> new file mode 100644
>> index 0000000..e9a2136
>> --- /dev/null
>> +++ b/android/audio-ipc-api.txt
>> @@ -0,0 +1,85 @@
>> +Bluetooth Audio Plugin
>> +======================
>> +
>> +The audio plugin happen to be in a different socket but all the rules for
>> +HAL socket apply here as well, the abstract socket name is
>> +"\0bluez_audio_socket" (tentative):
>> +
>> + .--Android--. .---Audio---.
>> + | daemon | | Plugin |
>> + | | Command | |
>> + | | <-------------------------- | |
>> + | | | |
>> + | | --------------------------> | |
>> + | | Response | |
>> + | | | |
>> + | | | |
>> + | | | |
>> + '-----------' '-----------'
>> +
>> +
>> + Audio HAL Daemon
>> + ----------------------------------------------------
>> +
>> + call dev->open() --> command 0x01
>> + return dev->open() <-- response 0x01
>> +
>> + call dev->open_output_stream() --> command 0x03
>> + return dev->open_output_stream() <-- response 0x03
>> +
>> + call stream_in->read() --> command 0x05
>> + return stream_in->read() <-- response 0x05
>> +
> I think it should be stream_out->write() here.
>
>> + call stream_in->common.standby() --> command 0x06
>> + return stream_in->common.standby() <-- response 0x06
>> +
> Also here: stream_out->common.standby()
I guess it should work both ways, but perhaps Android is never meant
to sink role nevertheless I would not limit the IPC to just source as
it could be useful and it doesn't need that much to work.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Lukasz Rymanowski @ 2013-12-16 16:27 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1387199317-27438-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Mon, Dec 16, 2013 at 2:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This IPC is used to communicate Android BlueZ daemon and AudioFlinger
> plugin.
> ---
> v2: Rework IPC commands to match Android Audio HAL
>
> android/Makefile.am | 3 +-
> android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 87 insertions(+), 1 deletion(-)
> create mode 100644 android/audio-ipc-api.txt
>
> diff --git a/android/Makefile.am b/android/Makefile.am
> index 79f30d7..ac00bb2 100644
> --- a/android/Makefile.am
> +++ b/android/Makefile.am
> @@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
> android/pixit-gap.txt android/pixit-hid.txt \
> android/pixit-opp.txt android/pixit-pan.txt \
> android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
> - android/pts-opp.txt android/pts-pbap.txt
> + android/pts-opp.txt android/pts-pbap.txt \
> + android/audio-ipc-api.txt
> diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
> new file mode 100644
> index 0000000..e9a2136
> --- /dev/null
> +++ b/android/audio-ipc-api.txt
> @@ -0,0 +1,85 @@
> +Bluetooth Audio Plugin
> +======================
> +
> +The audio plugin happen to be in a different socket but all the rules for
> +HAL socket apply here as well, the abstract socket name is
> +"\0bluez_audio_socket" (tentative):
> +
> + .--Android--. .---Audio---.
> + | daemon | | Plugin |
> + | | Command | |
> + | | <-------------------------- | |
> + | | | |
> + | | --------------------------> | |
> + | | Response | |
> + | | | |
> + | | | |
> + | | | |
> + '-----------' '-----------'
> +
> +
> + Audio HAL Daemon
> + ----------------------------------------------------
> +
> + call dev->open() --> command 0x01
> + return dev->open() <-- response 0x01
> +
> + call dev->open_output_stream() --> command 0x03
> + return dev->open_output_stream() <-- response 0x03
> +
> + call stream_in->read() --> command 0x05
> + return stream_in->read() <-- response 0x05
> +
I think it should be stream_out->write() here.
> + call stream_in->common.standby() --> command 0x06
> + return stream_in->common.standby() <-- response 0x06
> +
Also here: stream_out->common.standby()
> + call dev->close_output_stream() --> command 0x04
> + return dev->close_output_stream() <-- response 0x04
> +
> + call dev->close() --> command 0x02
> + return dev->close() <-- response 0x02
> +
> +Identifier: "audio" (BT_AUDIO_ID)
> +
> + Opcode 0x00 - Error response
> +
> + Response parameters: Status (1 octet)
> +
> + Opcode 0x01 - Open Audio Endpoint commmand
> +
> + Command parameters: Service UUID (16 octets)
> + Codec ID (1 octets)
> + Codec capabilities length (1 octets)
> + Codec capabilities (variable)
> + Number of codec presets (1 octets)
> + Codec preset # length (1 octets)
> + Codec preset # configuration (variable)
> + ...
> + Response parameters: Endpoint ID (1 octets)
> +
> + Opcode 0x02 - Close Audio Endpoint command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> +
> + Opcode 0x03 - Open Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: Codec configuration length (1 octets)
> + Codec configuration (1 octets)
> + File descriptor (inline)
> +
> + Opcode 0x04 - Close Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> +
> + Opcode 0x05 - Resume Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> +
> + Opcode 0x06 - Suspend Stream command
> +
> + Command parameters: Endpoint ID (1 octets)
> + Response parameters: <none>
> --
> 1.8.3.1
>
Otherwise looks ok to me.
BR
Lukasz
> --
> 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
^ permalink raw reply
* RFCOMM connection failing
From: Patrick Valsecchi @ 2013-12-16 15:55 UTC (permalink / raw)
To: linux-bluetooth
Hi,
I'm trying to connect my PC:
Linux ... 3.11.0-14-generic #21-Ubuntu SMP ... x86_64 x86_64 x86_64
GNU/Linux
0a5c:2198 Broadcom Corp. Bluetooth 3.0 Device
To connect with my bluetooth dive computer:
Shearwater Petrel
It fails with a "Transport endpoint is not connected (107)" most of the
time or go further but seem to have corrupted RFCOMM payload.
If I pass the USB device to a W7 VM (virtualbox) and try from them, the
communication works like charm.
So I went ahead and sniffed the USB communication in both cases using
wireshark. The two dumps () can be found here (UsbDumpFrom*.pcapng, can
be open using wireshark):
https://cloud.thus.ch/public.php?service=files&t=de2eabf30c82efa08cf546ff5045e585
Basically the Linux one is just stopping at frame 203 where it sends a
RFCOMM SABM, gets the answer and reports an error to the user. The
windows dump show the same RFCOMM SABM command in frame 191 and gets the
same answer but continues and everything works.
On IRC aholler told me you guys would prefer to get a btmon dump with
bluez v5. So I went ahead, installed bluez 5.12 and generated a dump
(yes, the comm still fails the same way) that you can find in the same
location as the dumps.
I'm stuck there. Can somebody help me go further?
Thanks
^ permalink raw reply
* Re: [PATCH 2/2] android/hidhost: Simplify handle_uhid_output
From: Luiz Augusto von Dentz @ 2013-12-16 15:19 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1387204652-14850-2-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
On Mon, Dec 16, 2013 at 4:37 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Make it use VLA for req buffer. This makes function simpler and also
> fix cutting req to 255 bytes (req_len was uint8_t)
> ---
> android/hidhost.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 0e0391a..76322af 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
> static void handle_uhid_output(struct hid_device *dev,
> struct uhid_output_req *output)
> {
> - int fd, i;
> - uint8_t *req = NULL;
> - uint8_t req_size = 0;
> + int fd;
> + unsigned int i;
> + uint8_t req[1 + (output->size / 2)];
Im not a fan of VLA and Ive seem even some static analyzer that would
warn about its use without first checking > 0 and have a check of
upper bond limit, so perhaps just having it set to UHID_DATA_MAX or
dynamically allocate a buffer to match the output MTU size of the
control channel would better imo.
--
Luiz Augusto von Dentz
^ permalink raw reply
* [PATCH 2/2] android/tester: Enable bthost after device is enabled
From: Andrei Emeltchenko @ 2013-12-16 14:52 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387205568-14461-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
This puts bthost to connectible mode allowing us to use powered setup
for connect to emulated device.
---
android/android-tester.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/android/android-tester.c b/android/android-tester.c
index 2a8c8d0..d0d3690 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -33,10 +33,15 @@
#include "src/shared/mgmt.h"
#include "src/shared/hciemu.h"
+#include "emulator/bthost.h"
+#include "monitor/bt.h"
+
#include <hardware/hardware.h>
#include <hardware/bluetooth.h>
#include <hardware/bt_sock.h>
+#include "utils.h"
+
#define adapter_props adapter_prop_bdaddr, adapter_prop_bdname, \
adapter_prop_uuids, adapter_prop_cod, \
adapter_prop_type, adapter_prop_scan_mode, \
@@ -463,6 +468,42 @@ failed:
close(fd);
}
+static void client_connectable_complete(uint16_t opcode, uint8_t status,
+ const void *param, uint8_t len,
+ void *user_data)
+{
+ switch (opcode) {
+ case BT_HCI_CMD_WRITE_SCAN_ENABLE:
+ case BT_HCI_CMD_LE_SET_ADV_ENABLE:
+ break;
+ default:
+ return;
+ }
+
+ tester_print("Client set connectable status 0x%02x", status);
+
+ if (status)
+ tester_setup_failed();
+ else
+ tester_setup_complete();
+}
+
+static void setup_powered_client(void)
+{
+ struct test_data *data = tester_get_data();
+ struct bthost *bthost;
+
+ tester_print("Controller powered on");
+
+ bthost = hciemu_client_get_host(data->hciemu);
+ bthost_set_cmd_complete_cb(bthost, client_connectable_complete, data);
+
+ if (data->hciemu_type == HCIEMU_TYPE_LE)
+ bthost_set_adv_enable(bthost, 0x01);
+ else
+ bthost_write_scan_enable(bthost, 0x03);
+}
+
static void adapter_state_changed_cb(bt_state_t state)
{
enum hal_bluetooth_callbacks_id hal_cb;
@@ -484,7 +525,7 @@ static void adapter_state_changed_cb(bt_state_t state)
break;
case adapter_test_setup_mode:
if (state == BT_STATE_ON)
- tester_setup_complete();
+ setup_powered_client();
else
tester_setup_failed();
break;
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/2] android/tester: Add missing break
From: Andrei Emeltchenko @ 2013-12-16 14:52 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
android/android-tester.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/android/android-tester.c b/android/android-tester.c
index eb938d0..2a8c8d0 100644
--- a/android/android-tester.c
+++ b/android/android-tester.c
@@ -487,6 +487,7 @@ static void adapter_state_changed_cb(bt_state_t state)
tester_setup_complete();
else
tester_setup_failed();
+ break;
default:
break;
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/2] android/hidhost: Simplify handle_uhid_output
From: Szymon Janc @ 2013-12-16 14:37 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
In-Reply-To: <1387204652-14850-1-git-send-email-szymon.janc@tieto.com>
Make it use VLA for req buffer. This makes function simpler and also
fix cutting req to 255 bytes (req_len was uint8_t)
---
android/hidhost.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/android/hidhost.c b/android/hidhost.c
index 0e0391a..76322af 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -156,29 +156,22 @@ static void hid_device_free(struct hid_device *dev)
static void handle_uhid_output(struct hid_device *dev,
struct uhid_output_req *output)
{
- int fd, i;
- uint8_t *req = NULL;
- uint8_t req_size = 0;
+ int fd;
+ unsigned int i;
+ uint8_t req[1 + (output->size / 2)];
if (!(dev->ctrl_io))
return;
- req_size = 1 + (output->size / 2);
- req = g_try_malloc0(req_size);
- if (!req)
- return;
-
req[0] = HID_MSG_SET_REPORT | output->rtype;
- for (i = 0; i < (req_size - 1); i++)
+ for (i = 0; i < (sizeof(req) - 1); i++)
sscanf((char *) &(output->data)[i * 2], "%hhx", &req[1 + i]);
fd = g_io_channel_unix_get_fd(dev->ctrl_io);
- if (write(fd, req, req_size) < 0)
+ if (write(fd, req, sizeof(req)) < 0)
error("error writing set_report: %s (%d)",
strerror(errno), errno);
-
- g_free(req);
}
static gboolean uhid_event_cb(GIOChannel *io, GIOCondition cond,
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/2] bluetooth/hidhost: Fix using feature event as output event
From: Szymon Janc @ 2013-12-16 14:37 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Szymon Janc
Rename handle_uhid_event to handle_uhid_output and make it handle only
output events.
---
android/hidhost.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/android/hidhost.c b/android/hidhost.c
index 8bfdfed..0e0391a 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -153,7 +153,8 @@ static void hid_device_free(struct hid_device *dev)
g_free(dev);
}
-static void handle_uhid_event(struct hid_device *dev, struct uhid_event *ev)
+static void handle_uhid_output(struct hid_device *dev,
+ struct uhid_output_req *output)
{
int fd, i;
uint8_t *req = NULL;
@@ -162,15 +163,14 @@ static void handle_uhid_event(struct hid_device *dev, struct uhid_event *ev)
if (!(dev->ctrl_io))
return;
- req_size = 1 + (ev->u.output.size / 2);
+ req_size = 1 + (output->size / 2);
req = g_try_malloc0(req_size);
if (!req)
return;
- req[0] = HID_MSG_SET_REPORT | ev->u.output.rtype;
+ req[0] = HID_MSG_SET_REPORT | output->rtype;
for (i = 0; i < (req_size - 1); i++)
- sscanf((char *) &(ev->u.output.data)[i * 2],
- "%hhx", &req[1 + i]);
+ sscanf((char *) &(output->data)[i * 2], "%hhx", &req[1 + i]);
fd = g_io_channel_unix_get_fd(dev->ctrl_io);
@@ -224,8 +224,10 @@ static gboolean uhid_event_cb(GIOChannel *io, GIOCondition cond,
* asleep This is optional, though. */
break;
case UHID_OUTPUT:
+ handle_uhid_output(dev, &ev.u.output);
+ break;
case UHID_FEATURE:
- handle_uhid_event(dev, &ev);
+ /* TODO */
break;
case UHID_OUTPUT_EV:
/* This is only sent by kernels prior to linux-3.11. It
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 BlueZ] android: Add initial Android Bluetooth Audio protocol API doc
From: Luiz Augusto von Dentz @ 2013-12-16 13:08 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This IPC is used to communicate Android BlueZ daemon and AudioFlinger
plugin.
---
v2: Rework IPC commands to match Android Audio HAL
android/Makefile.am | 3 +-
android/audio-ipc-api.txt | 85 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 1 deletion(-)
create mode 100644 android/audio-ipc-api.txt
diff --git a/android/Makefile.am b/android/Makefile.am
index 79f30d7..ac00bb2 100644
--- a/android/Makefile.am
+++ b/android/Makefile.am
@@ -111,4 +111,5 @@ EXTRA_DIST += android/Android.mk android/hal-ipc-api.txt android/README \
android/pixit-gap.txt android/pixit-hid.txt \
android/pixit-opp.txt android/pixit-pan.txt \
android/pixit-pbap.txt android/pts-gap.txt android/pts-hid.txt \
- android/pts-opp.txt android/pts-pbap.txt
+ android/pts-opp.txt android/pts-pbap.txt \
+ android/audio-ipc-api.txt
diff --git a/android/audio-ipc-api.txt b/android/audio-ipc-api.txt
new file mode 100644
index 0000000..e9a2136
--- /dev/null
+++ b/android/audio-ipc-api.txt
@@ -0,0 +1,85 @@
+Bluetooth Audio Plugin
+======================
+
+The audio plugin happen to be in a different socket but all the rules for
+HAL socket apply here as well, the abstract socket name is
+"\0bluez_audio_socket" (tentative):
+
+ .--Android--. .---Audio---.
+ | daemon | | Plugin |
+ | | Command | |
+ | | <-------------------------- | |
+ | | | |
+ | | --------------------------> | |
+ | | Response | |
+ | | | |
+ | | | |
+ | | | |
+ '-----------' '-----------'
+
+
+ Audio HAL Daemon
+ ----------------------------------------------------
+
+ call dev->open() --> command 0x01
+ return dev->open() <-- response 0x01
+
+ call dev->open_output_stream() --> command 0x03
+ return dev->open_output_stream() <-- response 0x03
+
+ call stream_in->read() --> command 0x05
+ return stream_in->read() <-- response 0x05
+
+ call stream_in->common.standby() --> command 0x06
+ return stream_in->common.standby() <-- response 0x06
+
+ call dev->close_output_stream() --> command 0x04
+ return dev->close_output_stream() <-- response 0x04
+
+ call dev->close() --> command 0x02
+ return dev->close() <-- response 0x02
+
+Identifier: "audio" (BT_AUDIO_ID)
+
+ Opcode 0x00 - Error response
+
+ Response parameters: Status (1 octet)
+
+ Opcode 0x01 - Open Audio Endpoint commmand
+
+ Command parameters: Service UUID (16 octets)
+ Codec ID (1 octets)
+ Codec capabilities length (1 octets)
+ Codec capabilities (variable)
+ Number of codec presets (1 octets)
+ Codec preset # length (1 octets)
+ Codec preset # configuration (variable)
+ ...
+ Response parameters: Endpoint ID (1 octets)
+
+ Opcode 0x02 - Close Audio Endpoint command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
+
+ Opcode 0x03 - Open Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: Codec configuration length (1 octets)
+ Codec configuration (1 octets)
+ File descriptor (inline)
+
+ Opcode 0x04 - Close Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
+
+ Opcode 0x05 - Resume Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
+
+ Opcode 0x06 - Suspend Stream command
+
+ Command parameters: Endpoint ID (1 octets)
+ Response parameters: <none>
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 4/5] l2cap-tester: Remove unneeded variable
From: Andrei Emeltchenko @ 2013-12-16 10:50 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: BlueZ development
In-Reply-To: <CAJdJm_NJCUAeMkvGMYLah3L__DECCd_ocPyd-F46KrcdmCuGMg@mail.gmail.com>
Hi Anderson,
On Mon, Dec 16, 2013 at 06:39:28AM -0400, Anderson Lizardo wrote:
> Hi Andrei,
>
> On Mon, Dec 16, 2013 at 4:57 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > --- a/tools/l2cap-tester.c
> > +++ b/tools/l2cap-tester.c
> > @@ -489,15 +489,14 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
> > {
> > const uint8_t *master_bdaddr;
> > struct sockaddr_l2 addr;
> > - int sk, err;
> > + int sk;
> >
> > sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | SOCK_NONBLOCK,
> > BTPROTO_L2CAP);
> > if (sk < 0) {
> > - err = -errno;
> > tester_warn("Can't create socket: %s (%d)", strerror(errno),
> > errno);
> > - return err;
> > + return -errno;
> > }
> >
> > master_bdaddr = hciemu_get_master_bdaddr(data->hciemu);
> > @@ -517,11 +516,10 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
> > addr.l2_bdaddr_type = BDADDR_BREDR;
> >
> > if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
> > - err = -errno;
> > tester_warn("Can't bind socket: %s (%d)", strerror(errno),
> > errno);
> > close(sk);
> > - return err;
> > + return -errno;
>
> This is not a good idea because close() will overwrite the original
> error if it fails as well. The previous situation is also valid
> because tester_warn() may call library functions that set errno.
Yes, though the return value is not used at all, we may just return -1.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH 4/5] l2cap-tester: Remove unneeded variable
From: Anderson Lizardo @ 2013-12-16 10:39 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: BlueZ development
In-Reply-To: <1387184262-21439-4-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Mon, Dec 16, 2013 at 4:57 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> --- a/tools/l2cap-tester.c
> +++ b/tools/l2cap-tester.c
> @@ -489,15 +489,14 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
> {
> const uint8_t *master_bdaddr;
> struct sockaddr_l2 addr;
> - int sk, err;
> + int sk;
>
> sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | SOCK_NONBLOCK,
> BTPROTO_L2CAP);
> if (sk < 0) {
> - err = -errno;
> tester_warn("Can't create socket: %s (%d)", strerror(errno),
> errno);
> - return err;
> + return -errno;
> }
>
> master_bdaddr = hciemu_get_master_bdaddr(data->hciemu);
> @@ -517,11 +516,10 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
> addr.l2_bdaddr_type = BDADDR_BREDR;
>
> if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
> - err = -errno;
> tester_warn("Can't bind socket: %s (%d)", strerror(errno),
> errno);
> close(sk);
> - return err;
> + return -errno;
This is not a good idea because close() will overwrite the original
error if it fails as well. The previous situation is also valid
because tester_warn() may call library functions that set errno.
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* CSR 8510 HID Proxy
From: Abimanyu G @ 2013-12-16 9:39 UTC (permalink / raw)
To: linux-bluetooth
Hi everyone,
hid2hci tool is not working on latest CSR 8510 chips can any one fix it.
Regards,
Abimanyu G
^ permalink raw reply
* Re: [PATCH 2/5] avdtp: Remove unneeded local variable
From: Johan Hedberg @ 2013-12-16 9:18 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1387184262-21439-2-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Mon, Dec 16, 2013, Andrei Emeltchenko wrote:
> ---
> profiles/audio/avdtp.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
I've applied all patches except these ones claiming to remove an
unneeded err variable.
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index f866b39..e12ad9d 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -773,10 +773,9 @@ static int get_send_buffer_size(int sk)
> socklen_t optlen = sizeof(size);
>
> if (getsockopt(sk, SOL_SOCKET, SO_SNDBUF, &size, &optlen) < 0) {
> - int err = -errno;
> - error("getsockopt(SO_SNDBUF) failed: %s (%d)", strerror(-err),
> - -err);
> - return err;
> + error("getsockopt(SO_SNDBUF) failed: %s (%d)", strerror(errno),
> + errno);
> + return -errno;
> }
This is not an unneeded variable. The error() call itself might cause
the value of errno to be modified and hence you'd be returning not the
getsockopt error but something else.
Johan
^ permalink raw reply
* Re: [PATCH 1/3] android/tester: Add Socket test read channel number after listen
From: Johan Hedberg @ 2013-12-16 9:14 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1387183137-19439-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Mon, Dec 16, 2013, Andrei Emeltchenko wrote:
> After listen() call Bluedroid sends channel number as (int) through file
> descriptor.
> ---
> android/android-tester.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
All three patches have been applied. Thanks.
Johan
^ permalink raw reply
* [PATCH 5/5] hciemu: Print error in case hci_vhci is not loaded
From: Andrei Emeltchenko @ 2013-12-16 8:57 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387184262-21439-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Error message should indicate that module is not loaded:
Opening /dev/vhci failed: No such file or directory
---
src/shared/hciemu.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/shared/hciemu.c b/src/shared/hciemu.c
index c2b4748..9f4bfaf 100644
--- a/src/shared/hciemu.c
+++ b/src/shared/hciemu.c
@@ -31,6 +31,7 @@
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
+#include <errno.h>
#include <sys/socket.h>
#include <glib.h>
@@ -216,6 +217,7 @@ static bool create_vhci(struct hciemu *hciemu)
fd = open("/dev/vhci", O_RDWR | O_NONBLOCK | O_CLOEXEC);
if (fd < 0) {
+ perror("Opening /dev/vhci failed");
btdev_destroy(btdev);
return false;
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH 4/5] l2cap-tester: Remove unneeded variable
From: Andrei Emeltchenko @ 2013-12-16 8:57 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1387184262-21439-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
err is only used to assign -errno before return.
---
tools/l2cap-tester.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/tools/l2cap-tester.c b/tools/l2cap-tester.c
index e4dade2..4b1e5e2 100644
--- a/tools/l2cap-tester.c
+++ b/tools/l2cap-tester.c
@@ -489,15 +489,14 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
{
const uint8_t *master_bdaddr;
struct sockaddr_l2 addr;
- int sk, err;
+ int sk;
sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | SOCK_NONBLOCK,
BTPROTO_L2CAP);
if (sk < 0) {
- err = -errno;
tester_warn("Can't create socket: %s (%d)", strerror(errno),
errno);
- return err;
+ return -errno;
}
master_bdaddr = hciemu_get_master_bdaddr(data->hciemu);
@@ -517,11 +516,10 @@ static int create_l2cap_sock(struct test_data *data, uint16_t psm)
addr.l2_bdaddr_type = BDADDR_BREDR;
if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
- err = -errno;
tester_warn("Can't bind socket: %s (%d)", strerror(errno),
errno);
close(sk);
- return err;
+ return -errno;
}
return sk;
--
1.8.3.2
^ 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