[PATCH BlueZ 0/2] Store address type on storage

[PATCH BlueZ 0/2] Store address type on storage

[Paulo Alcantara]

BLE devices have two different address types: public or
random. So we need to provide this address type on keys in
the "primary" file to avoid duplicate BLE address types
around the BlueZ.

This series changes the storage to store the address type
together with the device address.

Hemant Gupta has been working on this on "Store LE device
address type with primary list" but his patch hasn't been
accepted and he didn't send a new reviewed version of that
patch.

I have patches for the other storage files as well, I'll send
them when this series is integrated upstream.

Claudio Takahasi (1):
  core: Fix creating device from "primary" file

Paulo Alcantara (1):
  storage: Store BLE address type in "primary" file

 src/adapter.c |   10 +++++++---
 src/device.c  |    2 +-
 src/storage.c |   30 ++++++++++++++++++++++++++----
 src/storage.h |    5 +++--
 4 files changed, 37 insertions(+), 10 deletions(-)

-- 
1.7.7.6

[PATCH BlueZ 1/2] storage: Store BLE address type in "primary" file

[Paulo Alcantara]

BLE addressing types can be either public or random so the entries in
the "primary" file did not contain enough information to distinguish
which addressing type it's supposed to be (LE public or LE random).
Entries will now contain both BLE address number and BLE address type
as a single key in every entry in the file.
---
 src/device.c  |    2 +-
 src/storage.c |   40 ++++++++++++++++++++++++++++++++--------
 src/storage.h |    5 +++--
 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/src/device.c b/src/device.c
index 2695b16..fd13bc3 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1759,7 +1759,7 @@ static void store_services(struct btd_device *device)
 	adapter_get_address(adapter, &sba);
 	device_get_address(device, &dba, NULL);
 
-	write_device_services(&sba, &dba, str);
+	write_device_services(&sba, &dba, device->bdaddr_type, str);
 
 	g_free(str);
 }
diff --git a/src/storage.c b/src/storage.c
index 14e5ac3..815060e 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -61,6 +61,19 @@ static inline int create_filename(char *buf, size_t size,
 	return create_name(buf, size, STORAGEDIR, addr, name);
 }
 
+static inline char ba_type2char(uint8_t bdaddr_type)
+{
+	switch (bdaddr_type) {
+	case BDADDR_LE_PUBLIC:
+		return '1';
+	case BDADDR_LE_RANDOM:
+		return '2';
+	case BDADDR_BREDR:
+	default:
+		return '0';
+	}
+}
+
 int read_device_alias(const char *src, const char *dst, char *alias, size_t size)
 {
 	char filename[PATH_MAX + 1], *tmp;
@@ -1162,17 +1175,22 @@ int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 }
 
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
-							const char *services)
+			  uint8_t bdaddr_type, const char *services)
 {
-	char filename[PATH_MAX + 1], addr[18];
+	char filename[PATH_MAX + 1], key[20];
 
 	create_filename(filename, PATH_MAX, sba, "primary");
 
 	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
-	ba2str(dba, addr);
+	memset(key, 0, sizeof(key));
 
-	return textfile_put(filename, addr, services);
+	/* New format: address#type */
+	ba2str(dba, key);
+	key[17] = '#';
+	key[18] = ba_type2char(bdaddr_type);
+
+	return textfile_put(filename, key, services);
 }
 
 static void filter_keys(char *key, char *value, void *data)
@@ -1228,15 +1246,21 @@ int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba)
 	return textfile_del(filename, address);
 }
 
-char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba)
+char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
+							uint8_t bdaddr_type)
 {
-	char filename[PATH_MAX + 1], addr[18];
+	char filename[PATH_MAX + 1], key[20];
 
 	create_filename(filename, PATH_MAX, sba, "primary");
 
-	ba2str(dba, addr);
+	memset(key, 0, sizeof(key));
 
-	return textfile_caseget(filename, addr);
+	/* New format: address#type */
+	ba2str(dba, key);
+	key[17] = '#';
+	key[18] = ba_type2char(bdaddr_type);
+
+	return textfile_caseget(filename, key);
 }
 
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
diff --git a/src/storage.h b/src/storage.h
index 6255ae8..745aff8 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -76,9 +76,10 @@ gboolean read_blocked(const bdaddr_t *local, const bdaddr_t *remote);
 int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 							gboolean blocked);
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
-							const char *services);
+				uint8_t bdaddr_type, const char *services);
 int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba);
-char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba);
+char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
+							uint8_t bdaddr_type);
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
 					uint16_t handle, const char *chars);
 char *read_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
-- 
1.7.7.6

Re: [PATCH BlueZ 1/2] storage: Store BLE address type in "primary" file

[Johan Hedberg]

Hi Paulo,

On Fri, May 18, 2012, Paulo Alcantara wrote:
> +static inline char ba_type2char(uint8_t bdaddr_type)
> +{
> +	switch (bdaddr_type) {
> +	case BDADDR_LE_PUBLIC:
> +		return '1';
> +	case BDADDR_LE_RANDOM:
> +		return '2';
> +	case BDADDR_BREDR:
> +	default:
> +		return '0';
> +	}
> +}
> +
>  int read_device_alias(const char *src, const char *dst, char *alias, size_t size)
>  {
>  	char filename[PATH_MAX + 1], *tmp;
> @@ -1162,17 +1175,22 @@ int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
>  }
>  
>  int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
> -							const char *services)
> +			  uint8_t bdaddr_type, const char *services)
>  {
> -	char filename[PATH_MAX + 1], addr[18];
> +	char filename[PATH_MAX + 1], key[20];
>  
>  	create_filename(filename, PATH_MAX, sba, "primary");
>  
>  	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  
> -	ba2str(dba, addr);
> +	memset(key, 0, sizeof(key));
>  
> -	return textfile_put(filename, addr, services);
> +	/* New format: address#type */
> +	ba2str(dba, key);
> +	key[17] = '#';
> +	key[18] = ba_type2char(bdaddr_type);

Wouldn't sprintf(&key[17], "#%hhu", bdaddr_type); be much simpler?

> +char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
> +							uint8_t bdaddr_type)
>  {
> -	char filename[PATH_MAX + 1], addr[18];
> +	char filename[PATH_MAX + 1], key[20];
>  
>  	create_filename(filename, PATH_MAX, sba, "primary");
>  
> -	ba2str(dba, addr);
> +	memset(key, 0, sizeof(key));
>  
> -	return textfile_caseget(filename, addr);
> +	/* New format: address#type */
> +	ba2str(dba, key);
> +	key[17] = '#';
> +	key[18] = ba_type2char(bdaddr_type);

Same here.

Johan

Re: [PATCH BlueZ 1/2] storage: Store BLE address type in "primary" file

[Paulo Alcantara]

Hi Johan,

From: Johan Hedberg <johan.hedberg@gmail.com>
Date: Sat, 19 May 2012 09:41:24 +0300

> > +	/* New format: address#type */
> > +	ba2str(dba, key);
> > +	key[17] = '#';
> > +	key[18] = ba_type2char(bdaddr_type);
> 
> Wouldn't sprintf(&key[17], "#%hhu", bdaddr_type); be much simpler?

Agreed. Fixed it.

> > +	/* New format: address#type */
> > +	ba2str(dba, key);
> > +	key[17] = '#';
> > +	key[18] = ba_type2char(bdaddr_type);
> 
> Same here.

Fixed it too.


		Paulo

[PATCH BlueZ 2/2] core: Fix creating device from "primary" file

[Paulo Alcantara]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch removes the hard-coded address type for the BLE device
created from the storage.
---
 src/adapter.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index dafe595..8d86ca8 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
 	struct btd_adapter *adapter = user_data;
 	struct btd_device *device;
 	GSList *services, *uuids, *l;
+	char address[18];
+	uint8_t bdaddr_type;
+
+	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) != 2)
+		return;
 
 	if (g_slist_find_custom(adapter->devices,
-			key, (GCompareFunc) device_address_cmp))
+			address, (GCompareFunc) device_address_cmp))
 		return;
 
-	/* FIXME: Get the correct LE addr type (public/random) */
-	device = device_create(connection, adapter, key, BDADDR_LE_PUBLIC);
+	device = device_create(connection, adapter, address, bdaddr_type);
 	if (!device)
 		return;
 
-- 
1.7.7.6

Re: [PATCH BlueZ 2/2] core: Fix creating device from "primary" file

[Johan Hedberg]

Hi,

On Fri, May 18, 2012, Paulo Alcantara wrote:
> This patch removes the hard-coded address type for the BLE device
> created from the storage.
> ---
>  src/adapter.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index dafe595..8d86ca8 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
>  	struct btd_adapter *adapter = user_data;
>  	struct btd_device *device;
>  	GSList *services, *uuids, *l;
> +	char address[18];
> +	uint8_t bdaddr_type;
> +
> +	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) != 2)
> +		return;

What about entries created by older bluetoothd versions? Will they be
stuck in the storage forever without a way to remove them through the
usual API? Instead of checking for != 2 maybe the check should instead
be < 1 and bdaddr_type be pre-initialized to BDADDR_LE_PUBLIC.

Johan

Re: [PATCH BlueZ 2/2] core: Fix creating device from "primary" file

[Paulo Alcantara]

Hi Johan,

From: Johan Hedberg <johan.hedberg@gmail.com>
Date: Sat, 19 May 2012 09:43:41 +0300

> Hi,
> 
> On Fri, May 18, 2012, Paulo Alcantara wrote:
> > This patch removes the hard-coded address type for the BLE device
> > created from the storage.
> > ---
> >  src/adapter.c |   10 +++++++---
> >  1 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/adapter.c b/src/adapter.c
> > index dafe595..8d86ca8 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
> >  	struct btd_adapter *adapter = user_data;
> >  	struct btd_device *device;
> >  	GSList *services, *uuids, *l;
> > +	char address[18];
> > +	uint8_t bdaddr_type;
> > +
> > +	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) != 2)
> > +		return;
> 
> What about entries created by older bluetoothd versions? Will they be
> stuck in the storage forever without a way to remove them through the
> usual API? Instead of checking for != 2 maybe the check should instead
> be < 1 and bdaddr_type be pre-initialized to BDADDR_LE_PUBLIC.

I agree with you that we must keep backward compatibility here with older
entries. The check here would be "< 2" (in case we only have device address and
not its address type type) instead, and then pre-initialize bdaddr_type to
BDADDR_LE_PUBLIC.

Fixed it, thanks.


		Paulo

[PATCH BlueZ v2 0/2] Address type in keys on storage filesystem

[Paulo Alcantara]

BLE devices have two different address types: public or
random. So we need to provide this address type on keys
in the "primary" file to avoid duplicate BLE address types
around the BlueZ.

This series changes the storage to store the address type
together with the device address.

Hemant Gupta has been working on this on "Store LE device address
type with primary list" but his patch hasn't been accepted and
he didn't send a new reviewed version of that patch.

I have patches for the other storage files as well, I'll
send them soon. There is a potential problem in
device_address_cmp() function which checks the address only.
I am investigating how to fix it to compare address and type
without breaking the current code.

v2's changes:
     - Use sprintf() instead of setting values by indexing
       strings.
     - Keep backward compatibility with entries created by
       older bluetoothd versions.

Claudio Takahasi (1):
  core: Fix creating device from "primary" file

Paulo Alcantara (1):
  storage: Store BLE address type in "primary" file

 src/adapter.c |   10 +++++++---
 src/device.c  |    2 +-
 src/storage.c |   32 ++++++++++++++++++++++++--------
 src/storage.h |    5 +++--
 4 files changed, 35 insertions(+), 14 deletions(-)

-- 
1.7.7.6

[PATCH BlueZ v2 1/2] storage: Store BLE address type in "primary" file

[Paulo Alcantara]

