[PATCH 0/3] HDLC fixes

[PATCH 0/3] HDLC fixes

[Kristen Carlson Accardi]

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

Fix some issues with the HDLC code, and then use it in PPP.
The first 2 patches are a resend of previous patches, the last
is new to this series.

Kristen Carlson Accardi (3):
  hdlc: allow for scanning and escaping
  hdlc: handle wrapped buffers
  ppp: use GAtHDLC

 gatchat/gathdlc.c  |  121 +++++++++++++-----
 gatchat/gathdlc.h  |   12 ++
 gatchat/gatppp.c   |  347 +++++++++-------------------------------------------
 gatchat/ppp.h      |   11 ++
 gatchat/ppp_auth.c |    2 +-
 gatchat/ppp_cp.c   |    6 +-
 gatchat/ppp_net.c  |    3 +-
 7 files changed, 175 insertions(+), 327 deletions(-)

[PATCH 1/3] hdlc: allow for scanning and escaping

[Kristen Carlson Accardi]

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

PPP needs to inspect the packet protocol to see if a character
should be escaped.  Additionally, it needs to be able to compare
against recv and xmit accm.
---
 gatchat/gathdlc.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++----
 gatchat/gathdlc.h |   12 ++++++++++
 2 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
index 19df9c6..c5c02cf 100644
--- a/gatchat/gathdlc.c
+++ b/gatchat/gathdlc.c
@@ -47,6 +47,12 @@ struct _GAtHDLC {
 	gpointer receive_data;
 	GAtDebugFunc debugf;
 	gpointer debug_data;
+	GAtHDLCScanFunc scan_func;
+	gpointer scan_data;
+	GAtHDLCSendEscapeFunc send_escape_func;
+	gpointer send_escape_data;
+	GAtHDLCRecvEscapeFunc recv_escape_func;
+	gpointer recv_escape_data;
 };
 
 static void new_bytes(GAtHDLC *hdlc)
@@ -71,6 +77,13 @@ static void new_bytes(GAtHDLC *hdlc)
 			continue;
 		}
 
+		if (hdlc->recv_escape_func &&
+				hdlc->recv_escape_func(hdlc->recv_escape_data,
+				buf[pos])) {
+			pos++;
+			continue;
+		}
+
 		if (buf[pos] == 0x7d) {
 			if (pos + 2 > len)
 				break;
@@ -249,6 +262,35 @@ void g_at_hdlc_set_receive(GAtHDLC *hdlc, GAtReceiveFunc func,
 	hdlc->receive_data = user_data;
 }
 
+void g_at_hdlc_set_scan(GAtHDLC *hdlc, GAtHDLCScanFunc func, gpointer user_data)
+{
+	if (!hdlc)
+		return;
+
+	hdlc->scan_func = func;
+	hdlc->scan_data = user_data;
+}
+
+void g_at_hdlc_set_send_escape(GAtHDLC *hdlc, GAtHDLCSendEscapeFunc func,
+						gpointer user_data)
+{
+	if (!hdlc)
+		return;
+
+	hdlc->send_escape_func = func;
+	hdlc->send_escape_data = user_data;
+}
+
+void g_at_hdlc_set_recv_escape(GAtHDLC *hdlc, GAtHDLCRecvEscapeFunc func,
+						gpointer user_data)
+{
+	if (!hdlc)
+		return;
+
+	hdlc->recv_escape_func = func;
+	hdlc->recv_escape_data = user_data;
+}
+
 static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
@@ -302,11 +344,15 @@ static void wakeup_write(GAtHDLC *hdlc)
 				can_write_data, hdlc, write_watch_destroy);
 }
 
