[PATCH 1/4] Remove ancient NAME_SENT name resolution status

[PATCH 1/4] Remove ancient NAME_SENT name resolution status

[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

[PATCH 2/4] Use stored device name (if any) instead of EIR data

[Anderson Lizardo]

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

[PATCH 3/4] Remove outdated comments

[Anderson Lizardo]

---
 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

[PATCH 4/4] Fix leak of EIR data if RSSI does not change

[Anderson Lizardo]

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

Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data

[Luiz Augusto von Dentz]

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

Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data

[Anderson Lizardo]

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

Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data

[Johan Hedberg]

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

Re: [PATCH 1/4] Remove ancient NAME_SENT name resolution status

[Johan Hedberg]

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

Re: [PATCH 3/4] Remove outdated comments

[Johan Hedberg]

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

Re: [PATCH 4/4] Fix leak of EIR data if RSSI does not change

[Johan Hedberg]

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

Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data

[Anderson Lizardo]

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

Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data

[Anderson Lizardo]

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

[PATCH v2] Fix leak of EIR data if RSSI does not change

[Anderson Lizardo]

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

Re: [PATCH 2/4] Use stored device name (if any) instead of EIR data

[Anderson Lizardo]

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

Re: [PATCH 4/4] Fix leak of EIR data if RSSI does not change

[Anderson Lizardo]

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

Re: [PATCH v2] Fix leak of EIR data if RSSI does not change

[Johan Hedberg]

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

Re: [PATCH v2] Fix leak of EIR data if RSSI does not change

[Anderson Lizardo]

Hi Johan,

On Mon, Dec 27, 2010 at 5:48 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> 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.

I can split into two patches without problem.

> 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).

You are right. Somehow I forgot about the shortened name case while
trying to refactor this part of the code.

I'll resend the patch with just the memory leak fix (taking care to
not introduce another one!) and if I still have any idea to simplify
this code (which I don't believe anymore) a second patch will come.

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

[PATCH v3] Fix leak of EIR data if RSSI does not change

[Anderson Lizardo]

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 |   20 ++++++++++++++------
 src/adapter.h |    6 +++---
 src/event.c   |   49 ++++++++++++++++++++++++++++---------------------
 3 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 4af11bc..b7a65dc 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2923,10 +2923,18 @@ static void remove_same_uuid(gpointer data, gpointer user_data)
 	}
 }
 
+static void dev_prepend_uuid(gpointer data, gpointer user_data)
+{
+	struct remote_dev_info *dev = user_data;
+	char *new_uuid = data;
+
+	dev->services = g_slist_prepend(dev->services, g_strdup(new_uuid));
+}
+
 void 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)
+					bdaddr_t bdaddr, int8_t rssi,
+					uint8_t evt_type, const char *name,
+					GSList *services, uint8_t flags)
 {
 	struct remote_dev_info *dev;
 	gboolean new_dev;
@@ -2945,13 +2953,13 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
 						(GCompareFunc) dev_rssi_cmp);
 
 	g_slist_foreach(services, remove_same_uuid, dev);