BLE addressing types can be either public or random so the entries in
the "primary" file did not contain enough information to distinguish
which addressing type it's supposed to be (LE public or LE random).
Entries will now contain both BLE address number and BLE address type
as a single key in every entry in the file.
---
 src/device.c  |    2 +-
 src/storage.c |   32 ++++++++++++++++++++++++--------
 src/storage.h |    5 +++--
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/device.c b/src/device.c
index b180579..2bb428c 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1759,7 +1759,7 @@ static void store_services(struct btd_device *device)
 	adapter_get_address(adapter, &sba);
 	device_get_address(device, &dba, NULL);
 
-	write_device_services(&sba, &dba, str);
+	write_device_services(&sba, &dba, device->bdaddr_type, str);
 
 	g_free(str);
 }
diff --git a/src/storage.c b/src/storage.c
index 14e5ac3..5603cb8 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -1162,17 +1162,21 @@ int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 }
 
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
-							const char *services)
+			  uint8_t bdaddr_type, const char *services)
 {
-	char filename[PATH_MAX + 1], addr[18];
+	char filename[PATH_MAX + 1], key[20];
 
 	create_filename(filename, PATH_MAX, sba, "primary");
 
 	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
-	ba2str(dba, addr);
+	memset(key, 0, sizeof(key));
+
+	/* New format: address#type */
+	ba2str(dba, key);
+	sprintf(&key[17], "#%hhu", bdaddr_type);
 
-	return textfile_put(filename, addr, services);
+	return textfile_put(filename, key, services);
 }
 
 static void filter_keys(char *key, char *value, void *data)
@@ -1228,15 +1232,27 @@ int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba)
 	return textfile_del(filename, address);
 }
 
-char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba)
+char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
+							uint8_t bdaddr_type)
 {
-	char filename[PATH_MAX + 1], addr[18];
+	char filename[PATH_MAX + 1], key[20], *str;
 
 	create_filename(filename, PATH_MAX, sba, "primary");
 
-	ba2str(dba, addr);
+	memset(key, 0, sizeof(key));
 
-	return textfile_caseget(filename, addr);
+	/* New format: address#type */
+	ba2str(dba, key);
+	sprintf(&key[17], "#%hhu", bdaddr_type);
+
+	str = textfile_caseget(filename, key);
+	if (str != NULL)
+		return str;
+
+	/* Old format: address only */
+	key[17] = '\0';
+
+	return textfile_caseget(filename, key);
 }
 
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
diff --git a/src/storage.h b/src/storage.h
index 6255ae8..745aff8 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -76,9 +76,10 @@ gboolean read_blocked(const bdaddr_t *local, const bdaddr_t *remote);
 int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 							gboolean blocked);
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
-							const char *services);
+				uint8_t bdaddr_type, const char *services);
 int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba);
-char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba);
+char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
+							uint8_t bdaddr_type);
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
 					uint16_t handle, const char *chars);
 char *read_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
-- 
1.7.7.6

Re: [PATCH BlueZ v2 1/2] storage: Store BLE address type in "primary" file

[Johan Hedberg]

Hi Paulo,

On Mon, May 21, 2012, Paulo Alcantara wrote:
> +	/* New format: address#type */
> +	ba2str(dba, key);
> +	sprintf(&key[17], "#%hhu", bdaddr_type);

Since BlueZ 4.100 will be the first user space version being able to use
mgmtops with a kernel where mgmt is enabled by default (3.4) I don't
think we need to worry about old format vs new format, i.e. the code
comment above is unnecessary.

> +	/* New format: address#type */
> +	ba2str(dba, key);
> +	sprintf(&key[17], "#%hhu", bdaddr_type);
> +
> +	str = textfile_caseget(filename, key);
> +	if (str != NULL)
> +		return str;
> +
> +	/* Old format: address only */
> +	key[17] = '\0';
> +
> +	return textfile_caseget(filename, key);

Same thing here. I think we can leave out any code that tries to work
with the old storage format since we can consider LE support as
experimental/unstable in all previous user space releases.

Johan

Re: [PATCH BlueZ v2 1/2] storage: Store BLE address type in "primary" file

[Paulo Alcantara]

Hi Johan,

From: Johan Hedberg <johan.hedberg@gmail.com>
Date: Tue, 22 May 2012 11:05:36 +0300

> > +	/* New format: address#type */
> > +	ba2str(dba, key);
> > +	sprintf(&key[17], "#%hhu", bdaddr_type);
> 
> Since BlueZ 4.100 will be the first user space version being able to use
> mgmtops with a kernel where mgmt is enabled by default (3.4) I don't
> think we need to worry about old format vs new format, i.e. the code
> comment above is unnecessary.

Ok. When we were implemeting this, our first thought was to just ignore the old
storage formats and then use the new storage one.

So, later we decided to keep backward compatibily with the old storage format
since one might not want to delete his old entries so that the patch wouldn't
work with this new storage format (the old keys would never be found).

I'll remove the comment above. Thanks for pointing it out.

> 
> > +	/* New format: address#type */
> > +	ba2str(dba, key);
> > +	sprintf(&key[17], "#%hhu", bdaddr_type);
> > +
> > +	str = textfile_caseget(filename, key);
> > +	if (str != NULL)
> > +		return str;
> > +
> > +	/* Old format: address only */
> > +	key[17] = '\0';
> > +
> > +	return textfile_caseget(filename, key);
> 
> Same thing here. I think we can leave out any code that tries to work
> with the old storage format since we can consider LE support as
> experimental/unstable in all previous user space releases.

Fair enough. I'll remove the comment above, as well the code that leads with the
old storage format. Thanks again!


		Paulo

[PATCH BlueZ v2 2/2] core: Fix creating device from "primary" file

[Paulo Alcantara]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch removes the hard-coded address type for the BLE device
created from the storage.
---
 src/adapter.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index dafe595..1ca21e6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
 	struct btd_adapter *adapter = user_data;
 	struct btd_device *device;
 	GSList *services, *uuids, *l;
+	char address[18];
+	uint8_t bdaddr_type;
+
+	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) < 2)
+		bdaddr_type = BDADDR_LE_PUBLIC;
 
 	if (g_slist_find_custom(adapter->devices,
-			key, (GCompareFunc) device_address_cmp))
+			address, (GCompareFunc) device_address_cmp))
 		return;
 
-	/* FIXME: Get the correct LE addr type (public/random) */
-	device = device_create(connection, adapter, key, BDADDR_LE_PUBLIC);
+	device = device_create(connection, adapter, address, bdaddr_type);
 	if (!device)
 		return;
 
-- 
1.7.7.6

Re: [PATCH BlueZ v2 2/2] core: Fix creating device from "primary" file

[Johan Hedberg]

Hi,

On Mon, May 21, 2012, Paulo Alcantara wrote:
> From: Claudio Takahasi <claudio.takahasi@openbossa.org>
> 
> This patch removes the hard-coded address type for the BLE device
> created from the storage.
> ---
>  src/adapter.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index dafe595..1ca21e6 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
>  	struct btd_adapter *adapter = user_data;
>  	struct btd_device *device;
>  	GSList *services, *uuids, *l;
> +	char address[18];
> +	uint8_t bdaddr_type;
> +
> +	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) < 2)
> +		bdaddr_type = BDADDR_LE_PUBLIC;

That's not safe. What if sscanf returns 0 or a negative value? In that
case the address variable will remain uninitialized (which is why you
should have just followed my suggestion of testing for < 1 and returning
in such a case).

Thinking more about this situation I'm not sure if it's any better to
allow creation of old entries since you won't be able to remove them
anyway: the remove code looks for a bdaddr#type key which won't exist
and adding code to look for both types of keys is just bloating the code
base for a minor benefit.

So maybe your initial patch of failing in the case of sscanf returning < 2
is good enough after all.

Johan

Re: [PATCH BlueZ v2 2/2] core: Fix creating device from "primary" file

[Claudio Takahasi]

Hi Johan,

