[PATCH 1/6] shared: Add support for disonnect handler to GLib based IO handling

[PATCH 1/6] shared: Add support for disonnect handler to GLib based IO handling

[Szymon Janc]

---
 src/shared/io-glib.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
index 77ba19e..a4f982d 100644
--- a/src/shared/io-glib.c
+++ b/src/shared/io-glib.c
@@ -40,6 +40,10 @@ struct io {
 	io_callback_func_t write_callback;
 	io_destroy_func_t write_destroy;
 	void *write_data;
+	guint disconnect_watch;
+	io_callback_func_t disconnect_callback;
+	io_destroy_func_t disconnect_destroy;
+	void *disconnect_data;
 };
 
 static struct io *io_ref(struct io *io)
@@ -258,8 +262,61 @@ done:
 	return true;
 }
 
+static void disconnect_watch_destroy(gpointer user_data)
+{
+	struct io *io = user_data;
+
+	if (io->disconnect_destroy)
+		io->disconnect_destroy(io->disconnect_data);
+
+	io->disconnect_watch = 0;
+	io->disconnect_callback = NULL;
+	io->disconnect_destroy = NULL;
+	io->disconnect_data = NULL;
+
+	io_unref(io);
+}
+
+static gboolean disconnect_callback(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct io *io = user_data;
+	bool result;
+
+	if (io->disconnect_callback)
+		result = io->disconnect_callback(io, io->disconnect_data);
+	else
+		result = false;
+
+	return result ? TRUE : FALSE;
+}
+
 bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
 				void *user_data, io_destroy_func_t destroy)
 {
-	return false;
+	if (!io)
+		return false;
+
+	if (io->disconnect_watch > 0) {
+		g_source_remove(io->disconnect_watch);
+		io->disconnect_watch = 0;
+	}
+
+	if (!callback)
+		goto done;
+
+	io->disconnect_watch = g_io_add_watch_full(io->channel,
+				G_PRIORITY_DEFAULT,
+				G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				disconnect_callback, io_ref(io),
+				disconnect_watch_destroy);
+	if (io->disconnect_watch == 0)
+		return false;
+
+	io->disconnect_destroy = destroy;
+	io->disconnect_data = user_data;
+done:
+	io->disconnect_callback = callback;
+
+	return true;
 }
-- 
1.8.5.3

[PATCH 2/6] shared: Add support for disonnect handler to mainloop based IO handling

[Szymon Janc]

---
 src/shared/io-mainloop.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
index 981ab17..14ab128 100644
--- a/src/shared/io-mainloop.c
+++ b/src/shared/io-mainloop.c
@@ -42,6 +42,9 @@ struct io {
 	io_callback_func_t write_callback;
 	io_destroy_func_t write_destroy;
 	void *write_data;
+	io_callback_func_t disconnect_callback;
+	io_destroy_func_t disconnect_destroy;
+	void *disconnect_data;
 };
 
 static struct io *io_ref(struct io *io)
@@ -75,6 +78,9 @@ static void io_cleanup(void *user_data)
 	if (io->read_destroy)
 		io->read_destroy(io->read_data);
 
+	if (io->disconnect_destroy)
+		io->disconnect_destroy(io->disconnect_data);
+
 	if (io->close_on_destroy)
 		close(io->fd);
 
@@ -122,6 +128,21 @@ static void io_callback(int fd, uint32_t events, void *user_data)
 			mainloop_modify_fd(io->fd, io->events);
 		}
 	}
+
+	if ((events & EPOLLRDHUP) && io->disconnect_callback) {
+		if (!io->disconnect_callback(io, io->disconnect_data)) {
+			if (io->disconnect_destroy)
+				io->disconnect_destroy(io->disconnect_data);
+
+			io->disconnect_callback = NULL;
+			io->disconnect_destroy = NULL;
+			io->disconnect_data = NULL;
+
+			io->events &= ~EPOLLRDHUP;
+
+			mainloop_modify_fd(io->fd, io->events);
+		}
+	}
 }
 
 struct io *io_new(int fd)
@@ -246,5 +267,30 @@ bool io_set_write_handler(struct io *io, io_callback_func_t callback,
 bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
 				void *user_data, io_destroy_func_t destroy)
 {
-	return false;
+	uint32_t events;
+
+	if (!io || io->fd < 0)
+		return false;
+
+	if (io->disconnect_destroy)
+		io->disconnect_destroy(io->disconnect_data);
+
+	if (callback)
+		events = io->events | EPOLLRDHUP;
+	else
+		events = io->events & ~EPOLLRDHUP;
+
+	io->disconnect_callback = callback;
+	io->disconnect_destroy = destroy;
+	io->disconnect_data = user_data;
+
+	if (events == io->events)
+		return true;
+
+	if (mainloop_modify_fd(io->fd, events) < 0)
+		return false;
+
+	io->events = events;
+
+	return true;
 }
-- 
1.8.5.3

[PATCH 3/6] shared: Add support for disconnect handler in HFP

[Szymon Janc]

It is no longer possible to send any data after disconnection.
Extra reference is taken for disconnect watch to allow users to drop
own reference in disconnect callback.
---
 src/shared/hfp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 src/shared/hfp.h |  6 ++++++
 2 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 43e88a6..3944e97 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -51,6 +51,9 @@ struct hfp_gw {
 	hfp_debug_func_t debug_callback;
 	hfp_destroy_func_t debug_destroy;
 	void *debug_data;
+	hfp_disconnect_func_t disconnect_callback;
+	hfp_destroy_func_t disconnect_destroy;
+	void *disconnect_data;
 };
 
 static void write_watch_destroy(void *user_data)
@@ -227,6 +230,7 @@ void hfp_gw_unref(struct hfp_gw *hfp)
 
 	io_set_write_handler(hfp->io, NULL, NULL, NULL);
 	io_set_read_handler(hfp->io, NULL, NULL, NULL);
+	io_set_disconnect_handler(hfp->io, NULL, NULL, NULL);
 
 	io_destroy(hfp->io);
 	hfp->io = NULL;
