[PATCH BlueZ 1/2] core/device: Fix attempting to connect to random address over BR/EDR

[PATCH BlueZ 1/2] core/device: Fix attempting to connect to random address over BR/EDR

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

BR/EDR don't have the concept of random addresses so the only option is
to over LE in that case.
---
 src/device.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/device.c b/src/device.c
index fb766d1..b8556f1 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1748,6 +1748,10 @@ static uint8_t select_conn_bearer(struct btd_device *dev)
 	else if (!dev->bredr_state.bonded && dev->le_state.bonded)
 		return dev->bdaddr_type;
 
+	/* If the address is private it can only be connected over LE */
+	if (device_address_is_private(dev))
+		return dev->bdaddr_type;
+
 	if (dev->bredr_seen) {
 		bredr_last = current - dev->bredr_seen;
 		if (bredr_last > SEEN_TRESHHOLD)
-- 
2.7.4

[PATCH BlueZ 2/2] core/gatt-client: Fix not return empty array for ReadValue

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The response to ReadValue shall be an array which can be empty.
---
 src/gatt-client.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 2af6979..57a1f66 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -297,7 +297,7 @@ static void write_descriptor_cb(struct gatt_db_attribute *attr, int err,
 }
 
 static void async_dbus_op_reply(struct async_dbus_op *op, int err,
-				const uint8_t *value, size_t length)
+				const uint8_t *value, ssize_t length)
 {
 	const struct queue_entry *entry;
 	DBusMessage *reply;
@@ -319,7 +319,7 @@ static void async_dbus_op_reply(struct async_dbus_op *op, int err,
 			return;
 		}
 
-		if (value)
+		if (length >= 0)
 			message_append_byte_array(reply, value, length);
 
 send_reply:
@@ -489,7 +489,7 @@ static void write_result_cb(bool success, bool reliable_error,
 	}
 
 done:
-	async_dbus_op_reply(op, err, NULL, 0);
+	async_dbus_op_reply(op, err, NULL, -1);
 }
 
 static void write_cb(bool success, uint8_t att_ecode, void *user_data)
@@ -1126,7 +1126,7 @@ static void create_notify_reply(struct async_dbus_op *op, bool success,
 	else
 		err = att_ecode ? att_ecode : -ENOENT;
 
-	async_dbus_op_reply(op, err, NULL, 0);
+	async_dbus_op_reply(op, err, NULL, -1);
 }
 
 static void register_notify_cb(uint16_t att_ecode, void *user_data)
-- 
2.7.4

Re: [PATCH BlueZ 1/2] core/device: Fix attempting to connect to random address over BR/EDR

[Luiz Augusto von Dentz]

Hi,

On Fri, Sep 2, 2016 at 3:49 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> BR/EDR don't have the concept of random addresses so the only option is
> to over LE in that case.
> ---
>  src/device.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index fb766d1..b8556f1 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1748,6 +1748,10 @@ static uint8_t select_conn_bearer(struct btd_device *dev)
>         else if (!dev->bredr_state.bonded && dev->le_state.bonded)
>                 return dev->bdaddr_type;
>
> +       /* If the address is private it can only be connected over LE */
> +       if (device_address_is_private(dev))
> +               return dev->bdaddr_type;
> +
>         if (dev->bredr_seen) {
>                 bredr_last = current - dev->bredr_seen;
>                 if (bredr_last > SEEN_TRESHHOLD)
> --
> 2.7.4

Applied.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] core/device: Fix attempting to connect to random address over BR/EDR

[Johan Hedberg]

Hi Luiz,

On Fri, Sep 02, 2016, Luiz Augusto von Dentz wrote:
> BR/EDR don't have the concept of random addresses so the only option is
> to over LE in that case.
> ---
>  src/device.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/device.c b/src/device.c
> index fb766d1..b8556f1 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1748,6 +1748,10 @@ static uint8_t select_conn_bearer(struct btd_device *dev)
>  	else if (!dev->bredr_state.bonded && dev->le_state.bonded)
>  		return dev->bdaddr_type;
>  
> +	/* If the address is private it can only be connected over LE */
> +	if (device_address_is_private(dev))
> +		return dev->bdaddr_type;
> +