On Tue, May 22, 2012 at 5:00 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Mon, May 21, 2012, Paulo Alcantara wrote:
>> From: Claudio Takahasi <claudio.takahasi@openbossa.org>
>>
>> This patch removes the hard-coded address type for the BLE device
>> created from the storage.
>> ---
>>  src/adapter.c |   10 +++++++---
>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index dafe595..1ca21e6 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
>>       struct btd_adapter *adapter = user_data;
>>       struct btd_device *device;
>>       GSList *services, *uuids, *l;
>> +     char address[18];
>> +     uint8_t bdaddr_type;
>> +
>> +     if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) < 2)
>> +             bdaddr_type = BDADDR_LE_PUBLIC;
>
> That's not safe. What if sscanf returns 0 or a negative value? In that
> case the address variable will remain uninitialized (which is why you
> should have just followed my suggestion of testing for < 1 and returning
> in such a case).

IMO, it is not necessary to add extra verification since only
bluetoothd changes this file.
It will be an unnecessary overhead.

>
> Thinking more about this situation I'm not sure if it's any better to
> allow creation of old entries since you won't be able to remove them
> anyway: the remove code looks for a bdaddr#type key which won't exist
> and adding code to look for both types of keys is just bloating the code
> base for a minor benefit.
>
> So maybe your initial patch of failing in the case of sscanf returning < 2
> is good enough after all.
>
> Johan

As you mentioned in the reply for "v2 1/2" of this patch series, LE
was enabled by default recently, and there isn't releases supporting
officially LE. Keep the compatibility with the old format will not
give a valuable benefit.
We will fix the other issues that you pointed, and send the patch series again.

Claudio.

[PATCH BlueZ v3 0/3] Store address type on storage

[Paulo Alcantara]

BLE devices have two different address types: public or
random. So we need to provide this address type on keys
in the "primary" file to avoid duplicate BLE address types
around the BlueZ.

This series changes the storage to store the address type
together with the device address.

Hemant Gupta has been working on this on "Store LE device address
type with primary list" but his patch hasn't been accepted and
he didn't send a new reviewed version of that patch.

I have patches for the other storage files as well, I'll
send them soon. There is a potential problem in
device_address_cmp() function which checks the address only.
I am investigating how to fix it to compare address and type
without breaking the current code.

v3's changes:
     - (1/3): Remove unnecessary comment about the new storage
       	      format and code that leads with the old storage
	      format.

     - (2/3): Just use the initial version of this patch (the
       	      returning < 2 check in sscanf() function).

     - (3/3): New patch that fixes removing device services from
       	      "primary" file by passing a key with the new
	      storage format in delete_device_service(). Note
	      also that this function was modified to only treat
	      the primary case - since we have old storage format
	      still being used in the other LE-related storage
	      files. So, it will need to be modified in order to
	      delete device services of other files (characteristics,
	      ccc, etc) on other series.


Claudio Takahasi (1):
  core: Fix creating device from "primary" file

Paulo Alcantara (2):
  storage: Store BLE address type in "primary" file
  core: Fix removing device services from "primary" file

 src/adapter.c |   10 +++++++---
 src/device.c  |    4 ++--
 src/storage.c |   45 ++++++++++++++++++++++++++-------------------
 src/storage.h |    8 +++++---
 4 files changed, 40 insertions(+), 27 deletions(-)

-- 
1.7.7.6

[PATCH BlueZ v3 1/3] storage: Store BLE address type in "primary" file

[Paulo Alcantara]

BLE addressing types can be either public or random so the entries in
the "primary" file did not contain enough information to distinguish
which addressing type it's supposed to be (LE public or LE random).
Entries will now contain both BLE address number and BLE address type
as a single key in every entry in the file.
---
 src/device.c  |    2 +-
 src/storage.c |   19 +++++++++++--------
 src/storage.h |    5 +++--
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/device.c b/src/device.c
index 2695b16..fd13bc3 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1759,7 +1759,7 @@ static void store_services(struct btd_device *device)
 	adapter_get_address(adapter, &sba);
 	device_get_address(device, &dba, NULL);
 
-	write_device_services(&sba, &dba, str);
+	write_device_services(&sba, &dba, device->bdaddr_type, str);
 
 	g_free(str);
 }
