[PATCH 1/4] Rename buf to read_buf in GAtServer

[PATCH 1/4] Rename buf to read_buf in GAtServer

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 6136 bytes --]

Because we may introduce write_buf for sever response buffer.
---
 gatchat/gatserver.c |   54 +++++++++++++++++++++++++-------------------------
 1 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index a01ded6..16c976d 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -95,7 +95,7 @@ struct _GAtServer {
 	gpointer user_disconnect_data;		/* User disconnect data */
 	GAtDebugFunc debugf;			/* Debugging output function */
 	gpointer debug_data;			/* Data to pass to debug func */
-	struct ring_buffer *buf;		/* Current read buffer */
+	struct ring_buffer *read_buf;		/* Current read buffer */
 	guint max_read_attempts;		/* Max reads per select */
 	enum ParserState parser_state;
 };
@@ -257,9 +257,9 @@ out:
 
 static char *extract_line(GAtServer *p)
 {
-	unsigned int wrap = ring_buffer_len_no_wrap(p->buf);
+	unsigned int wrap = ring_buffer_len_no_wrap(p->read_buf);
 	unsigned int pos = 0;
-	unsigned char *buf = ring_buffer_read_ptr(p->buf, pos);
+	unsigned char *buf = ring_buffer_read_ptr(p->read_buf, pos);
 	int strip_front = 0;
 	int line_length = 0;
 	gboolean in_string = FALSE;
@@ -280,7 +280,7 @@ static char *extract_line(GAtServer *p)
 		pos += 1;
 
 		if (pos == wrap)
-			buf = ring_buffer_read_ptr(p->buf, pos);
+			buf = ring_buffer_read_ptr(p->read_buf, pos);
 	}
 
 	/* We will strip AT and \r */
@@ -289,17 +289,17 @@ static char *extract_line(GAtServer *p)
 	line = g_try_new(char, line_length + 1);
 
 	if (!line) {
-		ring_buffer_drain(p->buf, p->read_so_far);
+		ring_buffer_drain(p->read_buf, p->read_so_far);
 		return NULL;
 	}
 
 	/* Strip leading whitespace + AT */
-	ring_buffer_drain(p->buf, strip_front + 2);
+	ring_buffer_drain(p->read_buf, strip_front + 2);
 
 	pos = 0;
 	i = 0;
-	wrap = ring_buffer_len_no_wrap(p->buf);
-	buf = ring_buffer_read_ptr(p->buf, pos);
+	wrap = ring_buffer_len_no_wrap(p->read_buf);
+	buf = ring_buffer_read_ptr(p->read_buf, pos);
 
 	while (pos < (p->read_so_far - strip_front - 2)) {
 		if (*buf == '"')
@@ -314,11 +314,11 @@ static char *extract_line(GAtServer *p)
 		pos += 1;
 
 		if (pos == wrap)
-			buf = ring_buffer_read_ptr(p->buf, pos);
+			buf = ring_buffer_read_ptr(p->read_buf, pos);
 	}
 
 	/* Strip \r */
-	ring_buffer_drain(p->buf, p->read_so_far - strip_front - 2);
+	ring_buffer_drain(p->read_buf, p->read_so_far - strip_front - 2);
 
 	line[i] = '\0';
 
@@ -327,9 +327,9 @@ static char *extract_line(GAtServer *p)
 
 static void new_bytes(GAtServer *p)
 {
-	unsigned int len = ring_buffer_len(p->buf);
-	unsigned int wrap = ring_buffer_len_no_wrap(p->buf);
-	unsigned char *buf = ring_buffer_read_ptr(p->buf, p->read_so_far);
+	unsigned int len = ring_buffer_len(p->read_buf);
+	unsigned int wrap = ring_buffer_len_no_wrap(p->read_buf);
+	unsigned char *buf = ring_buffer_read_ptr(p->read_buf, p->read_so_far);
 	enum ParserState result;
 
 	while (p->server_io && (p->read_so_far < len)) {
@@ -340,7 +340,7 @@ static void new_bytes(GAtServer *p)
 		p->read_so_far += rbytes;
 
 		if (p->read_so_far == wrap) {
-			buf = ring_buffer_read_ptr(p->buf, p->read_so_far);
+			buf = ring_buffer_read_ptr(p->read_buf, p->read_so_far);
 			wrap = len;
 		}
 
@@ -354,7 +354,7 @@ static void new_bytes(GAtServer *p)
 			 * Empty commands must be OK by the DCE
 			 */
 			g_at_server_send_result(p, G_AT_SERVER_RESULT_OK);
-			ring_buffer_drain(p->buf, p->read_so_far);
+			ring_buffer_drain(p->read_buf, p->read_so_far);
 			break;
 
 		case PARSER_RESULT_COMMAND:
@@ -364,11 +364,11 @@ static void new_bytes(GAtServer *p)
 		case PARSER_RESULT_REPEAT_LAST:
 			/* TODO */
 			g_at_server_send_result(p, G_AT_SERVER_RESULT_OK);
-			ring_buffer_drain(p->buf, p->read_so_far);
+			ring_buffer_drain(p->read_buf, p->read_so_far);
 			break;
 
 		default:
-			ring_buffer_drain(p->buf, p->read_so_far);
+			ring_buffer_drain(p->read_buf, p->read_so_far);
 			break;
 		}
 
@@ -378,7 +378,7 @@ static void new_bytes(GAtServer *p)
 	}
 
 	/* We're overflowing the buffer, shutdown the socket */
-	if (p->buf && ring_buffer_avail(p->buf) == 0)
+	if (p->read_buf && ring_buffer_avail(p->read_buf) == 0)
 		g_source_remove(p->server_watch);
 }
 
