2012-08-09 07:56:20

by Mikel Astiz

[permalink] [raw]
Subject: [RFC BlueZ v2] mgmt-api: Add reason to device disconnect event

From: Mikel Astiz <[email protected]>

Extend the management API with the disconnect reason, as now reported
by the Kernel in MGMT_EV_DEVICE_DISCONNECTED.
---
Updated userland patch to test the recently submitted Kernel patches regarding ACL disconnect reason.

doc/mgmt-api.txt | 16 ++++++++++++++++
lib/mgmt.h | 6 ++++++
monitor/control.c | 19 +++++++++++++++----
src/mgmt.c | 13 ++++++++-----
4 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
index 51c9b46..c25f377 100644
--- a/doc/mgmt-api.txt
+++ b/doc/mgmt-api.txt
@@ -973,12 +973,28 @@ Event Code 0x000C
Controller Index: <controller id>
Event Parameters Address (6 Octets)
Address_Type (1 Octet)
+ Reason (1 Octet)

Possible values for the Address_Type parameter:
0 BR/EDR
1 LE Public
2 LE Random

+ Possible values for the Reason parameter:
+ 0 Unspecified
+ 1 Connection timeout
+ 2 Connection terminated by local host
+ 3 Connection terminated by remote host
+
+ Note that the local/remote distinction just determines which side
+ terminated the low-level connection, regardless of the
+ disconnection of the higher-level profiles.
+
+ This can sometimes be misleading and thus must be used with care.
+ For example, some hardware combinations would report a locally
+ initiated disconnection even if the user turned Bluetooth off in
+ the remote side.
+

Connect Failed Event
====================
diff --git a/lib/mgmt.h b/lib/mgmt.h
index 83dcd84..a2648bc 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -373,9 +373,15 @@ struct mgmt_ev_device_connected {
uint8_t eir[0];
} __packed;

+#define MGMT_DEV_DISCONN_UNKNOWN 0x00
+#define MGMT_DEV_DISCONN_TIMEOUT 0x01
+#define MGMT_DEV_DISCONN_LOCAL_HOST 0x02
+#define MGMT_DEV_DISCONN_REMOTE 0x03
+
#define MGMT_EV_DEVICE_DISCONNECTED 0x000C
struct mgmt_ev_device_disconnected {
struct mgmt_addr_info addr;
+ uint8_t reason;
} __packed;