@@ -307,7 +311,7 @@ bool hfp_gw_send_result(struct hfp_gw *hfp, enum hfp_result result)
 {
 	const char *str;
 
-	if (!hfp)
+	if (!hfp || !hfp->io)
 		return false;
 
 	switch (result) {
@@ -333,7 +337,7 @@ bool hfp_gw_send_result(struct hfp_gw *hfp, enum hfp_result result)
 
 bool hfp_gw_send_error(struct hfp_gw *hfp, enum hfp_error error)
 {
-	if (!hfp)
+	if (!hfp || !hfp->io)
 		return false;
 
 	if (ringbuf_printf(hfp->write_buf, "\r\n+CME ERROR: %u\r\n", error) < 0)
@@ -352,7 +356,7 @@ bool hfp_gw_send_info(struct hfp_gw *hfp, const char *format, ...)
 	char *fmt;
 	int len;
 
-	if (!hfp || !format)
+	if (!hfp || !hfp->io || !format)
 		return false;
 
 	if (asprintf(&fmt, "\r\n%s\r\n", format) < 0)
@@ -391,3 +395,53 @@ bool hfp_gw_set_command_handler(struct hfp_gw *hfp,
 
 	return true;
 }
+
+static void disconnect_watch_destroy(void *user_data)
+{
+	struct hfp_gw *hfp = user_data;
+
+	if (hfp->disconnect_destroy)
+		hfp->disconnect_destroy(hfp->disconnect_data);
+
+	hfp_gw_unref(hfp);
+}
+
+static bool io_disconnected(struct io *io, void *user_data)
+{
+	struct hfp_gw *hfp = user_data;
+
+	io_destroy(hfp->io);
+	hfp->io = NULL;
+
+	if (hfp->disconnect_callback)
+		hfp->disconnect_callback(hfp->disconnect_data);
+
+	return false;
+}
+
+bool hfp_gw_set_disconnect_handler(struct hfp_gw *hfp,
+					hfp_disconnect_func_t callback,
+					void *user_data,
+					hfp_destroy_func_t destroy)
+{
+	if (!hfp)
+		return false;
+
+	if (hfp->disconnect_destroy)
+		hfp->disconnect_destroy(hfp->disconnect_data);
+
+	if (!io_set_disconnect_handler(hfp->io, io_disconnected,
+						hfp_gw_ref(hfp),
+						disconnect_watch_destroy)) {
+		hfp->disconnect_callback = NULL;
+		hfp->disconnect_destroy = NULL;
+		hfp->disconnect_data = NULL;
+		return false;
+	}
+
+	hfp->disconnect_callback = callback;
+	hfp->disconnect_destroy = destroy;
+	hfp->disconnect_data = user_data;
+
+	return true;
+}
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 2eca6d8..75ec484 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -64,6 +64,7 @@ typedef void (*hfp_destroy_func_t)(void *user_data);
 typedef void (*hfp_debug_func_t)(const char *str, void *user_data);
 
 typedef void (*hfp_command_func_t)(const char *command, void *user_data);
+typedef void (*hfp_disconnect_func_t)(void *user_data);
 
 struct hfp_gw;
 
@@ -86,3 +87,8 @@ bool hfp_gw_send_info(struct hfp_gw *hfp, const char *format, ...)
 bool hfp_gw_set_command_handler(struct hfp_gw *hfp,
 				hfp_command_func_t callback,
 				void *user_data, hfp_destroy_func_t destroy);
+
+bool hfp_gw_set_disconnect_handler(struct hfp_gw *hfp,
+					hfp_disconnect_func_t callback,
+					void *user_data,
+					hfp_destroy_func_t destroy);
-- 
1.8.5.3

[PATCH 4/6] shared: Add support for shutdown to IO

[Szymon Janc]

This allows to locally shutdown IO.
---
 src/shared/io-glib.c     | 9 +++++++++
 src/shared/io-mainloop.c | 9 +++++++++
 src/shared/io.h          | 2 ++
 3 files changed, 20 insertions(+)

diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
index a4f982d..8290745 100644
--- a/src/shared/io-glib.c
+++ b/src/shared/io-glib.c
@@ -320,3 +320,12 @@ done:
 
 	return true;
 }
+
+bool io_shutdown(struct io *io)
+{
+	if (!io || !io->channel)
+		return false;
+
+	return g_io_channel_shutdown(io->channel, TRUE, NULL)
+							== G_IO_STATUS_NORMAL;
+}
diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
index 14ab128..f1e3b3b 100644
--- a/src/shared/io-mainloop.c
+++ b/src/shared/io-mainloop.c
@@ -26,6 +26,7 @@
 #endif
 
 #include <unistd.h>
+#include <sys/socket.h>
 
 #include "monitor/mainloop.h"
 #include "src/shared/util.h"
@@ -294,3 +295,11 @@ bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
 
 	return true;
 }
+
+bool io_shutdown(struct io *io)
+{
+	if (!io || io->fd < 0)
+		return false;
+
+	return shutdown(io->fd, SHUT_RDWR) == 0;
+}
diff --git a/src/shared/io.h b/src/shared/io.h
index 2c47e39..2106240 100644
--- a/src/shared/io.h
+++ b/src/shared/io.h
@@ -33,6 +33,8 @@ void io_destroy(struct io *io);
 int io_get_fd(struct io *io);
 bool io_set_close_on_destroy(struct io *io, bool do_close);
 
+bool io_shutdown(struct io *io);
+
 typedef bool (*io_callback_func_t)(struct io *io, void *user_data);
 
 bool io_set_read_handler(struct io *io, io_callback_func_t callback,
-- 
1.8.5.3

[PATCH 5/6] shared: Add support for local disconnect to HFP

[Szymon Janc]

This allows to locally trigger disconnection.
---
 src/shared/hfp.c | 8 ++++++++
 src/shared/hfp.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/src/shared/hfp.c b/src/shared/hfp.c
index 3944e97..2c793f9 100644
--- a/src/shared/hfp.c
+++ b/src/shared/hfp.c
@@ -445,3 +445,11 @@ bool hfp_gw_set_disconnect_handler(struct hfp_gw *hfp,
 
 	return true;
 }
+
+bool hfp_gw_disconnect(struct hfp_gw *hfp)
+{
+	if (!hfp)
+		return false;
+
+	return io_shutdown(hfp->io);
+}
diff --git a/src/shared/hfp.h b/src/shared/hfp.h
index 75ec484..5a86dfe 100644
--- a/src/shared/hfp.h
+++ b/src/shared/hfp.h
@@ -92,3 +92,5 @@ bool hfp_gw_set_disconnect_handler(struct hfp_gw *hfp,
 					hfp_disconnect_func_t callback,
 					void *user_data,
 					hfp_destroy_func_t destroy);
+
+bool hfp_gw_disconnect(struct hfp_gw *hfp);
-- 
1.8.5.3

[PATCH 6/6] android/handsfree: Use HFP code for connection handling

[Szymon Janc]

HFP code is now able to handle disconnection on its own so just use
this instead of using own watches.
---
 android/handsfree.c | 50 +++++++++++++-------------------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/android/handsfree.c b/android/handsfree.c
index 9482b2e..a973bd5 100644
--- a/android/handsfree.c
+++ b/android/handsfree.c
@@ -52,8 +52,6 @@
 static struct {
 	bdaddr_t bdaddr;
 	uint8_t state;
-	GIOChannel *io;
-	guint watch;
 	struct hfp_gw *gw;
 } device;
 
@@ -96,38 +94,23 @@ static void device_cleanup(void)
 		device.gw = NULL;
 	}
 
-	if (device.watch) {
-		g_source_remove(device.watch);
-		device.watch = 0;
-	}
-
-	if (device.io) {
-		g_io_channel_unref(device.io);
-		device.io = NULL;
-	}
-
 	device_set_state(HAL_EV_HANDSFREE_CONNECTION_STATE_DISCONNECTED);
 
 	memset(&device, 0, sizeof(device));
 }
 