diff --git a/src/storage.c b/src/storage.c
index 14e5ac3..33934fb 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -1162,17 +1162,18 @@ int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 }
 
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
-							const char *services)
+			  uint8_t bdaddr_type, const char *services)
 {
-	char filename[PATH_MAX + 1], addr[18];
+	char filename[PATH_MAX + 1], key[20];
 
 	create_filename(filename, PATH_MAX, sba, "primary");
 
 	create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
-	ba2str(dba, addr);
+	ba2str(dba, key);
+	sprintf(&key[17], "#%hhu", bdaddr_type);
 
-	return textfile_put(filename, addr, services);
+	return textfile_put(filename, key, services);
 }
 
 static void filter_keys(char *key, char *value, void *data)
@@ -1228,15 +1229,17 @@ int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba)
 	return textfile_del(filename, address);
 }
 
-char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba)
+char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
+							uint8_t bdaddr_type)
 {
-	char filename[PATH_MAX + 1], addr[18];
+	char filename[PATH_MAX + 1], key[20];
 
 	create_filename(filename, PATH_MAX, sba, "primary");
 
-	ba2str(dba, addr);
+	ba2str(dba, key);
+	sprintf(&key[17], "#%hhu", bdaddr_type);
 
-	return textfile_caseget(filename, addr);
+	return textfile_caseget(filename, key);
 }
 
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
diff --git a/src/storage.h b/src/storage.h
index 6255ae8..745aff8 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -76,9 +76,10 @@ gboolean read_blocked(const bdaddr_t *local, const bdaddr_t *remote);
 int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 							gboolean blocked);
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
-							const char *services);
+				uint8_t bdaddr_type, const char *services);
 int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba);
-char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba);
+char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
+							uint8_t bdaddr_type);
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
 					uint16_t handle, const char *chars);
 char *read_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
-- 
1.7.7.6

[PATCH BlueZ v3 2/3] core: Fix creating device from "primary" file

[Paulo Alcantara]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch removes the hard-coded address type for the BLE device
created from the storage.
---
 src/adapter.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index dafe595..18dd5b6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -1940,13 +1940,17 @@ static void create_stored_device_from_primary(char *key, char *value,
 	struct btd_adapter *adapter = user_data;
 	struct btd_device *device;
 	GSList *services, *uuids, *l;
+	char address[18];
+	uint8_t bdaddr_type;
+
+	if (sscanf(key, "%17s#%hhu", address, &bdaddr_type) < 2)
+		return;
 
 	if (g_slist_find_custom(adapter->devices,
-			key, (GCompareFunc) device_address_cmp))
+			address, (GCompareFunc) device_address_cmp))
 		return;
 
-	/* FIXME: Get the correct LE addr type (public/random) */
-	device = device_create(connection, adapter, key, BDADDR_LE_PUBLIC);
+	device = device_create(connection, adapter, address, bdaddr_type);
 	if (!device)
 		return;
 
-- 
1.7.7.6

[PATCH BlueZ v3 3/3] core: Fix removing device services from "primary" file

[Paulo Alcantara]

The BLE address type needs to be passed as parameter in
delete_device_service() function in order to remove a device from
"primary" file. This is why the entries in "primary" file will be using
the new storage format.
---
 src/device.c  |    2 +-
 src/storage.c |   26 +++++++++++++++-----------
 src/storage.h |    3 ++-
 3 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/device.c b/src/device.c
index fd13bc3..7cdd025 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1169,7 +1169,7 @@ static void device_remove_stored(struct btd_device *device)
 	delete_entry(&src, "profiles", addr);
 	delete_entry(&src, "trusts", addr);
 	delete_all_records(&src, &device->bdaddr);
-	delete_device_service(&src, &device->bdaddr);
+	delete_device_service(&src, &device->bdaddr, device->bdaddr_type);
 
 	if (device->blocked)
 		device_unblock(conn, device, TRUE, FALSE);
diff --git a/src/storage.c b/src/storage.c
index 33934fb..973d545 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -1206,27 +1206,31 @@ done:
 	g_slist_free_full(match.keys, g_free);
 }
 
