[PATCH_v3 0/4] Add support for NAP role

[PATCH_v3 0/4] Add support for NAP role

[Ravi kumar Veeramally]

v3: Fixed Johan's comments (removed fopen, fprintf and used open and write).

v2: Fixed Johan's comments.

v1: This patch set add support for NAP role. It register NAP server and create
  bnep bridge and listen for incoming connections from client devices.
  On incoming connection request it accepts connection, creates bnep interface
  and notifies control state and connection state infromation. Removes device
  on disconnect request. Android related changes are required to enable this
  role. Patches sent to respective ML.

Ravi kumar Veeramally (4):
  android/pan: Register Network Access Point
  android/pan: Listen for incoming connections and accept in NAP role
  android/pan: Implement PAN enable HAL api at daemon side
  android/pan: Remove connected PAN devices on profile unregister call

 android/pan.c | 346 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 334 insertions(+), 12 deletions(-)

-- 
1.8.3.2

[PATCH_v3 1/4] android/pan: Register Network Access Point

[Ravi kumar Veeramally]

Register NAP server and adds bnep bridge. Removes bridge
on destroy call. Bridge mechanism is needed when device acting
as a server and listen for incoming connections.
---
 android/pan.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 108 insertions(+), 4 deletions(-)

diff --git a/android/pan.c b/android/pan.c
index 38e353d..93078ba 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -28,6 +28,11 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include <glib.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <sys/wait.h>
+#include <net/if.h>
+#include <linux/sockios.h>
 
 #include "btio/btio.h"
 #include "lib/bluetooth.h"
@@ -45,11 +50,11 @@
 #include "bluetooth.h"
 
 #define SVC_HINT_NETWORKING 0x02
+#define BNEP_BRIDGE	"bnep"
 
 static bdaddr_t adapter_addr;
 GSList *devices = NULL;
 uint8_t local_role = HAL_PAN_ROLE_NONE;
-static uint32_t record_id = 0;
 
 struct pan_device {
 	char		iface[16];
@@ -60,6 +65,12 @@ struct pan_device {
 	struct bnep	*session;
 };
 
+static struct {
+	uint32_t	record_id;
+} nap_dev = {
+	.record_id = 0,
+};
+
 static int device_cmp(gconstpointer s, gconstpointer user_data)
 {
 	const struct pan_device *dev = s;
@@ -297,6 +308,91 @@ failed:
 	ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_DISCONNECT, status);
 }
 