This doesn't look right to me. The comment also isn't correct since a
dual mode device may use a resolvable private address for LE, i.e.
device_address_is_private(dev) would return true. IIRC the
dev->bdaddr_type value tells the LE address type when LE is supported.
If LE is not supported then dev->bdaddr_type is BDADDR_BREDR.

Johan

Re: [PATCH BlueZ 1/2] core/device: Fix attempting to connect to random address over BR/EDR

[Luiz Augusto von Dentz]

Hi Johan,

On Mon, Sep 5, 2016 at 9:02 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Luiz,
>
> On Fri, Sep 02, 2016, Luiz Augusto von Dentz wrote:
>> BR/EDR don't have the concept of random addresses so the only option is
>> to over LE in that case.
>> ---
>>  src/device.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/src/device.c b/src/device.c
>> index fb766d1..b8556f1 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -1748,6 +1748,10 @@ static uint8_t select_conn_bearer(struct btd_device *dev)
>>       else if (!dev->bredr_state.bonded && dev->le_state.bonded)
>>               return dev->bdaddr_type;
>>
>> +     /* If the address is private it can only be connected over LE */
>> +     if (device_address_is_private(dev))
>> +             return dev->bdaddr_type;
>> +
>
> This doesn't look right to me. The comment also isn't correct since a
> dual mode device may use a resolvable private address for LE, i.e.
> device_address_is_private(dev) would return true. IIRC the
> dev->bdaddr_type value tells the LE address type when LE is supported.

This is evaluated after checking if device is bonded, but it doesn't
actually matter because this device object can only use the random
address until it is resolved, if we later on resolve the address I
suppose the object is updated with its identity address and the
address type is changed to public.

> If LE is not supported then dev->bdaddr_type is BDADDR_BREDR.

But private addresses can only be used in LE, so
device_address_is_private will only return true for LE.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] core/device: Fix attempting to connect to random address over BR/EDR

[Johan Hedberg]

Hi Luiz,

On Mon, Sep 05, 2016, Luiz Augusto von Dentz wrote:
> On Mon, Sep 5, 2016 at 9:02 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi Luiz,
> >
> > On Fri, Sep 02, 2016, Luiz Augusto von Dentz wrote:
> >> BR/EDR don't have the concept of random addresses so the only option is
> >> to over LE in that case.
> >> ---
> >>  src/device.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/device.c b/src/device.c
> >> index fb766d1..b8556f1 100644
> >> --- a/src/device.c
> >> +++ b/src/device.c
> >> @@ -1748,6 +1748,10 @@ static uint8_t select_conn_bearer(struct btd_device *dev)
> >>       else if (!dev->bredr_state.bonded && dev->le_state.bonded)
> >>               return dev->bdaddr_type;
> >>
> >> +     /* If the address is private it can only be connected over LE */
> >> +     if (device_address_is_private(dev))
> >> +             return dev->bdaddr_type;
> >> +
> >
> > This doesn't look right to me. The comment also isn't correct since a
> > dual mode device may use a resolvable private address for LE, i.e.
> > device_address_is_private(dev) would return true. IIRC the
> > dev->bdaddr_type value tells the LE address type when LE is supported.
> 
> This is evaluated after checking if device is bonded, but it doesn't
> actually matter because this device object can only use the random
> address until it is resolved, if we later on resolve the address I
> suppose the object is updated with its identity address and the
> address type is changed to public.

If that's the case then you're right that the patch shouldn't break
anything. The comment is still confusing though.

> > If LE is not supported then dev->bdaddr_type is BDADDR_BREDR.
> 
> But private addresses can only be used in LE, so
> device_address_is_private will only return true for LE.

Why specifically private addresses and not random addresses in general?
The latter can be checked with dev->bdaddr_type == BDADDR_LE_RANDOM,
which is more lightweight than calling device_address_is_private().
Also, if your intention is to check for devices not supporting BR/EDR
then why don't you just check for !dev->bredr?

Johan