Re: [PATCH 2/4] Add GAtServer basic parsing support

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Zhenhua,

> +
> +#include <glib.h>
> +
> +#include "ringbuffer.h"
> +#include "gatresult.h"

Is this include really necessary?

> +#include "gatserver.h"
> +
> +#define DBG(fmt, arg...) g_print("%s:%s() " fmt, __FILE__, __FUNCTION__ ,
>  ## arg) +

Move this to gat.h

> +struct _GAtServer {
> +	gint ref_count;				/* Ref count */
> +	struct v250_settings *v250;		/* V.250 command setting */

This one doesn't need to be a pointer.

> +	GIOChannel *server_io;			/* Server IO */
> +	int server_watch;			/* Watch for server IO */
> +	char *modem_path;			/* Emulated modem path */

Get rid of this, there's no purpose for it.

> +static int at_server_parse(GAtServer *server, char *buf)
> +{
> +	int res = G_AT_SERVER_RESULT_ERROR;
> +	struct v250_settings *v250 = server->v250;
> +	gsize i = 0;
> +	char c;
> +
> +	/* skip space after "AT" or previous command */
> +	i = skip_space(buf, i);
> +
> +	c = buf[i];
> +	/* skip semicolon */
> +	if (c == ';')
> +		c = buf[++i];
> +
> +	if (g_ascii_isalpha(c) || c == '&')
> +		res = parse_v250_settings(server, buf + i);

This part makes no sense

> +	else if (c == v250->s3)
> +		res = G_AT_SERVER_RESULT_OK;
> +
> +	return res;
> +}
> +
> +static void parse_buffer(GAtServer *server, unsigned int len)
> +{
> +	int res = G_AT_SERVER_RESULT_ERROR;
> +	gsize i = 0;
> +	char *buf = NULL;
> +
> +	g_at_server_ref(server);

Lets get rid of this part for now.  It was a necessary evil inside GAtChat 
since the client can close the channel in the command result callback.  I 
don't think this is relevant for GAtServer.  If it ever becomes relevant we 
can add this back in.

> +
> +	buf = g_try_new0(char, len+1);

You're leaking the buf memory in this function.

> +
> +	if (!buf)
> +		goto out;
> +
> +	if (-1 == ring_buffer_read(server->buf, buf, len))
> +		goto out;
> +
> +	buf[len] = '\0';
> +
> +	DBG("received %s\n", buf);
> +
> +	/* skip header space */
> +	buf += skip_space(buf, i);
> +
> +	/* Make sure the command line prefix is "AT" or "at" */
> +	if (g_str_has_prefix(buf, "AT") ||
> +				g_str_has_prefix(buf, "at"))
> +		res = at_server_parse(server, (char *) buf + 2);
> +
> +	g_at_server_send_result_code(server, res);
> +
> +out:
> +	/* We're overflowing the buffer, shutdown the socket */
> +	if (server->buf && ring_buffer_avail(server->buf) == 0)
> +		g_at_server_shutdown(server);
> +
> +	g_at_server_unref(server);

Same here, see above.

> +}
> +
> +static gboolean received_data(GIOChannel *chan, GIOCondition cond,
> +				gpointer data)
> +{
> +	GAtServer *server = data;
> +	struct v250_settings *v250 = server->v250;
> +	GIOError err;
> +	gsize rbytes;
> +	gsize total_read = 0;
> +	unsigned char *total_buf = ring_buffer_write_ptr(server->buf);

You're totally abusing the ring buffer concept here, the fact that this works 
is pure luck.

> +	static gsize total_len;
> +
> +	if (cond & G_IO_NVAL)
> +		return FALSE;
> +
> +	if (cond & (G_IO_HUP | G_IO_ERR)) {
> +		g_at_server_shutdown(server);
> +
> +		return FALSE;
> +	}
> +
> +	/* Regardless of condition, try to read all the data available */
> +	do {
> +		unsigned char *buf;
> +		gsize toread;
> +
> +		rbytes = 0;
> +
> +		toread = ring_buffer_avail_no_wrap(server->buf);
> +
> +		if (toread == 0)
> +			break;
> +
> +		buf = ring_buffer_write_ptr(server->buf);
> +
> +		err = g_io_channel_read(chan, (char *) buf, toread, &rbytes);
> +
> +		total_read += rbytes;
> +
> +		if (rbytes > 0)
> +			ring_buffer_write_advance(server->buf, rbytes);
> +
> +	} while (err == G_IO_ERROR_NONE && rbytes > 0);
> +
> +	g_at_util_debug_chat(server->debugf, TRUE, (char *) total_buf,
> +						total_read, server->debug_data);
> +
> +	if (total_read == 0) {
> +		g_at_server_shutdown(server);

You should be checking rbytes == 0 && err != EAGAIN here.  Your check below is 
also redundant.

> +
> +		return FALSE;
> +	}
> +
> +	total_len += total_read;
> +
> +	/* Add '\0' to perform strchr */
> +	total_buf[total_read] = '\0';
> +
> +	/* Parse buffer until we meet the terminator so that
> +	 * all preceding characters will be processed
> +	 */
> +	if (strchr((char *) total_buf, v250->s3)) {
> +		parse_buffer(server, total_len);
> +
> +		total_len = 0;
> +	}

Should this be inside a while loop? Several commands can come in at once.

> +
> +	if (err != G_IO_ERROR_NONE && err != G_IO_ERROR_AGAIN)
> +		return FALSE;
> +
> +	return TRUE;
> +}
> +

> +GAtServer *g_at_server_new(GIOChannel *io, const char *modem_path)

Get rid of modem_path

> +{
> +	GAtServer *server;
> +
> +	if (!io || !modem_path)
> +		return NULL;
> +
> +	server = g_try_new0(GAtServer, 1);
> +	if (!server)
> +		return NULL;
> +
> +	server->ref_count = 1;
> +	server->v250 = v250_settings_create();
> +	server->server_io = io;
> +	server->modem_path = g_strdup(modem_path);

And here

> +	server->user_disconnect = NULL;
> +	server->user_disconnect_data = NULL;
> +	server->debugf = NULL;
> +	server->debug_data = NULL;
> +	server->buf = ring_buffer_new(4096);
> +
> +	if (!server->v250)
> +		goto error;
> +
> +	if (!server->buf)
> +		goto error;
> +
> +	if (!g_at_util_set_io(server->server_io))
> +		goto error;
> +
> +	server->server_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, NULL);
> +
> +	return server;
> +
> +error:
> +	if (server->buf)
> +		ring_buffer_free(server->buf);
> +
> +	if (server->modem_path)
> +		g_free(server->modem_path);

And here

> +
> +	if (server->v250)
> +		g_free(server->v250);
> +
> +	if (server)
> +		g_free(server);
> +
> +	return NULL;
> +}
> +
> +gboolean g_at_server_shutdown(GAtServer *server)
> +{
> +	if (!server)
> +		return FALSE;
> +
> +	if (server->modem_path)
> +		g_free(server->modem_path);
> +
> +	if (server->v250)
> +		g_free(server->v250);
> +
> +	ring_buffer_free(server->buf);
> +	server->buf = NULL;
> +
> +	if (server->server_watch) {
> +		g_source_remove(server->server_watch);
> +		server->server_watch = 0;
> +	}
> +
> +	if (server->server_io) {
> +		g_io_channel_shutdown(server->server_io, FALSE, NULL);
> +		g_io_channel_unref(server->server_io);
> +		server->server_io = NULL;
> +	}
> +
> +	if (server->user_disconnect)
> +		server->user_disconnect(server->user_disconnect_data);

We should not trigger a user_disconnect if we're shutting down normally.  The 
purpose is to detect when the remote side has disconnected the connection.

Regards,
-Denis