-static gboolean watch_cb(GIOChannel *chan, GIOCondition cond,
-							gpointer user_data)
+static void at_command_handler(const char *command, void *user_data)
 {
-	DBG("");
-
-	device.watch = 0;
-
-	device_cleanup();
+	hfp_gw_send_result(device.gw, HFP_RESULT_ERROR);
 
-	return FALSE;
+	hfp_gw_disconnect(device.gw);
 }
 
-static void at_command_handler(const char *command, void *user_data)
+static void disconnect_watch(void *user_data)
 {
-	hfp_gw_send_result(device.gw, HFP_RESULT_ERROR);
+	DBG("");
 
-	g_io_channel_shutdown(device.io, TRUE, NULL);
+	device_cleanup();
 }
 
 static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
@@ -139,22 +122,15 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 		goto failed;
 	}
 
-	g_io_channel_set_close_on_unref(chan, TRUE);
-
 	device.gw = hfp_gw_new(g_io_channel_unix_get_fd(chan));
 	if (!device.gw)
 		goto failed;
 
+	g_io_channel_set_close_on_unref(chan, FALSE);
+
 	hfp_gw_set_close_on_unref(device.gw, true);
 	hfp_gw_set_command_handler(device.gw, at_command_handler, NULL, NULL);
-
-	device.watch = g_io_add_watch(chan,
-					G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-					watch_cb, NULL);
-	if (device.watch == 0)
-		goto failed;
-
-	device.io = g_io_channel_ref(chan);
+	hfp_gw_set_disconnect_handler(device.gw, disconnect_watch, NULL, NULL);
 
 	device_set_state(HAL_EV_HANDSFREE_CONNECTION_STATE_CONNECTED);
 