+static int set_forward_delay(void)
+{
+	int fd, ret;
+	char path[41];
+
+	sprintf(path, "/sys/class/net/%s/bridge/forward_delay", BNEP_BRIDGE);
+
+	fd = open(path, O_RDWR);
+	if (fd < 0)
+		return -errno;
+
+	ret = write(fd, "0", sizeof("0"));
+	close(fd);
+
+	return ret;
+}
+
+static int nap_create_bridge(void)
+{
+	int sk, err;
+
+	DBG("%s", BNEP_BRIDGE);
+
+	sk = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+	if (sk < 0)
+		return -EOPNOTSUPP;
+
+	if (ioctl(sk, SIOCBRADDBR, BNEP_BRIDGE) < 0) {
+		err = -errno;
+		if (err != -EEXIST) {
+			close(sk);
+			return -EOPNOTSUPP;
+		}
+	}
+
+	err = set_forward_delay();
+	if (err < 0)
+		ioctl(sk, SIOCBRDELBR, BNEP_BRIDGE);
+
+	close(sk);
+
+	return err;
+}
+
+static int nap_remove_bridge(void)
+{
+	int sk, err;
+
+	DBG("%s", BNEP_BRIDGE);
+
+	sk = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
+	if (sk < 0)
+		return -EOPNOTSUPP;
+
+	err = ioctl(sk, SIOCBRDELBR, BNEP_BRIDGE);
+	close(sk);
+
+	if (err < 0)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static void destroy_nap_device(void)
+{
+	DBG("");
+
+	nap_remove_bridge();
+
+	nap_dev.record_id = 0;
+}
+
+static int register_nap_server(void)
+{
+	int err;
+
+	DBG("");
+
+	err = nap_create_bridge();
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static void bt_pan_enable(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_pan_enable *cmd = buf;
@@ -441,7 +537,15 @@ bool bt_pan_register(const bdaddr_t *addr)
 		return false;
 	}
 
-	record_id = rec->handle;
+	err = register_nap_server();
+	if (err < 0) {
+		bt_adapter_remove_record(rec->handle);
+		sdp_record_free(rec);
+		bnep_cleanup();
+		return false;
+	}
+
+	nap_dev.record_id = rec->handle;
 	ipc_register(HAL_SERVICE_ID_PAN, cmd_handlers,
 						G_N_ELEMENTS(cmd_handlers));
 
@@ -455,6 +559,6 @@ void bt_pan_unregister(void)
 	bnep_cleanup();
 
 	ipc_unregister(HAL_SERVICE_ID_PAN);
-	bt_adapter_remove_record(record_id);
-	record_id = 0;
+	bt_adapter_remove_record(nap_dev.record_id);
+	destroy_nap_device();
 }
-- 
1.8.3.2

[PATCH_v3 2/4] android/pan: Listen for incoming connections and accept in NAP role

[Ravi kumar Veeramally]

Listen for incoming connections and accept it. Create bnep interface
add it to bridge and notify control and connection state information
through HAL. Remove the device on disconnect request. If android
settings UI does not have bluetooth tethering enabled it immediately
sends disconnect signal.
---
 android/pan.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 190 insertions(+), 2 deletions(-)

diff --git a/android/pan.c b/android/pan.c
index 93078ba..0eef284 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -63,12 +63,17 @@ struct pan_device {
 	uint8_t		role;
 	GIOChannel	*io;
 	struct bnep	*session;
+	guint		watch;
 };
 
 static struct {
 	uint32_t	record_id;
+	guint		watch;
+	GIOChannel	*io;
 } nap_dev = {
 	.record_id = 0,
+	.watch = 0,
+	.io = NULL,
 };
 
 static int device_cmp(gconstpointer s, gconstpointer user_data)
@@ -81,13 +86,21 @@ static int device_cmp(gconstpointer s, gconstpointer user_data)
 
 static void pan_device_free(struct pan_device *dev)
 {
+	if (dev->watch > 0) {
+		bnep_server_delete(BNEP_BRIDGE, dev->iface, &dev->dst);
+		g_source_remove(dev->watch);
+		dev->watch = 0;
+	}
+
 	if (dev->io) {
 		g_io_channel_shutdown(dev->io, FALSE, NULL);
 		g_io_channel_unref(dev->io);
 		dev->io = NULL;
 	}
 
-	bnep_free(dev->session);
+	if (dev->session)
+		bnep_free(dev->session);
+
 	devices = g_slist_remove(devices, dev);
 	g_free(dev);
 
@@ -298,7 +311,7 @@ static void bt_pan_disconnect(const void *buf, uint16_t len)
 
 	dev = l->data;
 
-	if (dev->conn_state == HAL_PAN_STATE_CONNECTED)
+	if (dev->conn_state == HAL_PAN_STATE_CONNECTED && dev->session)
 		bnep_disconnect(dev->session);
 
 	bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
@@ -308,6 +321,154 @@ failed:
 	ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_DISCONNECT, status);
 }
 