-int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba)
+int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba,
+						uint8_t bdaddr_type)
 {
-	char filename[PATH_MAX + 1], address[18];
+	char filename[PATH_MAX + 1], key[20];
 
-	memset(address, 0, sizeof(address));
-	ba2str(dba, address);
+	memset(key, 0, sizeof(key));
+	ba2str(dba, key);
 
-	/* Deleting all characteristics of a given address */
+	/* Deleting all characteristics of a given key */
 	create_filename(filename, PATH_MAX, sba, "characteristic");
-	delete_by_pattern(filename, address);
+	delete_by_pattern(filename, key);
 
-	/* Deleting all attributes values of a given address */
+	/* Deleting all attributes values of a given key */
 	create_filename(filename, PATH_MAX, sba, "attributes");
-	delete_by_pattern(filename, address);
+	delete_by_pattern(filename, key);
 
-	/* Deleting all CCC values of a given address */
+	/* Deleting all CCC values of a given key */
 	create_filename(filename, PATH_MAX, sba, "ccc");
-	delete_by_pattern(filename, address);
+	delete_by_pattern(filename, key);
+
+	sprintf(&key[17], "#%hhu", bdaddr_type);
 
 	create_filename(filename, PATH_MAX, sba, "primary");
-	return textfile_del(filename, address);
+
+	return textfile_del(filename, key);
 }
 
 char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
diff --git a/src/storage.h b/src/storage.h
index 745aff8..d00976f 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -77,7 +77,8 @@ int write_blocked(const bdaddr_t *local, const bdaddr_t *remote,
 							gboolean blocked);
 int write_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
 				uint8_t bdaddr_type, const char *services);
-int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba);
+int delete_device_service(const bdaddr_t *sba, const bdaddr_t *dba,
+						uint8_t bdaddr_type);
 char *read_device_services(const bdaddr_t *sba, const bdaddr_t *dba,
 							uint8_t bdaddr_type);
 int write_device_characteristics(const bdaddr_t *sba, const bdaddr_t *dba,
-- 
1.7.7.6

Re: [PATCH BlueZ v3 0/3] Store address type on storage

[Johan Hedberg]

Hi Paulo,

On Tue, May 22, 2012, Paulo Alcantara wrote:
> BLE devices have two different address types: public or
> random. So we need to provide this address type on keys
> in the "primary" file to avoid duplicate BLE address types
> around the BlueZ.
> 
> This series changes the storage to store the address type
> together with the device address.
> 
> Hemant Gupta has been working on this on "Store LE device address
> type with primary list" but his patch hasn't been accepted and
> he didn't send a new reviewed version of that patch.
> 
> I have patches for the other storage files as well, I'll
> send them soon. There is a potential problem in
> device_address_cmp() function which checks the address only.
> I am investigating how to fix it to compare address and type
> without breaking the current code.
> 
> v3's changes:
>      - (1/3): Remove unnecessary comment about the new storage
>        	      format and code that leads with the old storage
> 	      format.
> 
>      - (2/3): Just use the initial version of this patch (the
>        	      returning < 2 check in sscanf() function).
> 
>      - (3/3): New patch that fixes removing device services from
>        	      "primary" file by passing a key with the new
> 	      storage format in delete_device_service(). Note
> 	      also that this function was modified to only treat
> 	      the primary case - since we have old storage format
> 	      still being used in the other LE-related storage
> 	      files. So, it will need to be modified in order to
> 	      delete device services of other files (characteristics,
> 	      ccc, etc) on other series.
> 
> 
> Claudio Takahasi (1):
>   core: Fix creating device from "primary" file
> 
> Paulo Alcantara (2):
>   storage: Store BLE address type in "primary" file
>   core: Fix removing device services from "primary" file
> 
>  src/adapter.c |   10 +++++++---
>  src/device.c  |    4 ++--
>  src/storage.c |   45 ++++++++++++++++++++++++++-------------------
>  src/storage.h |    8 +++++---
>  4 files changed, 40 insertions(+), 27 deletions(-)

All three patches have been applied. Thanks.

Johan