@@ -332,11 +308,11 @@ static void handle_disconnect(const void *buf, uint16_t len)
 		goto failed;
 	}
 
-	if (device.io) {
-		device_set_state(HAL_EV_HANDSFREE_CONNECTION_STATE_DISCONNECTING);
-		g_io_channel_shutdown(device.io, TRUE, NULL);
-	} else {
+	if (device.state == HAL_EV_HANDSFREE_CONNECTION_STATE_CONNECTING) {
 		device_cleanup();
+	} else {
+		device_set_state(HAL_EV_HANDSFREE_CONNECTION_STATE_DISCONNECTING);
+		hfp_gw_disconnect(device.gw);
 	}
 
 	status = HAL_STATUS_SUCCESS;
-- 
1.8.5.3

Re: [PATCH 1/6] shared: Add support for disonnect handler to GLib based IO handling

[Marcel Holtmann]

Hi Szymon,

> ---
> src/shared/io-glib.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> index 77ba19e..a4f982d 100644
> --- a/src/shared/io-glib.c
> +++ b/src/shared/io-glib.c
> @@ -40,6 +40,10 @@ struct io {
> 	io_callback_func_t write_callback;
> 	io_destroy_func_t write_destroy;
> 	void *write_data;
> +	guint disconnect_watch;
> +	io_callback_func_t disconnect_callback;
> +	io_destroy_func_t disconnect_destroy;
> +	void *disconnect_data;
> };
> 
> static struct io *io_ref(struct io *io)
> @@ -258,8 +262,61 @@ done:
> 	return true;
> }
> 
> +static void disconnect_watch_destroy(gpointer user_data)
> +{
> +	struct io *io = user_data;
> +
> +	if (io->disconnect_destroy)
> +		io->disconnect_destroy(io->disconnect_data);
> +
> +	io->disconnect_watch = 0;
> +	io->disconnect_callback = NULL;
> +	io->disconnect_destroy = NULL;
> +	io->disconnect_data = NULL;
> +
> +	io_unref(io);
> +}
> +
> +static gboolean disconnect_callback(GIOChannel *channel, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	struct io *io = user_data;
> +	bool result;
> +
> +	if (io->disconnect_callback)
> +		result = io->disconnect_callback(io, io->disconnect_data);
> +	else
> +		result = false;
> +
> +	return result ? TRUE : FALSE;
> +}
> +
> bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> 				void *user_data, io_destroy_func_t destroy)
> {
> -	return false;
> +	if (!io)
> +		return false;
> +
> +	if (io->disconnect_watch > 0) {
> +		g_source_remove(io->disconnect_watch);
> +		io->disconnect_watch = 0;
> +	}
> +
> +	if (!callback)
> +		goto done;
> +
> +	io->disconnect_watch = g_io_add_watch_full(io->channel,
> +				G_PRIORITY_DEFAULT,
> +				G_IO_HUP | G_IO_ERR | G_IO_NVAL,

If we are using G_IO_HUP here, we should remove it from the other callback handling?

> +				disconnect_callback, io_ref(io),
> +				disconnect_watch_destroy);
> +	if (io->disconnect_watch == 0)
> +		return false;
> +
> +	io->disconnect_destroy = destroy;
> +	io->disconnect_data = user_data;

we normally have an extra empty line here. You might also fix the previous patch then.

> +done:
> +	io->disconnect_callback = callback;
> +
> +	return true;
> }
> -- 
> 1.8.5.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/6] shared: Add support for disconnect handler in HFP

[Marcel Holtmann]

Hi Szymon,

> It is no longer possible to send any data after disconnection.
> Extra reference is taken for disconnect watch to allow users to drop
> own reference in disconnect callback.

I am not sure that is the best way to do it. I get the feeling that we might better have an internal bool that is tracking if we are connected/disconnected. Thoughts?

Regards

Marcel

Re: [PATCH 4/6] shared: Add support for shutdown to IO

[Marcel Holtmann]

Hi Szymon,

> This allows to locally shutdown IO.
> ---
> src/shared/io-glib.c     | 9 +++++++++
> src/shared/io-mainloop.c | 9 +++++++++
> src/shared/io.h          | 2 ++
> 3 files changed, 20 insertions(+)
> 
> diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> index a4f982d..8290745 100644
> --- a/src/shared/io-glib.c
> +++ b/src/shared/io-glib.c
> @@ -320,3 +320,12 @@ done:
> 
> 	return true;
> }
> +
> +bool io_shutdown(struct io *io)
> +{
> +	if (!io || !io->channel)
> +		return false;
> +
> +	return g_io_channel_shutdown(io->channel, TRUE, NULL)
> +							== G_IO_STATUS_NORMAL;
> +}
> diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
> index 14ab128..f1e3b3b 100644
> --- a/src/shared/io-mainloop.c
> +++ b/src/shared/io-mainloop.c
> @@ -26,6 +26,7 @@
> #endif
> 
> #include <unistd.h>
> +#include <sys/socket.h>
> 
> #include "monitor/mainloop.h"
> #include "src/shared/util.h"
> @@ -294,3 +295,11 @@ bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> 
> 	return true;
> }
> +
> +bool io_shutdown(struct io *io)
> +{
> +	if (!io || io->fd < 0)
> +		return false;
> +
> +	return shutdown(io->fd, SHUT_RDWR) == 0;
> +}

I have no problem doing this, but why is this actually needed? Is not closing the socket good enough? Or would be better also add a shutdown_on_unref option?

Regards

Marcel

Re: [PATCH 4/6] shared: Add support for shutdown to IO

[Szymon Janc]

Hi Marcel,

On Tuesday 11 of February 2014 09:02:15 Marcel Holtmann wrote:
> Hi Szymon,
> 
> > This allows to locally shutdown IO.
> > ---
> > src/shared/io-glib.c     | 9 +++++++++
> > src/shared/io-mainloop.c | 9 +++++++++
> > src/shared/io.h          | 2 ++
> > 3 files changed, 20 insertions(+)
> > 
> > diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> > index a4f982d..8290745 100644
> > --- a/src/shared/io-glib.c
> > +++ b/src/shared/io-glib.c
> > 
> > @@ -320,3 +320,12 @@ done:
> > 	return true;
> > 
> > }
> > +
> > +bool io_shutdown(struct io *io)
> > +{
> > +	if (!io || !io->channel)
> > +		return false;
> > +
> > +	return g_io_channel_shutdown(io->channel, TRUE, NULL)
> > +							== G_IO_STATUS_NORMAL;
> > +}
> > diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
> > index 14ab128..f1e3b3b 100644
> > --- a/src/shared/io-mainloop.c
> > +++ b/src/shared/io-mainloop.c
> > @@ -26,6 +26,7 @@
> > #endif
> > 
> > #include <unistd.h>
> > +#include <sys/socket.h>
> > 
> > #include "monitor/mainloop.h"
> > #include "src/shared/util.h"
> > @@ -294,3 +295,11 @@ bool io_set_disconnect_handler(struct io *io,
> > io_callback_func_t callback,> 
> > 	return true;
> > 
> > }
> > +
> > +bool io_shutdown(struct io *io)
> > +{
> > +	if (!io || io->fd < 0)
> > +		return false;
> > +
> > +	return shutdown(io->fd, SHUT_RDWR) == 0;
> > +}
> 
> I have no problem doing this, but why is this actually needed? Is not
> closing the socket good enough? Or would be better also add a
> shutdown_on_unref option?

This is to allow to read from the socket in case there is some data already 
received (FWIW). I also plan to add 'flush' or similar flag to it to allow 
graceful disconnect i.e. make something like this work as expected

hfp_gw_send_result(device.gw, HFP_RESULT_ERROR);
hfp_gw_disconnect(device.gw);

but this can be added later on if needed and we could go with close() for now 
as you suggested.

-- 
BR
Szymon Janc

Re: [PATCH 3/6] shared: Add support for disconnect handler in HFP

[Szymon Janc]

Hi Marcel,

On Tuesday 11 of February 2014 09:00:27 Marcel Holtmann wrote:
> Hi Szymon,
> 
> > It is no longer possible to send any data after disconnection.
> > Extra reference is taken for disconnect watch to allow users to drop
> > own reference in disconnect callback.
> 
> I am not sure that is the best way to do it. I get the feeling that we might
> better have an internal bool that is tracking if we are
> connected/disconnected. Thoughts?

You mean don't have a public HFP disconnect handler and just fail on write() 
or use bool instead of io pointer for state tracking?

-- 
BR
Szymon Janc

Re: [PATCH 1/6] shared: Add support for disonnect handler to GLib based IO handling

[Szymon Janc]

Hi Marcel,

On Tuesday 11 of February 2014 08:57:25 Marcel Holtmann wrote:
> Hi Szymon,
> 
> > ---
> > src/shared/io-glib.c | 59
> > +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 58
> > insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> > index 77ba19e..a4f982d 100644
> > --- a/src/shared/io-glib.c
> > +++ b/src/shared/io-glib.c
> > @@ -40,6 +40,10 @@ struct io {
> > 
> > 	io_callback_func_t write_callback;
> > 	io_destroy_func_t write_destroy;
> > 	void *write_data;
> > 
> > +	guint disconnect_watch;
> > +	io_callback_func_t disconnect_callback;
> > +	io_destroy_func_t disconnect_destroy;
> > +	void *disconnect_data;
> > };
> > 
> > static struct io *io_ref(struct io *io)
> > 
> > @@ -258,8 +262,61 @@ done:
> > 	return true;
> > 
> > }
> > 
> > +static void disconnect_watch_destroy(gpointer user_data)
> > +{
> > +	struct io *io = user_data;
> > +
> > +	if (io->disconnect_destroy)
> > +		io->disconnect_destroy(io->disconnect_data);
> > +
> > +	io->disconnect_watch = 0;
> > +	io->disconnect_callback = NULL;
> > +	io->disconnect_destroy = NULL;
> > +	io->disconnect_data = NULL;
> > +
> > +	io_unref(io);
> > +}
> > +
> > +static gboolean disconnect_callback(GIOChannel *channel, GIOCondition
> > cond, +							gpointer user_data)
> > +{
> > +	struct io *io = user_data;
> > +	bool result;
> > +
> > +	if (io->disconnect_callback)
> > +		result = io->disconnect_callback(io, io->disconnect_data);
> > +	else
> > +		result = false;
> > +
> > +	return result ? TRUE : FALSE;
> > +}
> > +
> > bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> > 
> > 				void *user_data, io_destroy_func_t destroy)
> > 
> > {
> > -	return false;
> > +	if (!io)
> > +		return false;
> > +
> > +	if (io->disconnect_watch > 0) {
> > +		g_source_remove(io->disconnect_watch);
> > +		io->disconnect_watch = 0;
> > +	}
> > +
> > +	if (!callback)
> > +		goto done;
> > +
> > +	io->disconnect_watch = g_io_add_watch_full(io->channel,
> > +				G_PRIORITY_DEFAULT,
> > +				G_IO_HUP | G_IO_ERR | G_IO_NVAL,
> 
> If we are using G_IO_HUP here, we should remove it from the other callback
> handling?

We should, will fix that in next version.

> > +				disconnect_callback, io_ref(io),
> > +				disconnect_watch_destroy);
> > +	if (io->disconnect_watch == 0)
> > +		return false;
> > +
> > +	io->disconnect_destroy = destroy;
> > +	io->disconnect_data = user_data;
> 
> we normally have an extra empty line here. You might also fix the previous
> patch then.

I'll fix that as well.

> > +done:
> > +	io->disconnect_callback = callback;
> > +
> > +	return true;
> > }