#define MGMT_EV_CONNECT_FAILED 0x000D
diff --git a/monitor/control.c b/monitor/control.c
index 159ba9d..c799327 100644
--- a/monitor/control.c
+++ b/monitor/control.c
@@ -226,18 +226,29 @@ static void mgmt_device_disconnected(uint16_t len, const void *buf)
{
const struct mgmt_ev_device_disconnected *ev = buf;
char str[18];
+ uint8_t reason;
+ uint16_t l;

- if (len < sizeof(*ev)) {
+ if (len < sizeof(struct mgmt_addr_info)) {
printf("* Malformed Device Disconnected control\n");
return;
}

+ if (len < sizeof(*ev)) {
+ reason = MGMT_DEV_DISCONN_UNKNOWN;
+ l = len;
+ } else {
+ reason = ev->reason;
+ l = sizeof(*ev);
+ }
+
ba2str(&ev->addr.bdaddr, str);

- printf("@ Device Disconnected: %s (%d)\n", str, ev->addr.type);
+ printf("@ Device Disconnected: %s (%d) reason %u\n", str, ev->addr.type,
+ reason);

- buf += sizeof(*ev);
- len -= sizeof(*ev);
+ buf += l;
+ len -= l;

packet_hexdump(buf, len);
}
diff --git a/src/mgmt.c b/src/mgmt.c
index c893972..131fe50 100644
--- a/src/mgmt.c
+++ b/src/mgmt.c
@@ -510,18 +510,21 @@ static void mgmt_device_connected(int sk, uint16_t index, void *buf, size_t len)
static void mgmt_device_disconnected(int sk, uint16_t index, void *buf,
size_t len)
{
- struct mgmt_addr_info *ev = buf;
+ struct mgmt_ev_device_disconnected *ev = buf;
struct controller_info *info;
char addr[18];

- if (len < sizeof(*ev)) {
+ if (len < sizeof(struct mgmt_addr_info)) {
error("Too small device_disconnected event");
return;
}

- ba2str(&ev->bdaddr, addr);
+ if (len < sizeof(*ev))
+ memset((char *) buf + len, 0, sizeof(*ev) - len);
+
+ ba2str(&ev->addr.bdaddr, addr);

- DBG("hci%u device %s disconnected", index, addr);
+ DBG("hci%u device %s disconnected reason %u", index, addr, ev->reason);

if (index > max_index) {
error("Unexpected index %u in device_disconnected event", index);
@@ -530,7 +533,7 @@ static void mgmt_device_disconnected(int sk, uint16_t index, void *buf,

info = &controllers[index];

- btd_event_disconn_complete(&info->bdaddr, &ev->bdaddr);
+ btd_event_disconn_complete(&info->bdaddr, &ev->addr.bdaddr);
}

static void mgmt_connect_failed(int sk, uint16_t index, void *buf, size_t len)
--
1.7.7.6



2012-08-17 09:07:34

by Mikel Astiz

[permalink] [raw]
Subject: Re: [RFC BlueZ v2] mgmt-api: Add reason to device disconnect event

Hi Johan,

On Thu, Aug 16, 2012 at 2:25 PM, Johan Hedberg <[email protected]> wrote:
> Hi Mikel,
>
> On Thu, Aug 09, 2012, Mikel Astiz wrote:
>> From: Mikel Astiz <[email protected]>
>>
>> Extend the management API with the disconnect reason, as now reported
>> by the Kernel in MGMT_EV_DEVICE_DISCONNECTED.
>> ---
>> Updated userland patch to test the recently submitted Kernel patches regarding ACL disconnect reason.
>>
>> doc/mgmt-api.txt | 16 ++++++++++++++++
>> lib/mgmt.h | 6 ++++++
>> monitor/control.c | 19 +++++++++++++++----
>> src/mgmt.c | 13 ++++++++-----
>> 4 files changed, 45 insertions(+), 9 deletions(-)
>
> Seems like you're missing an update to tools/btmgmt.c?

Indeed. I will integrate the changes in the next version of the patch.

>> --- a/monitor/control.c
>> +++ b/monitor/control.c
>> @@ -226,18 +226,29 @@ static void mgmt_device_disconnected(uint16_t len, const void *buf)
>> {
>> const struct mgmt_ev_device_disconnected *ev = buf;
>> char str[18];
>> + uint8_t reason;
>> + uint16_t l;
>
> I suppose size_t would be more appropriate instead of uint16_t as that's
> the return type of sizeof(). The variable name is also a bit
> uninformative and I'd have used something like consumed_len or
> parsed_len, but if this the convention in rest of the btmon code then I
> wont object to it.

I will rename the variable to consumed_len but I would rather leave it
as uint16_t in order to match the type of len.

>> --- a/src/mgmt.c
>> +++ b/src/mgmt.c
>> @@ -510,18 +510,21 @@ static void mgmt_device_connected(int sk, uint16_t index, void *buf, size_t len)
>> static void mgmt_device_disconnected(int sk, uint16_t index, void *buf,
>> size_t len)
>> {
>> - struct mgmt_addr_info *ev = buf;
>> + struct mgmt_ev_device_disconnected *ev = buf;
>> struct controller_info *info;
>> char addr[18];
>>
>> - if (len < sizeof(*ev)) {
>> + if (len < sizeof(struct mgmt_addr_info)) {
>> error("Too small device_disconnected event");
>> return;
>> }
>>
>> - ba2str(&ev->bdaddr, addr);
>> + if (len < sizeof(*ev))
>> + memset((char *) buf + len, 0, sizeof(*ev) - len);
>
> I don't like how this assumes that there is usable space after the end
> of the buffer given to this function. Could you instead use a temporary
> "uint8_t reason" variable like you do for btmon?

I will change this one too.

Cheers,
Mikel

2012-08-16 12:25:10

by Johan Hedberg

[permalink] [raw]
Subject: Re: [RFC BlueZ v2] mgmt-api: Add reason to device disconnect event

Hi Mikel,

On Thu, Aug 09, 2012, Mikel Astiz wrote:
> From: Mikel Astiz <[email protected]>
>
> Extend the management API with the disconnect reason, as now reported
> by the Kernel in MGMT_EV_DEVICE_DISCONNECTED.
> ---
> Updated userland patch to test the recently submitted Kernel patches regarding ACL disconnect reason.
>
> doc/mgmt-api.txt | 16 ++++++++++++++++
> lib/mgmt.h | 6 ++++++
> monitor/control.c | 19 +++++++++++++++----
> src/mgmt.c | 13 ++++++++-----
> 4 files changed, 45 insertions(+), 9 deletions(-)

Seems like you're missing an update to tools/btmgmt.c?

> --- a/monitor/control.c
> +++ b/monitor/control.c
> @@ -226,18 +226,29 @@ static void mgmt_device_disconnected(uint16_t len, const void *buf)
> {
> const struct mgmt_ev_device_disconnected *ev = buf;
> char str[18];
> + uint8_t reason;
> + uint16_t l;

I suppose size_t would be more appropriate instead of uint16_t as that's
the return type of sizeof(). The variable name is also a bit
uninformative and I'd have used something like consumed_len or
parsed_len, but if this the convention in rest of the btmon code then I
wont object to it.

> --- a/src/mgmt.c
> +++ b/src/mgmt.c
> @@ -510,18 +510,21 @@ static void mgmt_device_connected(int sk, uint16_t index, void *buf, size_t len)
> static void mgmt_device_disconnected(int sk, uint16_t index, void *buf,
> size_t len)
> {
> - struct mgmt_addr_info *ev = buf;
> + struct mgmt_ev_device_disconnected *ev = buf;
> struct controller_info *info;
> char addr[18];
>
> - if (len < sizeof(*ev)) {
> + if (len < sizeof(struct mgmt_addr_info)) {
> error("Too small device_disconnected event");
> return;
> }
>
> - ba2str(&ev->bdaddr, addr);
> + if (len < sizeof(*ev))
> + memset((char *) buf + len, 0, sizeof(*ev) - len);

I don't like how this assumes that there is usable space after the end
of the buffer given to this function. Could you instead use a temporary
"uint8_t reason" variable like you do for btmon?

Johan