-static inline void hdlc_put(GAtHDLC *hdlc, guint8 *buf, gsize *pos, guint8 c)
+static inline void hdlc_put(GAtHDLC *hdlc, guint8 *buf, gsize *pos, guint8 c,
+				gboolean scan_result)
 {
 	gsize i = *pos;
 
-	if (c == 0x7e || c == 0x7d) {
+	if ((hdlc->send_escape_func &&
+				hdlc->send_escape_func(hdlc->send_escape_data,
+							c, scan_result))
+			|| c == 0x7e || c == 0x7d) {
 		buf[i++] = 0x7d;
 		buf[i++] = c ^ 0x20;
 	} else
@@ -321,6 +367,10 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 	unsigned int space, i = 0;
 	guint16 fcs = 0xffff;
 	gsize pos;
+	gboolean scan_result = FALSE;
+
+	if (hdlc->scan_func)
+		scan_result = hdlc->scan_func(hdlc->scan_data, data);
 
 	do {
 		space = ring_buffer_avail_no_wrap(hdlc->write_buffer);
@@ -332,12 +382,12 @@ gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 
 		while (size--) {
 			fcs = crc_ccitt_byte(fcs, data[i]);
-			hdlc_put(hdlc, buf, &pos, data[i++]);
+			hdlc_put(hdlc, buf, &pos, data[i++], scan_result);
 		}
 
 		fcs ^= 0xffff;
-		hdlc_put(hdlc, buf, &pos, fcs & 0xff);
-		hdlc_put(hdlc, buf, &pos, fcs >> 8);
+		hdlc_put(hdlc, buf, &pos, fcs & 0xff, scan_result);
+		hdlc_put(hdlc, buf, &pos, fcs >> 8, scan_result);
 
 		buf[pos++] = 0x7e;
 
diff --git a/gatchat/gathdlc.h b/gatchat/gathdlc.h
index a295f08..f1adeb7 100644
--- a/gatchat/gathdlc.h
+++ b/gatchat/gathdlc.h
@@ -32,6 +32,12 @@ struct _GAtHDLC;
 
 typedef struct _GAtHDLC GAtHDLC;
 
+typedef gboolean (*GAtHDLCSendEscapeFunc)(gpointer user_data, guint8 c,
+						gboolean scan_result);
+typedef gboolean (*GAtHDLCRecvEscapeFunc)(gpointer user_data, guint8 c);
+
+typedef gboolean (*GAtHDLCScanFunc)(gpointer user_data,
+						const unsigned char *data);
 GAtHDLC *g_at_hdlc_new(GIOChannel *channel);
 
 GAtHDLC *g_at_hdlc_ref(GAtHDLC *hdlc);
@@ -43,6 +49,12 @@ void g_at_hdlc_set_receive(GAtHDLC *hdlc, GAtReceiveFunc func,
 							gpointer user_data);
 gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size);
 
+void g_at_hdlc_set_scan(GAtHDLC *hdlc, GAtHDLCScanFunc func,
+							gpointer user_data);
+void g_at_hdlc_set_send_escape(GAtHDLC *hdlc, GAtHDLCSendEscapeFunc func,
+						gpointer user_data);
+void g_at_hdlc_set_recv_escape(GAtHDLC *hdlc, GAtHDLCRecvEscapeFunc func,
+						gpointer user_data);
 #ifdef __cplusplus
 }
 #endif
-- 
1.6.6.1

[PATCH 2/3] hdlc: handle wrapped buffers

[Kristen Carlson Accardi]

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

---
 gatchat/gathdlc.c |   75 ++++++++++++++++++++++++++++------------------------
 1 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
index c5c02cf..13e15a1 100644
--- a/gatchat/gathdlc.c
+++ b/gatchat/gathdlc.c
@@ -59,11 +59,12 @@ static void new_bytes(GAtHDLC *hdlc)
 {
 	unsigned int len = ring_buffer_len(hdlc->read_buffer);
 	unsigned char *buf = ring_buffer_read_ptr(hdlc->read_buffer, 0);
+	unsigned int wrap = ring_buffer_len_no_wrap(hdlc->read_buffer);
 	unsigned char val;
 	unsigned int pos = 0;
 
 	while (pos < len) {
-		if (buf[pos] == 0x7e) {
+		if (*buf == 0x7e) {
 			if (hdlc->receive_func && hdlc->decode_offset > 2 &&
 						hdlc->decode_fcs == 0xf0b8) {
 				hdlc->receive_func(hdlc->decode_buffer,
@@ -74,29 +75,44 @@ static void new_bytes(GAtHDLC *hdlc)
 			hdlc->decode_fcs = 0xffff;
 			hdlc->decode_offset = 0;
 			pos++;
+			buf++;
+			if (pos == wrap)
+				buf = ring_buffer_read_ptr(hdlc->read_buffer,
+								pos);
 			continue;
 		}
 
 		if (hdlc->recv_escape_func &&
 				hdlc->recv_escape_func(hdlc->recv_escape_data,
-				buf[pos])) {
+				*buf)) {
 			pos++;
+			buf++;
+			if (pos == wrap)
+				buf = ring_buffer_read_ptr(hdlc->read_buffer,
+								pos);
 			continue;
 		}
 
-		if (buf[pos] == 0x7d) {
+		if (*buf == 0x7d) {
 			if (pos + 2 > len)
 				break;
 			pos++;
-			val = buf[pos] ^ 0x20;
+			buf++;
+			if (pos == wrap)
+				buf = ring_buffer_read_ptr(hdlc->read_buffer,
+								pos);
+			val = *buf ^ 0x20;
 		} else
-			val = buf[pos];
+			val = *buf;
 
 		hdlc->decode_buffer[hdlc->decode_offset] = val;
 		hdlc->decode_fcs = crc_ccitt_byte(hdlc->decode_fcs, val);
 
 		hdlc->decode_offset++;
 		pos++;
+		buf++;
+		if (pos == wrap)
+			buf = ring_buffer_read_ptr(hdlc->read_buffer, pos);
 	}
 
 	ring_buffer_drain(hdlc->read_buffer, pos);
@@ -344,55 +360,44 @@ static void wakeup_write(GAtHDLC *hdlc)
 				can_write_data, hdlc, write_watch_destroy);
 }
 
-static inline void hdlc_put(GAtHDLC *hdlc, guint8 *buf, gsize *pos, guint8 c,
-				gboolean scan_result)
+static inline void hdlc_buffer_put_char(struct ring_buffer *buffer,
+					const char val)
 {
-	gsize i = *pos;
+	ring_buffer_write(buffer, &val, 1);
+}
 
+static inline void hdlc_put(GAtHDLC *hdlc, guint8 c, gboolean scan_result)
+{
 	if ((hdlc->send_escape_func &&
 				hdlc->send_escape_func(hdlc->send_escape_data,
 							c, scan_result))
 			|| c == 0x7e || c == 0x7d) {
-		buf[i++] = 0x7d;
-		buf[i++] = c ^ 0x20;
+		hdlc_buffer_put_char(hdlc->write_buffer, 0x7d);
+		hdlc_buffer_put_char(hdlc->write_buffer, c ^ 0x20);
 	} else
-		buf[i++] = c;
-
-	*pos = i;
+		hdlc_buffer_put_char(hdlc->write_buffer, c);
 }
 
 gboolean g_at_hdlc_send(GAtHDLC *hdlc, const unsigned char *data, gsize size)
 {
-	unsigned char *buf;
-	unsigned int space, i = 0;
 	guint16 fcs = 0xffff;
-	gsize pos;
 	gboolean scan_result = FALSE;
+	unsigned int i = 0;
 
 	if (hdlc->scan_func)
 		scan_result = hdlc->scan_func(hdlc->scan_data, data);
 
-	do {
-		space = ring_buffer_avail_no_wrap(hdlc->write_buffer);
-		if (space == 0)
-			break;
+	hdlc_buffer_put_char(hdlc->write_buffer, 0x7e);
 
-		buf = ring_buffer_write_ptr(hdlc->write_buffer);
-		pos = 0;
-
-		while (size--) {
-			fcs = crc_ccitt_byte(fcs, data[i]);
-			hdlc_put(hdlc, buf, &pos, data[i++], scan_result);
-		}
-
-		fcs ^= 0xffff;
-		hdlc_put(hdlc, buf, &pos, fcs & 0xff, scan_result);
-		hdlc_put(hdlc, buf, &pos, fcs >> 8, scan_result);
-
-		buf[pos++] = 0x7e;
+	while (size--) {
+		fcs = crc_ccitt_byte(fcs, data[i]);
+		hdlc_put(hdlc, data[i++], scan_result);
+	}
 
-		ring_buffer_write_advance(hdlc->write_buffer, pos);
-	} while (0);
+	fcs ^= 0xffff;
+	hdlc_put(hdlc, fcs & 0xff, scan_result);
+	hdlc_put(hdlc, fcs >> 8, scan_result);
+	hdlc_buffer_put_char(hdlc->write_buffer, 0x7e);
 
 	wakeup_write(hdlc);
 
-- 
1.6.6.1

[PATCH 3/3] ppp: use GAtHDLC

[Kristen Carlson Accardi]

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

---
 gatchat/gatppp.c   |  347 +++++++++-------------------------------------------
 gatchat/ppp.h      |   11 ++
 gatchat/ppp_auth.c |    2 +-
 gatchat/ppp_cp.c   |    6 +-
 gatchat/ppp_net.c  |    3 +-
 5 files changed, 75 insertions(+), 294 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index e1e49e6..5b6ba4e 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -34,6 +34,7 @@
 
 #include <glib.h>
 
+#include "gathdlc.h"
 #include "gatutil.h"
 #include "gatppp.h"
 #include "crc-ccitt.h"
@@ -41,10 +42,6 @@
 
 #define DEFAULT_MRU	1500
 
-#define BUFFERSZ	(DEFAULT_MRU * 2)
-
-#define PPP_ESC		0x7d
-#define PPP_FLAG_SEQ 	0x7e
 #define PPP_ADDR_FIELD	0xff
 #define PPP_CTRL	0x03
 
@@ -55,24 +52,19 @@ struct _GAtPPP {
 	struct pppcp_data *ipcp;
 	struct ppp_net *net;
 	struct ppp_chap *chap;
-	guint8 buffer[BUFFERSZ];
-	int index;
 	gint mru;
 	char username[256];
 	char password[256];
 	guint32 xmit_accm[8];
 	guint32 recv_accm;
-	GIOChannel *modem;
 	GAtPPPConnectFunc connect_cb;
 	gpointer connect_data;
 	GAtDisconnectFunc disconnect_cb;
 	gpointer disconnect_data;
-	gint read_watch;
-	gint write_watch;
 	GAtDebugFunc debugf;
 	gpointer debug_data;
 	int record_fd;
-	GQueue *xmit_queue;
+	GAtHDLC *hdlc;
 };
 
 void ppp_debug(GAtPPP *ppp, const char *str)
@@ -91,81 +83,59 @@ struct frame_buffer {
 	guint8 bytes[0];
 };
 
+static void ppp_record(GAtPPP *ppp, gboolean in, guint8 *data, guint16 length)
+{
+	guint16 len = htons(length);
+	guint32 ts;
+	struct timeval now;
+	unsigned char id;
+	int err;
+
+	if (ppp->record_fd < 0)
+		return;
+
+	gettimeofday(&now, NULL);
+	ts = htonl(now.tv_sec & 0xffffffff);
+
+	id = 0x07;
+	err = write(ppp->record_fd, &id, 1);
+	err = write(ppp->record_fd, &ts, 4);
+
+	id = in ? 0x02 : 0x01;
+	err = write(ppp->record_fd, &id, 1);
+	err = write(ppp->record_fd, &len, 2);
+	err = write(ppp->record_fd, data, length);
+}
+
 /*
  * escape any chars less than 0x20, and check the transmit accm table to
  * see if this character should be escaped.
  */
-static gboolean ppp_escape(GAtPPP *ppp, guint8 c, gboolean lcp)
+static gboolean ppp_escape(gpointer user_data, guint8 c, gboolean lcp)
 {
+	GAtPPP *ppp = user_data;
+
 	if ((lcp && c < 0x20) || (ppp->xmit_accm[c >> 5] & (1 << (c & 0x1f))))
 		return TRUE;
 	return FALSE;
 }
 
-static void ppp_put(GAtPPP *ppp, guint8 *buf, int *pos,
-			guint8 c, gboolean lcp)
+static gboolean ppp_hdlc_scan(gpointer user_data, const unsigned char *data)
 {
-	int i = *pos;
-
-	/* escape characters if needed,  copy into buf, increment pos */
-	if (ppp_escape(ppp, c, lcp)) {
-		buf[i++] = PPP_ESC;
-		buf[i++] = c ^ 0x20;
-	} else
-		buf[i++] = c;
-	*pos = i;
-}
+	guint16 proto = ppp_proto(data);
 
-static struct frame_buffer *ppp_encode(GAtPPP *ppp, guint8 *data, int len)
-{
-	int pos = 0;
-	int i = 0;
-	guint16 fcs = PPPINITFCS16;
-	guint16 proto = get_host_short(data);
-	gboolean lcp = (proto == LCP_PROTOCOL);
-	guint8 *frame;
-	struct frame_buffer *fb =
-		g_try_malloc0(BUFFERSZ + sizeof(struct frame_buffer));
-
-	if (!fb)
-		return NULL;
-	frame = fb->bytes;
-
-	/* copy in the HDLC framing */
-	frame[pos++] = PPP_FLAG_SEQ;
-
-	/* from here till end flag, calculate FCS over each character */
-	fcs = crc_ccitt_byte(fcs, PPP_ADDR_FIELD);
-	ppp_put(ppp, frame, &pos, PPP_ADDR_FIELD, lcp);
-	fcs = crc_ccitt_byte(fcs, PPP_CTRL);
-	ppp_put(ppp, frame, &pos, PPP_CTRL, lcp);
-
-	/*
-	 * for each byte, first calculate FCS, then do escaping if
-	 * neccessary
-	 */
-	while (len--) {
-		fcs = crc_ccitt_byte(fcs, data[i]);
-		ppp_put(ppp, frame, &pos, data[i++], lcp);
-	}
-
-	/* add FCS */
-	fcs ^= 0xffff;		/* complement */
-	ppp_put(ppp, frame, &pos, (guint8)(fcs & 0x00ff), lcp);
-	ppp_put(ppp, frame, &pos, (guint8)((fcs >> 8) & 0x00ff), lcp);
-
-	/* add flag */
-	frame[pos++] = PPP_FLAG_SEQ;
-
-	fb->len = pos;
-	return fb;
+	if (proto == LCP_PROTOCOL)
+		return TRUE;
+	return FALSE;
 }
 
-/* called when we have received a complete ppp frame */
-static void ppp_recv(GAtPPP *ppp, struct frame_buffer *frame)
+static void ppp_hdlc_recv(const unsigned char *data, gsize size, gpointer user_data)
 {
-	guint16 protocol = ppp_proto(frame->bytes);
-	guint8 *packet = ppp_info(frame->bytes);
+	GAtPPP *ppp = user_data;
+	guint16 protocol = ppp_proto(data);
+	guint8 *packet = (guint8 *) ppp_info(data);
+
+	ppp_record(ppp, TRUE, (guint8 *) data, size);
 
 	switch (protocol) {
 	case PPP_IP_PROTO:
@@ -186,144 +156,18 @@ static void ppp_recv(GAtPPP *ppp, struct frame_buffer *frame)
 		}
 		/* fall through */
 	default:
-		pppcp_send_protocol_reject(ppp->lcp, frame->bytes, frame->len);
+		pppcp_send_protocol_reject(ppp->lcp, (guint8 *) data, size);
 		break;
 	};
 }
 
-static struct frame_buffer *ppp_decode(GAtPPP *ppp, guint8 *frame)
+static gboolean ppp_recv_escape(gpointer user_data, guint8 c)
 {
-	guint8 *data;
-	guint pos;
-	int i;
-	guint16 fcs;
-	struct frame_buffer *fb;
-
-	fb = g_try_malloc0(sizeof(struct frame_buffer) + ppp->mru + 10);
-	if (!fb)
-		return NULL;
-	data = fb->bytes;
-
-	/* skip the first flag char */
-	pos = 1;
-
-	fcs = PPPINITFCS16;
-	i = 0;
-
-	while (frame[pos] != PPP_FLAG_SEQ) {
-		/* Skip the characters in receive ACCM */
-		if (frame[pos] < 0x20 &&
-				(ppp->recv_accm & (1 << frame[pos])) != 0) {
-			pos++;
-			continue;
-		}
-
-		/* scan for escape character */
-		if (frame[pos] == PPP_ESC) {
-			/* skip that char */
-			pos++;
-			data[i] = frame[pos] ^ 0x20;
-		} else
-			data[i] = frame[pos];
-
-		fcs = crc_ccitt_byte(fcs, data[i]);
-
-		i++; pos++;
-	}
+	GAtPPP *ppp = user_data;
 
-	fb->len = i;
-
-	/* see if we have a good FCS */
-	if (fcs != PPPGOODFCS16) {
-		g_free(fb);
-		return NULL;
-	}
-
-	return fb;
-}
-
-static void ppp_feed(GAtPPP *ppp, guint8 *data, gsize len)
-{
-	guint pos = 0;
-	struct frame_buffer *frame;
-
-	/* collect bytes until we detect we have received a complete frame */
-	/* examine the data.  If we are at the beginning of a new frame,
-	 * allocate memory to buffer the frame.
-	 */
-
-	for (pos = 0; pos < len; pos++) {
-		if (data[pos] == PPP_FLAG_SEQ) {
-			if (ppp->index != 0) {
-				/* store last flag character & decode */
-				ppp->buffer[ppp->index++] = data[pos];
-				frame = ppp_decode(ppp, ppp->buffer);
-				if (frame) {
-					/* process receive frame */
-					ppp_recv(ppp, frame);
-					g_free(frame);
-				}
-
-				/* zero buffer */
-				memset(ppp->buffer, 0, BUFFERSZ);
-				ppp->index = 0;
-				continue;
-			}
-		}
-		/* copy byte to buffer */
-		if (ppp->index < BUFFERSZ)
-			ppp->buffer[ppp->index++] = data[pos];
-	}
-}
-
-static void ppp_record(GAtPPP *ppp, gboolean in, guint8 *data, guint16 length)
-{
-	guint16 len = htons(length);
-	guint32 ts;
-	struct timeval now;
-	unsigned char id;
-	int err;
-
-	if (ppp->record_fd < 0)
-		return;
-
-	gettimeofday(&now, NULL);
-	ts = htonl(now.tv_sec & 0xffffffff);
-
-	id = 0x07;
-	err = write(ppp->record_fd, &id, 1);
-	err = write(ppp->record_fd, &ts, 4);
-
-	id = in ? 0x02 : 0x01;
-	err = write(ppp->record_fd, &id, 1);
-	err = write(ppp->record_fd, &len, 2);
-	err = write(ppp->record_fd, data, length);
-}
-
-static gboolean ppp_read_cb(GIOChannel *channel, GIOCondition cond,
-								gpointer data)
-{
-	GAtPPP *ppp = data;
-	GIOStatus status;
-	gchar buf[256];
-	gsize bytes_read;
-	GError *error = NULL;
-
-	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP))
-		return FALSE;
-
-	if (cond & G_IO_IN) {
-		status = g_io_channel_read_chars(channel, buf, 256,
-				&bytes_read, &error);
-		if (bytes_read > 0) {
-			ppp_record(ppp, TRUE, (guint8 *) buf, bytes_read);
-			ppp_feed(ppp, (guint8 *) buf, bytes_read);
-		}
-		if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN)
-			return FALSE;
-	}
-
-	return TRUE;
+	if (c < 0x20 && (ppp->recv_accm & (1 << c)) != 0)
+		return TRUE;
+	return FALSE;
 }
 
 static void ppp_dead(GAtPPP *ppp)
@@ -371,12 +215,6 @@ void ppp_enter_phase(GAtPPP *ppp, enum ppp_phase phase)
 	}
 }
 
-static void read_watcher_destroy_notify(GAtPPP *ppp)
-{
-	ppp->read_watch = 0;
-	pppcp_signal_down(ppp->lcp);
-}
-
 void ppp_set_auth(GAtPPP *ppp, const guint8* auth_data)
 {
 	guint16 proto = get_host_short(auth_data);
@@ -540,13 +378,7 @@ void g_at_ppp_unref(GAtPPP *ppp)
 	if (ppp->record_fd > fileno(stderr))
 		close(ppp->record_fd);
 
-	/* cleanup queue */
-	g_queue_free(ppp->xmit_queue);
-
-	/* cleanup modem channel */
-	g_source_remove(ppp->read_watch);
-	g_source_remove(ppp->write_watch);
-	g_io_channel_unref(ppp->modem);
+	g_at_hdlc_unref(ppp->hdlc);
 
 	lcp_free(ppp->lcp);
 	ppp_chap_free(ppp->chap);
@@ -566,11 +398,15 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem)
 	if (!ppp)
 		return NULL;
 
-	if (!g_at_util_setup_io(modem, G_IO_FLAG_NONBLOCK)) {
+	ppp->hdlc = g_at_hdlc_new(modem);
+	if (ppp->hdlc == NULL) {
 		g_free(ppp);
 		return NULL;
 	}
-	g_io_channel_set_buffered(modem, FALSE);
+	g_at_hdlc_set_receive(ppp->hdlc, ppp_hdlc_recv, ppp);
+	g_at_hdlc_set_recv_escape(ppp->hdlc, ppp_recv_escape, ppp);
+	g_at_hdlc_set_send_escape(ppp->hdlc, ppp_escape, ppp);
+	g_at_hdlc_set_scan(ppp->hdlc, ppp_hdlc_scan, ppp);
 
 	ppp->ref_count = 1;
 
@@ -580,70 +416,16 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem)
 	ppp->xmit_accm[0] = ~0U;
 	ppp->xmit_accm[3] = 0x60000000; /* 0x7d, 0x7e */
 
