* Re: [PATCH v2] Fix leak of EIR data if RSSI does not change
From: Johan Hedberg @ 2010-12-27 21:48 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293473827-26418-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> If RSSI value does not change, memory used by parsed EIR data would leak
> because it would not be assigned to the remote_dev_info structure.
>
> Also simplify related code and replace a couple of g_free()'s with
> free() (simply because they were allocated with malloc()).
Beware of words like "also" and "and" in commit messages. Often they are
a good indication that the patch shold be split into multiple ones.
Though in this case I can't really make up my mind if it's fine since
the changes are so strongly related. I'll leave it up to you to think
about it and decide if you can cleanly split this into two patches or
not.
> @@ -529,6 +537,14 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> else
> name_status = NAME_NOT_REQUIRED;
>
> + /* Update storage if EIR contains the complete device name */
> + if (eir_data.name && eir_data.name_complete) {
> + write_device_name(local, peer, eir_data.name);
> + name_status = NAME_NOT_REQUIRED;
> + g_free(eir_data.name);
> + eir_data.name = NULL;
> + }
> +
> create_name(filename, PATH_MAX, STORAGEDIR, local_addr, "aliases");
> alias = textfile_get(filename, peer_addr);
>
> @@ -547,28 +563,13 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> } else
> legacy = TRUE;
>
> - if (eir_data.name) {
> - if (eir_data.name_complete) {
> - write_device_name(local, peer, eir_data.name);
> - name_status = NAME_NOT_REQUIRED;
> -
> - if (name)
> - g_free(name);
> -
> - name = eir_data.name;
> - } else {
> - if (name)
> - free(eir_data.name);
> - else
> - name = eir_data.name;
> - }
> - }
> -
> - adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
> - legacy, eir_data.services, name_status);
> + if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
> + alias, legacy, eir_data.services,
> + name_status))
> + free_eir_data(&eir_data);
>
> - g_free(name);
> - g_free(alias);
> + free(name);
> + free(alias);
> }
>
> void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer,
It looks to me like shortened names are completely ignored after this
patch, even to the extent that if there's a shortened name the memory
will be leaked if adapter_update_found_devices returns TRUE.
If we don't have any name in storage and all we have is a shortened name
then I think it should be included in the DeviceFound signal (which is
what the existing code does).
Johan
^ permalink raw reply
* Re: [PATCH 4/4] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-27 18:23 UTC (permalink / raw)
To: Anderson Lizardo, linux-bluetooth
In-Reply-To: <20101227151122.GD17203@jh-x301>
Hi Johan,
On Mon, Dec 27, 2010 at 11:11 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> In general this seems fine, but to avoid future memory leaks in case you
> go ahead and change the eir_data struct wouldn't it be better to have a
> separate free_eir_data() function which does the g_slist_foreach,
> g_slist_free and g_free?
I followed your suggestion and created a free_eir_data(). I also did a
simple code reordering that simplified the complete EIR name storage.
See v2 of this patch. (2/4 should be ignored).
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 18:20 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-2-git-send-email-anderson.lizardo@openbossa.org>
This patch should be ignored. Instead, I incorporated a small
refactoring on v2 of patch 4/4.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* [PATCH v2] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-27 18:17 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-4-git-send-email-anderson.lizardo@openbossa.org>
If RSSI value does not change, memory used by parsed EIR data would leak
because it would not be assigned to the remote_dev_info structure.
Also simplify related code and replace a couple of g_free()'s with
free() (simply because they were allocated with malloc()).
---
src/adapter.c | 19 ++++++++++++-------
src/adapter.h | 11 ++++++-----
src/event.c | 47 ++++++++++++++++++++++++-----------------------
3 files changed, 42 insertions(+), 35 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index bfc2b8b..4b10189 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2917,7 +2917,7 @@ static void remove_same_uuid(gpointer data, gpointer user_data)
}
}
-void adapter_update_device_from_info(struct btd_adapter *adapter,
+gboolean adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr, int8_t rssi,
uint8_t evt_type, char *name,
GSList *services, uint8_t flags)
@@ -2931,7 +2931,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
dev->le = TRUE;
dev->evt_type = evt_type;
} else if (dev->rssi == rssi)
- return;
+ return FALSE;
dev->rssi = rssi;
@@ -2951,12 +2951,15 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
/* FIXME: check if other information was changed before emitting the
* signal */
adapter_emit_device_found(adapter, dev);
+
+ return TRUE;
}
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- GSList *services, name_status_t name_status)
+gboolean adapter_update_found_devices(struct btd_adapter *adapter,
+ bdaddr_t *bdaddr, int8_t rssi, uint32_t class,
+ const char *name, const char *alias,
+ gboolean legacy, GSList *services,
+ name_status_t name_status)
{
struct remote_dev_info *dev;
gboolean new_dev;
@@ -2975,7 +2978,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev->legacy = legacy;
dev->name_status = name_status;
} else if (dev->rssi == rssi)
- return;
+ return FALSE;
dev->rssi = rssi;
@@ -2986,6 +2989,8 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev->services = g_slist_concat(dev->services, services);
adapter_emit_device_found(adapter, dev);
+
+ return TRUE;
}
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index 857eec8..cee0bd4 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -134,14 +134,15 @@ int adapter_get_state(struct btd_adapter *adapter);
int adapter_get_discover_type(struct btd_adapter *adapter);
struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
struct remote_dev_info *match);
-void adapter_update_device_from_info(struct btd_adapter *adapter,
+gboolean adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr, int8_t rssi,
uint8_t evt_type, char *name,
GSList *services, uint8_t flags);
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- GSList *services, name_status_t name_status);
+gboolean adapter_update_found_devices(struct btd_adapter *adapter,
+ bdaddr_t *bdaddr, int8_t rssi, uint32_t class,
+ const char *name, const char *alias,
+ gboolean legacy, GSList *services,
+ name_status_t name_status);
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
void adapter_emit_device_found(struct btd_adapter *adapter,
struct remote_dev_info *dev);
diff --git a/src/event.c b/src/event.c
index 178cea8..5beedb8 100644
--- a/src/event.c
+++ b/src/event.c
@@ -431,6 +431,13 @@ static int parse_eir_data(struct eir_data *eir, uint8_t *eir_data,
return 0;
}
+static void free_eir_data(struct eir_data *eir)
+{
+ g_slist_foreach(eir->services, (GFunc) g_free, NULL);
+ g_slist_free(eir->services);
+ g_free(eir->name);
+}
+
void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
{
struct btd_adapter *adapter;
@@ -452,9 +459,10 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
rssi = *(info->data + info->length);
- adapter_update_device_from_info(adapter, info->bdaddr, rssi,
+ if (!adapter_update_device_from_info(adapter, info->bdaddr, rssi,
info->evt_type, eir_data.name,
- eir_data.services, eir_data.flags);
+ eir_data.services, eir_data.flags))
+ free_eir_data(&eir_data);
}
static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
@@ -529,6 +537,14 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
else
name_status = NAME_NOT_REQUIRED;
+ /* Update storage if EIR contains the complete device name */
+ if (eir_data.name && eir_data.name_complete) {
+ write_device_name(local, peer, eir_data.name);
+ name_status = NAME_NOT_REQUIRED;
+ g_free(eir_data.name);
+ eir_data.name = NULL;
+ }
+
create_name(filename, PATH_MAX, STORAGEDIR, local_addr, "aliases");
alias = textfile_get(filename, peer_addr);
@@ -547,28 +563,13 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else
legacy = TRUE;
- if (eir_data.name) {
- if (eir_data.name_complete) {
- write_device_name(local, peer, eir_data.name);
- name_status = NAME_NOT_REQUIRED;
-
- if (name)
- g_free(name);
-
- name = eir_data.name;
- } else {
- if (name)
- free(eir_data.name);
- else
- name = eir_data.name;
- }
- }
-
- adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
- legacy, eir_data.services, name_status);
+ if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
+ alias, legacy, eir_data.services,
+ name_status))
+ free_eir_data(&eir_data);
- g_free(name);
- g_free(alias);
+ free(name);
+ free(alias);
}
void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer,
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Bluetooth: l2cap: fix misuse of logical operation in place of bitop
From: David Sterba @ 2010-12-27 18:00 UTC (permalink / raw)
To: padovan
Cc: linux-bluetooth, netdev, linux-kernel, David Sterba,
Marcel Holtmann, João Paulo Rechi Vita
CC: Marcel Holtmann <marcel@holtmann.org>
CC: "Gustavo F. Padovan" <padovan@profusion.mobi>
CC: João Paulo Rechi Vita <jprvita@profusion.mobi>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
net/bluetooth/l2cap.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index cd8f6ea..bdfdfdc 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1893,8 +1893,8 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
if (pi->mode == L2CAP_MODE_STREAMING) {
l2cap_streaming_send(sk);
} else {
- if (pi->conn_state & L2CAP_CONN_REMOTE_BUSY &&
- pi->conn_state && L2CAP_CONN_WAIT_F) {
+ if ((pi->conn_state & L2CAP_CONN_REMOTE_BUSY) &&
+ (pi->conn_state & L2CAP_CONN_WAIT_F)) {
err = len;
break;
}
--
1.7.3.4.626.g73e7b
^ permalink raw reply related
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 15:33 UTC (permalink / raw)
To: Anderson Lizardo, Luiz Augusto von Dentz, linux-bluetooth
In-Reply-To: <20101227145322.GA16529@jh-x301>
Hi,
On Mon, Dec 27, 2010 at 10:53 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Mon, Dec 27, 2010, Anderson Lizardo wrote:
>> > if the variable name is set
>> > than your could would skip eir name completely while the code did
>> > actually use the eir name, probably the eir name is updated more
>> > frequently so it is probably the most updated one and we should in
>> > fact update the storage if they don't match.
>>
>> I could not find on spec any mention that the "complete" EIR name may
>> change.
>
> It also doesn't say that it will not change. So the safe thing would be
> not to make any assumption about its permanence, right?
Agreed. I'll adapt this patch in a attempt to at least simplify the
current logic.
> Naturally the user of the remote device might change is name (if the
> remote device has such a feature) and then the EIR data would change
> too. Note that even if there's a name in storage it doesn't mean that
> it's from the same discovery session. It might be from a previous
> discovery session a long time ago and the user of the remote device
> might have changed its name since then. So imho a complete name in an
> EIR should always override whatever is in storage.
Ok, I'll keep this semantics but try to simplify the current logic.
> For the case of shortened names I don't think there's an indisputably
> right answer. The question is what is better: that which is "complete"
> or that which is more recent. I've got the feeling that the complete
> version would be better for the user, but someone might of course
> disagree with this.
For now, I'll keep the current (from master) behavior of only storing
complete EIR names and using the stored name in favor of the shortened
one. My intent is not to change behavior unless there is some need to
do so.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 15:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <AANLkTi=06P4ZpGO-sp7czZtn9FGLkBwPnrxNqDdGspfy@mail.gmail.com>
Hi,
On Mon, Dec 27, 2010 at 10:35 AM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> I've not found any references to the spec saying the shortened name
> has to be a prefix. I believe it is left to the implementer how to
> display such shortened names. bluez so far has made no distinction on
> the display of shortened names vs. complete names.
Interestingly, on Volume 3, Part C, 11.1.2, it is mentioned that for
Advertising Data (i.e. LE devices):
"A shortened name shall only contain contiguous
characters from the beginning of the full name. For example, if the device name
is ‘BT_Device_Name’ then the shortened name over BR/EDR could be
‘BT_Device’ while the shortened name on LE could be ‘BT_Dev’."
Nothing is said on the EIR format, though (see Vol 3, Part C, 8.1.2).
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [RFCv2] Bluetooth: Use non-flushable by default L2CAP data packets
From: Andrei Emeltchenko @ 2010-12-27 15:13 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101223020615.GA26284@vigoh>
Hi Gustavo,
On Thu, Dec 23, 2010 at 4:06 AM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-12-16 16:54:26 +0200]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> Modification of Nick Pelly <npelly@google.com> patch.
>>
>> With Bluetooth 2.1 ACL packets can be flushable or non-flushable. This commit
>> makes ACL data packets non-flushable by default on compatible chipsets, and
>> adds the BT_FLUSHABLE socket option to explicitly request flushable ACL
>> data packets for a given L2CAP socket. This is useful for A2DP data which can
>> be safely discarded if it can not be delivered within a short time (while
>> other ACL data should not be discarded).
>>
>> Note that making ACL data flushable has no effect unless the automatic flush
>> timeout for that ACL link is changed from its default of 0 (infinite).
>>
>> Default packet types (for compatible chipsets):
>> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
>> Bluetooth HCI H4
>> Bluetooth HCI ACL Packet
>> .... 0000 0000 0010 = Connection Handle: 0x0002
>> ..00 .... .... .... = PB Flag: First Non-automatically Flushable Packet (0)
>> 00.. .... .... .... = BC Flag: Point-To-Point (0)
>> Data Total Length: 8
>> Bluetooth L2CAP Packet
>>
>> After setting BT_FLUSHABLE
>> (sock.setsockopt(274 /*SOL_BLUETOOTH*/, 8 /* BT_FLUSHABLE */, 1 /* flush */))
>> Frame 34: 13 bytes on wire (104 bits), 13 bytes captured (104 bits)
>> Bluetooth HCI H4
>> Bluetooth HCI ACL Packet
>> .... 0000 0000 0010 = Connection Handle: 0x0002
>> ..10 .... .... .... = PB Flag: First Automatically Flushable Packet (2)
>> 00.. .... .... .... = BC Flag: Point-To-Point (0)
>> Data Total Length: 8
>> Bluetooth L2CAP Packet
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> ---
>> include/net/bluetooth/bluetooth.h | 5 ++++
>> include/net/bluetooth/hci.h | 2 +
>> include/net/bluetooth/hci_core.h | 1 +
>> include/net/bluetooth/l2cap.h | 2 +
>> net/bluetooth/hci_core.c | 6 +++-
>> net/bluetooth/l2cap.c | 48 ++++++++++++++++++++++++++++++++++--
>> 6 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 0c5e725..ed7d775 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -64,6 +64,11 @@ struct bt_security {
>>
>> #define BT_DEFER_SETUP 7
>>
>> +#define BT_FLUSHABLE 8
>> +
>> +#define BT_FLUSHABLE_OFF 0
>> +#define BT_FLUSHABLE_ON 1
>> +
>> #define BT_INFO(fmt, arg...) printk(KERN_INFO "Bluetooth: " fmt "\n" , ## arg)
>> #define BT_ERR(fmt, arg...) printk(KERN_ERR "%s: " fmt "\n" , __func__ , ## arg)
>> #define BT_DBG(fmt, arg...) pr_debug("%s: " fmt "\n" , __func__ , ## arg)
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index 29a7a8c..333d5cb 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -150,6 +150,7 @@ enum {
>> #define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
>>
>> /* ACL flags */
>> +#define ACL_START_NO_FLUSH 0x00
>> #define ACL_CONT 0x01
>> #define ACL_START 0x02
>> #define ACL_ACTIVE_BCAST 0x04
>> @@ -193,6 +194,7 @@ enum {
>> #define LMP_EDR_ESCO_3M 0x40
>> #define LMP_EDR_3S_ESCO 0x80
>>
>> +#define LMP_NO_FLUSH 0x01
>
> Isn't this 0x40? As stated on Core v4.0 (Volume 2, part C, 3.3 Feature Mask
> Definition)
>
>> #define LMP_SIMPLE_PAIR 0x08
>>
>> /* Connection modes */
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 1992fac..9778bc8 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -456,6 +456,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_sniffsubr_capable(dev) ((dev)->features[5] & LMP_SNIFF_SUBR)
>> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
>> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
>> +#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
>>
>> /* ----- HCI protocols ----- */
>> struct hci_proto {
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 7ad25ca..af35711 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -75,6 +75,7 @@ struct l2cap_conninfo {
>> #define L2CAP_LM_TRUSTED 0x0008
>> #define L2CAP_LM_RELIABLE 0x0010
>> #define L2CAP_LM_SECURE 0x0020
>> +#define L2CAP_LM_FLUSHABLE 0x0040
>
> Not using this flag in the code.
>
>>
>> /* L2CAP command codes */
>> #define L2CAP_COMMAND_REJ 0x01
>> @@ -327,6 +328,7 @@ struct l2cap_pinfo {
>> __u8 sec_level;
>> __u8 role_switch;
>> __u8 force_reliable;
>> + __u8 flushable;
>>
>> __u8 conf_req[64];
>> __u8 conf_len;
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 51c61f7..c0d776b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1380,7 +1380,7 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>>
>> skb->dev = (void *) hdev;
>> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
>> + hci_add_acl_hdr(skb, conn->handle, flags);
>>
>> list = skb_shinfo(skb)->frag_list;
>> if (!list) {
>> @@ -1398,12 +1398,14 @@ void hci_send_acl(struct hci_conn *conn, struct sk_buff *skb, __u16 flags)
>> spin_lock_bh(&conn->data_q.lock);
>>
>> __skb_queue_tail(&conn->data_q, skb);
>
> add an empty line here.
>
>> + flags &= ~ACL_START;
>> + flags |= ACL_CONT;
>> do {
>> skb = list; list = list->next;
>>
>> skb->dev = (void *) hdev;
>> bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
>> - hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
>> + hci_add_acl_hdr(skb, conn->handle, flags);
>>
>> BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index c791fcd..efa60eb 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -362,13 +362,19 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
>> static inline void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, void *data)
>> {
>> struct sk_buff *skb = l2cap_build_cmd(conn, code, ident, len, data);
>> + u8 flags;
>>
>> BT_DBG("code 0x%2.2x", code);
>>
>> if (!skb)
>> return;
>>
>> - hci_send_acl(conn->hcon, skb, 0);
>> + if (lmp_no_flush_capable(conn->hcon->hdev))
>> + flags = ACL_START_NO_FLUSH;
>> + else
>> + flags = ACL_START;
>> +
>> + hci_send_acl(conn->hcon, skb, flags);
>> }
>>
>> static inline void l2cap_send_sframe(struct l2cap_pinfo *pi, u16 control)
>> @@ -900,6 +906,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>> pi->sec_level = l2cap_pi(parent)->sec_level;
>> pi->role_switch = l2cap_pi(parent)->role_switch;
>> pi->force_reliable = l2cap_pi(parent)->force_reliable;
>> + pi->flushable = l2cap_pi(parent)->flushable;
>> } else {
>> pi->imtu = L2CAP_DEFAULT_MTU;
>> pi->omtu = 0;
>> @@ -915,6 +922,7 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>> pi->sec_level = BT_SECURITY_LOW;
>> pi->role_switch = 0;
>> pi->force_reliable = 0;
>> + pi->flushable = BT_FLUSHABLE_OFF;
>
> You need to check for the no_flush feature here, right?
we have here uninitialized socket with no access to conn->hcon->hdev.
I am thinking about adding check below:
if (parent) {
...
if (lmp_no_flush_capable(hcon->hdev))
pi->flushable = BT_FLUSHABLE_OFF;
else
pi->flushable = BT_FLUSHABLE_ON;
else {
...
pi->flushable = BT_FLUSHABLE_OFF;
}
then we could skip checking for lmp_no_flush_capable in l2cap_do_send
Regards,
Andrei
>
>> }
>>
>> /* Default config options */
>> @@ -1450,10 +1458,17 @@ static void l2cap_drop_acked_frames(struct sock *sk)
>> static inline void l2cap_do_send(struct sock *sk, struct sk_buff *skb)
>> {
>> struct l2cap_pinfo *pi = l2cap_pi(sk);
>> + struct hci_conn *hcon = pi->conn->hcon;
>> + u16 flags;
>>
>> BT_DBG("sk %p, skb %p len %d", sk, skb, skb->len);
>>
>> - hci_send_acl(pi->conn->hcon, skb, 0);
>> + if (lmp_no_flush_capable(hcon->hdev) && !l2cap_pi(sk)->flushable)
>> + flags = ACL_START_NO_FLUSH;
>> + else
>> + flags = ACL_START;
>> +
>> + hci_send_acl(hcon, skb, flags);
>
> There is another call to hci_send_acl() in l2cap. It is in
> l2cap_send_sframe(), but I'm still wondering if we shall flush those packets
> or not in the case flushable was set. Now I'm thinking that don't.
>
> Then you need to add the same check in l2cap_send_cmd to l2cap_send_sframe().
>
>> }
>>
>> static void l2cap_streaming_send(struct sock *sk)
>> @@ -2045,6 +2060,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
>> static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, char __user *optval, unsigned int optlen)
>> {
>> struct sock *sk = sock->sk;
>> + struct hci_conn *hcon = l2cap_pi(sk)->conn->hcon;
>> struct bt_security sec;
>> int len, err = 0;
>> u32 opt;
>> @@ -2098,6 +2114,26 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>> bt_sk(sk)->defer_setup = opt;
>> break;
>>
>> + case BT_FLUSHABLE:
>> + if (get_user(opt, (u32 __user *) optval)) {
>> + err = -EFAULT;
>> + break;
>> + }
>> +
>> + if (opt > BT_FLUSHABLE_ON) {
>> + err = -EINVAL;
>> + break;
>> + }
>> +
>> + if (opt == BT_FLUSHABLE_OFF &&
>> + !lmp_no_flush_capable(hcon->hdev)) {
>
> I'm not really happy with !lmp_no_flush... 2 negatives in the same place, but
> that is the feature name. So we can't do too much here. Btw, the "No" in the
> feature name is making my brain hurt. ;)
>
> regards,
>
> --
> Gustavo F. Padovan
> http://profusion.mobi
>
^ permalink raw reply
* Re: [PATCH 4/4] Fix leak of EIR data if RSSI does not change
From: Johan Hedberg @ 2010-12-27 15:11 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-4-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> --- a/src/event.c
> +++ b/src/event.c
> @@ -452,9 +452,13 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
>
> rssi = *(info->data + info->length);
>
> - adapter_update_device_from_info(adapter, info->bdaddr, rssi,
> + if (!adapter_update_device_from_info(adapter, info->bdaddr, rssi,
> info->evt_type, eir_data.name,
> - eir_data.services, eir_data.flags);
> + eir_data.services, eir_data.flags)) {
> + g_slist_foreach(eir_data.services, (GFunc) g_free, NULL);
> + g_slist_free(eir_data.services);
> + g_free(eir_data.name);
> + }
> }
>
> static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
> @@ -556,8 +560,12 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> } else if (eir_data.name != NULL)
> g_free(eir_data.name);
>
> - adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
> - legacy, eir_data.services, name_status);
> + if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
> + alias, legacy, eir_data.services,
> + name_status)) {
> + g_slist_foreach(eir_data.services, (GFunc) g_free, NULL);
> + g_slist_free(eir_data.services);
> + }
>
> g_free(name);
> g_free(alias);
In general this seems fine, but to avoid future memory leaks in case you
go ahead and change the eir_data struct wouldn't it be better to have a
separate free_eir_data() function which does the g_slist_foreach,
g_slist_free and g_free?
Johan
^ permalink raw reply
* Re: [PATCH 3/4] Remove outdated comments
From: Johan Hedberg @ 2010-12-27 15:09 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-3-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> ---
> src/event.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
The patch has been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 1/4] Remove ancient NAME_SENT name resolution status
From: Johan Hedberg @ 2010-12-27 15:08 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> The NAME_SENT status was introduced on commit
> d6a16516a9f6deae8342f00e8186b02d0019a1e1, when there was a
> "RemoteNameUpdate" D-Bus signal. Nowadays, there is no such signal, and
> the device name (if any) is always sent on "DeviceFound" signal.
> ---
> src/adapter.h | 1 -
> src/event.c | 19 -------------------
> 2 files changed, 0 insertions(+), 20 deletions(-)
Thanks. The patch has been pushed upstream.
Johan
^ permalink raw reply
* Re: [PATCH] Fix printing D-Bus errors when headset record could not be found
From: Johan Hedberg @ 2010-12-27 15:07 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1293452841-7141-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Mon, Dec 27, 2010, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> When connection is started via headset_config_stream there is no D-Bus
> message to reply to.
> ---
> audio/headset.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Johan Hedberg @ 2010-12-27 14:53 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: Luiz Augusto von Dentz, linux-bluetooth
In-Reply-To: <AANLkTi=06P4ZpGO-sp7czZtn9FGLkBwPnrxNqDdGspfy@mail.gmail.com>
Hi,
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> > if the variable name is set
> > than your could would skip eir name completely while the code did
> > actually use the eir name, probably the eir name is updated more
> > frequently so it is probably the most updated one and we should in
> > fact update the storage if they don't match.
>
> I could not find on spec any mention that the "complete" EIR name may
> change.
It also doesn't say that it will not change. So the safe thing would be
not to make any assumption about its permanence, right?
Naturally the user of the remote device might change is name (if the
remote device has such a feature) and then the EIR data would change
too. Note that even if there's a name in storage it doesn't mean that
it's from the same discovery session. It might be from a previous
discovery session a long time ago and the user of the remote device
might have changed its name since then. So imho a complete name in an
EIR should always override whatever is in storage.
For the case of shortened names I don't think there's an indisputably
right answer. The question is what is better: that which is "complete"
or that which is more recent. I've got the feeling that the complete
version would be better for the user, but someone might of course
disagree with this.
Johan
^ permalink raw reply
* Re: [PATCH 2/4] Two merges very similar functions in bluetooth.c
From: Anderson Lizardo @ 2010-12-27 14:41 UTC (permalink / raw)
To: Michal.Labedzki; +Cc: linux-bluetooth
In-Reply-To: <AANLkTi=vSPvd7nFb=aus4Gpkkp3MFdiDv=HwndhYdUvB@mail.gmail.com>
On Mon, Dec 27, 2010 at 10:39 AM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi,
>
> On Mon, Dec 27, 2010 at 10:35 AM, <Michal.Labedzki@tieto.com> wrote:
>> On Mon, Dec 27, 2010, Anderson Lizardo wrote:
>>> IMHO, you should keep str2ba and drop the few occurrences of
>>> strtoba(). Same applies to ba2str/batostr.
>>
>>> strtoba() allocates memory by itself, so if you use it instead, you
>>> need to deallocate memory. str2ba(), on the other hand, uses a buffer
>>> given as argument.
>>
>> These function are similar, but not the same. ba2str/str2ba reverse bdaddr, but
>> batostr/strtoba not. I wrote relevant documentation comments, because names look
>> similar, code look similar, but results are different.
>
> Ok, I confess the function names are confusing. One has to look at
> their implementation to make sure which one to use...
Just one more comment: on your commit description, it may appear that
you are merging the functions, while in fact you are just factoring
out common code. I suggest rewording the message so it makes clear you
are factoring out code.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Two merges very similar functions in bluetooth.c
From: Anderson Lizardo @ 2010-12-27 14:39 UTC (permalink / raw)
To: Michal.Labedzki; +Cc: linux-bluetooth
In-Reply-To: <E50901D4F2CF69428D43141B7C8586790B4B2D056C@EXMB03.eu.tieto.com>
Hi,
On Mon, Dec 27, 2010 at 10:35 AM, <Michal.Labedzki@tieto.com> wrote:
> On Mon, Dec 27, 2010, Anderson Lizardo wrote:
>> IMHO, you should keep str2ba and drop the few occurrences of
>> strtoba(). Same applies to ba2str/batostr.
>
>> strtoba() allocates memory by itself, so if you use it instead, you
>> need to deallocate memory. str2ba(), on the other hand, uses a buffer
>> given as argument.
>
> These function are similar, but not the same. ba2str/str2ba reverse bdaddr, but
> batostr/strtoba not. I wrote relevant documentation comments, because names look
> similar, code look similar, but results are different.
Ok, I confess the function names are confusing. One has to look at
their implementation to make sure which one to use...
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Two merges very similar functions in bluetooth.c
From: Michal.Labedzki @ 2010-12-27 14:35 UTC (permalink / raw)
To: anderson.lizardo; +Cc: linux-bluetooth
In-Reply-To: <AANLkTik-sV2+k8_j6xt5F1MAPb-7t9K8JTEzm12f--c4@mail.gmail.com>
On Mon, Dec 27, 2010, Anderson Lizardo wrote:
> IMHO, you should keep str2ba and drop the few occurrences of
> strtoba(). Same applies to ba2str/batostr.
> strtoba() allocates memory by itself, so if you use it instead, you
> need to deallocate memory. str2ba(), on the other hand, uses a buffer
> given as argument.
These function are similar, but not the same. ba2str/str2ba reverse bdaddr, but
batostr/strtoba not. I wrote relevant documentation comments, because names look
similar, code look similar, but results are different.
Regards
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 14:35 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikcZRjC2KwOpHSqS_6wMcT8skKBMbhMzs=ugK+F@mail.gmail.com>
On Mon, Dec 27, 2010 at 10:14 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> It seems your path changes more than just ignore the shortened names,
> which I believed was the purpose here
Note that it does not completely ignore shortened names. If no
complete EIR name has been received yet, it will use the shortened
name (but not store it, as it is temporary). Once a complete name has
been received, it will be stored and used from now on. Actually this
has been the original behavior regarding storage, my patch simply
makes sure any further complete names are ignored, assuming the first
"complete" one is definitive and that the device is not supposed to
rename the "complete" name.
> if the variable name is set
> than your could would skip eir name completely while the code did
> actually use the eir name, probably the eir name is updated more
> frequently so it is probably the most updated one and we should in
> fact update the storage if they don't match.
I could not find on spec any mention that the "complete" EIR name may
change. Note that the original code only store complete EIR name and
the name coming from name resolving.
> Now about the shortened, Im not completely sure if we should ignore it
> or just append something to it, like e.g. ... or ~1, in case the spec
> say it is always the prefix we can at least use it as an hint if the
> name we have resolved does have changed.
As I mentioned above, the shortened names are only ignored after a
"complete" EIR name has been received. I think this is a sane
behavior, assuming the shortened names are temporary (e.g. while name
resolution is not possible or while the device could not send the
complete name using EIR).
I've not found any references to the spec saying the shortened name
has to be a prefix. I believe it is left to the implementer how to
display such shortened names. bluez so far has made no distinction on
the display of shortened names vs. complete names.
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Luiz Augusto von Dentz @ 2010-12-27 14:14 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1293456068-11615-2-git-send-email-anderson.lizardo@openbossa.org>
Hi,
On Mon, Dec 27, 2010 at 3:21 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> The stored device name comes from either name resolution or from a
> "complete" EIR name. If available, use it and ignore any further name
> present on EIR data.
> ---
> src/event.c | 16 ++++------------
> 1 files changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/src/event.c b/src/event.c
> index 6598e37..f445dd1 100644
> --- a/src/event.c
> +++ b/src/event.c
> @@ -549,22 +549,14 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
> } else
> legacy = TRUE;
>
> - if (eir_data.name) {
> + if (name == NULL && eir_data.name != NULL) {
> if (eir_data.name_complete) {
> write_device_name(local, peer, eir_data.name);
> name_status = NAME_NOT_REQUIRED;
> -
> - if (name)
> - g_free(name);
> -
> - name = eir_data.name;
> - } else {
> - if (name)
> - free(eir_data.name);
> - else
> - name = eir_data.name;
> }
> - }
> + name = eir_data.name;
> + } else if (eir_data.name != NULL)
> + g_free(eir_data.name);
>
> adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
> legacy, eir_data.services, name_status);
It seems your path changes more than just ignore the shortened names,
which I believed was the purpose here, if the variable name is set
than your could would skip eir name completely while the code did
actually use the eir name, probably the eir name is updated more
frequently so it is probably the most updated one and we should in
fact update the storage if they don't match.
Now about the shortened, Im not completely sure if we should ignore it
or just append something to it, like e.g. ... or ~1, in case the spec
say it is always the prefix we can at least use it as an hint if the
name we have resolved does have changed.
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* [PATCH 4/4] Fix leak of EIR data if RSSI does not change
From: Anderson Lizardo @ 2010-12-27 13:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-1-git-send-email-anderson.lizardo@openbossa.org>
If RSSI value does not change, memory used by parsed EIR data would leak
because it would not be assigned to the remote_dev_info structure.
---
src/adapter.c | 19 ++++++++++++-------
src/adapter.h | 11 ++++++-----
src/event.c | 16 ++++++++++++----
3 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index fddf0ad..aff1262 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2917,7 +2917,7 @@ static void remove_same_uuid(gpointer data, gpointer user_data)
}
}
-void adapter_update_device_from_info(struct btd_adapter *adapter,
+gboolean adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr, int8_t rssi,
uint8_t evt_type, char *name,
GSList *services, uint8_t flags)
@@ -2931,7 +2931,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
dev->le = TRUE;
dev->evt_type = evt_type;
} else if (dev->rssi == rssi)
- return;
+ return FALSE;
dev->rssi = rssi;
@@ -2951,12 +2951,15 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
/* FIXME: check if other information was changed before emitting the
* signal */
adapter_emit_device_found(adapter, dev);
+
+ return TRUE;
}
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- GSList *services, name_status_t name_status)
+gboolean adapter_update_found_devices(struct btd_adapter *adapter,
+ bdaddr_t *bdaddr, int8_t rssi, uint32_t class,
+ const char *name, const char *alias,
+ gboolean legacy, GSList *services,
+ name_status_t name_status)
{
struct remote_dev_info *dev;
gboolean new_dev;
@@ -2975,7 +2978,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev->legacy = legacy;
dev->name_status = name_status;
} else if (dev->rssi == rssi)
- return;
+ return FALSE;
dev->rssi = rssi;
@@ -2986,6 +2989,8 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev->services = g_slist_concat(dev->services, services);
adapter_emit_device_found(adapter, dev);
+
+ return TRUE;
}
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index 857eec8..cee0bd4 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -134,14 +134,15 @@ int adapter_get_state(struct btd_adapter *adapter);
int adapter_get_discover_type(struct btd_adapter *adapter);
struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
struct remote_dev_info *match);
-void adapter_update_device_from_info(struct btd_adapter *adapter,
+gboolean adapter_update_device_from_info(struct btd_adapter *adapter,
bdaddr_t bdaddr, int8_t rssi,
uint8_t evt_type, char *name,
GSList *services, uint8_t flags);
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- GSList *services, name_status_t name_status);
+gboolean adapter_update_found_devices(struct btd_adapter *adapter,
+ bdaddr_t *bdaddr, int8_t rssi, uint32_t class,
+ const char *name, const char *alias,
+ gboolean legacy, GSList *services,
+ name_status_t name_status);
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
void adapter_emit_device_found(struct btd_adapter *adapter,
struct remote_dev_info *dev);
diff --git a/src/event.c b/src/event.c
index 15fac94..795686e 100644
--- a/src/event.c
+++ b/src/event.c
@@ -452,9 +452,13 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
rssi = *(info->data + info->length);
- adapter_update_device_from_info(adapter, info->bdaddr, rssi,
+ if (!adapter_update_device_from_info(adapter, info->bdaddr, rssi,
info->evt_type, eir_data.name,
- eir_data.services, eir_data.flags);
+ eir_data.services, eir_data.flags)) {
+ g_slist_foreach(eir_data.services, (GFunc) g_free, NULL);
+ g_slist_free(eir_data.services);
+ g_free(eir_data.name);
+ }
}
static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
@@ -556,8 +560,12 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else if (eir_data.name != NULL)
g_free(eir_data.name);
- adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
- legacy, eir_data.services, name_status);
+ if (!adapter_update_found_devices(adapter, peer, rssi, class, name,
+ alias, legacy, eir_data.services,
+ name_status)) {
+ g_slist_foreach(eir_data.services, (GFunc) g_free, NULL);
+ g_slist_free(eir_data.services);
+ }
g_free(name);
g_free(alias);
--
1.7.0.4
^ permalink raw reply related
* [PATCH 3/4] Remove outdated comments
From: Anderson Lizardo @ 2010-12-27 13:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-1-git-send-email-anderson.lizardo@openbossa.org>
---
src/event.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/src/event.c b/src/event.c
index f445dd1..15fac94 100644
--- a/src/event.c
+++ b/src/event.c
@@ -444,7 +444,6 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
return;
}
- /* Extract UUIDs from advertising data if any */
memset(&eir_data, 0, sizeof(eir_data));
err = parse_eir_data(&eir_data, info->data, info->length);
if (err < 0)
@@ -518,7 +517,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
adapter_set_state(adapter, state);
}
- /* Extract UUIDs from extended inquiry response if any */
memset(&eir_data, 0, sizeof(eir_data));
err = parse_eir_data(&eir_data, data, EIR_DATA_LENGTH);
if (err < 0)
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/4] Use stored device name (if any) instead of EIR data
From: Anderson Lizardo @ 2010-12-27 13:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1293456068-11615-1-git-send-email-anderson.lizardo@openbossa.org>
The stored device name comes from either name resolution or from a
"complete" EIR name. If available, use it and ignore any further name
present on EIR data.
---
src/event.c | 16 ++++------------
1 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/src/event.c b/src/event.c
index 6598e37..f445dd1 100644
--- a/src/event.c
+++ b/src/event.c
@@ -549,22 +549,14 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else
legacy = TRUE;
- if (eir_data.name) {
+ if (name == NULL && eir_data.name != NULL) {
if (eir_data.name_complete) {
write_device_name(local, peer, eir_data.name);
name_status = NAME_NOT_REQUIRED;
-
- if (name)
- g_free(name);
-
- name = eir_data.name;
- } else {
- if (name)
- free(eir_data.name);
- else
- name = eir_data.name;
}
- }
+ name = eir_data.name;
+ } else if (eir_data.name != NULL)
+ g_free(eir_data.name);
adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
legacy, eir_data.services, name_status);
--
1.7.0.4
^ permalink raw reply related
* [PATCH 1/4] Remove ancient NAME_SENT name resolution status
From: Anderson Lizardo @ 2010-12-27 13:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
The NAME_SENT status was introduced on commit
d6a16516a9f6deae8342f00e8186b02d0019a1e1, when there was a
"RemoteNameUpdate" D-Bus signal. Nowadays, there is no such signal, and
the device name (if any) is always sent on "DeviceFound" signal.
---
src/adapter.h | 1 -
src/event.c | 19 -------------------
2 files changed, 0 insertions(+), 20 deletions(-)
diff --git a/src/adapter.h b/src/adapter.h
index ab83011..857eec8 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -61,7 +61,6 @@ typedef enum {
NAME_NOT_REQUIRED, /* used by get remote name without name resolving */
NAME_REQUIRED, /* remote name needs be resolved */
NAME_REQUESTED, /* HCI remote name request was sent */
- NAME_SENT /* D-Bus signal RemoteNameUpdated sent */
} name_status_t;
struct btd_adapter;
diff --git a/src/event.c b/src/event.c
index cfc47bf..6598e37 100644
--- a/src/event.c
+++ b/src/event.c
@@ -487,7 +487,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
struct btd_adapter *adapter;
struct btd_device *device;
char local_addr[18], peer_addr[18], *alias, *name;
- struct remote_dev_info *dev, match;
name_status_t name_status;
struct eir_data eir_data;
int state, err;
@@ -525,20 +524,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
if (err < 0)
error("Error parsing EIR data: %s (%d)", strerror(-err), -err);
- memset(&match, 0, sizeof(struct remote_dev_info));
- bacpy(&match.bdaddr, peer);
- match.name_status = NAME_SENT;
- /* if found: don't send the name again */
- dev = adapter_search_found_devices(adapter, &match);
- if (dev) {
- g_free(eir_data.name);
- adapter_update_found_devices(adapter, peer, rssi, class,
- NULL, NULL, dev->legacy,
- eir_data.services,
- NAME_NOT_REQUIRED);
- return;
- }
-
/* the inquiry result can be triggered by NON D-Bus client */
if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
adapter_has_discov_sessions(adapter))
@@ -581,10 +566,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
}
}
- if (name && eir_data.name_complete)
- name_status = NAME_SENT;
-
- /* add in the list to track name sent/pending */
adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
legacy, eir_data.services, name_status);
--
1.7.0.4
^ permalink raw reply related
* [PATCH] Fix printing D-Bus errors when headset record could not be found
From: Luiz Augusto von Dentz @ 2010-12-27 12:27 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
When connection is started via headset_config_stream there is no D-Bus
message to reply to.
---
audio/headset.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/audio/headset.c b/audio/headset.c
index 72bf5b6..53a594d 100644
--- a/audio/headset.c
+++ b/audio/headset.c
@@ -1445,7 +1445,8 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
error("Unable to get service record: %s (%d)",
strerror(-err), -err);
p->err = -err;
- error_connect_failed(dev->conn, p->msg, p->err);
+ if (p->msg)
+ error_connect_failed(dev->conn, p->msg, p->err);
goto failed;
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/4] Filtering interface name from program arguments
From: Anderson Lizardo @ 2010-12-27 12:13 UTC (permalink / raw)
To: Michal Labedzki; +Cc: linux-bluetooth
In-Reply-To: <1293447745-8697-1-git-send-email-michal.labedzki@tieto.com>
Hi,
On Mon, Dec 27, 2010 at 7:02 AM, Michal Labedzki
<michal.labedzki@tieto.com> wrote:
> +/* return 1 if string is ecimal number without leading zeros
> + * return 0 if not */
Typo: ecimal -> decimal
> +static int is_devnumber(const char *c)
> +{
> + if (c[0] == '0' && c[1] != 0)
> + return 0;
> +
> + while (*c) {
> + if (*c < '0' || *c > '9')
> + return 0;
> + ++c;
> + }
> + return 1;
> +}
> +
> +/* return HCI dev_id from string like "hciX", where X is dev_id
> + * return -1 if string not match */
Suggestion: "return -1 if str has invalid format"
> +int hci_filter_devname(const char *str)
> +{
> + int dev_id;
> +
> + if ((strlen(str) >= 4) && (!strncmp(str, "hci", 3)) &&
> + (is_devnumber(str + 3)))
> + dev_id = atoi(str + 3);
> + else
> + dev_id = -1;
> +
> + if (dev_id >= HCI_MAX_DEV)
> + dev_id = -1;
> +
> + return dev_id;
> +}
> +
> +/* return dev_id, which is on UP state, from 'hciX' or 'bdaddr'
> + * otherwise return -1 */
> int hci_devid(const char *str)
> {
> bdaddr_t ba;
> - int id = -1;
> + int dev_id;
>
> - if (!strncmp(str, "hci", 3) && strlen(str) >= 4) {
> - id = atoi(str + 3);
> - if (hci_devba(id, &ba) < 0)
> - return -1;
> - } else {
> + dev_id = hci_filter_devname(str);
> + if (dev_id < 0) {
> errno = ENODEV;
> str2ba(str, &ba);
> - id = hci_for_each_dev(HCI_UP, __same_bdaddr, (long) &ba);
> + dev_id = hci_for_each_dev(HCI_UP, __same_bdaddr, (long) &ba);
> + } else {
> + if (hci_devba(dev_id, &ba) < 0)
> + return -1;
> }
Use "else if (hci_devba(...))" and drop the extra brackets.
>
> - return id;
> + return dev_id;
> }
> diff --git a/test/hciemu.c b/test/hciemu.c
> index a20374f..ae33d72 100644
> --- a/test/hciemu.c
> +++ b/test/hciemu.c
> @@ -1234,15 +1234,16 @@ int main(int argc, char *argv[])
> exit(1);
> }
>
> - if (strlen(argv[0]) > 3 && !strncasecmp(argv[0], "hci", 3)) {
> + dev = hci_filter_devname(argv[0]);
> + if (dev < 0) {
> + if (getbdaddrbyname(argv[0], &vdev.bdaddr) < 0)
> + exit(1);
> + } else {
> dev = hci_devid(argv[0]);
> if (dev < 0) {
> perror("Invalid device");
> exit(1);
> }
> - } else {
> - if (getbdaddrbyname(argv[0], &vdev.bdaddr) < 0)
> - exit(1);
> }
>
> if (detach) {
> diff --git a/test/l2test.c b/test/l2test.c
> index 17883a9..438ba39 100644
> --- a/test/l2test.c
> +++ b/test/l2test.c
> @@ -1101,6 +1101,7 @@ int main(int argc, char *argv[])
> {
> struct sigaction sa;
> int opt, sk, mode = RECV, need_addr = 0;
> + int devid;
>
> bacpy(&bdaddr, BDADDR_ANY);
>
> @@ -1175,10 +1176,11 @@ int main(int argc, char *argv[])
> break;
>
> case 'i':
> - if (!strncasecmp(optarg, "hci", 3))
> - hci_devba(atoi(optarg + 3), &bdaddr);
> - else
> + devid = hci_filter_devname(optarg);
> + if (devid < 0)
> str2ba(optarg, &bdaddr);
> + else
> + hci_devba(devid, &bdaddr);
> break;
>
> case 'P':
> diff --git a/test/rctest.c b/test/rctest.c
> index b3804f5..a828ad9 100644
> --- a/test/rctest.c
> +++ b/test/rctest.c
> @@ -579,7 +579,7 @@ static void usage(void)
> "\t-m multiple connects\n");
>
> printf("Options:\n"
> - "\t[-b bytes] [-i device] [-P channel] [-U uuid]\n"
> + "\t[-b bytes] [-i hciX|bdaddr] [-P channel] [-U uuid]\n"
> "\t[-L seconds] enabled SO_LINGER option\n"
> "\t[-W seconds] enable deferred setup\n"
> "\t[-B filename] use data packets from file\n"
> @@ -597,6 +597,7 @@ int main(int argc, char *argv[])
> {
> struct sigaction sa;
> int opt, sk, mode = RECV, need_addr = 0;
> + int devid;
>
> bacpy(&bdaddr, BDADDR_ANY);
>
> @@ -644,10 +645,11 @@ int main(int argc, char *argv[])
> break;
>
> case 'i':
> - if (!strncasecmp(optarg, "hci", 3))
> - hci_devba(atoi(optarg + 3), &bdaddr);
> - else
> + devid = hci_filter_devname(optarg);
> + if (devid < 0)
> str2ba(optarg, &bdaddr);
> + else
> + hci_devba(devid, &bdaddr);
> break;
>
> case 'P':
> diff --git a/tools/ciptool.c b/tools/ciptool.c
> index edce9da..ec602ef 100644
> --- a/tools/ciptool.c
> +++ b/tools/ciptool.c
> @@ -427,7 +427,7 @@ static void usage(void)
> "\n");
>
> printf("Options:\n"
> - "\t-i [hciX|bdaddr] Local HCI device or BD Address\n"
> + "\t-i <hciX|bdaddr> Local HCI device or BD Address\n"
> "\t-h, --help Display help\n"
> "\n");
>
> @@ -455,10 +455,11 @@ int main(int argc, char *argv[])
> while ((opt = getopt_long(argc, argv, "+i:h", main_options, NULL)) != -1) {
> switch(opt) {
> case 'i':
> - if (!strncmp(optarg, "hci", 3))
> - hci_devba(atoi(optarg + 3), &bdaddr);
> - else
> + i = hci_filter_devname(optarg);
> + if (i < 0)
> str2ba(optarg, &bdaddr);
> + else
> + hci_devba(i, &bdaddr);
> break;
> case 'h':
> usage();
> diff --git a/tools/hciconfig.c b/tools/hciconfig.c
> index f0ae11c..e20963d 100644
> --- a/tools/hciconfig.c
> +++ b/tools/hciconfig.c
> @@ -1849,7 +1849,13 @@ int main(int argc, char *argv[])
> exit(0);
> }
>
> - di.dev_id = atoi(argv[0] + 3);
> + i = hci_filter_devname(argv[0]);
> + if (i < 0) {
> + fprintf(stderr, "No valid device name.\n");
> + exit(1);
> + }
> + di.dev_id = i;
> +
> argc--; argv++;
>
> if (ioctl(ctl, HCIGETDEVINFO, (void *) &di)) {
> diff --git a/tools/hcitool.c b/tools/hcitool.c
> index d50adaf..dc70e63 100644
> --- a/tools/hcitool.c
> +++ b/tools/hcitool.c
> @@ -2560,8 +2560,8 @@ static void usage(void)
> printf("Usage:\n"
> "\thcitool [options] <command> [command parameters]\n");
> printf("Options:\n"
> - "\t--help\tDisplay help\n"
> - "\t-i dev\tHCI device\n");
> + "\t--help Display help\n"
> + "\t-i <hciX|bdaddr> Local HCI device or BD Address\n");
> printf("Commands:\n");
> for (i = 0; command[i].cmd; i++)
> printf("\t%-4s\t%s\n", command[i].cmd,
> diff --git a/tools/l2ping.c b/tools/l2ping.c
> index 29fb3d0..dd0ccbd 100644
> --- a/tools/l2ping.c
> +++ b/tools/l2ping.c
> @@ -255,7 +255,8 @@ static void usage(void)
> {
> printf("l2ping - L2CAP ping\n");
> printf("Usage:\n");
> - printf("\tl2ping [-i device] [-s size] [-c count] [-t timeout] [-d delay] [-f] [-r] [-v] <bdaddr>\n");
> + printf("\tl2ping [-i <hciX|bdaddr> local hci or bd address] [-s size]"
> + "[-c count] [-t timeout] [-d delay] [-f] [-r] [-v] <bdaddr>\n");
The change above looks strange. You could drop "local hci or bd
address" (it is implicit from "<hciX|bdaddr>").
> printf("\t-f Flood ping (delay = 0)\n");
> printf("\t-r Reverse ping\n");
> printf("\t-v Verify request and response payload\n");
> @@ -264,17 +265,18 @@ static void usage(void)
> int main(int argc, char *argv[])
> {
> int opt;
> -
> + int devid;
Add an empty line here.
> /* Default options */
> bacpy(&bdaddr, BDADDR_ANY);
>
> while ((opt=getopt(argc,argv,"i:d:s:c:t:frv")) != EOF) {
> switch(opt) {
> case 'i':
> - if (!strncasecmp(optarg, "hci", 3))
> - hci_devba(atoi(optarg + 3), &bdaddr);
> - else
> + devid = hci_filter_devname(optarg);
> + if (devid < 0)
> str2ba(optarg, &bdaddr);
> + else
> + hci_devba(devid, &bdaddr);
> break;
>
> case 'd':
> diff --git a/tools/main.c b/tools/main.c
> index 6800445..c000fba 100644
> --- a/tools/main.c
> +++ b/tools/main.c
> @@ -753,10 +753,11 @@ int main(int argc, char *argv[])
> while ((opt = getopt_long(argc, argv, "+i:f:rahAESML:", main_options, NULL)) != -1) {
> switch(opt) {
> case 'i':
> - if (strncmp(optarg, "hci", 3) == 0)
> - hci_devba(atoi(optarg + 3), &bdaddr);
> - else
> + dev_id = hci_filter_devname(optarg);
> + if (dev_id < 0)
> str2ba(optarg, &bdaddr);
> + else
> + hci_devba(dev_id, &bdaddr);
> break;
>
> case 'f':
> diff --git a/tools/sdptool.c b/tools/sdptool.c
> index 140a46a..c782e62 100644
> --- a/tools/sdptool.c
> +++ b/tools/sdptool.c
> @@ -4209,7 +4209,7 @@ static void usage(void)
> "\tsdptool [options] <command> [command parameters]\n");
> printf("Options:\n"
> "\t-h\t\tDisplay help\n"
> - "\t-i\t\tSpecify source interface\n");
> + "\t-i\t\tSpecify source interface or bdaddr\n");
>
> printf("Commands:\n");
> for (i = 0; command[i].cmd; i++)
> @@ -4242,10 +4242,11 @@ int main(int argc, char *argv[])
> while ((opt=getopt_long(argc, argv, "+i:h", main_options, NULL)) != -1) {
> switch(opt) {
> case 'i':
> - if (!strncmp(optarg, "hci", 3))
> - hci_devba(atoi(optarg + 3), &interface);
> - else
> + i = hci_filter_devname(optarg);
> + if (i < 0)
> str2ba(optarg, &interface);
> + else
> + hci_devba(i, &interface);
> break;
>
> case 'h':
> --
> 1.7.0.4
>
> --
> 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
>
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 3/4] Fix memory leaks after using strtoba, batostr
From: Anderson Lizardo @ 2010-12-27 11:53 UTC (permalink / raw)
To: Michal Labedzki; +Cc: linux-bluetooth
In-Reply-To: <1293447745-8697-3-git-send-email-michal.labedzki@tieto.com>
Hi,
This patch would not be necessary if you keep only str2ba() and
ba2str() (like I commented on patch 2/4).
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox