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.h Public include file gatchat/gatppp.c Implementation of PPP API gatchat/ppp.h Internal header file gatchat/ppp.c Generic PPP stuff (maybe not needed) gatchat/ppp_<proto>.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 _GAtPPPPhase { > + DEAD = 0, > + ESTABLISHMENT, > + AUTHENTICATION, > + NETWORK, > + TERMINATION, > +} GAtPPPPhase; > + > +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; > + GAtPPPPhase 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: > + ppp_terminate(ppp); > + break; > + case DEAD: > + ppp_dead(ppp); > + break; > + case NETWORK: > + ppp_network(ppp); > + break; > + default: > + break; > + } Since this is an enum anyway. Please skip the default statement and just ensure you handle all states. For the ones that don't apply, add just empty statements. That way the compiler will warn you if you add a new state to the enum later on. And you accidentally forget to add code for handling that state. And I do prefer having a prefix here. So PPP_DEAD, PPP_NETWORK etc. > +void g_at_ppp_unref(GAtPPP *ppp) > +{ > + gboolean last; > + > + last = g_atomic_int_dec_and_test(&ppp->ref_count); > + if (last) { > + g_at_ppp_shutdown(ppp); > + g_free(ppp); > + } > +} Not using the last variable here is better. You really want this to be atomic. Just check the return value of the function directly. > + /* think I still need to do this */ > + g_io_channel_set_buffered(modem, FALSE); I have sounds really picky now ;) Comment should be either with FIXME if it is something that is unclear. Or better at least in third person. > + 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] = 0x60000000; /* 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 */ I know that your other patches are going to add this code later one. However having these prefixed with FIXME would be better. Not that it is a big thing if we see the patch series as a whole, but would be better for future things like this. Regards Marcel _______________________________________________ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono