[PATCH 1/5] android/pan: Fix wrong struct parameter in disconnect function call

[PATCH 1/5] android/pan: Fix wrong struct parameter in disconnect function call

[Ravi kumar Veeramally]

---
 android/pan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/android/pan.c b/android/pan.c
index 2a11f46..fba86b8 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -58,7 +58,8 @@ static uint8_t bt_pan_connect(struct hal_cmd_pan_connect *cmd, uint16_t len)
 	return HAL_STATUS_FAILED;
 }
 
-static uint8_t bt_pan_disconnect(struct hal_cmd_pan_connect *cmd, uint16_t len)
+static uint8_t bt_pan_disconnect(struct hal_cmd_pan_disconnect *cmd,
+								uint16_t len)
 {
 	DBG("Not Implemented");
 
-- 
1.8.3.2

[PATCH 2/5] android/hal-pan: Return error in case of unsupported PAN roles

[Ravi kumar Veeramally]

---
 android/hal-pan.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/android/hal-pan.c b/android/hal-pan.c
index 2bc560e..30facd4 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -77,6 +77,9 @@ static bt_status_t pan_enable(int local_role)
 	if (!interface_ready())
 		return BT_STATUS_NOT_READY;
 
+	if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
+		return BT_STATUS_UNSUPPORTED;
+
 	cmd.local_role = local_role;
 
 	return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
@@ -112,6 +115,14 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
 	if (!interface_ready())
 		return BT_STATUS_NOT_READY;
 
+	if (!((local_role == BTPAN_ROLE_PANNAP &&
+			remote_role == BTPAN_ROLE_PANU) ||
+		(local_role == BTPAN_ROLE_PANU &&
+			remote_role == BTPAN_ROLE_PANNAP) ||
+		(local_role == BTPAN_ROLE_PANU &&
+			remote_role == BTPAN_ROLE_PANU)))
+		return BT_STATUS_UNSUPPORTED;
+
 	memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
 	cmd.local_role = local_role;
 	cmd.remote_role = remote_role;
-- 
1.8.3.2

Re: [PATCH 2/5] android/hal-pan: Return error in case of unsupported PAN roles

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
>  android/hal-pan.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/android/hal-pan.c b/android/hal-pan.c
> index 2bc560e..30facd4 100644
> --- a/android/hal-pan.c
> +++ b/android/hal-pan.c
> @@ -77,6 +77,9 @@ static bt_status_t pan_enable(int local_role)
>  	if (!interface_ready())
>  		return BT_STATUS_NOT_READY;
>  
> +	if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
> +		return BT_STATUS_UNSUPPORTED;
> +
>  	cmd.local_role = local_role;
>  
>  	return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
> @@ -112,6 +115,14 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
>  	if (!interface_ready())
>  		return BT_STATUS_NOT_READY;
>  
> +	if (!((local_role == BTPAN_ROLE_PANNAP &&
> +			remote_role == BTPAN_ROLE_PANU) ||
> +		(local_role == BTPAN_ROLE_PANU &&
> +			remote_role == BTPAN_ROLE_PANNAP) ||
> +		(local_role == BTPAN_ROLE_PANU &&
> +			remote_role == BTPAN_ROLE_PANU)))
> +		return BT_STATUS_UNSUPPORTED;

First of all you've got incorrect indentation here which make this hard
to read (the return statement being on the same column as parts of the
if-statement. When you break lines indent at least by two tabs to make
a clear separation from the actual branch code.

Secondly, shouldn't we be checking that the given local role has been
enabled (through pan_enable)? Or does the daemon do that checking? In
fact is there a clear reason not to let the daemon do all the
verification checks and return an error status over IPC?

Thirdly, this if-statement takes a while to parse, so I'm wondering
whether it'd be clearer to format it in a bit more readable way, e.g.:

        switch (local_role) {
        case BTPAN_ROLE_PANNAP:
                if (remote_role != BTPAN_ROLE_PANU)
                        return BT_STATUS_UNSUPPORTED;
                break;
        case BTPAN_ROLE_PANU:
                if (remote_role != BTPAN_ROLE_PANNAP &&
                                                remote_role != BTPAN_ROLE_PANU)
                        return BT_STATUS_UNSUPPORTED;
                break;
        default:
                return BT_STATUS_UNSUPPORTED;
        }

Thoughts?

Johan

Re: [PATCH 2/5] android/hal-pan: Return error in case of unsupported PAN roles

[Ravi Kumar Veeramally]

On 11/19/2013 03:27 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> ---
>>   android/hal-pan.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/android/hal-pan.c b/android/hal-pan.c
>> index 2bc560e..30facd4 100644
>> --- a/android/hal-pan.c
>> +++ b/android/hal-pan.c
>> @@ -77,6 +77,9 @@ static bt_status_t pan_enable(int local_role)
>>   	if (!interface_ready())
>>   		return BT_STATUS_NOT_READY;
>>   
>> +	if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
>> +		return BT_STATUS_UNSUPPORTED;
>> +
>>   	cmd.local_role = local_role;
>>   
>>   	return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
>> @@ -112,6 +115,14 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
>>   	if (!interface_ready())
>>   		return BT_STATUS_NOT_READY;
>>   
>> +	if (!((local_role == BTPAN_ROLE_PANNAP &&
>> +			remote_role == BTPAN_ROLE_PANU) ||
>> +		(local_role == BTPAN_ROLE_PANU &&
>> +			remote_role == BTPAN_ROLE_PANNAP) ||
>> +		(local_role == BTPAN_ROLE_PANU &&
>> +			remote_role == BTPAN_ROLE_PANU)))
>> +		return BT_STATUS_UNSUPPORTED;
> First of all you've got incorrect indentation here which make this hard
> to read (the return statement being on the same column as parts of the
> if-statement. When you break lines indent at least by two tabs to make
> a clear separation from the actual branch code.
   Ok.
>
> Secondly, shouldn't we be checking that the given local role has been
> enabled (through pan_enable)? Or does the daemon do that checking? In
> fact is there a clear reason not to let the daemon do all the
> verification checks and return an error status over IPC?
   I thought it would be easier to do basic checks on supported combinations
  at HAL level  to reduce the ipc traffic.
  I don't know exactly whether UI can send these combinations, but we 
can at least
try with haltest tool.
>
> Thirdly, this if-statement takes a while to parse, so I'm wondering
> whether it'd be clearer to format it in a bit more readable way, e.g.:
>
>          switch (local_role) {
>          case BTPAN_ROLE_PANNAP:
>                  if (remote_role != BTPAN_ROLE_PANU)
>                          return BT_STATUS_UNSUPPORTED;
>                  break;
>          case BTPAN_ROLE_PANU:
>                  if (remote_role != BTPAN_ROLE_PANNAP &&
>                                                  remote_role != BTPAN_ROLE_PANU)
>                          return BT_STATUS_UNSUPPORTED;
>                  break;
>          default:
>                  return BT_STATUS_UNSUPPORTED;
>          }
>
> Thoughts?
   Yes, make sense to use 'switch' for more readability.
   Is it ok to send _v2 with switch or better to handle in daemon?

  Thanks,
  Ravi.

[PATCH 3/5] android: Handle multiple init(register) and cleanup(unregister) calls properly

[Ravi kumar Veeramally]

This can be tested with haltest.
---
 android/a2dp.c      | 6 ++++++
 android/bluetooth.c | 6 ++++++
 android/hidhost.c   | 6 ++++++
 android/pan.c       | 6 ++++++
 4 files changed, 24 insertions(+)

diff --git a/android/a2dp.c b/android/a2dp.c
index 74d0082..28a1250 100644
--- a/android/a2dp.c
+++ b/android/a2dp.c
@@ -332,6 +332,9 @@ bool bt_a2dp_register(int sk, const bdaddr_t *addr)
 
 	DBG("");
 
+	if (notification_sk > 0)
+		return false;
+
 	bacpy(&adapter_addr, addr);
 
 	server = bt_io_listen(connect_cb, NULL, NULL, NULL, &err,
@@ -365,6 +368,9 @@ void bt_a2dp_unregister(void)
 {
 	DBG("");
 
+	if (notification_sk < 0)
+		return;
+
 	notification_sk = -1;
 
 	bt_adapter_remove_record(record_id);
diff --git a/android/bluetooth.c b/android/bluetooth.c
index 7dc2ec3..7805d42 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -2275,6 +2275,9 @@ bool bt_bluetooth_register(int sk)
 {
 	DBG("");
 
+	if (notification_sk > 0)
+		return false;
+
 	notification_sk = sk;
 
 	return true;
@@ -2284,5 +2287,8 @@ void bt_bluetooth_unregister(void)
 {
 	DBG("");
 
+	if (notification_sk < 0)
+		return;
+
 	notification_sk = -1;
 }
diff --git a/android/hidhost.c b/android/hidhost.c
index 842b8ad..a929a94 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -1190,6 +1190,9 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
 
 	DBG("");
 
+	if (notification_sk > 0)
+		return false;
+
 	bacpy(&adapter_addr, addr);
 
 	ctrl_io = bt_io_listen(connect_cb, NULL, NULL, NULL, &err,
@@ -1224,6 +1227,9 @@ void bt_hid_unregister(void)
 {
 	DBG("");
 
+	if (notification_sk < 0)
+		return;
+
 	notification_sk = -1;
 
 	if (ctrl_io) {
diff --git a/android/pan.c b/android/pan.c
index fba86b8..970a8a7 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -95,6 +95,9 @@ bool bt_pan_register(int sk, const bdaddr_t *addr)
 {
 	DBG("");
 
+	if (notification_sk > 0)
+		return false;
+
 	notification_sk = sk;
 
 	return true;
@@ -104,5 +107,8 @@ void bt_pan_unregister(void)
 {
 	DBG("");
 
+	if (notification_sk < 0)
+		return;
+
 	notification_sk = -1;
 }
-- 
1.8.3.2

Re: [PATCH 3/5] android: Handle multiple init(register) and cleanup(unregister) calls properly

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> @@ -2275,6 +2275,9 @@ bool bt_bluetooth_register(int sk)
>  {
>  	DBG("");
>  
> +	if (notification_sk > 0)

0 is a valid file descriptor value so the check should be >= 0

> @@ -1190,6 +1190,9 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>  
>  	DBG("");
>  
> +	if (notification_sk > 0)
> +		return false;

Same here.

> @@ -95,6 +95,9 @@ bool bt_pan_register(int sk, const bdaddr_t *addr)
>  {
>  	DBG("");
>  
> +	if (notification_sk > 0)
> +		return false;

And here.

Johan

Re: [PATCH 3/5] android: Handle multiple init(register) and cleanup(unregister) calls properly

[Ravi Kumar Veeramally]

Hi,

On 11/19/2013 03:30 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> @@ -2275,6 +2275,9 @@ bool bt_bluetooth_register(int sk)
>>   {
>>   	DBG("");
>>   
>> +	if (notification_sk > 0)
> 0 is a valid file descriptor value so the check should be >= 0
>
>> @@ -1190,6 +1190,9 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>>   
>>   	DBG("");
>>   
>> +	if (notification_sk > 0)
>> +		return false;
> Same here.
>
>> @@ -95,6 +95,9 @@ bool bt_pan_register(int sk, const bdaddr_t *addr)
>>   {
>>   	DBG("");
>>   
>> +	if (notification_sk > 0)
>> +		return false;
> And here.
>
> Johan
>
  Ok.

  Thanks,
  Ravi.

[PATCH 4/5] android/hidhost: Handle error case properly in interrupt_connect_cb

[Ravi kumar Veeramally]

---
 android/hidhost.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index a929a94..05a3fe4 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -522,7 +522,6 @@ static int uhid_create(struct hid_device *dev)
 	dev->uhid_fd = open(UHID_DEVICE_FILE, O_RDWR | O_CLOEXEC);
 	if (dev->uhid_fd < 0) {
 		error("Failed to open uHID device: %s", strerror(errno));
-		bt_hid_notify_state(dev, HAL_HIDHOST_STATE_NO_HID);
 		return -errno;
 	}
 
@@ -541,7 +540,6 @@ static int uhid_create(struct hid_device *dev)
 		error("Failed to create uHID device: %s", strerror(errno));
 		close(dev->uhid_fd);
 		dev->uhid_fd = -1;
-		bt_hid_notify_state(dev, HAL_HIDHOST_STATE_NO_HID);
 		return -errno;
 	}
 
@@ -559,16 +557,20 @@ static void interrupt_connect_cb(GIOChannel *chan, GError *conn_err,
 							gpointer user_data)
 {
 	struct hid_device *dev = user_data;
+	uint8_t state;
 
 	DBG("");
 
 	if (conn_err) {
 		error("%s", conn_err->message);
+		state = HAL_HIDHOST_STATE_FAILED;
 		goto failed;
 	}
 
-	if (uhid_create(dev) < 0)
+	if (uhid_create(dev) < 0) {
+		state = HAL_HIDHOST_STATE_NO_HID;
 		goto failed;
+	}
 
 	dev->intr_watch = g_io_add_watch(dev->intr_io,
 				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
@@ -579,6 +581,7 @@ static void interrupt_connect_cb(GIOChannel *chan, GError *conn_err,
 	return;
 
 failed:
+	bt_hid_notify_state(dev, state);
 	hid_device_free(dev);
 }
 
-- 
1.8.3.2

Re: [PATCH 4/5] android/hidhost: Handle error case properly in interrupt_connect_cb

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
>  android/hidhost.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Could you add a bit more descriptive commit message to this one which
explains what was broken (and how) and how your patch fixes it. When I
get a patch with an empty commit message I expect it to be very trivial
and instantly understandable, but that's not the case here.

Johan

Re: [PATCH 4/5] android/hidhost: Handle error case properly in interrupt_connect_cb

[Ravi Kumar Veeramally]

Hi Johan,

On 11/19/2013 03:31 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> ---
>>   android/hidhost.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
> Could you add a bit more descriptive commit message to this one which
> explains what was broken (and how) and how your patch fixes it. When I
> get a patch with an empty commit message I expect it to be very trivial
> and instantly understandable, but that's not the case here.
   I will write more details.

  Regards,
  Ravi.

[PATCH 5/5] android/hidhost: Free all connected devices in profile cleanup call

[Ravi kumar Veeramally]

---
 android/hidhost.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/android/hidhost.c b/android/hidhost.c
index 05a3fe4..20dedc4 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -1226,6 +1226,14 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
 	return true;
 }
 
+static void free_hid_devices(gpointer data, gpointer user_data)
+{
+	struct hid_device *dev = data;
+
+	bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTED);
+	hid_device_free(dev);
+}
+
 void bt_hid_unregister(void)
 {
 	DBG("");
@@ -1233,6 +1241,8 @@ void bt_hid_unregister(void)
 	if (notification_sk < 0)
 		return;
 
+	g_slist_foreach(devices, free_hid_devices, NULL);
+
 	notification_sk = -1;
 
 	if (ctrl_io) {
-- 
1.8.3.2

Re: [PATCH 5/5] android/hidhost: Free all connected devices in profile cleanup call

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
>  android/hidhost.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 05a3fe4..20dedc4 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -1226,6 +1226,14 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>  	return true;
>  }
>  
> +static void free_hid_devices(gpointer data, gpointer user_data)
> +{
> +	struct hid_device *dev = data;
> +
> +	bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTED);
> +	hid_device_free(dev);
> +}
> +
>  void bt_hid_unregister(void)
>  {
>  	DBG("");
> @@ -1233,6 +1241,8 @@ void bt_hid_unregister(void)
>  	if (notification_sk < 0)
>  		return;
>  
> +	g_slist_foreach(devices, free_hid_devices, NULL);

I suppose you also need to set devices = NULL; after this call?

Johan

Re: [PATCH 5/5] android/hidhost: Free all connected devices in profile cleanup call

[Ravi Kumar Veeramally]

Hi Johan,

On 11/19/2013 03:35 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
>> ---
>>   android/hidhost.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/android/hidhost.c b/android/hidhost.c
>> index 05a3fe4..20dedc4 100644
>> --- a/android/hidhost.c
>> +++ b/android/hidhost.c
>> @@ -1226,6 +1226,14 @@ bool bt_hid_register(int sk, const bdaddr_t *addr)
>>   	return true;
>>   }
>>   
>> +static void free_hid_devices(gpointer data, gpointer user_data)
>> +{
>> +	struct hid_device *dev = data;
>> +
>> +	bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTED);
>> +	hid_device_free(dev);
>> +}
>> +
>>   void bt_hid_unregister(void)
>>   {
>>   	DBG("");
>> @@ -1233,6 +1241,8 @@ void bt_hid_unregister(void)
>>   	if (notification_sk < 0)
>>   		return;
>>   
>> +	g_slist_foreach(devices, free_hid_devices, NULL);
> I suppose you also need to set devices = NULL; after this call?
  Sure.

  Thanks,
  Ravi.

Re: [PATCH 1/5] android/pan: Fix wrong struct parameter in disconnect function call

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 19, 2013, Ravi kumar Veeramally wrote:
> ---
>  android/pan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This patch has been applied. Thanks.

Johan