-- 
BR
Szymon Janc

Re: [PATCH 4/6] shared: Add support for shutdown to IO

[Marcel Holtmann]

Hi Szymon,

>>> This allows to locally shutdown IO.
>>> ---
>>> src/shared/io-glib.c     | 9 +++++++++
>>> src/shared/io-mainloop.c | 9 +++++++++
>>> src/shared/io.h          | 2 ++
>>> 3 files changed, 20 insertions(+)
>>> 
>>> diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
>>> index a4f982d..8290745 100644
>>> --- a/src/shared/io-glib.c
>>> +++ b/src/shared/io-glib.c
>>> 
>>> @@ -320,3 +320,12 @@ done:
>>> 	return true;
>>> 
>>> }
>>> +
>>> +bool io_shutdown(struct io *io)
>>> +{
>>> +	if (!io || !io->channel)
>>> +		return false;
>>> +
>>> +	return g_io_channel_shutdown(io->channel, TRUE, NULL)
>>> +							== G_IO_STATUS_NORMAL;
>>> +}
>>> diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
>>> index 14ab128..f1e3b3b 100644
>>> --- a/src/shared/io-mainloop.c
>>> +++ b/src/shared/io-mainloop.c
>>> @@ -26,6 +26,7 @@
>>> #endif
>>> 
>>> #include <unistd.h>
>>> +#include <sys/socket.h>
>>> 
>>> #include "monitor/mainloop.h"
>>> #include "src/shared/util.h"
>>> @@ -294,3 +295,11 @@ bool io_set_disconnect_handler(struct io *io,
>>> io_callback_func_t callback,> 
>>> 	return true;
>>> 
>>> }
>>> +
>>> +bool io_shutdown(struct io *io)
>>> +{
>>> +	if (!io || io->fd < 0)
>>> +		return false;
>>> +
>>> +	return shutdown(io->fd, SHUT_RDWR) == 0;
>>> +}
>> 
>> I have no problem doing this, but why is this actually needed? Is not
>> closing the socket good enough? Or would be better also add a
>> shutdown_on_unref option?
> 
> This is to allow to read from the socket in case there is some data already 
> received (FWIW). I also plan to add 'flush' or similar flag to it to allow 
> graceful disconnect i.e. make something like this work as expected
> 
> hfp_gw_send_result(device.gw, HFP_RESULT_ERROR);
> hfp_gw_disconnect(device.gw);
> 
> but this can be added later on if needed and we could go with close() for now 
> as you suggested.

then lets just go with io_shutdown() for now. We can change this at any time once we know what other users really need. In the long run I want everything in android/ use the IO framework.

Regards

Marcel

Re: [PATCH 3/6] shared: Add support for disconnect handler in HFP

[Marcel Holtmann]

Hi Szymon,

>>> It is no longer possible to send any data after disconnection.
>>> Extra reference is taken for disconnect watch to allow users to drop
>>> own reference in disconnect callback.
>> 
>> I am not sure that is the best way to do it. I get the feeling that we might
>> better have an internal bool that is tracking if we are
>> connected/disconnected. Thoughts?
> 
> You mean don't have a public HFP disconnect handler and just fail on write() 
> or use bool instead of io pointer for state tracking?

we should still have a disconnect handler. However we should not take these extra reference and have these extra checks for something.

I rather see us tracking the actual state of the IO and not trying to reference count our way into something insane. It is dangerous to have to many indirect reference counts.

Regards

Marcel