-	dev->services = g_slist_concat(dev->services, services);
+	g_slist_foreach(services, dev_prepend_uuid, dev);
 
 	dev->flags = flags;
 
 	if (name) {
 		g_free(dev->name);
-		dev->name = name;
+		dev->name = g_strdup(name);
 	}
 
 	/* FIXME: check if other information was changed before emitting the
@@ -2989,7 +2997,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 						(GCompareFunc) dev_rssi_cmp);
 
 	g_slist_foreach(services, remove_same_uuid, dev);
-	dev->services = g_slist_concat(dev->services, services);
+	g_slist_foreach(services, dev_prepend_uuid, dev);
 
 	adapter_emit_device_found(adapter, dev);
 }
diff --git a/src/adapter.h b/src/adapter.h
index 1fbc221..a655cdb 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -135,9 +135,9 @@ 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,
-						bdaddr_t bdaddr, int8_t rssi,
-						uint8_t evt_type, char *name,
-						GSList *services, uint8_t flags);
+					bdaddr_t bdaddr, int8_t rssi,
+					uint8_t evt_type, const 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,
diff --git a/src/event.c b/src/event.c
index 178cea8..b1054d7 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;
@@ -455,6 +462,8 @@ void btd_event_advertising_report(bdaddr_t *local, le_advertising_info *info)
 	adapter_update_device_from_info(adapter, info->bdaddr, rssi,
 					info->evt_type, eir_data.name,
 					eir_data.services, eir_data.flags);
+
+	free_eir_data(&eir_data);
 }
 
 static void update_lastseen(bdaddr_t *sba, bdaddr_t *dba)
@@ -491,6 +500,7 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	int state, err;
 	dbus_bool_t legacy;
 	unsigned char features[8];
+	const char *dev_name;
 
 	ba2str(local, local_addr);
 	ba2str(peer, peer_addr);
@@ -517,11 +527,6 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 		adapter_set_state(adapter, state);
 	}
 
-	memset(&eir_data, 0, sizeof(eir_data));
-	err = parse_eir_data(&eir_data, data, EIR_DATA_LENGTH);
-	if (err < 0)
-		error("Error parsing EIR data: %s (%d)", strerror(-err), -err);
-
 	/* the inquiry result can be triggered by NON D-Bus client */
 	if (adapter_get_discover_type(adapter) & DISC_RESOLVNAME &&
 				adapter_has_discov_sessions(adapter))
@@ -547,28 +552,30 @@ void btd_event_device_found(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	} else
 		legacy = TRUE;
 
-	if (eir_data.name) {
+	memset(&eir_data, 0, sizeof(eir_data));
+	err = parse_eir_data(&eir_data, data, EIR_DATA_LENGTH);
+	if (err < 0)
+		error("Error parsing EIR data: %s (%d)", strerror(-err), -err);
+
+	/* Complete EIR names are always used. Shortened EIR names are only
+	 * used if there is no name already in storage. */
+	dev_name = name;
+	if (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;
-		}
+			dev_name = eir_data.name;
+		} else if (name == NULL)
+			dev_name = eir_data.name;
 	}
 
-	adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
-					legacy, eir_data.services, name_status);
+	adapter_update_found_devices(adapter, peer, rssi, class, dev_name,
+					alias, legacy, eir_data.services,
+					name_status);
 
-	g_free(name);
-	g_free(alias);
+	free_eir_data(&eir_data);
+	free(name);
+	free(alias);
 }
 
 void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer,
-- 
1.7.0.4

Re: [PATCH v2] Fix leak of EIR data if RSSI does not change

[Anderson Lizardo]

Hi Johan,

On Mon, Dec 27, 2010 at 6:24 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> On Mon, Dec 27, 2010 at 5:48 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> 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.
>
> I can split into two patches without problem.

I just sent a v3 of this patch, which I hope fix all issues. It is
bigger than previous versions, but I assure it is solely to fix the
leak of EIR data. The other benefits are just "side effects":

* That if() clause which handles EIR name has been simplified, by
removing free()/g_free()
* I avoid passing ownership of memory allocated in src/event.c to
src/adapter.c, which would potentially become memory leaks if not
managed carefully.

I hope these side effects are not of a big issue to be kept in a single patch.

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH v3] Fix leak of EIR data if RSSI does not change

[Johan Hedberg]

Hi Lizardo,

On Tue, Dec 28, 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.
> ---
>  src/adapter.c |   20 ++++++++++++++------
>  src/adapter.h |    6 +++---
>  src/event.c   |   49 ++++++++++++++++++++++++++++---------------------
>  3 files changed, 45 insertions(+), 30 deletions(-)

Thanks. This latest version looks fine to me. The patch has been pushed
upstream.

Johan