-	ppp->index = 0;
-
-	/* intialize the queue */
-	ppp->xmit_queue = g_queue_new();
-
 	/* initialize the lcp state */
 	ppp->lcp = lcp_new(ppp);
 
 	/* initialize IPCP state */
 	ppp->ipcp = ipcp_new(ppp);
-
-	/* start listening for packets from the modem */
-	ppp->read_watch = g_io_add_watch_full(modem, G_PRIORITY_DEFAULT,
-				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-				ppp_read_cb, ppp,
-				(GDestroyNotify)read_watcher_destroy_notify);
-
-	ppp->modem = modem;
 	ppp->record_fd = -1;
 
 	return ppp;
 }
 
-static gboolean ppp_xmit_cb(GIOChannel *channel, GIOCondition cond,
-				gpointer data)
-{
-	GAtPPP *ppp = data;
-	struct frame_buffer *fb;
-	GError *error = NULL;
-	GIOStatus status;
-	gsize bytes_written;
-
-	if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
-		return FALSE;
-
-	if (cond & G_IO_OUT) {
-		while ((fb = g_queue_peek_head(ppp->xmit_queue))) {
-			status = g_io_channel_write_chars(ppp->modem,
-					(gchar *) fb->bytes, fb->len,
-					&bytes_written, &error);
-			if (status != G_IO_STATUS_NORMAL &&
-				status != G_IO_STATUS_AGAIN)
-				return FALSE;
-
-			if (bytes_written < fb->len)
-				return TRUE;
-
-			ppp_record(ppp, FALSE, fb->bytes, bytes_written);
-			g_free(g_queue_pop_head(ppp->xmit_queue));
-		}
-	}
-	return FALSE;
-}
-
-static void ppp_xmit_destroy_notify(gpointer destroy_data)
-{
-	GAtPPP *ppp = destroy_data;
-
-	ppp->write_watch = 0;
-
-	if (ppp->phase == PPP_PHASE_DEAD)
-		ppp_dead(ppp);
-}
-
 /*
  * transmit out through the lower layer interface
  *
@@ -651,22 +433,11 @@ static void ppp_xmit_destroy_notify(gpointer destroy_data)
  */
 void ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen)
 {
-	struct frame_buffer *fb;
-
-	/*
-	 * do the octet stuffing.  Add 2 bytes to the infolen to
-	 * include the protocol field.
-	 */
-	fb = ppp_encode(ppp, packet, infolen + 2);
-	if (!fb) {
-		g_printerr("Failed to encode packet to transmit\n");
-		return;
-	}
-	/* push decoded frame onto xmit queue */
-	g_queue_push_tail(ppp->xmit_queue, fb);
+	struct ppp_header *header = (struct ppp_header *) packet;
+	gsize len = infolen + sizeof(struct ppp_header);
 
-	/* transmit this whenever we can write without blocking */
-	ppp->write_watch = g_io_add_watch_full(ppp->modem, G_PRIORITY_DEFAULT,
-				G_IO_OUT | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-				ppp_xmit_cb, ppp, ppp_xmit_destroy_notify);
+	header->address = PPP_ADDR_FIELD;
+	header->control = PPP_CTRL;
+	g_at_hdlc_send(ppp->hdlc, packet, len);
+	ppp_record(ppp, FALSE, packet, len);
 }
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index a8a0486..c41bb05 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -39,6 +39,8 @@ struct ppp_chap;
 struct ppp_net;
 
 struct ppp_header {
+	guint8 address;
+	guint8 control;
 	guint16 proto;
 	guint8 info[0];
 } __attribute__((packed));
@@ -84,6 +86,15 @@ static inline void __put_unaligned_short(void *p, guint16 val)
 #define ppp_proto(packet) \
 	(get_host_short(packet + 2))
 
+#define ppp_hdlc_proto(packet) \
+	(get_host_short(packet + 4))
+
+#define ppp_hdlc_info(packet) \
+	(packet + 6)
+
+#define ppp_hdlc_packet(packet) \
+	((guint8 *) packet + 2)
+
 /* LCP related functions */
 struct pppcp_data *lcp_new(GAtPPP *ppp);
 void lcp_free(struct pppcp_data *lcp);
diff --git a/gatchat/ppp_auth.c b/gatchat/ppp_auth.c
index 57203ab..b8c0628 100644
--- a/gatchat/ppp_auth.c
+++ b/gatchat/ppp_auth.c
@@ -83,7 +83,7 @@ static void chap_process_challenge(struct ppp_chap *chap, guint8 *packet)
 	 */
 	digest_len = g_checksum_type_get_length(chap->method);
 	response_length = digest_len + sizeof(*header) + 1;
-	ppp_packet = g_try_malloc0(response_length + 2);
+	ppp_packet = g_try_malloc0(response_length + sizeof(struct ppp_header));
 	if (!ppp_packet)
 		goto challenge_out;
 
diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index 6cd3681..ce8c94a 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -61,10 +61,8 @@ static const char *pppcp_event_strings[] = {
 	g_free(str); \
 } while (0);
 
-#define PPP_HEADROOM	2
-
 #define pppcp_to_ppp_packet(p) \
-	(((guint8 *) p) - PPP_HEADROOM)
+	(((guint8 *) p) - sizeof(struct ppp_header))
 
 #define INITIAL_RESTART_TIMEOUT	3	/* restart interval in seconds */
 #define MAX_TERMINATE		2
@@ -206,7 +204,7 @@ static struct pppcp_packet *pppcp_packet_new(struct pppcp_data *data,
 	struct ppp_header *ppp_packet;
 	guint16 packet_length = bufferlen + sizeof(*packet);
 
-	ppp_packet = g_try_malloc0(packet_length + 2);
+	ppp_packet = g_try_malloc0(packet_length + sizeof(struct ppp_header));
 	if (!ppp_packet)
 		return NULL;
 
diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
index 325e859..9fbd1d7 100644
--- a/gatchat/ppp_net.c
+++ b/gatchat/ppp_net.c
@@ -80,7 +80,8 @@ static gboolean ppp_net_callback(GIOChannel *channel, GIOCondition cond,
 
 	if (cond & G_IO_IN) {
 		/* leave space to add PPP protocol field */
-		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
+		status = g_io_channel_read_chars(channel,
+				buf + sizeof(struct ppp_header), MAX_PACKET,
 				&bytes_read, &error);
 		if (bytes_read > 0) {
 			ppp->proto = htons(PPP_IP_PROTO);
-- 
1.6.6.1

Re: [PATCH 1/3] hdlc: allow for scanning and escaping

[Marcel Holtmann]

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

Hi Kristen,

> PPP needs to inspect the packet protocol to see if a character
> should be escaped.  Additionally, it needs to be able to compare
> against recv and xmit accm.

can we just set the ACCM values instead of having an extra function
call. I think that calling an extra function for every single byte is a
really bad idea and the compiler can not optimize it at all.

Regards

Marcel

Re: [PATCH 3/3] ppp: use GAtHDLC

[Marcel Holtmann]

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

Hi Kristen,

> +static void ppp_record(GAtPPP *ppp, gboolean in, guint8 *data, guint16 length)
> +{
> +	guint16 len = htons(length);
> +	guint32 ts;
> +	struct timeval now;
> +	unsigned char id;
> +	int err;
> +
> +	if (ppp->record_fd < 0)
> +		return;
> +
> +	gettimeofday(&now, NULL);
> +	ts = htonl(now.tv_sec & 0xffffffff);
> +
> +	id = 0x07;
> +	err = write(ppp->record_fd, &id, 1);
> +	err = write(ppp->record_fd, &ts, 4);
> +
> +	id = in ? 0x02 : 0x01;
> +	err = write(ppp->record_fd, &id, 1);
> +	err = write(ppp->record_fd, &len, 2);
> +	err = write(ppp->record_fd, data, length);
> +}
> +

there is one change that has to happen first here. We need to add
g_at_hdlc_set_recording. And this needs to take a file name and an
actual format parameter. With pppdump being the initial format.

Then the g_at_ppp_set_recoding can just call the HDLC recording function
and set this. I do wanna have this in a central place. And at the same
time allow recording of other HDLC based data formats that Wireshark
eventually can support.

Regards

Marcel

Re: [PATCH 2/3] hdlc: handle wrapped buffers

[Marcel Holtmann]

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

Hi Kristen,

> ---
>  gatchat/gathdlc.c |   75 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c
> index c5c02cf..13e15a1 100644
> --- a/gatchat/gathdlc.c
> +++ b/gatchat/gathdlc.c
> @@ -59,11 +59,12 @@ static void new_bytes(GAtHDLC *hdlc)
>  {
>  	unsigned int len = ring_buffer_len(hdlc->read_buffer);
>  	unsigned char *buf = ring_buffer_read_ptr(hdlc->read_buffer, 0);
> +	unsigned int wrap = ring_buffer_len_no_wrap(hdlc->read_buffer);
>  	unsigned char val;
>  	unsigned int pos = 0;
>  
>  	while (pos < len) {
> -		if (buf[pos] == 0x7e) {
> +		if (*buf == 0x7e) {
>  			if (hdlc->receive_func && hdlc->decode_offset > 2 &&
>  						hdlc->decode_fcs == 0xf0b8) {
>  				hdlc->receive_func(hdlc->decode_buffer,

please do this patch fist and send it separately. Intermixing it with
the ACCM support makes it hard to review.

So I like to see the patch series this way:

1) Handle ringbuffer wrapping in HDLC
2) Add recording support for HDLC
3) Add ACCM support to HDLC
4) Port PPP to use HDLC

Regards

Marcel

Re: [PATCH 2/3] hdlc: handle wrapped buffers

[Marcel Holtmann]

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

Hi Kristen,

> So I like to see the patch series this way:
> 
> 1) Handle ringbuffer wrapping in HDLC
> 2) Add recording support for HDLC
> 3) Add ACCM support to HDLC
> 4) Port PPP to use HDLC

so I have done 1) now in a way that it is efficient and doesn't become
the bottle-neck for PPP. What is the status of the other 3 tasks?

Regards

Marcel