+static gboolean nap_watchdog_cb(GIOChannel *chan, GIOCondition cond,
+							gpointer user_data)
+{
+	struct pan_device *dev = user_data;
+
+	DBG("disconnected");
+
+	bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
+
+	return FALSE;
+}
+static gboolean nap_setup_cb(GIOChannel *chan, GIOCondition cond,
+							gpointer user_data)
+{
+	struct pan_device *dev = user_data;
+	uint8_t packet[BNEP_MTU];
+	struct bnep_setup_conn_req *req = (void *) packet;
+	uint16_t src_role, dst_role, rsp = BNEP_CONN_NOT_ALLOWED;
+	int sk, n;
+
+	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
+		error("Hangup or error or inval on BNEP socket");
+		return FALSE;
+	}
+
+	sk = g_io_channel_unix_get_fd(chan);
+
+	/* Reading BNEP_SETUP_CONNECTION_REQUEST_MSG */
+	n = read(sk, packet, sizeof(packet));
+	if (n  < 0) {
+		error("read(): %s(%d)", strerror(errno), errno);
+		goto failed;
+	}
+
+	/* Highest known control command id BNEP_FILTER_MULT_ADDR_RSP 0x06 */
+	if (req->type == BNEP_CONTROL &&
+			req->ctrl > BNEP_FILTER_MULT_ADDR_RSP) {
+		error("cmd not understood");
+		bnep_send_ctrl_rsp(sk, BNEP_CONTROL, BNEP_CMD_NOT_UNDERSTOOD,
+								req->ctrl);
+		goto failed;
+	}
+
+	if (req->type != BNEP_CONTROL || req->ctrl != BNEP_SETUP_CONN_REQ) {
+		error("cmd is not BNEP_SETUP_CONN_REQ %02X %02X", req->type,
+								req->ctrl);
+		goto failed;
+	}
+
+	rsp = bnep_setup_decode(req, &dst_role, &src_role);
+	if (rsp) {
+		error("bnep_setup_decode failed");
+		goto failed;
+	}
+
+	rsp = bnep_setup_chk(dst_role, src_role);
+	if (rsp) {
+		error("benp_setup_chk failed");
+		goto failed;
+	}
+
+	if (bnep_server_add(sk, dst_role, BNEP_BRIDGE, dev->iface,
+							&dev->dst) < 0) {
+		error("server_connadd failed");
+		rsp = BNEP_CONN_NOT_ALLOWED;
+		goto failed;
+	}
+
+	rsp = BNEP_SUCCESS;
+	bnep_send_ctrl_rsp(sk, BNEP_CONTROL, BNEP_SETUP_CONN_RSP, rsp);
+
+	dev->watch = g_io_add_watch(chan, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+							nap_watchdog_cb, dev);
+	g_io_channel_unref(dev->io);
+	dev->io = NULL;
+
+	bt_pan_notify_ctrl_state(dev, HAL_PAN_CTRL_ENABLED);
+	bt_pan_notify_conn_state(dev, HAL_PAN_STATE_CONNECTED);
+
+	return FALSE;
+
+failed:
+	bnep_send_ctrl_rsp(sk, BNEP_CONTROL, BNEP_SETUP_CONN_RSP, rsp);
+	pan_device_free(dev);
+
+	return FALSE;
+}
+
+static void nap_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
+{
+	struct pan_device *dev = user_data;
+
+	DBG("");
+
+	if (err) {
+		error("%s", err->message);
+		bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
+		return;
+	}
+
+	g_io_channel_set_close_on_unref(chan, TRUE);
+	dev->watch = g_io_add_watch(chan,
+				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				nap_setup_cb, dev);
+}
+
+static void nap_confirm_cb(GIOChannel *chan, gpointer data)
+{
+	struct pan_device *dev = NULL;
+	bdaddr_t dst;
+	char address[18];
+	GError *err = NULL;
+
+	DBG("");
+
+	bt_io_get(chan, &err, BT_IO_OPT_DEST_BDADDR, &dst,
+			BT_IO_OPT_DEST, address, BT_IO_OPT_INVALID);
+	if (err) {
+		error("%s", err->message);
+		g_error_free(err);
+		goto failed;
+	}
+
+	DBG("incoming connect request from %s", address);
+	dev = g_new0(struct pan_device, 1);
+	bacpy(&dev->dst, &dst);
+	local_role = HAL_PAN_ROLE_NAP;
+	dev->role = HAL_PAN_ROLE_PANU;
+
+	dev->io = g_io_channel_ref(chan);
+	g_io_channel_set_close_on_unref(dev->io, TRUE);
+
+	if (!bt_io_accept(dev->io, nap_connect_cb, dev, NULL, &err)) {
+		error("bt_io_accept: %s", err->message);
+		g_error_free(err);
+		goto failed;
+	}
+
+	devices = g_slist_append(devices, dev);
+	bt_pan_notify_conn_state(dev, HAL_PAN_STATE_CONNECTING);
+
+	return;
+
+failed:
+	g_free(dev);
+	bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
+}
+
 static int set_forward_delay(void)
 {
 	int fd, ret;
@@ -378,10 +539,22 @@ static void destroy_nap_device(void)
 	nap_remove_bridge();
 
 	nap_dev.record_id = 0;
+
+	if (nap_dev.watch > 0) {
+		g_source_remove(nap_dev.watch);
+		nap_dev.watch = 0;
+	}
+
+	if (nap_dev.io) {
+		g_io_channel_shutdown(nap_dev.io, FALSE, NULL);
+		g_io_channel_unref(nap_dev.io);
+		nap_dev.io = NULL;
+	}
 }
 
 static int register_nap_server(void)
 {
+	GError *gerr;
 	int err;
 
 	DBG("");
@@ -390,6 +563,21 @@ static int register_nap_server(void)
 	if (err < 0)
 		return err;
 
+	nap_dev.io = bt_io_listen(NULL, nap_confirm_cb, NULL, NULL, &gerr,
+					BT_IO_OPT_SOURCE_BDADDR, &adapter_addr,
+					BT_IO_OPT_PSM, BNEP_PSM,
+					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
+					BT_IO_OPT_OMTU, BNEP_MTU,
+					BT_IO_OPT_IMTU, BNEP_MTU,
+					BT_IO_OPT_INVALID);
+
+	if (!nap_dev.io) {
+		destroy_nap_device();
+		error("%s", gerr->message);
+		g_error_free(gerr);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
1.8.3.2

[PATCH_v3 3/4] android/pan: Implement PAN enable HAL api at daemon side

[Ravi kumar Veeramally]

---
 android/pan.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/android/pan.c b/android/pan.c
index 0eef284..0e12245 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -585,18 +585,38 @@ static void bt_pan_enable(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_pan_enable *cmd = buf;
 	uint8_t status;
+	int err;
+
+	DBG("");
+
+	if (local_role == cmd->local_role) {
+		status = HAL_STATUS_SUCCESS;
+		goto reply;
+	}
+
+	/* destroy existing server */
+	destroy_nap_device();
 
 	switch (cmd->local_role) {
 	case HAL_PAN_ROLE_PANU:
-	case HAL_PAN_ROLE_NAP:
-		DBG("Not Implemented");
-		status  = HAL_STATUS_FAILED;
-		break;
-	default:
 		status = HAL_STATUS_UNSUPPORTED;
-		break;
+		goto reply;
+	case HAL_PAN_ROLE_NONE:
+		status = HAL_STATUS_SUCCESS;
+		goto reply;
+	}
+
+	local_role = cmd->local_role;
+	err = register_nap_server();
+	if (err < 0) {
+		status = HAL_STATUS_FAILED;
+		destroy_nap_device();
+		goto reply;
 	}
 
+	status = HAL_STATUS_SUCCESS;
+
+reply:
 	ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE, status);
 }
 
-- 
1.8.3.2

[PATCH_v3 4/4] android/pan: Remove connected PAN devices on profile unregister call

[Ravi kumar Veeramally]

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

diff --git a/android/pan.c b/android/pan.c
index 0e12245..b933196 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -760,10 +760,20 @@ bool bt_pan_register(const bdaddr_t *addr)
 	return true;
 }
 
+static void pan_device_disconnected(gpointer data, gpointer user_data)
+{
+	struct pan_device *dev = data;
+
+	bt_pan_notify_conn_state(dev, HAL_PAN_STATE_DISCONNECTED);
+}
+
 void bt_pan_unregister(void)
 {
 	DBG("");
 
+	g_slist_foreach(devices, pan_device_disconnected, NULL);
+	devices = NULL;
+
 	bnep_cleanup();
 
 	ipc_unregister(HAL_SERVICE_ID_PAN);
-- 
1.8.3.2

Re: [PATCH_v3 2/4] android/pan: Listen for incoming connections and accept in NAP role

[Luiz Augusto von Dentz]

Hi Ravi,

On Tue, Jan 7, 2014 at 2:31 PM, Ravi kumar Veeramally
<ravikumar.veeramally@linux.intel.com> wrote:
> Listen for incoming connections and accept it. Create bnep interface
> add it to bridge and notify control and connection state information
> through HAL. Remove the device on disconnect request. If android
> settings UI does not have bluetooth tethering enabled it immediately
> sends disconnect signal.
> ---
>  android/pan.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 190 insertions(+), 2 deletions(-)
>
> diff --git a/android/pan.c b/android/pan.c
> index 93078ba..0eef284 100644
> --- a/android/pan.c
> +++ b/android/pan.c
> @@ -63,12 +63,17 @@ struct pan_device {
>         uint8_t         role;
>         GIOChannel      *io;
>         struct bnep     *session;
> +       guint           watch;
>  };
>
>  static struct {
>         uint32_t        record_id;
> +       guint           watch;
> +       GIOChannel      *io;
>  } nap_dev = {
>         .record_id = 0,
> +       .watch = 0,
> +       .io = NULL,
>  };
>
>  static int device_cmp(gconstpointer s, gconstpointer user_data)
> @@ -81,13 +86,21 @@ static int device_cmp(gconstpointer s, gconstpointer user_data)
>
>  static void pan_device_free(struct pan_device *dev)
>  {
> +       if (dev->watch > 0) {
> +               bnep_server_delete(BNEP_BRIDGE, dev->iface, &dev->dst);
> +               g_source_remove(dev->watch);
> +               dev->watch = 0;

Usually it is not necessary to assign anything to struct members when
you are freeing the whole struct since its memory gonna be freed
anyway.

Re: [PATCH_v3 1/4] android/pan: Register Network Access Point

[Szymon Janc]

Hi Ravi,

> Register NAP server and adds bnep bridge. Removes bridge
> on destroy call. Bridge mechanism is needed when device acting
> as a server and listen for incoming connections.
> ---
>  android/pan.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 108 insertions(+), 4 deletions(-)
> 
> diff --git a/android/pan.c b/android/pan.c
> index 38e353d..93078ba 100644
> --- a/android/pan.c
> +++ b/android/pan.c
> @@ -28,6 +28,11 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <glib.h>
> +#include <sys/ioctl.h>
> +#include <sys/socket.h>
> +#include <sys/wait.h>
> +#include <net/if.h>
> +#include <linux/sockios.h>
>  
>  #include "btio/btio.h"
>  #include "lib/bluetooth.h"
> @@ -45,11 +50,11 @@
>  #include "bluetooth.h"
>  
>  #define SVC_HINT_NETWORKING 0x02
> +#define BNEP_BRIDGE	"bnep"
>  
>  static bdaddr_t adapter_addr;
>  GSList *devices = NULL;
>  uint8_t local_role = HAL_PAN_ROLE_NONE;
> -static uint32_t record_id = 0;
>  
>  struct pan_device {
>  	char		iface[16];
> @@ -60,6 +65,12 @@ struct pan_device {
>  	struct bnep	*session;
>  };
>  
> +static struct {
> +	uint32_t	record_id;
> +} nap_dev = {
> +	.record_id = 0,
> +};
> +
>  static int device_cmp(gconstpointer s, gconstpointer user_data)
>  {
>  	const struct pan_device *dev = s;
> @@ -297,6 +308,91 @@ failed:
>  	ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_DISCONNECT, status);
>  }
>  
> +static int set_forward_delay(void)
> +{
> +	int fd, ret;
> +	char path[41];
> +
> +	sprintf(path, "/sys/class/net/%s/bridge/forward_delay", BNEP_BRIDGE);
> +
> +	fd = open(path, O_RDWR);
> +	if (fd < 0)
> +		return -errno;
> +
> +	ret = write(fd, "0", sizeof("0"));
> +	close(fd);
> +
> +	return ret;
> +}
> +
> +static int nap_create_bridge(void)
> +{
> +	int sk, err;
> +
> +	DBG("%s", BNEP_BRIDGE);
> +
> +	sk = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
> +	if (sk < 0)
> +		return -EOPNOTSUPP;
> +
> +	if (ioctl(sk, SIOCBRADDBR, BNEP_BRIDGE) < 0) {
> +		err = -errno;
> +		if (err != -EEXIST) {
> +			close(sk);
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	err = set_forward_delay();
> +	if (err < 0)
> +		ioctl(sk, SIOCBRDELBR, BNEP_BRIDGE);
> +
> +	close(sk);
> +
> +	return err;
> +}
> +
> +static int nap_remove_bridge(void)
> +{
> +	int sk, err;
> +
> +	DBG("%s", BNEP_BRIDGE);
> +
> +	sk = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0);
> +	if (sk < 0)
> +		return -EOPNOTSUPP;
> +
> +	err = ioctl(sk, SIOCBRDELBR, BNEP_BRIDGE);
> +	close(sk);
> +
> +	if (err < 0)
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
> +static void destroy_nap_device(void)
> +{
> +	DBG("");
> +
> +	nap_remove_bridge();
> +
> +	nap_dev.record_id = 0;
> +}
> +
> +static int register_nap_server(void)
> +{
> +	int err;
> +
> +	DBG("");
> +
> +	err = nap_create_bridge();
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
>  static void bt_pan_enable(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_pan_enable *cmd = buf;
> @@ -441,7 +537,15 @@ bool bt_pan_register(const bdaddr_t *addr)
>  		return false;
>  	}
>  
> -	record_id = rec->handle;
> +	err = register_nap_server();
> +	if (err < 0) {
> +		bt_adapter_remove_record(rec->handle);
> +		sdp_record_free(rec);
> +		bnep_cleanup();
> +		return false;
> +	}
> +
> +	nap_dev.record_id = rec->handle;
>  	ipc_register(HAL_SERVICE_ID_PAN, cmd_handlers,
>  						G_N_ELEMENTS(cmd_handlers));
>  
> @@ -455,6 +559,6 @@ void bt_pan_unregister(void)
>  	bnep_cleanup();
>  
>  	ipc_unregister(HAL_SERVICE_ID_PAN);
> -	bt_adapter_remove_record(record_id);
> -	record_id = 0;
> +	bt_adapter_remove_record(nap_dev.record_id);
> +	destroy_nap_device();

I would either zero nap_dev.record_id right after removing record or move
removing record to destroy_nap_device().

-- 
BR
Szymon Janc

Re: [PATCH_v3 3/4] android/pan: Implement PAN enable HAL api at daemon side

[Szymon Janc]

Hi Ravi,

> ---
>  android/pan.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/android/pan.c b/android/pan.c
> index 0eef284..0e12245 100644
> --- a/android/pan.c
> +++ b/android/pan.c
> @@ -585,18 +585,38 @@ static void bt_pan_enable(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_pan_enable *cmd = buf;
>  	uint8_t status;
> +	int err;
> +
> +	DBG("");
> +
> +	if (local_role == cmd->local_role) {
> +		status = HAL_STATUS_SUCCESS;
> +		goto reply;
> +	}
> +
> +	/* destroy existing server */
> +	destroy_nap_device();
>  
>  	switch (cmd->local_role) {
>  	case HAL_PAN_ROLE_PANU:
> -	case HAL_PAN_ROLE_NAP:
> -		DBG("Not Implemented");
> -		status  = HAL_STATUS_FAILED;
> -		break;
> -	default:
>  		status = HAL_STATUS_UNSUPPORTED;
> -		break;
> +		goto reply;
> +	case HAL_PAN_ROLE_NONE:
> +		status = HAL_STATUS_SUCCESS;
> +		goto reply;
> +	}

Why do you remove default here? Daemon should verify parameter correctness.

> +
> +	local_role = cmd->local_role;
> +	err = register_nap_server();
> +	if (err < 0) {
> +		status = HAL_STATUS_FAILED;
> +		destroy_nap_device();
> +		goto reply;
>  	}
>  
> +	status = HAL_STATUS_SUCCESS;
> +
> +reply:
>  	ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE, status);
>  }
>  
> 

-- 
BR
Szymon Janc

Re: [PATCH_v3 1/4] android/pan: Register Network Access Point

[Ravi kumar Veeramally]

Hi Szymon,

On 01/08/2014 11:18 AM, Szymon Janc wrote:
> Hi Ravi,
>
>> +
>> +	nap_dev.record_id = rec->handle;
>>   	ipc_register(HAL_SERVICE_ID_PAN, cmd_handlers,
>>   						G_N_ELEMENTS(cmd_handlers));
>>   
>> @@ -455,6 +559,6 @@ void bt_pan_unregister(void)
>>   	bnep_cleanup();
>>   
>>   	ipc_unregister(HAL_SERVICE_ID_PAN);
>> -	bt_adapter_remove_record(record_id);
>> -	record_id = 0;
>> +	bt_adapter_remove_record(nap_dev.record_id);
>> +	destroy_nap_device();
> I would either zero nap_dev.record_id right after removing record or move
> removing record to destroy_nap_device().

  OK, I will zero it right after removing record.

  Thanks,
  Ravi.

Re: [PATCH_v3 3/4] android/pan: Implement PAN enable HAL api at daemon side

[Ravi kumar Veeramally]

Hi Szymon,

On 01/08/2014 11:24 AM, Szymon Janc wrote:
> Hi Ravi,
>
>> ---
>>   android/pan.c | 32 ++++++++++++++++++++++++++------
>>   1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/android/pan.c b/android/pan.c
>> index 0eef284..0e12245 100644
>> --- a/android/pan.c
>> +++ b/android/pan.c
>> @@ -585,18 +585,38 @@ static void bt_pan_enable(const void *buf, uint16_t len)
>>   {
>>   	const struct hal_cmd_pan_enable *cmd = buf;
>>   	uint8_t status;
>> +	int err;
>> +
>> +	DBG("");
>> +
>> +	if (local_role == cmd->local_role) {
>> +		status = HAL_STATUS_SUCCESS;
>> +		goto reply;
>> +	}
>> +
>> +	/* destroy existing server */
>> +	destroy_nap_device();
>>   
>>   	switch (cmd->local_role) {
>>   	case HAL_PAN_ROLE_PANU:
>> -	case HAL_PAN_ROLE_NAP:
>> -		DBG("Not Implemented");
>> -		status  = HAL_STATUS_FAILED;
>> -		break;
>> -	default:
>>   		status = HAL_STATUS_UNSUPPORTED;
>> -		break;
>> +		goto reply;
>> +	case HAL_PAN_ROLE_NONE:
>> +		status = HAL_STATUS_SUCCESS;
>> +		goto reply;
>> +	}
> Why do you remove default here? Daemon should verify parameter correctness.

   Yes, default should be there, I think I was still in the impression of
  HAL side parameter validation. I will correct it in next version.

  Thanks,
  Ravi.
>
>> +
>> +	local_role = cmd->local_role;
>> +	err = register_nap_server();
>> +	if (err < 0) {
>> +		status = HAL_STATUS_FAILED;
>> +		destroy_nap_device();
>> +		goto reply;
>>   	}
>>   
>> +	status = HAL_STATUS_SUCCESS;
>> +
>> +reply:
>>   	ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE, status);
>>   }
>>   
>>

Re: [PATCH_v3 2/4] android/pan: Listen for incoming connections and accept in NAP role

[Ravi kumar Veeramally]

Hi Luiz,

On 01/08/2014 11:07 AM, Luiz Augusto von Dentz wrote:
> Hi Ravi,
>
> On Tue, Jan 7, 2014 at 2:31 PM, Ravi kumar Veeramally
> <ravikumar.veeramally@linux.intel.com> wrote:
>> Listen for incoming connections and accept it. Create bnep interface
>> add it to bridge and notify control and connection state information
>> through HAL. Remove the device on disconnect request. If android
>> settings UI does not have bluetooth tethering enabled it immediately
>> sends disconnect signal.
>> ---
>>   android/pan.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 190 insertions(+), 2 deletions(-)
>>
>> diff --git a/android/pan.c b/android/pan.c
>> index 93078ba..0eef284 100644
>> --- a/android/pan.c
>> +++ b/android/pan.c
>> @@ -63,12 +63,17 @@ struct pan_device {
>>          uint8_t         role;
>>          GIOChannel      *io;
>>          struct bnep     *session;
>> +       guint           watch;
>>   };
>>
>>   static struct {
>>          uint32_t        record_id;
>> +       guint           watch;
>> +       GIOChannel      *io;
>>   } nap_dev = {
>>          .record_id = 0,
>> +       .watch = 0,
>> +       .io = NULL,
>>   };
>>
>>   static int device_cmp(gconstpointer s, gconstpointer user_data)
>> @@ -81,13 +86,21 @@ static int device_cmp(gconstpointer s, gconstpointer user_data)
>>
>>   static void pan_device_free(struct pan_device *dev)
>>   {
>> +       if (dev->watch > 0) {
>> +               bnep_server_delete(BNEP_BRIDGE, dev->iface, &dev->dst);
>> +               g_source_remove(dev->watch);
>> +               dev->watch = 0;
> Usually it is not necessary to assign anything to struct members when
> you are freeing the whole struct since its memory gonna be freed
> anyway.

   Yes you are right. Profiles/network/connection.c keeps device data 
after disconnecting,
   it will remove only on device remove call. Assigning zero or NULL 
make sense there.
   But in bluez-android device struct t is removed from list on 
disconnected state.
   Here it won't make much sense. I will remove it in my next version.

  Thanks,
  Ravi.