PPP Patches v3

Implement new options for gsmdial to use PPP and set the user name and password for authentication if needed.

---
 gatchat/gsmdial.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 84 insertions(+), 3 deletions(-)

gatchat/gsmdial.c +33 -0

index 2087e70..aee9eea 100644
@@ -36,6 +36,7 @@
 #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 };

@@ -48,7 +49,11 @@ static gint option_cid = 0;
 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 GAtPPP *ppp;
 static GAtChat *control;
 static GAtChat *modem;
 static GMainLoop *event_loop;

@@ -223,6 +228,76 @@ static void at_cgact_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	g_at_chat_send(modem, buf, none_prefix, NULL, NULL, NULL);
 }
 
+static void print_ip_address(const char *label, guint32 ip_addr)
+{
+	struct in_addr addr;
+	char buf[INET_ADDRSTRLEN];
+
+	addr.s_addr = ip_addr;
+
+	if (inet_ntop(AF_INET, &addr, buf, INET_ADDRSTRLEN))
+		g_print("%s: %s\n", label, buf);
+}
+
+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 */
+	print_ip_address("IP Address", ip_addr);
+	print_ip_address("Primary DNS Server", dns1);
+	print_ip_address("Secondary DNS Server", dns2);
+}
+
+static void ppp_disconnect(GAtPPP *ppp, gpointer user_data)
+{

gatchat/gsmdial.c +41 -3

+	}
+
+	if (option_ppp == FALSE)
+		return;
+
+	/* get the data IO channel */
+	channel = g_at_chat_get_channel(modem);
+
+	/*
+	 * shutdown gatchat or else it tries to take all the input
+	 * from the modem and does not let PPP get it.
+	 */
+	g_at_chat_shutdown(control);
+	g_at_chat_shutdown(modem);
+
+	/* open ppp */
+	ppp = g_at_ppp_new(channel);
+	if (!ppp) {
+		g_print("Unable to create 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];

@@ -233,9 +308,9 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	}
 
 	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,

@@ -452,6 +527,12 @@ static GOptionEntry options[] = {
 				"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 },
 };
 

Signature · 6 lines

1.6.6.1

_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

gatchat/gsmdial.c static void at_cgact_up_cb(gboolean ok, GAtResult *result, gpointer user_data) · line 268

Quoted 7 lines

gatchat/gsmdial.c +7 -0

+static void connect_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	GIOChannel *channel;
+
+	if (!ok) {
+		g_print("Unable to define context\n");
+		exit(1);

I guess we should not call exit here. Just return.

Changes since v2 - corrected whitespace.

Quoted 1 line

Changes since v2 - corrected whitespace.

all the patches have been applied.

So form now on please send smaller patches as soon as you have them ready and tested. If you need comments or have questions, you can use the mailing list as well.

Hi Kristen,

Quoted 7 lines

Quoted 1 line

Changes since v2 - corrected whitespace.

all the patches have been applied.

So form now on please send smaller patches as soon as you have them ready and tested. If you need comments or have questions, you can use the mailing list as well.

some additional comments from my side.

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

Also please find access to a 32-bit system if you are running 64-bit or vice versa. Our build system is strict and will turn turn all casting mistakes in an error.

Please be present on #ofono IRC as much as possible so people can report such things. Remember that your code is now part of the tree and it can break the build.

Regards

Marcel

Quoted 19 lines

Quoted 7 lines

Quoted 1 line

Changes since v2 - corrected whitespace.

all the patches have been applied.

So form now on please send smaller patches as soon as you have them ready and tested. If you need comments or have questions, you can use the mailing list as well.

some additional comments from my side.

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

Also please find access to a 32-bit system if you are running 64-bit or vice versa. Our build system is strict and will turn turn all casting mistakes in an error.

Quoted 3 lines

Please be present on #ofono IRC as much as possible so people can report such things. Remember that your code is now part of the tree and it can break the build.

I just started reviewing the changes you made and the first patch I hit I discovered that you broke something. Would you like to revert the patches and let me review them first, or should I just send a patch to the mailing list to fix what you broke?

Hi Kristen,

Quoted 19 lines

Quoted 19 lines

Quoted 7 lines

Quoted 1 line

Changes since v2 - corrected whitespace.

all the patches have been applied.

So form now on please send smaller patches as soon as you have them ready and tested. If you need comments or have questions, you can use the mailing list as well.

some additional comments from my side.

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

Also please find access to a 32-bit system if you are running 64-bit or vice versa. Our build system is strict and will turn turn all casting mistakes in an error.

Quoted 8 lines

Quoted 3 lines

Please be present on #ofono IRC as much as possible so people can report such things. Remember that your code is now part of the tree and it can break the build.

I just started reviewing the changes you made and the first patch I hit I discovered that you broke something. Would you like to revert the patches and let me review them first, or should I just send a patch to the mailing list to fix what you broke?

send patches to the mailing list to fix them. The PPP code is in development so it is just fine. I prefer to keep reverts for code that is suppose to be stable.

If I did break something, then I am worried that the code gets too complex. Such little changes should not have broken anything. Except I made a stupid typo or thinking mistake (which happens of course). If it fundamentally changes the code flow, then we do have a bigger problem.

Regards

Marcel

Quoted 28 lines

Quoted 19 lines

Quoted 19 lines

Quoted 7 lines

Quoted 1 line

Changes since v2 - corrected whitespace.

all the patches have been applied.

So form now on please send smaller patches as soon as you have them ready and tested. If you need comments or have questions, you can use the mailing list as well.

some additional comments from my side.

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

Also please find access to a 32-bit system if you are running 64-bit or vice versa. Our build system is strict and will turn turn all casting mistakes in an error.

Quoted 8 lines

Quoted 3 lines

Please be present on #ofono IRC as much as possible so people can report such things. Remember that your code is now part of the tree and it can break the build.

I just started reviewing the changes you made and the first patch I hit I discovered that you broke something. Would you like to revert the patches and let me review them first, or should I just send a patch to the mailing list to fix what you broke?

Quoted 8 lines

send patches to the mailing list to fix them. The PPP code is in development so it is just fine. I prefer to keep reverts for code that is suppose to be stable.

If I did break something, then I am worried that the code gets too complex. Such little changes should not have broken anything. Except I made a stupid typo or thinking mistake (which happens of course). If it fundamentally changes the code flow, then we do have a bigger problem.

I wanted this code to at least be able to send a packet over the wire, and with your changes we can't do that anymore. The problem is that your little cosmetic change really did change the flow, simply because you didn't think through what you were doing. Of course it is up to you if you want to have this code be completely non-functional rather than simply doing the revert. I'll be glad to submit a patch later which fixes your fix and adds some comments so you know what is actually happening.

So Denis and I looked through my changes and he figured out what might have broken it. So your attempt at doing code flow optimization with g_list_length() throw me on the wrong error path. Please don't do this since all the GLib functions will do proper empty list checking.

Quoted 4 lines

So Denis and I looked through my changes and he figured out what might have broken it. So your attempt at doing code flow optimization with g_list_length() throw me on the wrong error path. Please don't do this since all the GLib functions will do proper empty list checking.

I reviewed your patch and it seems that you didn't break it this time :). If you ever have questions about the code, feel free to ask me.

please try to keep the code flow inside the functions as simple as possible. Otherwise things like this happen.

And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type.

Quoted 5 lines

And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type.

Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned.

Hi Kristen,

Quoted 6 lines

Quoted 5 lines

And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type.

Quoted 7 lines

Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned.

I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions.

Regards

Marcel

On Tue, 23 Mar 2010 10:56:00 -0700

Quoted 22 lines · Marcel Holtmann

Hi Kristen,

Quoted 6 lines

Quoted 5 lines

And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type.

Quoted 8 lines

Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned.

I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions.

Quoted 3 lines

Regards

Marcel

so is_proto_handler was casting a 16 bit value to an 8 bit value? Or are
you just complaining about having g_list_find_custom in the routines. It would really be easier on me to understand what you are trying to tell me if you you posted your patches to the list next time.

Quoted 14 lines

Quoted 6 lines

Quoted 5 lines

And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type.

Quoted 7 lines

Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned.

Quoted 4 lines

I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions.

so is_proto_handler was casting a 16 bit value to an 8 bit value? Or are you just complaining about having g_list_find_custom in the routines. It would really be easier on me to understand what you are trying to tell me if you you posted your patches to the list next time.

Quoted 27 lines · On Mon, 22 Mar 2010 19:22:00 -0700 Marcel Holtmann <[email protected]> wrote:

Hi Kristen,

Quoted 7 lines

Quoted 1 line

Changes since v2 - corrected whitespace.

all the patches have been applied.

So form now on please send smaller patches as soon as you have them ready and tested. If you need comments or have questions, you can use the mailing list as well.

some additional comments from my side.

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

Also please find access to a 32-bit system if you are running 64-bit or vice versa. Our build system is strict and will turn turn all casting mistakes in an error.

Please be present on #ofono IRC as much as possible so people can report such things. Remember that your code is now part of the tree and it can break the build.

Regards

Quoted 3 lines

Marcel

Marcel - I just started reviewing the changes you made and the first patch I hit I discovered that you broke something. Would you like to revert the patches and let me review them first, or should I just send a patch to the mailing list to fix what you broke?

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

Quoted 4 lines

If you have variables that are only valid inside the scope of a statement, then I prefer if you only declare them there. This makes the code a bit simpler to read. Check the patch I just pushed to give you an example.

If you're serious about that, you should turn on C99 support. Only then you can use C++-style for-loops.