[patch 1/6] Add PPP protocol support with HDLC framing
This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a different patch. Index: ofono/Makefile.am === --- ofono.orig/Makefile.am 2010-03-10 16:58:09.915955860 -0800 +++ ofono/Makefile.am 2010-03-10 16:58:12.039961039 -0800 @@ -55,7 +55,9 @@ gatchat/gattty.h gatchat/gattty.c \ gatchat/gatutil.h gatchat/gatutil.c \ gatchat/gat.h \ - gatchat/gatserver.h gatchat/gatserver.c + gatchat/gatserver.h gatchat/gatserver.c \ + gatchat/gatppp.c gatchat/gatppp.h \ + gatchat/gatppp_internal.h udev_files = plugins/ofono.rules Index: ofono/gatchat/gatppp_internal.h === --- /dev/null 1970-01-01 00:00:00.0 + +++ ofono/gatchat/gatppp_internal.h 2010-03-10 16:58:12.039961039 -0800 @@ -0,0 +1,97 @@ +/* + * + * PPP library with GLib integration + * + * Copyright (C) 2010 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +#ifndef __G_AT_PPP_INTERNAL_H +#define __G_AT_PPP_INTERNAL_H + +#ifdef __cplusplus +extern "C" { +#endif + +#define DEFAULT_MRU1500 +#define BUFFERSZ DEFAULT_MRU*2 +#define DEFAULT_ACCM 0x +#define PPP_ESC0x7d +#define PPP_FLAG_SEQ 0x7e +#define PPP_ADDR_FIELD 0xff +#define PPP_CTRL 0x03 +#define LCP_PROTOCOL 0xc021 +#define CHAP_PROTOCOL 0xc223 +#define PPP_HEADROOM 2 +#define HDLC_HEADROOM 3 +#define HDLC_TAIL 3 +#define MD55 + +enum PPP_PHASE { + DEAD = 0, + ESTABLISHMENT, + AUTHENTICATION, + NETWORK, + TERMINATION, +}; + +enum PPP_EVENTS { + PPP_UP = 1, + PPP_OPENED, + PPP_SUCCESS, + PPP_NONE, + PPP_CLOSING, + PPP_FAIL, + PPP_DOWN +}; + +struct ppp_packet_handler { + guint16 proto; + void (*handler)(gpointer priv, guint8 *packet); + gpointer priv; +}; + +#define ppp_generate_event(l, e) \ + __ppp_generate_event(l, e) + +#define ppp_transmit(l, p, i) \ + __ppp_transmit(l, p, i) + +#define ppp_register_packet_handler(h) \ + __ppp_register_packet_handler(h) + +#define ppp_info(packet) \ + (packet+4) + +#define ppp_proto(packet) \ + (ntohs(*((guint16*)(packet+2 + +void __ppp_generate_event(struct ppp_link *link, guint event); +void __ppp_register_packet_handler(struct ppp_packet_handler *handler); +void __ppp_transmit(struct ppp_link *link, guint8 *packet, guint infolen); +void __ppp_set_auth(struct ppp_link *link, guint8 *auth_data); +void __ppp_set_recv_accm(struct ppp_link *link, guint32 accm); +guint32 __ppp_get_xmit_accm(struct ppp_link *link); +void __ppp_set_pfc(struct ppp_link *link, gboolean pfc); +gboolean __ppp_get_pfc(struct ppp_link *link); +void __ppp_set_acfc(struct ppp_link *link, gboolean acfc); +gboolean __ppp_get_acfc(struct ppp_link *link); +#ifdef __cplusplus +} +#endif + +#endif /* __G_AT_PPP_INTERNAL_H */ + Index: ofono/gatchat/gatppp.c === --- /dev/null 1970-01-01 00:00:00.0 + +++ ofono/gatchat/gatppp.c 2010-03-10 16:58:12.039961039 -0800 @@ -0,0 +1,557 @@ +/* + * + * PPP library with GLib integration + * + * Copyright (C) 2010 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +#ifdef HAVE_CONFIG_H +#include +#endif + +#include +#include +#include +#include +#include +#include + +#include +
[patch 1/6] Add PPP protocol support with HDLC framing
This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a different patch. --- Makefile.am |4 gatchat/gatppp.c | 552 ++ gatchat/gatppp.h | 59 gatchat/gatppp_internal.h | 97 4 files changed, 711 insertions(+), 1 deletion(-) Index: ofono/Makefile.am === --- ofono.orig/Makefile.am 2010-03-16 15:16:53.536470034 -0700 +++ ofono/Makefile.am 2010-03-16 15:33:46.144468807 -0700 @@ -55,7 +55,9 @@ gatchat/gattty.h gatchat/gattty.c \ gatchat/gatutil.h gatchat/gatutil.c \ gatchat/gat.h \ - gatchat/gatserver.h gatchat/gatserver.c + gatchat/gatserver.h gatchat/gatserver.c \ + gatchat/gatppp.c gatchat/gatppp.h \ + gatchat/gatppp_internal.h udev_files = plugins/ofono.rules Index: ofono/gatchat/gatppp_internal.h === --- /dev/null 1970-01-01 00:00:00.0 + +++ ofono/gatchat/gatppp_internal.h 2010-03-16 15:33:46.144468807 -0700 @@ -0,0 +1,97 @@ +/* + * + * PPP library with GLib integration + * + * Copyright (C) 2010 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ +#define DEFAULT_MRU1500 +#define BUFFERSZ DEFAULT_MRU*2 +#define DEFAULT_ACCM 0x +#define PPP_ESC0x7d +#define PPP_FLAG_SEQ 0x7e +#define PPP_ADDR_FIELD 0xff +#define PPP_CTRL 0x03 +#define LCP_PROTOCOL 0xc021 +#define CHAP_PROTOCOL 0xc223 +#define PPP_HEADROOM 2 +#define HDLC_HEADROOM 3 +#define HDLC_TAIL 3 +#define MD55 + +typedef enum _GAthase { + DEAD = 0, + ESTABLISHMENT, + AUTHENTICATION, + NETWORK, + TERMINATION, +} GAthase; + +typedef enum _GAtPPPEvents { + PPP_UP = 1, + PPP_OPENED, + PPP_SUCCESS, + PPP_NONE, + PPP_CLOSING, + PPP_FAIL, + PPP_DOWN +} GAtPPPEvents; + +struct ppp_packet_handler { + guint16 proto; + void (*handler)(gpointer priv, guint8 *packet); + gpointer priv; +}; + +#define ppp_info(packet) \ + (packet+4) + +#define ppp_proto(packet) \ + (ntohs(*((guint16*)(packet+2 + +struct _GAtPPP { + gint ref_count; + GAthase phase; + guint8 buffer[BUFFERSZ]; + int index; + gint mru; + guint16 auth_proto; + char user_name[256]; + char passwd[256]; + gboolean pfc; + gboolean acfc; + guint32 xmit_accm[8]; + guint32 recv_accm; + GIOChannel *modem; + GQueue *event_queue; + GQueue *recv_queue; + GAtPPPConnectFunc connect_cb; + gpointer connect_data; + GAtPPPDisconnectFunc disconnect_cb; + gpointer disconnect_data; + gint modem_watch; +}; + +void __ppp_generate_event(GAtPPP *ppp, GAtPPPEvents event); +void __ppp_register_packet_handler(struct ppp_packet_handler *handler); +void __ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen); +void __ppp_set_auth(GAtPPP *ppp, guint8 *auth_data); +void __ppp_set_recv_accm(GAtPPP *ppp, guint32 accm); +guint32 __ppp_get_xmit_accm(GAtPPP *ppp); +void __ppp_set_pfc(GAtPPP *ppp, gboolean pfc); +gboolean __ppp_get_pfc(GAtPPP *ppp); +void __ppp_set_acfc(GAtPPP *ppp, gboolean acfc); +gboolean __ppp_get_acfc(GAtPPP *ppp); Index: ofono/gatchat/gatppp.c === --- /dev/null 1970-01-01 00:00:00.0 + +++ ofono/gatchat/gatppp.c 2010-03-16 15:34:30.492565338 -0700 @@ -0,0 +1,552 @@ +/* + * + * PPP library with GLib integration + * + * Copyright (C) 2010 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * G
[patch 1/6] Add PPP protocol support with HDLC framing
This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a different patch. --- Makefile.am |4 gatchat/gatppp.c | 133 gatchat/gatppp.h | 59 +++ gatchat/ppp.c| 455 +++ gatchat/ppp.h| 130 +++ 5 files changed, 780 insertions(+), 1 deletion(-) Index: ofono/Makefile.am === --- ofono.orig/Makefile.am 2010-03-19 11:49:00.060469174 -0700 +++ ofono/Makefile.am 2010-03-19 11:49:02.652468489 -0700 @@ -55,7 +55,9 @@ gatchat/gattty.h gatchat/gattty.c \ gatchat/gatutil.h gatchat/gatutil.c \ gatchat/gat.h \ - gatchat/gatserver.h gatchat/gatserver.c + gatchat/gatserver.h gatchat/gatserver.c \ + gatchat/gatppp.c gatchat/gatppp.h \ + gatchat/ppp.c gatchat/ppp.h udev_files = plugins/ofono.rules Index: ofono/gatchat/gatppp.c === --- /dev/null 1970-01-01 00:00:00.0 + +++ ofono/gatchat/gatppp.c 2010-03-19 11:49:02.652468489 -0700 @@ -0,0 +1,133 @@ +/* + * + * PPP library with GLib integration + * + * Copyright (C) 2009-2010 Intel Corporation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +#ifdef HAVE_CONFIG_H +#include +#endif + +#include +#include +#include +#include +#include +#include + +#include + +#include "gatutil.h" +#include "gatppp.h" +#include "ppp.h" + +/* Administrative Open */ +void g_at_ppp_open(GAtPPP *ppp) +{ + /* send an OPEN event to the lcp layer */ +} + +void g_at_ppp_set_connect_function(GAtPPP *ppp, + GAtPPPConnectFunc callback, gpointer user_data) +{ + ppp->connect_cb = callback; + ppp->connect_data = user_data; +} + +void g_at_ppp_set_disconnect_function(GAtPPP *ppp, + GAtPPPDisconnectFunc callback, + gpointer user_data) +{ + ppp->disconnect_cb = callback; + ppp->disconnect_data = user_data; +} + +void g_at_ppp_shutdown(GAtPPP *ppp) +{ + /* close the ppp */ + ppp_close(ppp); + + /* clean up all the queues */ + g_queue_free(ppp->event_queue); + g_queue_free(ppp->recv_queue); + + /* cleanup modem channel */ + g_source_remove(ppp->modem_watch); + g_io_channel_unref(ppp->modem); +} + +void g_at_ppp_ref(GAtPPP *ppp) +{ + g_atomic_int_inc(&ppp->ref_count); +} + +void g_at_ppp_unref(GAtPPP *ppp) +{ + if (g_atomic_int_dec_and_test(&ppp->ref_count)) { + g_at_ppp_shutdown(ppp); + g_free(ppp); + } +} + +GAtPPP *g_at_ppp_new(GIOChannel *modem) +{ + GAtPPP *ppp; + + ppp = g_try_malloc0(sizeof(GAtPPP)); + if (!ppp) + return NULL; + + ppp->modem = g_io_channel_ref(modem); + if (!g_at_util_setup_io(ppp->modem, G_IO_FLAG_NONBLOCK)) { + g_io_channel_unref(modem); + g_free(ppp); + return NULL; + } + g_io_channel_set_buffered(modem, FALSE); + + ppp->ref_count = 1; + + /* set options to defaults */ + ppp->mru = DEFAULT_MRU; + ppp->recv_accm = DEFAULT_ACCM; + ppp->xmit_accm[0] = DEFAULT_ACCM; + ppp->xmit_accm[3] = 0x6000; /* 0x7d, 0x7e */ + ppp->pfc = FALSE; + ppp->acfc = FALSE; + + /* allocate the queues */ + ppp->event_queue = g_queue_new(); + ppp->recv_queue = g_queue_new(); + + ppp->index = 0; + + /* initialize the lcp state */ + + + /* initialize the autentication state */ + + + /* intialize the network state */ + + /* start listening for packets from the modem */ + ppp->modem_watch = g_io_add_watch(modem, + G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL, + ppp_cb, ppp); + + return ppp; +} Index: ofono/gatchat/gatppp.h === --- /dev/null 1970-01-01 00:00:00.0 + +++ ofono/gatchat/gatppp.h 2010-03-19 11:49:02.652468489 -0700 @
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > This patch implements the basic PPP protocol. LCP, NCP etc. are handled in > a different patch. > > Index: ofono/Makefile.am > === > --- ofono.orig/Makefile.am2010-03-10 16:58:09.915955860 -0800 > +++ ofono/Makefile.am 2010-03-10 16:58:12.039961039 -0800 > @@ -55,7 +55,9 @@ > gatchat/gattty.h gatchat/gattty.c \ > gatchat/gatutil.h gatchat/gatutil.c \ > gatchat/gat.h \ > - gatchat/gatserver.h gatchat/gatserver.c > + gatchat/gatserver.h gatchat/gatserver.c \ > + gatchat/gatppp.c gatchat/gatppp.h \ > + gatchat/gatppp_internal.h > > udev_files = plugins/ofono.rules > > Index: ofono/gatchat/gatppp_internal.h > === > --- /dev/null 1970-01-01 00:00:00.0 + > +++ ofono/gatchat/gatppp_internal.h 2010-03-10 16:58:12.039961039 -0800 > @@ -0,0 +1,97 @@ > +/* > + * > + * PPP library with GLib integration > + * > + * Copyright (C) 2010 Intel Corporation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 > USA + * > + */ > + > +#ifndef __G_AT_PPP_INTERNAL_H > +#define __G_AT_PPP_INTERNAL_H > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#define DEFAULT_MRU 1500 > +#define BUFFERSZ DEFAULT_MRU*2 > +#define DEFAULT_ACCM 0x > +#define PPP_ESC 0x7d > +#define PPP_FLAG_SEQ 0x7e > +#define PPP_ADDR_FIELD 0xff > +#define PPP_CTRL 0x03 > +#define LCP_PROTOCOL 0xc021 > +#define CHAP_PROTOCOL0xc223 > +#define PPP_HEADROOM 2 > +#define HDLC_HEADROOM3 > +#define HDLC_TAIL3 > +#define MD5 5 > + > +enum PPP_PHASE { > + DEAD = 0, > + ESTABLISHMENT, > + AUTHENTICATION, > + NETWORK, > + TERMINATION, > +}; > + > +enum PPP_EVENTS { > + PPP_UP = 1, > + PPP_OPENED, > + PPP_SUCCESS, > + PPP_NONE, > + PPP_CLOSING, > + PPP_FAIL, > + PPP_DOWN > +}; > + > +struct ppp_packet_handler { > + guint16 proto; > + void (*handler)(gpointer priv, guint8 *packet); > + gpointer priv; > +}; > + > +#define ppp_generate_event(l, e) \ > + __ppp_generate_event(l, e) Why do we need this define? > + > +#define ppp_transmit(l, p, i) \ > + __ppp_transmit(l, p, i) > + and this? > +#define ppp_register_packet_handler(h) \ > + __ppp_register_packet_handler(h) or this? > + > +#define ppp_info(packet) \ > + (packet+4) > + > +#define ppp_proto(packet) \ > + (ntohs(*((guint16*)(packet+2 > + > +void __ppp_generate_event(struct ppp_link *link, guint event); > +void __ppp_register_packet_handler(struct ppp_packet_handler *handler); > +void __ppp_transmit(struct ppp_link *link, guint8 *packet, guint infolen); > +void __ppp_set_auth(struct ppp_link *link, guint8 *auth_data); > +void __ppp_set_recv_accm(struct ppp_link *link, guint32 accm); > +guint32 __ppp_get_xmit_accm(struct ppp_link *link); > +void __ppp_set_pfc(struct ppp_link *link, gboolean pfc); > +gboolean __ppp_get_pfc(struct ppp_link *link); > +void __ppp_set_acfc(struct ppp_link *link, gboolean acfc); > +gboolean __ppp_get_acfc(struct ppp_link *link); > +#ifdef __cplusplus > +} > +#endif > + > +#endif /* __G_AT_PPP_INTERNAL_H */ > + > Index: ofono/gatchat/gatppp.c > === > --- /dev/null 1970-01-01 00:00:00.0 + > +++ ofono/gatchat/gatppp.c2010-03-10 16:58:12.039961039 -0800 > @@ -0,0 +1,557 @@ > +/* > + * > + * PPP library with GLib integration > + * > + * Copyright (C) 2010 Intel Corporation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Publ
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > +struct ppp_link * g_at_ppp_new(GIOChannel *modem); > +void g_at_ppp_open(struct ppp_link *link); > +void g_at_ppp_set_connect_function(struct ppp_link *link, > +GAtPPPConnectFunc callback, gpointer user_data); > +void g_at_ppp_set_disconnect_function(struct ppp_link *link, > + GAtPPPDisconnectFunc callback, > + gpointer user_data); > +void g_at_ppp_shutdown(struct ppp_link *link); > +void g_at_ppp_ref(struct ppp_link *link); > +void g_at_ppp_unref(struct ppp_link *link); Almost forgot, let us not use struct ppp_link here, but instead use GAtPpp *ppp as the parameter. Again, mostly for consistency with how glib / gatchat does this. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Denis, > > +struct ppp_link * g_at_ppp_new(GIOChannel *modem); > > +void g_at_ppp_open(struct ppp_link *link); > > +void g_at_ppp_set_connect_function(struct ppp_link *link, > > + GAtPPPConnectFunc callback, gpointer user_data); > > +void g_at_ppp_set_disconnect_function(struct ppp_link *link, > > + GAtPPPDisconnectFunc callback, > > + gpointer user_data); > > +void g_at_ppp_shutdown(struct ppp_link *link); > > +void g_at_ppp_ref(struct ppp_link *link); > > +void g_at_ppp_unref(struct ppp_link *link); > > Almost forgot, let us not use struct ppp_link here, but instead use GAtPpp > *ppp as the parameter. Again, mostly for consistency with how glib / gatchat > does this. I am actually okay with GAtPPP since it looks much nicer. True camel case is making it harder to read. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
On Thu, 11 Mar 2010 20:17:49 -0600 Denis Kenzior wrote: > > > + } else { > > + /* store last flag character */ > > + link->buffer[link->index++] = data[pos]; > > + frame = ppp_decode(link, link->buffer); > > This function along with ppp_decode do almost exactly the same thing as > gsm0710_advanced_extract_frame in gsm0710.c. They both do HDLC frame > decoding, and the only difference I can see is in the fcs table. Can we > combine these somehow? Possibly - although in theory in addition to the escaping that you do in the gsm0710 code, we have to support a negotiated accm (which you see I've not yet implemented here). We also in theory should support PFC and ACFC (which the one modem I tested with required, otherwise it refused to ack my Configure-Request). I think there may eventually be enough differences to keep these separate. ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > On Thu, 11 Mar 2010 20:17:49 -0600 > > Denis Kenzior wrote: > > > + } else { > > > + /* store last flag character */ > > > + link->buffer[link->index++] = data[pos]; > > > + frame = ppp_decode(link, link->buffer); > > > > This function along with ppp_decode do almost exactly the same thing as > > gsm0710_advanced_extract_frame in gsm0710.c. They both do HDLC frame > > decoding, and the only difference I can see is in the fcs table. Can we > > combine these somehow? > > Possibly - although in theory in addition to the escaping that you do > in the gsm0710 code, we have to support a negotiated accm (which you see > I've not yet implemented here). We also in theory should support PFC and > ACFC (which the one modem I tested with required, otherwise it refused to > ack my Configure-Request). I think there may eventually be enough > differences to keep these separate. > You will have to explain to me what that all means ;) However, it would be ideal if we can create a set of utilities that can be shared between ppp and mux code (maybe with extra configuration parameters turning on/off or passing in parameters required for pfc/acfc/accm support.) No sense writing, testing and debugging (and more importantly maintaining) the same code twice. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a > different patch. > > --- > Makefile.am |4 > gatchat/gatppp.c | 552 > ++ > gatchat/gatppp.h | 59 > gatchat/gatppp_internal.h | 97 > 4 files changed, 711 insertions(+), 1 deletion(-) so before we merge this patch and then continue development in the oFono tree, we have some renaming to do. Denis and I talked about it on IRC. And what we actually want is something like this: gatchat/gatppp.hPublic include file gatchat/gatppp.cImplementation of PPP API gatchat/ppp.h Internal header file gatchat/ppp.c Generic PPP stuff (maybe not needed) gatchat/ppp_.c Protocol specific extensions This makes it similar to what we do with gsm7010.[ch] and ringbuffer.[ch]. So this means for internal structures and functions the prefix should be ppp_*. Similar to what we do with GSM 70.10 and the ring buffer implementation. We might have mislead you with parts of the previous review. I apologize for that. The more we are looking at your patches it becomes clearer which direction we wanna take. > + > +typedef enum _GAthase { > + DEAD = 0, > + ESTABLISHMENT, > + AUTHENTICATION, > + NETWORK, > + TERMINATION, > +} GAthase; > + > +typedef enum _GAtPPPEvents { > + PPP_UP = 1, > + PPP_OPENED, > + PPP_SUCCESS, > + PPP_NONE, > + PPP_CLOSING, > + PPP_FAIL, > + PPP_DOWN > +} GAtPPPEvents; If we don't expose them in public headers, then typedefs are not needed. We actually dislike typedefs. The only reason for having them in public GAtChat headers is to confirm with GLib style. > +struct ppp_packet_handler { > + guint16 proto; > + void (*handler)(gpointer priv, guint8 *packet); > + gpointer priv; > +}; > + > +#define ppp_info(packet) \ > + (packet+4) > + > +#define ppp_proto(packet) \ > + (ntohs(*((guint16*)(packet+2 Arithmetic operations requires spaces. Even in defines. So it packet + 4 etc. When you cast don't forget whitepaces. So (guint16 *) (packet... > +struct _GAtPPP { > + gint ref_count; > + GAthase phase; > + guint8 buffer[BUFFERSZ]; > + int index; > + gint mru; > + guint16 auth_proto; > + char user_name[256]; > + char passwd[256]; > + gboolean pfc; > + gboolean acfc; > + guint32 xmit_accm[8]; > + guint32 recv_accm; > + GIOChannel *modem; > + GQueue *event_queue; > + GQueue *recv_queue; > + GAtPPPConnectFunc connect_cb; > + gpointer connect_data; > + GAtPPPDisconnectFunc disconnect_cb; > + gpointer disconnect_data; > + gint modem_watch; > +}; > + > +void __ppp_generate_event(GAtPPP *ppp, GAtPPPEvents event); > +void __ppp_register_packet_handler(struct ppp_packet_handler *handler); > +void __ppp_transmit(GAtPPP *ppp, guint8 *packet, guint infolen); > +void __ppp_set_auth(GAtPPP *ppp, guint8 *auth_data); > +void __ppp_set_recv_accm(GAtPPP *ppp, guint32 accm); > +guint32 __ppp_get_xmit_accm(GAtPPP *ppp); > +void __ppp_set_pfc(GAtPPP *ppp, gboolean pfc); > +gboolean __ppp_get_pfc(GAtPPP *ppp); > +void __ppp_set_acfc(GAtPPP *ppp, gboolean acfc); > +gboolean __ppp_get_acfc(GAtPPP *ppp); Just use ppp_* here and forget about __ppp_*. I will add autofoo magic to ensure we are not accidentally exporting these. It is not your concern. > + * PPP library with GLib integration > + * > + * Copyright (C) 2010 Intel Corporation. All rights reserved. Nitpick here. Copyright is 2009-2010. Applies to all new files you are adding. > + /* > + * do the octet stuffing. Add 2 bytes to the infolen to > + * include the protocol field. > + */ > + frame = ppp_encode(ppp, packet, infolen+2, &framelen); > + if (!frame) { > + g_printerr("Failed to encode packet to transmit\n"); > + return; > + } > + > + /* transmit through the lower layer interface */ > + /* > + * TBD - should we just put this on a queue and transmit when > + * we won't block, or allow ourselves to block here? > + */ > + status = g_io_channel_write_chars(ppp->modem, (gchar *) frame, > + framelen, &bytes_written, &error); > + > + g_free(frame); > + > +} Look out for unneeded empty lines at the end of functions. We don't want them. > +static void ppp_transition_phase(GAtPPP *ppp, guint phase) > +{ > + /* don't do anything if we're already there */ > + if (ppp->phase == phase) > + return; > + > + /* set new phase */ > + ppp->phase = phase; > + > + switch(phase) { > + case ESTABLISHMENT: > + ppp_ppp_establishment(ppp); > + break; > + case AUTHENTICATION: > + ppp_authenticate(ppp); > + break; > + case TERMINATION: > +
Re: [patch 1/6] Add PPP protocol support with HDLC framing
Hi Kristen, > This patch implements the basic PPP protocol. LCP, NCP etc. are handled in a > different patch. > > --- > Makefile.am |4 > gatchat/gatppp.c | 133 > gatchat/gatppp.h | 59 +++ > gatchat/ppp.c| 455 > +++ > gatchat/ppp.h| 130 +++ > 5 files changed, 780 insertions(+), 1 deletion(-) please keep the patches whitespace clean. Otherwise you will make it really hard and complicated for everybody if we have to do whitespace cleanup in between patches. Applying: Add PPP protocol support with HDLC framing /data/devel/ofono/.git/rebase-apply/patch:572: space before tab in indent. if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN) /data/devel/ofono/.git/rebase-apply/patch:573: space before tab in indent. return FALSE; fatal: 2 lines add whitespace errors. Using an editor that visualizes whitespaces and tabs might help here. And empty lines at the end of a file. Please remove them and if your editor is too stupid, then please pick a different editor. Applying: Add PPP protocol support with HDLC framing /data/devel/ofono/.git/rebase-apply/patch:166: new blank line at EOF. + /data/devel/ofono/.git/rebase-apply/patch:230: new blank line at EOF. + fatal: 2 lines add whitespace errors. Before you submit any patches, you might wanna quickly check with git am that they still apply. My .gitconfig uses a strict apply policy: [diff] color = auto check = true [apply] whitespace = error [format-patch] check = true Having diff and format-patch color your whitespace damages is always a good idea. Then you see what is wrong. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono