Add PPP protocol support with HDLC framing
A message that is not in the archive (sent off-list or not yet received): [email protected]
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(-)
Makefile.am +3 -1
@@ -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
gatchat/gatppp_internal.h new file +97 -0
97 lines not on this page · raw message
gatchat/gatppp.c new file +552 -0
552 lines not on this page · raw message
gatchat/gatppp.h new file +59 -0
59 lines not on this page · raw message
Hi Kristen,
Quoted 9 lines
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.
Quoted 8 lines
gatchat/gatppp_internal.h new file +8
-0+ +typedef enum _GAtPPPPhase { +DEAD =0 , +ESTABLISHMENT , +AUTHENTICATION , +NETWORK , +TERMINATION , +}GAtPPPPhase ;
gatchat/gatppp_internal.h line 51
Quoted 10 lines
gatchat/gatppp_internal.h new file +10
-0+ +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.
gatchat/gatppp_internal.h line 63
Quoted 11 lines
gatchat/gatppp_internal.h new file +11
-0+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...
Quoted 23 lines
gatchat/gatppp_internal.h new file +23
-0+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; +}; +
gatchat/gatppp_internal.h line 97
Quoted 10 lines
gatchat/gatppp_internal.h new file +10
-0+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.
Quoted 3 lines
+ * 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.
Quoted 11 lines
gatchat/gatppp.c new file +11
-0+/* +* 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 */
gatchat/gatppp.c line 322
Quoted 10 lines
gatchat/gatppp.c new file +10
-0+/* +* 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.
Quoted 18 lines
gatchat/gatppp.c new file +18
-0+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);
gatchat/gatppp.c line 416
Quoted 10 lines
gatchat/gatppp.c new file +10
-0+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.
gatchat/gatppp.c line 502
Quoted 10 lines
gatchat/gatppp.c new file +10
-0+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.
gatchat/gatppp.c line 520
Quoted 2 lines
gatchat/gatppp.c new file +2
-0+/* 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.
Quoted 13 lines
gatchat/gatppp.c new file +13
-0+ 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 ();
gatchat/gatppp.c line 544
Quoted 10 lines
gatchat/gatppp.c new file +10
-0+ + 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
PATCH 2/6 Generic PPP control protocol
In reply to a message not in the archiveImplement a generic control protocol that can be shared by both LCP and NCP implementations.
--- Makefile.am | 3 gatchat/gatppp_internal.h | 2 gatchat/gatpppcp.c | 1509 ++++++++++++++++++++++++++++++++++++++++++++++ gatchat/gatpppcp.h | 133 ++++ 4 files changed, 1646 insertions(+), 1 deletion(-)
Makefile.am +2 -1
@@ -57,7 +57,8 @@ gatchat/gat.h \ gatchat/gatserver.h gatchat/gatserver.c \ gatchat/gatppp.c gatchat/gatppp.h \ - gatchat/gatppp_internal.h + gatchat/gatppp_internal.h gatchat/gatpppcp.c + gatchat/gatpppcp.h udev_files = plugins/ofono.rules
gatchat/gatpppcp.c new file +1509 -0
1,509 lines not on this page · raw message
gatchat/gatpppcp.h new file +133 -0
133 lines not on this page · raw message
gatchat/gatppp_internal.h +2 -0
@@ -18,6 +18,8 @@* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA * */ +#include "gatpppcp.h" +#define DEFAULT_MRU 1500#define BUFFERSZ DEFAULT_MRU*2#define DEFAULT_ACCM 0x00000000
Re: [patch 2/6] Generic PPP control protocol
Hi Kristen,
Quoted 5 lines
Makefile.am | 3 gatchat/gatppp_internal.h | 2 gatchat/gatpppcp.c | 1509 ++++++++++++++++++++++++++++++++++++++++++++++ gatchat/gatpppcp.h | 133 ++++ 4 files changed, 1646 insertions(+), 1 deletion(-)
please call them gatchat/ppp_cp.[ch].
gatchat/gatpppcp.c line 38
Quoted 3 lines
gatchat/gatpppcp.c new file +3
-0+static gboolean pppcp_timeout (gpointer user); +static void pppcp_handle_event (gpointer user); +static gint is_option (struct ppp_option *o,gpointer user);
In general we dislike forward declarations and only allow them if the are really really needed. Are they?
gatchat/gatpppcp.c line 50
Quoted 11 lines
gatchat/gatpppcp.c new file +11
-0+const char *pppcp_state_strings[] = + {"INITIAL" ,"STARTING" ,"CLOSED" ,"STOPPED" ,"CLOSING" ,"STOPPING" , +"REQSENT" ,"ACKRCVD" ,"ACKSENT" ,"OPENED" }; + +#ifdef DEBUG +#define pppcp_trace (p) \ + (g_print("%s: current state %d:%s\n", __FUNCTION__, \ + p->state, pppcp_state_strings[p->state])) +#else +#define pppcp_trace (p) +#endif
Should the pppcp_state_strings variable be inside the ifdef DEBUG?
Quoted 9 lines
gatchat/gatpppcp.c new file +9
-0+enum { +INITIAL_RESTART_TIMEOUT =3000 , +MAX_TERMINATE =2 , +MAX_CONFIGURE =10 , +MAX_FAILURE =5 , +CP_HEADER_SZ =4 , + +/* states */ +INITIAL =0 ,
gatchat/gatpppcp.c line 86
Quoted 10 lines
gatchat/gatpppcp.c new file +10
-0+STARTING =1 , +CLOSED =2 , +STOPPED =3 , +CLOSING =4 , +STOPPING =5 , +REQSENT =6 , +ACKRCVD =7 , +ACKSENT =8 , + OPENED = 9, +};
Please make it two enums with separate comments what they are. For the top ones I would prefer defines actually.
gatchat/gatpppcp.c line 90
Quoted 3 lines
gatchat/gatpppcp.c new file +3
-0+static struct pppcp_packet *pppcp_packet_new (struct pppcp_data *data, +guint type,guint bufferlen) +{
This is pppcp_packet *pppcp_packet_new() actually. Same as with variables. No difference here.
gatchat/gatpppcp.c line 99
Quoted 9 lines
gatchat/gatpppcp.c new file +9
-0+struct pppcp_header *packet; +guint8 *ppp_packet; + + ppp_packet =g_try_malloc0 (bufferlen +sizeof (*packet) +2 ); +if (!ppp_packet) +return NULL ; + +/* add our protocol information */ + *((guint16 *)ppp_packet) =htons (data->proto);
You can not do this. It will break on non-x86 platforms where unaligned access needs special handling.
gatchat/gatpppcp.c line 107
Quoted 7 lines
gatchat/gatpppcp.c new file +7
-0+/* advance past protocol to add CP header information */ + packet = (struct pppcp_header *) (ppp_packet +2 ); + + packet->length =htons ((guint16)(bufferlen +sizeof (*packet))); + packet->code = type; +return ((struct pppcp_packet *)packet); +}
At little bit too much nasty casting for my taste.
Also if you cast don't forget the extra whitespace. It is *) packet.
gatchat/gatpppcp.c line 134
Quoted 3 lines
gatchat/gatpppcp.c new file +3
-0+static gboolean pppcp_timeout (gpointer user) +{ +struct pppcp_data *data = user;
It is better to use user_data as variable name here. User is pretty much confusing. It applies to other locations as well.
gatchat/gatpppcp.c line 154
Quoted 3 lines
gatchat/gatpppcp.c new file +3
-0+ event =g_malloc (sizeof (struct pppcp_event ) + len); +if (!event) +return NULL ;
Seems you meant g_try_malloc here.
Quoted 1 line
+/* what about locking, do we need to worry about that here */
As long as we are single-thread and mainloop driven, we don't have to worry here.
gatchat/gatpppcp.c line 520
Quoted 8 lines
gatchat/gatpppcp.c new file +8
-0+static void pppcp_transition_state (int new_state,struct pppcp_data *data) +{ +/* +* if switching from a state where +* TO events occur, to one where they +* may not, shut off the timer +*/ +switch (new_state) {
It is switch (new_state) btw.
gatchat/gatpppcp.c line 533
Quoted 13 lines
gatchat/gatpppcp.c new file +13
-0+case INITIAL : +case STARTING : +case CLOSED : +case STOPPED : +case OPENED : +/* if timer is running, stop it */ +if (pppcp_timer_is_running (data)) +pppcp_stop_timer (data); +break ; +default : +break ; +/* do nothing */ + }
Please don't use the default statement. See my explanation in the other email why we don't want it. Let the compiler be a safe-guard.
Quoted 7 lines
gatchat/gatpppcp.c new file +7
-0+static void pppcp_up_event (struct pppcp_data *data,guint8 *packet,guint len) +{ +pppcp_trace (data); +switch (data->state) { +case INITIAL : +/* switch state to CLOSED */ +pppcp_transition_state (CLOSED , data);
gatchat/gatpppcp.c line 553
Quoted 10 lines
gatchat/gatpppcp.c new file +10
-0+break ; +case STARTING : +/* irc, scr/6 */ +pppcp_initialize_restart_count (data, data->max_configure); +pppcp_send_configure_request (data); +pppcp_transition_state (REQSENT , data); +break ; +default : +/* all other are illegal */ +pppcp_illegal_event (data->state,UP );
Same here. Please spell the illegal ones out. I know it looks like more work, but it is important to make it explicit. Please check all of these.
gatchat/gatpppcp.c line 113
Quoted 3 lines
gatchat/gatpppcp.c new file +3
-0+ } + +}
Remove the extra empty line before the function end.
Quoted 5 lines
gatchat/gatpppcp.c new file +5
-0+static void verify_config_option (gpointer elem,gpointer user) +{ +struct ppp_option *config = elem; +struct pppcp_data *data = user; +GList *list =NULL ;
gatchat/gatpppcp.c line 1068
Quoted 10 lines
gatchat/gatpppcp.c new file +10
-0+struct ppp_option *option; + +/* +* determine whether this config option is in the +* acceptable options list +*/ +if (g_list_length (data->acceptable_options)) + list =g_list_find_custom (data->acceptable_options, +GUINT_TO_POINTER (config->type), + (GCompareFunc ) is_option);
Don't do this GCompareFunc casting. It is a dead-trap. Please use create is_option with proper parameters and assign the void pointers inside the callback to right types.
We have seen way too many mistakes with wrong function prototypes that changed over time. Not casting makes the compiler warn us at least if we screwed up.
Quoted 9 lines
gatchat/gatpppcp.c new file +9
-0+static void remove_config_option (gpointer elem,gpointer user) +{ +struct ppp_option *config = elem; +struct pppcp_data *data = user; +GList *list =NULL ; + +/* +* determine whether this config option is in the +* applied options list
gatchat/gatpppcp.c line 1104
Quoted 10 lines
gatchat/gatpppcp.c new file +10
-0+*/ +if (g_list_length (data->config_options)) { + list =g_list_find_custom (data->config_options, +GUINT_TO_POINTER (config->type), + (GCompareFunc ) is_option); +if (list) + data->config_options = +g_list_delete_link (data->config_options, list); + } +}
Why us list = NULL assigned? I don't see the need for it. If it is not needed, then please don't do it.
gatchat/gatpppcp.c line 1159
Quoted 6 lines
gatchat/gatpppcp.c new file +6
-0+/* make sure all required config options were included */ +g_list_foreach (data->config_options, verify_config_option, data); + +if (g_list_length (data->unacceptable_options) || +g_list_length (data->rejected_options)) +return RCR_MINUS ;
You need a double indentation for the second g_list_length() here. Otherwise it might look like code that suppose to be part of the block that gets executed. What out for these. While the compiler understands them, the human brain might get puzzled.
gatchat/gatpppcp.h line 28
Quoted 7 lines
gatchat/gatpppcp.h new file +7
-0+#ifndef __PPPCP_H +#define __PPPCP_H + +#ifdef __cplusplus +extern "C" { +#endif +
Both are not needed.
Quoted 1 line
+struct
pppcp_data;
Why is this needed?
Quoted 5 lines
gatchat/gatpppcp.h new file +5
-0+ +/* PPPCP CODE FIELD */ +enum PPPCP_CODE { +CONFIGURE_REQUEST =1 , +CONFIGURE_ACK ,
gatchat/gatpppcp.h line 44
Quoted 10 lines
gatchat/gatpppcp.h new file +10
-0+CONFIGURE_NAK , +CONFIGURE_REJECT , +TERMINATE_REQUEST , +TERMINATE_ACK , +CODE_REJECT , +PROTOCOL_REJECT , +ECHO_REQUEST , +ECHO_REPLY , +DISCARD_REQUEST +};
The comment is not really adding any value here. Make it more descriptive or just leave it out.
Regards
Marcel
Re: [patch 2/6] Generic PPP control protocol
Quoted 3 lines · On Wed, 17 Mar 2010 00:06:34 -0700 Marcel Holtmann <[email protected]> wrote:
Quoted 1 line
+struct
pppcp_data;Why is this needed?
this is needed because we have some circular dependencies in the pppcp_data structure and the pppcp_actions structure.
Re: [patch 2/6] Generic PPP control protocol
Hi Kristen,
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+struct
pppcp_data;Why is this needed?
this is needed because we have some circular dependencies in the
pppcp_datastructure and thepppcp_actionsstructure.
you might be absolutely right that this is needed. And it is cleanest way to do it. However every time I see forward declarations and hear circular dependencies, I question if the code might not be too complex.
Please keep this in mind. We will revisit this again when doing a deep down dive review after the merge.
Regards
Marcel
PATCH 3/6 LCP support
In reply to a message not in the archiveImplement LCP support for the PPP protocol
--- Makefile.am | 6 - gatchat/gatppp.c | 53 +++++++++ gatchat/gatppp_internal.h | 8 + gatchat/gatppplcp.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 307 insertions(+), 4 deletions(-)
Makefile.am +3 -3
@@ -56,9 +56,9 @@ gatchat/gatutil.h gatchat/gatutil.c \ gatchat/gat.h \ gatchat/gatserver.h gatchat/gatserver.c \ - gatchat/gatppp.c gatchat/gatppp.h \ - gatchat/gatppp_internal.h gatchat/gatpppcp.c - gatchat/gatpppcp.h + gatchat/gatppp_internal.h gatchat/gatpppcp.c \ + gatchat/gatpppcp.h gatchat/gatppp.c \ + gatchat/gatppplcp.c gatchat/gatppp.h udev_files = plugins/ofono.rules
gatchat/gatppp.c +52 -1
93 lines not on this page · raw message
gatchat/gatppp_internal.h +8 -0
@@ -67,6 +67,7 @@struct _GAtPPP {gint ref_count;GAtPPPPhase phase; +struct pppcp_data *lcp;guint8 buffer[BUFFERSZ ];int index;gint mru;
@@ -97,3 +98,10 @@gboolean __ppp_get_pfc (GAtPPP *ppp);void __ppp_set_acfc (GAtPPP *ppp,gboolean acfc);gboolean __ppp_get_acfc (GAtPPP *ppp); +struct pppcp_data *lcp_new (GAtPPP *ppp); +void lcp_free (struct pppcp_data *lcp); +void lcp_open (struct pppcp_data *data); +void lcp_close (struct pppcp_data *data); +void lcp_establish (struct pppcp_data *data); +void lcp_terminate (struct pppcp_data *data); +
gatchat/gatppplcp.c new file +244 -0
244 lines not on this page · raw message
Re: [patch 3/6] LCP support
Hi Kristen,
Quoted 4 lines
Makefile.am | 6 - gatchat/gatppp.c | 53 +++++++++ gatchat/gatppp_internal.h | 8 + gatchat/gatppplcp.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++
this should be gatchat/ppp_lcp.c
gatchat/gatppp.c line 513
Quoted 3 lines
gatchat/gatppp.c +3
-0+void __ppp_set_auth (GAtPPP *ppp,guint8 * auth_data) +{ +guint16 proto =ntohs (*(guint16 *)auth_data);
This are again one of these constructs that will break on non-x86 hardware.
I think you need to create ppp_get_unaligned and ppp_put_unaligned. If all of them are be16 anyway, you could do ppp_get_unaligned_be16 etc.
Regards
Marcel
Re: [patch 3/6] LCP support
Quoted 16 lines · On Wed, 17 Mar 2010 00:09:25 -0700 Marcel Holtmann <[email protected]> wrote:
Hi Kristen,
Quoted 4 lines
Makefile.am | 6 - gatchat/gatppp.c | 53 +++++++++ gatchat/gatppp_internal.h | 8 + gatchat/gatppplcp.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++this should be
gatchat/ppp_lcp.cgatchat/gatppp.c line 513
Quoted 3 lines
gatchat/gatppp.c +3
-0+void __ppp_set_auth (GAtPPP *ppp,guint8 * auth_data) +{ +guint16 proto =ntohs (*(guint16 *)auth_data);This are again one of these constructs that will break on non-x86 hardware.
Quoted 6 lines
I think you need to create
ppp_get_unalignedandppp_put_unaligned. If all of them are be16 anyway, you could doppp_get_unaligned_be16etc.Regards
Marcel
Would it make more sense to pack the structures that are meant to represent bytes off the wire? I personally prefer that to making a different function for be vs. le. But I am not sure if we can assume we are using gcc and have __attribute__((packed)) support?
Re: [patch 3/6] LCP support
Hi Kristen,
Quoted 14 lines
Quoted 14 lines
Quoted 4 lines
Makefile.am | 6 - gatchat/gatppp.c | 53 +++++++++ gatchat/gatppp_internal.h | 8 + gatchat/gatppplcp.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++this should be
gatchat/ppp_lcp.cgatchat/gatppp.c line 513
Quoted 3 lines
gatchat/gatppp.c +3
-0+void __ppp_set_auth (GAtPPP *ppp,guint8 * auth_data) +{ +guint16 proto =ntohs (*(guint16 *)auth_data);This are again one of these constructs that will break on non-x86 hardware.
Quoted 7 lines
Quoted 2 lines
I think you need to create
ppp_get_unalignedandppp_put_unaligned. If all of them are be16 anyway, you could doppp_get_unaligned_be16etc.Would it make more sense to pack the structures that are meant to represent bytes off the wire? I personally prefer that to making a different function for be vs. le. But I am not sure if we can assume we are using gcc and have __attribute__((packed)) support?
we assume GCC anyway and that is fine. So if you wanna go for ((packed)) attribute, then that is fine with me, too.
Regards
Marcel
PATCH 4/6 CHAP with MD5 authentication
In reply to a message not in the archiveImplement authentication support with CHAP and MD5 digest
--- Makefile.am | 3 gatchat/gatppp.c | 12 ++ gatchat/gatppp.h | 2 gatchat/gatppp_internal.h | 15 +++ gatchat/gatpppauth.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 260 insertions(+), 2 deletions(-)
Makefile.am +2 -1
@@ -58,7 +58,8 @@ gatchat/gatserver.h gatchat/gatserver.c \ gatchat/gatppp_internal.h gatchat/gatpppcp.c \ gatchat/gatpppcp.h gatchat/gatppp.c \ - gatchat/gatppplcp.c gatchat/gatppp.h + gatchat/gatppplcp.c gatchat/gatpppauth.c \ + gatchat/gatppp.h udev_files = plugins/ofono.rules
gatchat/gatppp.c +11 -1
@@ -490,6 +490,9 @@/* remove lcp */ lcp_free (ppp->lcp); + +/* remove auth */ +auth_free (ppp->auth); }void g_at_ppp_ref (GAtPPP *ppp)
@@ -508,6 +511,12 @@ } } +void g_at_ppp_set_credentials (GAtPPP *ppp,gchar *username, +gchar *passwd) +{ +auth_set_credentials (ppp->auth, username, passwd); +} +void __ppp_set_auth (GAtPPP *ppp,guint8 * auth_data) {guint16 proto =ntohs (*(guint16 *)auth_data);
@@ -515,6 +524,7 @@switch (proto) {case CHAP_PROTOCOL :/* get the algorithm */ +auth_set_proto (ppp->auth, proto, auth_data[2 ]);break ;default :g_printerr ("unknown authentication proto\n" );
@@ -590,7 +600,7 @@ ppp->lcp =lcp_new (ppp);/* initialize the autentication state */ - + ppp->auth =auth_new (ppp);/* intialize the network state */
gatchat/gatppp.h +2 -0
@@ -51,6 +51,8 @@void g_at_ppp_shutdown (GAtPPP *ppp);void g_at_ppp_ref (GAtPPP *ppp);void g_at_ppp_unref (GAtPPP *ppp); +void g_at_ppp_set_credentials (GAtPPP *ppp,gchar *username, +gchar *passwd); #ifdef __cplusplus }#endif
gatchat/gatppp_internal.h +15 -0
@@ -64,10 +64,20 @@#define ppp_proto (packet) \ (ntohs(*((guint16*)(packet+2)))) +struct auth_data { +guint16 proto; +gpointer proto_data; +void (*process_packet)(struct auth_data *data,guint8 *packet); +GAtPPP *ppp; +gchar *username; +gchar *passwd; +}; +struct _GAtPPP {gint ref_count;GAtPPPPhase phase;struct pppcp_data *lcp; +struct auth_data *auth;guint8 buffer[BUFFERSZ ];int index;gint mru;
@@ -104,4 +114,9 @@void lcp_close (struct pppcp_data *data);void lcp_establish (struct pppcp_data *data);void lcp_terminate (struct pppcp_data *data); +void auth_set_credentials (struct auth_data *data,gchar *username, +gchar *passwd); +void auth_set_proto (struct auth_data *data,guint16 proto,guint8 method); +struct auth_data *auth_new (GAtPPP *ppp); +void auth_free (struct auth_data *auth);
gatchat/gatpppauth.c new file +230 -0
230 lines not on this page · raw message
Re: [patch 4/6] CHAP with MD5 authentication
Hi Kristen,
Quoted 7 lines
Implement authentication support with CHAP and MD5 digest
--- Makefile.am | 3 gatchat/gatppp.c | 12 ++ gatchat/gatppp.h | 2 gatchat/gatppp_internal.h | 15 +++ gatchat/gatpppauth.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++
and here we do gatchat/ppp_auth.c.
gatchat/gatppp.h line 55
Quoted 9 lines
===================================================================
gatchat/gatppp.h +2
-0@@ -51,6 +51,8 @@void g_at_ppp_shutdown (GAtPPP *ppp);void g_at_ppp_ref (GAtPPP *ppp);void g_at_ppp_unref (GAtPPP *ppp); +void g_at_ppp_set_credentials (GAtPPP *ppp,gchar *username, +gchar *passwd);
Please don't use gchar. That thing is the most stupid thing the GLib guys ever did. Even I fell for it some time ago. It is just wrong and these days even the GLib guys admit that it was an big mistake.
Also these parameters should be const char * actually.
Regards
Marcel
PATCH 5/6 IP support for PPP
In reply to a message not in the archiveImplement IPCP support. Creates a tun interface to pass IP traffic.
--- Makefile.am | 2 gatchat/gatppp.c | 2 gatchat/gatppp_internal.h | 12 + gatchat/gatpppnet.c | 369 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 384 insertions(+), 1 deletion(-)
Makefile.am +1 -1
@@ -59,7 +59,7 @@ gatchat/gatppp_internal.h gatchat/gatpppcp.c \ gatchat/gatpppcp.h gatchat/gatppp.c \ gatchat/gatppplcp.c gatchat/gatpppauth.c \ - gatchat/gatppp.h + gatchat/gatpppnet.c gatchat/gatppp.h udev_files = plugins/ofono.rules
gatchat/gatppp_internal.h +12 -0
@@ -73,11 +73,19 @@gchar *passwd; }; +struct net_data { +GAtPPP *ppp; +char *if_name; +GIOChannel *channel; +struct pppcp_data *ipcp; +}; +struct _GAtPPP {gint ref_count;GAtPPPPhase phase;struct pppcp_data *lcp;struct auth_data *auth; +struct net_data *net;guint8 buffer[BUFFERSZ ];int index;gint mru;
@@ -119,4 +127,8 @@void auth_set_proto (struct auth_data *data,guint16 proto,guint8 method);struct auth_data *auth_new (GAtPPP *ppp);void auth_free (struct auth_data *auth); +struct net_data *net_new (GAtPPP *ppp); +void net_open (struct net_data *data); +void net_free (struct net_data *data); +void net_close (struct net_data *data);
gatchat/gatpppnet.c new file +369 -0
369 lines not on this page · raw message
gatchat/gatppp.c +2 -0
@@ -388,6 +388,7 @@static void ppp_network (GAtPPP *ppp) {/* bring network phase up */ +net_open (ppp->net); }static void ppp_transition_phase (GAtPPP *ppp,guint phase)
@@ -603,6 +604,7 @@ ppp->auth =auth_new (ppp);/* intialize the network state */ + ppp->net =net_new (ppp);/* start listening for packets from the modem */ ppp->modem_watch = g_io_add_watch(modem,
Re: [patch 5/6] IP support for PPP
Hi Kristen,
Quoted 1 line
Signed-off-by Kristen Carlson Accardi <[email protected]>
we don't do signed-off-by statements inside oFono. So please skip these and fix your .gitconfig.
Quoted 1 line
Implement IPCP support. Creates a tun interface to pass IP traffic.
Also even if we would be doing signed-off-by statements, you messed them up and have it in the wrong order ;)
Quoted 7 lines
--- Makefile.am | 2 gatchat/gatppp.c | 2 gatchat/gatppp_internal.h | 12 + gatchat/gatpppnet.c | 369 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 384 insertions(+), 1 deletion(-)
Use gatchat/ppp_net.c here.
gatchat/gatppp_internal.h line 82
Quoted 7 lines
gatchat/gatppp_internal.h +7
-0+struct net_data { +GAtPPP *ppp; +char *if_name; +GIOChannel *channel; +struct pppcp_data *ipcp; +}; +
I would prefer if you use struct ppp_net.
gatchat/gatppp_internal.h line 133
Quoted 4 lines
gatchat/gatppp_internal.h +4
-0+struct net_data *net_new (GAtPPP *ppp); +void net_open (struct net_data *data); +void net_free (struct net_data *data); +void net_close (struct net_data *data);
These are better ppp_net_new() etc.
gatchat/gatpppnet.c line 5
Quoted 8 lines
gatchat/gatpppnet.c new file +5
-0@@ -0,0 +1,369 @@ +/* +* +* AT chat library with GLib integration +* +* Copyright (C) 2008-2009 Intel Corporation. All rights reserved.
Now I have to be picky with the copyright headers. Please make sure they are in sync.
Quoted 10 lines
gatchat/gatpppnet.c new file +10
-0+/****** IPCP support ****************/ +enum { +/* supported codes */ +IPCP_SUPPORTED_CODES = (1 <<CONFIGURE_REQUEST ) | + (1 <<CONFIGURE_ACK ) | + (1 <<CONFIGURE_NAK ) | + (1 <<CONFIGURE_REJECT ) | + (1 <<TERMINATE_REQUEST ) | + (1 <<TERMINATE_ACK ) | + (1 <<CODE_REJECT ),
gatchat/gatpppnet.c line 218
Quoted 10 lines
gatchat/gatpppnet.c new file +10
-0+ +IPCP_PROTO =0x8021 , + +/* option types */ +IP_ADDRESSES =1 , +IP_COMPRESSION_PROTO =2 , +IP_ADDRESS =3 , +PRIMARY_DNS_SERVER =129 , + SECONDARY_DNS_SERVER = 131, +};
As mentioned before. If you wanna keep enums, please split them into multiple with comments in front of them. It should be clear that they represent different things.
gatchat/gatpppnet.c line 229
Quoted 10 lines
gatchat/gatpppnet.c new file +10
-0+static guint32 bytes_to_32 (guint8 *bytes) +{ +union addr { +guint8 bytes[4 ]; +guint32 word; + } a; + +memcpy (a.bytes, bytes,4 ); +return (ntohl (a.word)); +}
This works, but is pretty ugly.
Doesn't GLib has functions to ensure retrieve unaligned data? BlueZ has the GCC magic that is required to do this right.
Regards
Marcel
Re: [patch 5/6] IP support for PPP
Quoted 13 lines · On Tue, 16 Mar 2010 23:38:33 -0700 Marcel Holtmann <[email protected]> wrote:
gatchat/gatpppnet.c line 229
Quoted 10 lines
gatchat/gatpppnet.c new file +10
-0+static guint32 bytes_to_32 (guint8 *bytes) +{ +union addr { +guint8 bytes[4 ]; +guint32 word; + } a; + +memcpy (a.bytes, bytes,4 ); +return (ntohl (a.word)); +}This works, but is pretty ugly.
Quoted 3 lines
Doesn't GLib has functions to ensure retrieve unaligned data? BlueZ has the GCC magic that is required to do this right.
I have looked everywhere for something nice from glib, but I'm not seeing it. As far as I can tell, most people just do the bit shifting manually -- but that's what I had originally and you didn't like it. So I'm only seeing 2 options here, this way or the original way. If you know of the glib function to use, please let me know what it is.
Re: [patch 5/6] IP support for PPP
Hi Kristen,
Quoted 17 lines
Quoted 13 lines
gatchat/gatpppnet.c line 229
Quoted 10 lines
gatchat/gatpppnet.c new file +10
-0+static guint32 bytes_to_32 (guint8 *bytes) +{ +union addr { +guint8 bytes[4 ]; +guint32 word; + } a; + +memcpy (a.bytes, bytes,4 ); +return (ntohl (a.word)); +}This works, but is pretty ugly.
Quoted 3 lines
Doesn't GLib has functions to ensure retrieve unaligned data? BlueZ has the GCC magic that is required to do this right.
Quoted 5 lines
I have looked everywhere for something nice from glib, but I'm not seeing it. As far as I can tell, most people just do the bit shifting manually -- but that's what I had originally and you didn't like it. So I'm only seeing 2 options here, this way or the original way. If you know of the glib function to use, please let me know what it is.
I don't see how bit shifting is gonna help you against the unaligned access of your buffer.
Regards
Marcel
Re: [patch 5/6] IP support for PPP
Quoted 25 lines · On Thu, 18 Mar 2010 12:26:15 +0100 Marcel Holtmann <[email protected]> wrote:
Hi Kristen,
Quoted 17 lines
Quoted 13 lines
gatchat/gatpppnet.c line 229
Quoted 10 lines
gatchat/gatpppnet.c new file +10
-0+static guint32 bytes_to_32 (guint8 *bytes) +{ +union addr { +guint8 bytes[4 ]; +guint32 word; + } a; + +memcpy (a.bytes, bytes,4 ); +return (ntohl (a.word)); +}This works, but is pretty ugly.
Quoted 3 lines
Doesn't GLib has functions to ensure retrieve unaligned data? BlueZ has the GCC magic that is required to do this right.
Quoted 5 lines
I have looked everywhere for something nice from glib, but I'm not seeing it. As far as I can tell, most people just do the bit shifting manually -- but that's what I had originally and you didn't like it. So I'm only seeing 2 options here, this way or the original way. If you know of the glib function to use, please let me know what it is.
Quoted 8 lines
I don't see how bit shifting is gonna help you against the unaligned access of your buffer.
Regards
Marcel
I wound up implementing both the packed data structures to represent PPP packets (etc) as well as get_unaligned() to handle cases like this. It does still seem hard to believe that glib doesn't already have a way to access unaligned data, but I got tired of looking for it.
PATCH 6/6 Allow gsmdial to use gatchat ppp support
In reply to a message not in the archiveAdd option to use PPP to gsmdial.
--- gatchat/gsmdial.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 3 deletions(-)
gatchat/gsmdial.c +79 -3
@@ -29,10 +29,13 @@#include <errno.h> #include <unistd.h> #include <sys/signalfd.h> +#include <arpa/inet.h> +#include <net/if.h> #include <glib.h> #include <gatchat.h> #include <gattty.h> +#include <gatppp.h> static const char *none_prefix[] = {NULL };static const char *cgreg_prefix[] = {"+CGREG:" ,NULL };
@@ -45,10 +48,14 @@static gchar *option_apn =NULL ;static gint option_offmode =0 ;static gboolean option_legacy = FALSE; +static gboolean option_ppp = FALSE; +static gchar *option_username =NULL ; +static gchar *option_password =NULL ;static GAtChat *control;static GAtChat *modem;static GMainLoop *event_loop; +static GAtPPP *ppp;enum state {STATE_NONE =0 ,
@@ -220,6 +227,69 @@g_at_chat_send (modem, buf, none_prefix,NULL ,NULL ,NULL ); } +static void print_ip_address (guint32 ip_addr) +{ +struct in_addr addr; + addr.s_addr = ip_addr; +g_print ("%s\n" ,inet_ntoa (addr)); +} + +static void ppp_connect (GAtPPP *ppp,GAtPPPConnectStatus success, +guint32 ip_addr,guint32 dns1,guint32 dns2, +gpointer user_data) +{ +if (success !=G_AT_PPP_CONNECT_SUCCESS ) { +g_print ("Failed to create PPP interface!\n" ); +return ; + } +/* print out the negotiated address and dns server */ +g_print ("Negotiated IP Address: " ); +print_ip_address (ip_addr); + +g_print ("Primary DNS Server: " ); +print_ip_address (dns1); + +g_print ("Secondary DNS Server: " ); +print_ip_address (dns2); +} + +static void ppp_disconnect (GAtPPP *ppp,gpointer user_data) +{ +g_print ("PPP Link down\n" ); +} + +static void connect_cb (gboolean ok,GAtResult *result,gpointer user_data) +{ +GIOChannel *channel; + +if (!ok) { +g_print ("Unable to define context\n" ); +exit (1 ); + } + +if (option_ppp == FALSE) +return ; + +/* get the data IO channel */ + channel =g_at_chat_get_channel (modem); + +/* open ppp */ + ppp =g_at_ppp_new (channel); +if (!ppp) { +g_print ("Unable to obtain PPP object\n" ); +return ; + } +g_at_ppp_set_credentials (ppp, option_username, + option_password); + +/* set connect and disconnect callbacks */ +g_at_ppp_set_connect_function (ppp, ppp_connect,NULL ); +g_at_ppp_set_disconnect_function (ppp, ppp_disconnect,NULL ); + +/* open the ppp connection */ +g_at_ppp_open (ppp); +} +static void at_cgdcont_cb (gboolean ok,GAtResult *result,gpointer user_data) {char buf[64 ];
@@ -230,9 +300,9 @@ }if (option_legacy == TRUE) { -sprintf (buf,"ATD*99***%u#" , option_cid); -g_at_chat_send (modem, buf, none_prefix, -NULL ,NULL ,NULL ); +sprintf (buf,"ATD*99***%u#" , option_cid); +g_at_chat_send (modem, buf, none_prefix, + connect_cb,NULL ,NULL ); }else {sprintf (buf,"AT+CGACT=1,%u" , option_cid); g_at_chat_send(control, buf, none_prefix,
@@ -406,6 +476,12 @@ "Specify CFUN offmode" }, {"legacy" ,'l' ,0 ,G_OPTION_ARG_NONE , &option_legacy, "Use ATD*99***<cid>#" }, + { "ppp", 'P', 0, G_OPTION_ARG_NONE, &option_ppp, + "Connect using PPP" }, + { "username", 'u', 0, G_OPTION_ARG_STRING, &option_username, + "Specify PPP username" }, + { "password", 'w', 0, G_OPTION_ARG_STRING, &option_password, + "Specifiy PPP password" }, { NULL }, };
Re: [patch 6/6] Allow gsmdial to use gatchat ppp support
Hi Kristen,
Quoted 4 lines
static GAtChat *control;
static GAtChat *modem;
static GMainLoop*event_loop;
+static GAtPPP *ppp;
I hate myself for this nitpick, but please group GAtPPP *ppp with the others before the mainloop variable.
gatchat/gsmdial.c line 235
Quoted 6 lines
gatchat/gsmdial.c +6
-0+static void print_ip_address (guint32 ip_addr) +{ +struct in_addr addr; + addr.s_addr = ip_addr; +g_print ("%s\n" ,inet_ntoa (addr)); +}
Please use print_ip_address(const char *label, guint32 ip_addr) as mentioned in the previous review.
gatchat/gsmdial.c line 281
Quoted 6 lines
gatchat/gsmdial.c +6
-0+/* open ppp */ + ppp =g_at_ppp_new (channel); +if (!ppp) { +g_print ("Unable to obtain PPP object\n" ); +return ; + }
I would call it "Unable to create PPP object", because you are creating. My understanding is the obtaining means that it would be there in the first place. It is a nitpick.
Regards
Marcel