@@ -397,13 +397,13 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 		return FALSE;
 
 	do {
-		toread = ring_buffer_avail_no_wrap(server->buf);
+		toread = ring_buffer_avail_no_wrap(server->read_buf);
 
 		if (toread == 0)
 			break;
 
 		rbytes = 0;
-		buf = ring_buffer_write_ptr(server->buf);
+		buf = ring_buffer_write_ptr(server->read_buf);
 
 		err = g_io_channel_read(channel, (char *) buf, toread, &rbytes);
 		g_at_util_debug_chat(TRUE, (char *)buf, rbytes,
@@ -414,7 +414,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 		total_read += rbytes;
 
 		if (rbytes > 0)
-			ring_buffer_write_advance(server->buf, rbytes);
+			ring_buffer_write_advance(server->read_buf, rbytes);
 	} while (err == G_IO_ERROR_NONE && rbytes > 0 &&
 					read_count < server->max_read_attempts);
 
@@ -434,8 +434,8 @@ static void server_watcher_destroy_notify(GAtServer *server)
 {
 	server->server_watch = 0;
 
-	ring_buffer_free(server->buf);
-	server->buf = NULL;
+	ring_buffer_free(server->read_buf);
+	server->read_buf = NULL;
 
 	server->server_io = NULL;
 
@@ -470,10 +470,10 @@ GAtServer *g_at_server_new(GIOChannel *io)
 	server->ref_count = 1;
 	v250_settings_create(&server->v250);
 	server->server_io = io;
-	server->buf = ring_buffer_new(4096);
+	server->read_buf = ring_buffer_new(4096);
 	server->max_read_attempts = 3;
 
-	if (!server->buf)
+	if (!server->read_buf)
 		goto error;
 
 	if (!g_at_util_setup_io(server->server_io, G_IO_FLAG_NONBLOCK))
@@ -487,8 +487,8 @@ GAtServer *g_at_server_new(GIOChannel *io)
 	return server;
 
 error:
-	if (server->buf)
-		ring_buffer_free(server->buf);
+	if (server->read_buf)
+		ring_buffer_free(server->read_buf);
 
 	if (server)
 		g_free(server);
-- 
1.6.6.1

[PATCH 2/4] Replace sprintf with snprintf

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

---
 gatchat/gatserver.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index 16c976d..ad6afb9 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -116,9 +116,10 @@ static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
 		return;
 
 	if (v250.is_v1)
-		sprintf(buf, "%c%c%s%c%c", t, r, result_str, t, r);
+		snprintf(buf, sizeof(buf), "%c%c%s%c%c", t, r, result_str,
+				t, r);
 	else
-		sprintf(buf, "%u%c", (unsigned int) result, t);
+		snprintf(buf, sizeof(buf), "%u%c", (unsigned int) result, t);
 
 	g_at_util_debug_chat(FALSE, buf, strlen(buf),
 				server->debugf, server->debug_data);
-- 
1.6.6.1

[PATCH 3/4] Rename server_io to channel

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 2289 bytes --]

To make it consistent with GAtChat.
---
 gatchat/gatserver.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index ad6afb9..d131b0c 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -88,7 +88,7 @@ struct v250_settings {
 struct _GAtServer {
 	gint ref_count;				/* Ref count */
 	struct v250_settings v250;		/* V.250 command setting */
-	GIOChannel *server_io;			/* Server IO */
+	GIOChannel *channel;			/* Server IO */
 	int server_watch;			/* Watch for server IO */
 	guint read_so_far;			/* Number of bytes processed */
 	GAtDisconnectFunc user_disconnect;	/* User disconnect func */
@@ -124,7 +124,7 @@ static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
 	g_at_util_debug_chat(FALSE, buf, strlen(buf),
 				server->debugf, server->debug_data);
 
-	g_io_channel_write(server->server_io, (char *) buf, strlen(buf),
+	g_io_channel_write(server->channel, (char *) buf, strlen(buf),
 							&wbuf);
 }
 
@@ -333,7 +333,7 @@ static void new_bytes(GAtServer *p)
 	unsigned char *buf = ring_buffer_read_ptr(p->read_buf, p->read_so_far);
 	enum ParserState result;
 
-	while (p->server_io && (p->read_so_far < len)) {
+	while (p->channel && (p->read_so_far < len)) {
 		gsize rbytes = MIN(len - p->read_so_far, wrap - p->read_so_far);
 		result = server_feed(p, (char *)buf, &rbytes);
 
@@ -438,7 +438,7 @@ static void server_watcher_destroy_notify(GAtServer *server)
 	ring_buffer_free(server->read_buf);
 	server->read_buf = NULL;
 
-	server->server_io = NULL;
+	server->channel = NULL;
 
 	if (server->user_disconnect)
 		server->user_disconnect(server->user_disconnect_data);
@@ -470,14 +470,14 @@ GAtServer *g_at_server_new(GIOChannel *io)
 
 	server->ref_count = 1;
 	v250_settings_create(&server->v250);
-	server->server_io = io;
+	server->channel = io;
 	server->read_buf = ring_buffer_new(4096);
 	server->max_read_attempts = 3;
 
 	if (!server->read_buf)
 		goto error;
 
-	if (!g_at_util_setup_io(server->server_io, G_IO_FLAG_NONBLOCK))
+	if (!g_at_util_setup_io(server->channel, G_IO_FLAG_NONBLOCK))
 		goto error;
 
 	server->server_watch = g_io_add_watch_full(io, G_PRIORITY_DEFAULT,
-- 
1.6.6.1

[PATCH 4/4] Do not trigger user disconnect at g_at_shutdown

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 3756 bytes --]

Similar to gatchat, introduce read_watcher_destroy_notify and do
not trigger user disconnect at g_at_shutdown. Delay destroy of
gatserver until read_watcher is destroyed.
---
 gatchat/gatserver.c |   44 ++++++++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index d131b0c..bf7e847 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -89,7 +89,7 @@ struct _GAtServer {
 	gint ref_count;				/* Ref count */
 	struct v250_settings v250;		/* V.250 command setting */
 	GIOChannel *channel;			/* Server IO */
-	int server_watch;			/* Watch for server IO */
+	guint read_watch;			/* GSource read id, 0 if none */
 	guint read_so_far;			/* Number of bytes processed */
 	GAtDisconnectFunc user_disconnect;	/* User disconnect func */
 	gpointer user_disconnect_data;		/* User disconnect data */
@@ -98,6 +98,7 @@ struct _GAtServer {
 	struct ring_buffer *read_buf;		/* Current read buffer */
 	guint max_read_attempts;		/* Max reads per select */
 	enum ParserState parser_state;
+	gboolean destroyed;			/* Re-entrancy guard */
 };
 
 static void g_at_server_send_result(GAtServer *server, GAtServerResult result)
@@ -380,7 +381,7 @@ static void new_bytes(GAtServer *p)
 
 	/* We're overflowing the buffer, shutdown the socket */
 	if (p->read_buf && ring_buffer_avail(p->read_buf) == 0)
-		g_source_remove(p->server_watch);
+		g_source_remove(p->read_watch);
 }
 
 static gboolean received_data(GIOChannel *channel, GIOCondition cond,
@@ -431,17 +432,27 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 	return TRUE;
 }
 
-static void server_watcher_destroy_notify(GAtServer *server)
+static void g_at_server_cleanup(GAtServer *server)
 {
-	server->server_watch = 0;
-
+	/* Cleanup all received data */
 	ring_buffer_free(server->read_buf);
 	server->read_buf = NULL;
 
 	server->channel = NULL;
+}
+
+static void read_watcher_destroy_notify(GAtServer *server)
+{
+	g_at_server_cleanup(server);
+	server->read_watch = 0;
+
+	server->channel = NULL;
 
 	if (server->user_disconnect)
 		server->user_disconnect(server->user_disconnect_data);
+
+	if (server->destroyed)
+		g_free(server);
 }
 
 static void v250_settings_create(struct v250_settings *v250)
@@ -480,10 +491,10 @@ GAtServer *g_at_server_new(GIOChannel *io)
 	if (!g_at_util_setup_io(server->channel, G_IO_FLAG_NONBLOCK))
 		goto error;
 
-	server->server_watch = g_io_add_watch_full(io, G_PRIORITY_DEFAULT,
+	server->read_watch = g_io_add_watch_full(io, G_PRIORITY_DEFAULT,
 				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
 				received_data, server,
-				(GDestroyNotify)server_watcher_destroy_notify);
+				(GDestroyNotify)read_watcher_destroy_notify);
 
 	return server;
 
@@ -520,6 +531,16 @@ void g_at_server_unref(GAtServer *server)
 		return;
 
 	g_at_server_shutdown(server);
+
+	/* glib delays the destruction of the watcher until it exits, this
+	 * means we can't free the data just yet, even though we've been
+	 * destroyed already.  We have to wait until the read_watcher
+	 * destroy function gets called
+	 */
+	if (server->read_watch != 0)
+		server->destroyed = TRUE;
+	else
+		g_free(server);
 }
 
 gboolean g_at_server_shutdown(GAtServer *server)
@@ -531,13 +552,8 @@ gboolean g_at_server_shutdown(GAtServer *server)
 	server->user_disconnect = NULL;
 	server->user_disconnect_data = NULL;
 
-	if (server->server_watch) {
-		g_source_remove(server->server_watch);
-		server->server_watch = 0;
-	}
-
-	g_free(server);
-	server = NULL;
+	if (server->read_watch)
+		g_source_remove(server->read_watch);
 
 	return TRUE;
 }
-- 
1.6.6.1

Re: [PATCH 1/4] Rename buf to read_buf in GAtServer

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 147 bytes --]

Hi Zhenhua,

> Because we may introduce write_buf for sever response buffer.
> ---

This patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/4] Replace sprintf with snprintf

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 177 bytes --]

Hi Zhenhua,

> ---
>  gatchat/gatserver.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 

This patch has been applied.  Thanks.

Regards,
-Denis

Re: [PATCH 3/4] Rename server_io to channel

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 114 bytes --]

Hi Zhenhua,

> To make it consistent with GAtChat.

This patch has been applied, thanks.

Regards,
-Denis