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
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
