lte-api: protocol and authentication properties

---
  src/lte.c | 242 +++++++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 186 insertions(+), 56 deletions(-)

src/lte.c +84 -11

index 23fe8e1c..2412fcec 100644
@@ -3,6 +3,7 @@
   *  oFono - Open Source Telephony
   *
   *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
   *
   *  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

@@ -39,7 +40,11 @@
 
  #define SETTINGS_STORE "lte"
  #define SETTINGS_GROUP "Settings"
-#define DEFAULT_APN_KEY "DefaultAccessPointName"
+#define LTE_APN "DefaultAccessPointName"
+#define LTE_PROTO "Protocol"
+#define LTE_USERNAME "Username"
+#define LTE_PASSWORD "Password"
+#define LTE_AUTH_METHOD "AuthenticationMethod"
 
  struct ofono_lte {
  	const struct ofono_lte_driver *driver;

@@ -57,6 +62,10 @@ static GSList *g_drivers = NULL;
  static void lte_load_settings(struct ofono_lte *lte)
  {
  	char *apn;
+	char *proto_str;
+	char *auth_method_str;
+	char *username;
+	char *password;
 
  	if (lte->imsi == NULL)
  		return;

@@ -69,19 +78,57 @@ static void lte_load_settings(struct ofono_lte *lte)
  		return;
  	}
 
-	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
-					DEFAULT_APN_KEY, NULL);
-	if (apn) {
+	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_APN, NULL);
+	proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_PROTO, NULL);
+	auth_method_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_AUTH_METHOD, NULL);
+	username = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_USERNAME, NULL);
+	password = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_PASSWORD, NULL);
+	if (apn)
  		strcpy(lte->info.apn, apn);
-		g_free(apn);
-	}
+
+	if (proto_str == NULL)
+		proto_str = g_strdup("ip");
+
+	/* this must have a valid default */
+	if (!gprs_proto_from_string(proto_str, &lte->info.proto))
+		lte->info.proto = OFONO_GPRS_PROTO_IP;
+
+	if (auth_method_str == NULL)
+		auth_method_str = g_strdup("none");
+
+	/* this must have a valid default */
+	if (!gprs_auth_method_from_string(auth_method_str,
+							&lte->info.auth_method))
+		lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+	if (username)
+		strcpy(lte->info.username, username);
+
+	if (password)
+		strcpy(lte->info.password, password);
+
+	g_free(apn);
+	g_free(proto_str);
+	g_free(auth_method_str);
+	g_free(username);
+	g_free(password);
  }
 
  static DBusMessage *lte_get_properties(DBusConnection *conn,
  					DBusMessage *msg, void *data)
  {
  	struct ofono_lte *lte = data;
+	const char *proto = gprs_proto_to_string(lte->info.proto);
  	const char *apn = lte->info.apn;
+	const char* auth_method =
+			gprs_auth_method_to_string(lte->info.auth_method);
+	const char *username = lte->info.username;
+	const char *password = lte->info.password;
  	DBusMessage *reply;
  	DBusMessageIter iter;
  	DBusMessageIter dict;

@@ -95,20 +142,28 @@ static DBusMessage *lte_get_properties(DBusConnection *conn,
  	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
  					OFONO_PROPERTIES_ARRAY_SIGNATURE,
  					&dict);
-	ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING, &apn);
+	ofono_dbus_dict_append(&dict, LTE_APN, DBUS_TYPE_STRING, &apn);
+	ofono_dbus_dict_append(&dict, LTE_PROTO, DBUS_TYPE_STRING, &proto);
+	ofono_dbus_dict_append(&dict, LTE_AUTH_METHOD, DBUS_TYPE_STRING,
+					&auth_method);
+	ofono_dbus_dict_append(&dict, LTE_USERNAME, DBUS_TYPE_STRING,
+					&username);
+	ofono_dbus_dict_append(&dict, LTE_PASSWORD, DBUS_TYPE_STRING,
+					&password);
  	dbus_message_iter_close_container(&iter, &dict);
 
  	return reply;
  }
 
  static void lte_set_default_attach_info_cb(const struct ofono_error *error,
-						void *data)
+								void *data)
  {
  	struct ofono_lte *lte = data;
  	const char *path = __ofono_atom_get_path(lte->atom);
  	DBusConnection *conn = ofono_dbus_get_connection();
  	DBusMessage *reply;
-	const char *apn = lte->info.apn;
+	const char *key;
+	const char *propstr = NULL;
 
  	DBG("%s error %d", path, error->type);
 

@@ -118,55 +173,64 @@ static void lte_set_default_attach_info_cb(const struct ofono_error *error,
  		return;
  	}
 
-	g_strlcpy(lte->info.apn, lte->pending_info.apn,
-			OFONO_GPRS_MAX_APN_LENGTH + 1);
+	if (!g_str_equal(lte->pending_info.apn, lte->info.apn)) {
+		g_strlcpy(lte->info.apn, lte->pending_info.apn,
+					OFONO_GPRS_MAX_APN_LENGTH + 1);
+		key = LTE_APN;
+		propstr = lte->info.apn;
+	} else if (lte->pending_info.proto != lte->info.proto) {
+		lte->info.proto = lte->pending_info.proto;
+		key = LTE_PROTO;
+		propstr = gprs_proto_to_string(lte->info.proto);
+	} else if (lte->pending_info.auth_method != lte->info.auth_method) {
+		lte->info.auth_method = lte->pending_info.auth_method;
+		key = LTE_AUTH_METHOD;
+		propstr = gprs_auth_method_to_string(lte->info.auth_method);
+
+	} else if (!g_str_equal(lte->pending_info.username,
+							lte->info.username)) {
+		g_strlcpy(lte->info.username, lte->pending_info.username,
+					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+		key = LTE_USERNAME;
+		propstr = lte->info.username;

src/lte.c +71 -45

+		 * Neverthelss, in this case it will answer the D-Bus message
+		 * but emit no signal
+		 */
+		key = NULL;
+	}
+
+	reply = dbus_message_new_method_return(lte->pending);
+	__ofono_dbus_pending_reply(&lte->pending, reply);
+
+	if(!key)
+		return;
 
  	if (lte->settings) {
-		if (strlen(lte->info.apn) == 0)
-			/* Clear entry on empty APN. */
-			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
-						DEFAULT_APN_KEY, NULL);
+		if (!*propstr)
+			/* Clear entry on empty string. */
+			g_key_file_remove_key(lte->settings,
+				SETTINGS_GROUP, key, NULL);
  		else
-			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
-						DEFAULT_APN_KEY, lte->info.apn);
+			g_key_file_set_string(lte->settings,
+				SETTINGS_GROUP, key, propstr);
 
  		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
  	}
 
-	reply = dbus_message_new_method_return(lte->pending);
-	__ofono_dbus_pending_reply(&lte->pending, reply);
-
  	ofono_dbus_signal_property_changed(conn, path,
  					OFONO_CONNECTION_CONTEXT_INTERFACE,
-					DEFAULT_APN_KEY,
-					DBUS_TYPE_STRING, &apn);
-}
-
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
-				DBusConnection *conn, DBusMessage *msg,
-				const char *apn)
-{
-	if (lte->driver->set_default_attach_info == NULL)
-		return __ofono_error_not_implemented(msg);
-
-	if (lte->pending)
-		return __ofono_error_busy(msg);
-
-	if (g_str_equal(apn, lte->info.apn))
-		return dbus_message_new_method_return(msg);
-
-	/* We do care about empty value: it can be used for reset. */
-	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
-		return __ofono_error_invalid_format(msg);
-
-	lte->pending = dbus_message_ref(msg);
-
-	g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
-
-	lte->driver->set_default_attach_info(lte, &lte->pending_info,
-					lte_set_default_attach_info_cb, lte);
-
-	return NULL;
+					key,
+					DBUS_TYPE_STRING, &propstr);
  }
 
  static DBusMessage *lte_set_property(DBusConnection *conn,

@@ -177,6 +241,14 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
  	DBusMessageIter var;
  	const char *property;
  	const char *str;
+	enum ofono_gprs_auth_method auth_method;
+	enum ofono_gprs_proto proto;
+
+	if (lte->driver->set_default_attach_info == NULL)
+		return __ofono_error_not_implemented(msg);
+
+	if (lte->pending)
+		return __ofono_error_busy(msg);
 
  	if (!dbus_message_iter_init(msg, &iter))
  		return __ofono_error_invalid_args(msg);

@@ -192,16 +264,64 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 
  	dbus_message_iter_recurse(&iter, &var);
 
-	if (!strcmp(property, DEFAULT_APN_KEY)) {
-		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
-			return __ofono_error_invalid_args(msg);
+	if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&var, &str);
 
-		dbus_message_iter_get_basic(&var, &str);
+	if ((strcmp(property, LTE_APN) == 0)) {
 
-		return lte_set_default_apn(lte, conn, msg, str);
-	}
+		if (g_str_equal(str, lte->info.apn))
+			return dbus_message_new_method_return(msg);
+
+		/* We do care about empty value: it can be used for reset. */
+		if (is_valid_apn(str) == FALSE && str[0] != '\0')
+			return __ofono_error_invalid_format(msg);
+
+		g_strlcpy(lte->pending_info.apn, str,
+					OFONO_GPRS_MAX_APN_LENGTH + 1);
+
+	} else if ((strcmp(property, LTE_PROTO) == 0)) {
+
+		if (!gprs_proto_from_string(str, &proto))
+			return __ofono_error_invalid_format(msg);
+
+		if (proto == lte->info.proto)
+			return dbus_message_new_method_return(msg);
+
+		lte->pending_info.proto = proto;
+
+	} else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
+
+		if (!gprs_auth_method_from_string(str, &auth_method))
+			return __ofono_error_invalid_format(msg);
+
+		if (proto == lte->info.proto)
+			return dbus_message_new_method_return(msg);
 
-	return __ofono_error_invalid_args(msg);
+	} else if (strcmp(property, LTE_USERNAME) == 0) {
+
+		if (g_str_equal(str, lte->info.username))
+			return dbus_message_new_method_return(msg);
+
+		g_strlcpy(lte->pending_info.username, str,
+					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+
+	} else if (strcmp(property, LTE_PASSWORD) == 0) {
+
+		if (g_str_equal(str, lte->info.password))
+			return dbus_message_new_method_return(msg);

src/lte.c +4 -0

+	return dbus_message_ref(msg);;
  }
 
  static const GDBusMethodTable lte_methods[] = {

@@ -374,6 +494,16 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
  	return lte->driver_data;
  }
 
+void ofono_lte_set_reg_info(struct ofono_modem *modem)
+{
+	struct ofono_lte *lte = __ofono_atom_find(OFONO_ATOM_TYPE_LTE, modem);

Hi,

Quoted 167 lines · Giacinto Cifelli

---
  src/lte.c | 242 +++++++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 186 insertions(+), 56 deletions(-)

src/lte.c +90 -11

index 23fe8e1c..2412fcec 100644
@@ -3,6 +3,7 @@
   *  oFono - Open Source Telephony
   *
   *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
   *
   *  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

@@ -39,7 +40,11 @@
 
  #define SETTINGS_STORE "lte"
  #define SETTINGS_GROUP "Settings"
-#define DEFAULT_APN_KEY "DefaultAccessPointName"
+#define LTE_APN "DefaultAccessPointName"
+#define LTE_PROTO "Protocol"
+#define LTE_USERNAME "Username"
+#define LTE_PASSWORD "Password"
+#define LTE_AUTH_METHOD "AuthenticationMethod"
 
  struct ofono_lte {
      const struct ofono_lte_driver *driver;

@@ -57,6 +62,10 @@ static GSList *g_drivers = NULL;
  static void lte_load_settings(struct ofono_lte *lte)
  {
      char *apn;
+     char *proto_str;
+     char *auth_method_str;
+     char *username;
+     char *password;
 
      if (lte->imsi == NULL)
              return;

@@ -69,19 +78,57 @@ static void lte_load_settings(struct ofono_lte *lte)
              return;
      }
 
-     apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
-                                     DEFAULT_APN_KEY, NULL);
-     if (apn) {
+     apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_APN, NULL);
+     proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_PROTO, NULL);
+     auth_method_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_AUTH_METHOD, NULL);
+     username = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_USERNAME, NULL);
+     password = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_PASSWORD, NULL);
+     if (apn)
              strcpy(lte->info.apn, apn);
-             g_free(apn);
-     }
+
+     if (proto_str == NULL)
+             proto_str = g_strdup("ip");
+
+     /* this must have a valid default */
+     if (!gprs_proto_from_string(proto_str, &lte->info.proto))
+             lte->info.proto = OFONO_GPRS_PROTO_IP;
+
+     if (auth_method_str == NULL)
+             auth_method_str = g_strdup("none");
+
+     /* this must have a valid default */
+     if (!gprs_auth_method_from_string(auth_method_str,
+                                                     &lte->info.auth_method))
+             lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+     if (username)
+             strcpy(lte->info.username, username);
+
+     if (password)
+             strcpy(lte->info.password, password);
+
+     g_free(apn);
+     g_free(proto_str);
+     g_free(auth_method_str);
+     g_free(username);
+     g_free(password);
  }
 
  static DBusMessage *lte_get_properties(DBusConnection *conn,
                                      DBusMessage *msg, void *data)
  {
      struct ofono_lte *lte = data;
+     const char *proto = gprs_proto_to_string(lte->info.proto);
      const char *apn = lte->info.apn;
+     const char* auth_method =
+                     gprs_auth_method_to_string(lte->info.auth_method);
+     const char *username = lte->info.username;
+     const char *password = lte->info.password;
      DBusMessage *reply;
      DBusMessageIter iter;
      DBusMessageIter dict;

@@ -95,20 +142,28 @@ static DBusMessage *lte_get_properties(DBusConnection *conn,
      dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
                                      OFONO_PROPERTIES_ARRAY_SIGNATURE,
                                      &dict);
-     ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING, &apn);
+     ofono_dbus_dict_append(&dict, LTE_APN, DBUS_TYPE_STRING, &apn);
+     ofono_dbus_dict_append(&dict, LTE_PROTO, DBUS_TYPE_STRING, &proto);
+     ofono_dbus_dict_append(&dict, LTE_AUTH_METHOD, DBUS_TYPE_STRING,
+                                     &auth_method);
+     ofono_dbus_dict_append(&dict, LTE_USERNAME, DBUS_TYPE_STRING,
+                                     &username);
+     ofono_dbus_dict_append(&dict, LTE_PASSWORD, DBUS_TYPE_STRING,
+                                     &password);
      dbus_message_iter_close_container(&iter, &dict);
 
      return reply;
  }
 
  static void lte_set_default_attach_info_cb(const struct ofono_error *error,
-                                             void *data)
+                                                             void *data)
  {
      struct ofono_lte *lte = data;
      const char *path = __ofono_atom_get_path(lte->atom);
      DBusConnection *conn = ofono_dbus_get_connection();
      DBusMessage *reply;
-     const char *apn = lte->info.apn;
+     const char *key;
+     const char *propstr = NULL;
 
      DBG("%s error %d", path, error->type);
 

@@ -118,55 +173,64 @@ static void lte_set_default_attach_info_cb(const struct ofono_error *error,
              return;
      }
 
-     g_strlcpy(lte->info.apn, lte->pending_info.apn,
-                     OFONO_GPRS_MAX_APN_LENGTH + 1);
+     if (!g_str_equal(lte->pending_info.apn, lte->info.apn)) {
+             g_strlcpy(lte->info.apn, lte->pending_info.apn,
+                                     OFONO_GPRS_MAX_APN_LENGTH + 1);
+             key = LTE_APN;
+             propstr = lte->info.apn;
+     } else if (lte->pending_info.proto != lte->info.proto) {
+             lte->info.proto = lte->pending_info.proto;
+             key = LTE_PROTO;
+             propstr = gprs_proto_to_string(lte->info.proto);
+     } else if (lte->pending_info.auth_method != lte->info.auth_method) {
+             lte->info.auth_method = lte->pending_info.auth_method;
+             key = LTE_AUTH_METHOD;
+             propstr = gprs_auth_method_to_string(lte->info.auth_method);
+
+     } else if (!g_str_equal(lte->pending_info.username,
+                                                     lte->info.username)) {
+             g_strlcpy(lte->info.username, lte->pending_info.username,
+                                     OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+             key = LTE_USERNAME;
+             propstr = lte->info.username;
+     } else if (!g_str_equal(lte->pending_info.password,
+                                                     lte->info.password)) {
+             g_strlcpy(lte->info.password, lte->pending_info.password,
+                                     OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+             key = LTE_PASSWORD;
+             propstr = lte->info.password;

src/lte.c static void lte_set_default_attach_info_cb(const struct ofono_error *error, · line 205

Quoted 4 lines

src/lte.c +4 -0

+     } else {
+             /*
+              * this should never happen, because no property change is
+              * checked before.

If this should never happen, handling it _gracefully_ doesn't make much sense. If this _can't_ happen, skip the check; if it _might but shouldn't_, log an error, at least.

Quoted 151 lines

src/lte.c +79 -45

+              * Neverthelss, in this case it will answer the D-Bus message
+              * but emit no signal
+              */
+             key = NULL;
+     }
+
+     reply = dbus_message_new_method_return(lte->pending);
+     __ofono_dbus_pending_reply(&lte->pending, reply);
+
+     if(!key)
+             return;
 
      if (lte->settings) {
-             if (strlen(lte->info.apn) == 0)
-                     /* Clear entry on empty APN. */
-                     g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
-                                             DEFAULT_APN_KEY, NULL);
+             if (!*propstr)
+                     /* Clear entry on empty string. */
+                     g_key_file_remove_key(lte->settings,
+                             SETTINGS_GROUP, key, NULL);
              else
-                     g_key_file_set_string(lte->settings, SETTINGS_GROUP,
-                                             DEFAULT_APN_KEY, lte->info.apn);
+                     g_key_file_set_string(lte->settings,
+                             SETTINGS_GROUP, key, propstr);
 
              storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
      }
 
-     reply = dbus_message_new_method_return(lte->pending);
-     __ofono_dbus_pending_reply(&lte->pending, reply);
-
      ofono_dbus_signal_property_changed(conn, path,
                                      OFONO_CONNECTION_CONTEXT_INTERFACE,
-                                     DEFAULT_APN_KEY,
-                                     DBUS_TYPE_STRING, &apn);
-}
-
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
-                             DBusConnection *conn, DBusMessage *msg,
-                             const char *apn)
-{
-     if (lte->driver->set_default_attach_info == NULL)
-             return __ofono_error_not_implemented(msg);
-
-     if (lte->pending)
-             return __ofono_error_busy(msg);
-
-     if (g_str_equal(apn, lte->info.apn))
-             return dbus_message_new_method_return(msg);
-
-     /* We do care about empty value: it can be used for reset. */
-     if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
-             return __ofono_error_invalid_format(msg);
-
-     lte->pending = dbus_message_ref(msg);
-
-     g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
-
-     lte->driver->set_default_attach_info(lte, &lte->pending_info,
-                                     lte_set_default_attach_info_cb, lte);
-
-     return NULL;
+                                     key,
+                                     DBUS_TYPE_STRING, &propstr);
  }
 
  static DBusMessage *lte_set_property(DBusConnection *conn,

@@ -177,6 +241,14 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
      DBusMessageIter var;
      const char *property;
      const char *str;
+     enum ofono_gprs_auth_method auth_method;
+     enum ofono_gprs_proto proto;
+
+     if (lte->driver->set_default_attach_info == NULL)
+             return __ofono_error_not_implemented(msg);
+
+     if (lte->pending)
+             return __ofono_error_busy(msg);
 
      if (!dbus_message_iter_init(msg, &iter))
              return __ofono_error_invalid_args(msg);

@@ -192,16 +264,64 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 
      dbus_message_iter_recurse(&iter, &var);
 
-     if (!strcmp(property, DEFAULT_APN_KEY)) {
-             if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
-                     return __ofono_error_invalid_args(msg);
+     if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+             return __ofono_error_invalid_args(msg);
+
+     dbus_message_iter_get_basic(&var, &str);
 
-             dbus_message_iter_get_basic(&var, &str);
+     if ((strcmp(property, LTE_APN) == 0)) {
 
-             return lte_set_default_apn(lte, conn, msg, str);
-     }
+             if (g_str_equal(str, lte->info.apn))
+                     return dbus_message_new_method_return(msg);
+
+             /* We do care about empty value: it can be used for reset. */
+             if (is_valid_apn(str) == FALSE && str[0] != '\0')
+                     return __ofono_error_invalid_format(msg);
+
+             g_strlcpy(lte->pending_info.apn, str,
+                                     OFONO_GPRS_MAX_APN_LENGTH + 1);
+
+     } else if ((strcmp(property, LTE_PROTO) == 0)) {
+
+             if (!gprs_proto_from_string(str, &proto))
+                     return __ofono_error_invalid_format(msg);
+
+             if (proto == lte->info.proto)
+                     return dbus_message_new_method_return(msg);
+
+             lte->pending_info.proto = proto;
+
+     } else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
+
+             if (!gprs_auth_method_from_string(str, &auth_method))
+                     return __ofono_error_invalid_format(msg);
+
+             if (proto == lte->info.proto)
+                     return dbus_message_new_method_return(msg);
 
-     return __ofono_error_invalid_args(msg);
+     } else if (strcmp(property, LTE_USERNAME) == 0) {
+
+             if (g_str_equal(str, lte->info.username))
+                     return dbus_message_new_method_return(msg);
+
+             g_strlcpy(lte->pending_info.username, str,
+                                     OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+
+     } else if (strcmp(property, LTE_PASSWORD) == 0) {
+
+             if (g_str_equal(str, lte->info.password))
+                     return dbus_message_new_method_return(msg);
+
+             g_strlcpy(lte->pending_info.password, str,
+                                     OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+
+     } else
+             return __ofono_error_invalid_args(msg);
+
+     lte->pending = dbus_message_ref(msg);

src/lte.c static DBusMessage *lte_set_property(DBusConnection *conn, · line 323

Quoted 2 lines

src/lte.c +2 -0

+     lte->driver->set_default_attach_info(lte, &lte->pending_info,
+                                     lte_set_default_attach_info_cb, lte);

Note here that you've short-circuited the atmodem implementation (in patch 5/6) of this function so that the callback gets invoked immediately instead of on a subsequent mainloop iteration. That often leads to surprising results... not sure if that's the case here, but the dbus_message_ref that follows looks suspicious.

Quoted 9 lines

src/lte.c +1 -0

+     return dbus_message_ref(msg);;
  }
 
  static const GDBusMethodTable lte_methods[] = {

@@ -374,6 +494,16 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
      return lte->driver_data;
  }
 

src/lte.c void *ofono_lte_get_data(const struct ofono_lte *lte) · line 509

Quoted 13 lines

src/lte.c +10 -0

+void ofono_lte_set_reg_info(struct ofono_modem *modem)
+{
+     struct ofono_lte *lte = __ofono_atom_find(OFONO_ATOM_TYPE_LTE, modem);
+
+     if(!lte || !lte->driver || !lte->driver->set_reg_info)
+             return;
+
+     lte->driver->set_reg_info(lte, &lte->info);
+}
+
  struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
  {
      return __ofono_atom_get_modem(lte->atom);

/Jonas

src/lte.c +3 -3

@@ -69,19 +78,57 @@ static void lte_load_settings(struct ofono_lte *lte)
  		return;
  	}
 
-	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
-					DEFAULT_APN_KEY, NULL);
-	if (apn) {
+	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_APN, NULL);
+	proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,

src/lte.c +12 -1

-	}
+
+	if (proto_str == NULL)
+		proto_str = g_strdup("ip");
+
+	/* this must have a valid default */
+	if (!gprs_proto_from_string(proto_str, &lte->info.proto))
+		lte->info.proto = OFONO_GPRS_PROTO_IP;
+
+	if (auth_method_str == NULL)
+		auth_method_str = g_strdup("none");
+
+	/* this must have a valid default */

src/lte.c +5 -0

+	g_free(apn);
+	g_free(proto_str);
+	g_free(auth_method_str);
+	g_free(username);
+	g_free(password);
  }
 

static void lte_set_default_attach_info_cb(const struct ofono_error *error,

src/lte.c +28 -4

-						void *data)
+								void *data)
  {
  	struct ofono_lte *lte = data;
  	const char *path = __ofono_atom_get_path(lte->atom);
  	DBusConnection *conn = ofono_dbus_get_connection();
  	DBusMessage *reply;
-	const char *apn = lte->info.apn;
+	const char *key;
+	const char *propstr = NULL;
 
  	DBG("%s error %d", path, error->type);

@@ -118,55 +173,64 @@ static void lte_set_default_attach_info_cb(const struct ofono_error *error,
  		return;
  	}
 
-	g_strlcpy(lte->info.apn, lte->pending_info.apn,
-			OFONO_GPRS_MAX_APN_LENGTH + 1);
+	if (!g_str_equal(lte->pending_info.apn, lte->info.apn)) {
+		g_strlcpy(lte->info.apn, lte->pending_info.apn,
+					OFONO_GPRS_MAX_APN_LENGTH + 1);
+		key = LTE_APN;
+		propstr = lte->info.apn;
+	} else if (lte->pending_info.proto != lte->info.proto) {
+		lte->info.proto = lte->pending_info.proto;
+		key = LTE_PROTO;
+		propstr = gprs_proto_to_string(lte->info.proto);
+	} else if (lte->pending_info.auth_method != lte->info.auth_method) {
+		lte->info.auth_method = lte->pending_info.auth_method;
+		key = LTE_AUTH_METHOD;
+		propstr = gprs_auth_method_to_string(lte->info.auth_method);
+
+	} else if (!g_str_equal(lte->pending_info.username,
+							lte->info.username)) {
+		g_strlcpy(lte->info.username, lte->pending_info.username,
+					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+		key = LTE_USERNAME;
+		propstr = lte->info.username;
+	} else if (!g_str_equal(lte->pending_info.password,
+							lte->info.password)) {
+		g_strlcpy(lte->info.password, lte->pending_info.password,
+					OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+		key = LTE_PASSWORD;

src/lte.c +8 -4

+
+	reply = dbus_message_new_method_return(lte->pending);
+	__ofono_dbus_pending_reply(&lte->pending, reply);
+
+	if(!key)
+		return;
 
  	if (lte->settings) {
-		if (strlen(lte->info.apn) == 0)
-			/* Clear entry on empty APN. */
-			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
-						DEFAULT_APN_KEY, NULL);
+		if (!*propstr)
+			/* Clear entry on empty string. */

src/lte.c +32 -38

-	reply = dbus_message_new_method_return(lte->pending);
-	__ofono_dbus_pending_reply(&lte->pending, reply);
-
  	ofono_dbus_signal_property_changed(conn, path,
  					OFONO_CONNECTION_CONTEXT_INTERFACE,
-					DEFAULT_APN_KEY,
-					DBUS_TYPE_STRING, &apn);
-}
-
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
-				DBusConnection *conn, DBusMessage *msg,
-				const char *apn)
-{
-	if (lte->driver->set_default_attach_info == NULL)
-		return __ofono_error_not_implemented(msg);
-
-	if (lte->pending)
-		return __ofono_error_busy(msg);
-
-	if (g_str_equal(apn, lte->info.apn))
-		return dbus_message_new_method_return(msg);
-
-	/* We do care about empty value: it can be used for reset. */
-	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
-		return __ofono_error_invalid_format(msg);
-
-	lte->pending = dbus_message_ref(msg);
-
-	g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
-
-	lte->driver->set_default_attach_info(lte, &lte->pending_info,
-					lte_set_default_attach_info_cb, lte);
-
-	return NULL;
+					key,
+					DBUS_TYPE_STRING, &propstr);
  }
 
  static DBusMessage *lte_set_property(DBusConnection *conn,

@@ -177,6 +241,14 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
  	DBusMessageIter var;
  	const char *property;
  	const char *str;
+	enum ofono_gprs_auth_method auth_method;
+	enum ofono_gprs_proto proto;
+
+	if (lte->driver->set_default_attach_info == NULL)
+		return __ofono_error_not_implemented(msg);
+
+	if (lte->pending)
+		return __ofono_error_busy(msg);
 
  	if (!dbus_message_iter_init(msg, &iter))
  		return __ofono_error_invalid_args(msg);

@@ -192,16 +264,64 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 
  	dbus_message_iter_recurse(&iter, &var);
 
-	if (!strcmp(property, DEFAULT_APN_KEY)) {
-		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
-			return __ofono_error_invalid_args(msg);
+	if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&var, &str);
 
-		dbus_message_iter_get_basic(&var, &str);
+	if ((strcmp(property, LTE_APN) == 0)) {
 
-		return lte_set_default_apn(lte, conn, msg, str);
-	}
+		if (g_str_equal(str, lte->info.apn))
+			return dbus_message_new_method_return(msg);
+
+		/* We do care about empty value: it can be used for reset. */
+		if (is_valid_apn(str) == FALSE && str[0] != '\0')
+			return __ofono_error_invalid_format(msg);
+
+		g_strlcpy(lte->pending_info.apn, str,
+					OFONO_GPRS_MAX_APN_LENGTH + 1);
+
+	} else if ((strcmp(property, LTE_PROTO) == 0)) {
+
+		if (!gprs_proto_from_string(str, &proto))
+			return __ofono_error_invalid_format(msg);
+
+		if (proto == lte->info.proto)
+			return dbus_message_new_method_return(msg);

src/lte.c +13 -1

-	return __ofono_error_invalid_args(msg);
+	} else if (strcmp(property, LTE_USERNAME) == 0) {
+
+		if (g_str_equal(str, lte->info.username))
+			return dbus_message_new_method_return(msg);
+
+		g_strlcpy(lte->pending_info.username, str,
+					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+
+	} else if (strcmp(property, LTE_PASSWORD) == 0) {
+
+		if (g_str_equal(str, lte->info.password))
+			return dbus_message_new_method_return(msg);
+

src/lte.c +0 -0

  }
 
  static const GDBusMethodTable lte_methods[] = {

@@ -374,6 +494,16 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
  	return lte->driver_data;
  }
 

Hi Giacinto,

Quoted 15 lines

src/lte.c +8 -3

@@ -69,19 +78,57 @@ static void lte_load_settings(struct ofono_lte *lte)
              return;
      }
 
-     apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
-                                     DEFAULT_APN_KEY, NULL);
-     if (apn) {
+     apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_APN, NULL);
+     proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_PROTO, NULL);
+     auth_method_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_AUTH_METHOD, NULL);
+     username = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_USERNAME, NULL);

src/lte.c static void lte_load_settings(struct ofono_lte *lte) · line 76

Quoted 5 lines

src/lte.c +3 -1

+     password = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+                             LTE_PASSWORD, NULL);
+     if (apn)
              strcpy(lte->info.apn, apn);
-             g_free(apn);

So we may want to be more paranoid here, similar to how load_context in gprs.c works.

Quoted 14 lines

src/lte.c +12 -1

-     }
+
+     if (proto_str == NULL)
+             proto_str = g_strdup("ip");
+
+     /* this must have a valid default */
+     if (!gprs_proto_from_string(proto_str, &lte->info.proto))
+             lte->info.proto = OFONO_GPRS_PROTO_IP;
+
+     if (auth_method_str == NULL)
+             auth_method_str = g_strdup("none");
+
+     /* this must have a valid default */

src/lte.c static void lte_load_settings(struct ofono_lte *lte) · line 114

Quoted 10 lines

src/lte.c +10 -0

+     if (!gprs_auth_method_from_string(auth_method_str,
+                                                     &lte->info.auth_method))
+             lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+     if (username)
+             strcpy(lte->info.username, username);
+
+     if (password)
+             strcpy(lte->info.password, password);
+

Again, might want to be more paranoid here and ensure username / password don't overflow buffers.

Quoted 7 lines

src/lte.c +5 -0

+     g_free(apn);
+     g_free(proto_str);
+     g_free(auth_method_str);
+     g_free(username);
+     g_free(password);
  }
 

<snip>

Quoted 52 lines

static void lte_set_default_attach_info_cb(const struct ofono_error *error,

src/lte.c +35 -4

-                                             void *data)
+                                                             void *data)
  {
      struct ofono_lte *lte = data;
      const char *path = __ofono_atom_get_path(lte->atom);
      DBusConnection *conn = ofono_dbus_get_connection();
      DBusMessage *reply;
-     const char *apn = lte->info.apn;
+     const char *key;
+     const char *propstr = NULL;
 
      DBG("%s error %d", path, error->type);

@@ -118,55 +173,64 @@ static void lte_set_default_attach_info_cb(const struct ofono_error *error,
              return;
      }
 
-     g_strlcpy(lte->info.apn, lte->pending_info.apn,
-                     OFONO_GPRS_MAX_APN_LENGTH + 1);
+     if (!g_str_equal(lte->pending_info.apn, lte->info.apn)) {
+             g_strlcpy(lte->info.apn, lte->pending_info.apn,
+                                     OFONO_GPRS_MAX_APN_LENGTH + 1);
+             key = LTE_APN;
+             propstr = lte->info.apn;
+     } else if (lte->pending_info.proto != lte->info.proto) {
+             lte->info.proto = lte->pending_info.proto;
+             key = LTE_PROTO;
+             propstr = gprs_proto_to_string(lte->info.proto);
+     } else if (lte->pending_info.auth_method != lte->info.auth_method) {
+             lte->info.auth_method = lte->pending_info.auth_method;
+             key = LTE_AUTH_METHOD;
+             propstr = gprs_auth_method_to_string(lte->info.auth_method);
+
+     } else if (!g_str_equal(lte->pending_info.username,
+                                                     lte->info.username)) {
+             g_strlcpy(lte->info.username, lte->pending_info.username,
+                                     OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+             key = LTE_USERNAME;
+             propstr = lte->info.username;
+     } else if (!g_str_equal(lte->pending_info.password,
+                                                     lte->info.password)) {
+             g_strlcpy(lte->info.password, lte->pending_info.password,
+                                     OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+             key = LTE_PASSWORD;
+             propstr = lte->info.password;
+     } else {
+             /*
+              * this should never happen, because no property change is
+              * checked before.
+              * Neverthelss, in this case it will answer the D-Bus message
+              * but emit no signal

src/lte.c static void lte_set_default_attach_info_cb(const struct ofono_error *error, · line 210

Quoted 3 lines

src/lte.c +3 -0

+              */
+             key = NULL;
+     }

Ugh. I'm not really liking this at all. The property name and the new value are already available inside the dbus_message object (e.g. lte->pending). There's nothing wrong with parsing that message again or simply store pointers to the data inside the dbus-message.

Quoted 22 lines

src/lte.c +12 -6

+
+     reply = dbus_message_new_method_return(lte->pending);
+     __ofono_dbus_pending_reply(&lte->pending, reply);
+
+     if(!key)
+             return;
 
      if (lte->settings) {
-             if (strlen(lte->info.apn) == 0)
-                     /* Clear entry on empty APN. */
-                     g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
-                                             DEFAULT_APN_KEY, NULL);
+             if (!*propstr)
+                     /* Clear entry on empty string. */
+                     g_key_file_remove_key(lte->settings,
+                             SETTINGS_GROUP, key, NULL);
              else
-                     g_key_file_set_string(lte->settings, SETTINGS_GROUP,
-                                             DEFAULT_APN_KEY, lte->info.apn);
+                     g_key_file_set_string(lte->settings,
+                             SETTINGS_GROUP, key, propstr);
 

src/lte.c static void lte_set_default_attach_info_cb(const struct ofono_error *error, · line 228

Quoted 2 lines

src/lte.c +0 -0

              storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
      }

I can see this applying for APN and maybe Username/Password. But not the other settings...?

Quoted 94 lines

src/lte.c +36 -38

-     reply = dbus_message_new_method_return(lte->pending);
-     __ofono_dbus_pending_reply(&lte->pending, reply);
-
      ofono_dbus_signal_property_changed(conn, path,
                                      OFONO_CONNECTION_CONTEXT_INTERFACE,
-                                     DEFAULT_APN_KEY,
-                                     DBUS_TYPE_STRING, &apn);
-}
-
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
-                             DBusConnection *conn, DBusMessage *msg,
-                             const char *apn)
-{
-     if (lte->driver->set_default_attach_info == NULL)
-             return __ofono_error_not_implemented(msg);
-
-     if (lte->pending)
-             return __ofono_error_busy(msg);
-
-     if (g_str_equal(apn, lte->info.apn))
-             return dbus_message_new_method_return(msg);
-
-     /* We do care about empty value: it can be used for reset. */
-     if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
-             return __ofono_error_invalid_format(msg);
-
-     lte->pending = dbus_message_ref(msg);
-
-     g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
-
-     lte->driver->set_default_attach_info(lte, &lte->pending_info,
-                                     lte_set_default_attach_info_cb, lte);
-
-     return NULL;
+                                     key,
+                                     DBUS_TYPE_STRING, &propstr);
  }
 
  static DBusMessage *lte_set_property(DBusConnection *conn,

@@ -177,6 +241,14 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
      DBusMessageIter var;
      const char *property;
      const char *str;
+     enum ofono_gprs_auth_method auth_method;
+     enum ofono_gprs_proto proto;
+
+     if (lte->driver->set_default_attach_info == NULL)
+             return __ofono_error_not_implemented(msg);
+
+     if (lte->pending)
+             return __ofono_error_busy(msg);
 
      if (!dbus_message_iter_init(msg, &iter))
              return __ofono_error_invalid_args(msg);

@@ -192,16 +264,64 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 
      dbus_message_iter_recurse(&iter, &var);
 
-     if (!strcmp(property, DEFAULT_APN_KEY)) {
-             if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
-                     return __ofono_error_invalid_args(msg);
+     if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+             return __ofono_error_invalid_args(msg);
+
+     dbus_message_iter_get_basic(&var, &str);
 
-             dbus_message_iter_get_basic(&var, &str);
+     if ((strcmp(property, LTE_APN) == 0)) {
 
-             return lte_set_default_apn(lte, conn, msg, str);
-     }
+             if (g_str_equal(str, lte->info.apn))
+                     return dbus_message_new_method_return(msg);
+
+             /* We do care about empty value: it can be used for reset. */
+             if (is_valid_apn(str) == FALSE && str[0] != '\0')
+                     return __ofono_error_invalid_format(msg);
+
+             g_strlcpy(lte->pending_info.apn, str,
+                                     OFONO_GPRS_MAX_APN_LENGTH + 1);
+
+     } else if ((strcmp(property, LTE_PROTO) == 0)) {
+
+             if (!gprs_proto_from_string(str, &proto))
+                     return __ofono_error_invalid_format(msg);
+
+             if (proto == lte->info.proto)
+                     return dbus_message_new_method_return(msg);
+
+             lte->pending_info.proto = proto;
+
+     } else if (strcmp(property, LTE_AUTH_METHOD) == 0) {

src/lte.c static DBusMessage *lte_set_property(DBusConnection *conn, · line 300

Quoted 6 lines

src/lte.c +6 -0

+
+             if (!gprs_auth_method_from_string(str, &auth_method))
+                     return __ofono_error_invalid_format(msg);
+
+             if (proto == lte->info.proto)
+                     return dbus_message_new_method_return(msg);

Umm, method? How well tested is this submission?

Quoted 21 lines

src/lte.c +18 -1

-     return __ofono_error_invalid_args(msg);
+     } else if (strcmp(property, LTE_USERNAME) == 0) {
+
+             if (g_str_equal(str, lte->info.username))
+                     return dbus_message_new_method_return(msg);
+
+             g_strlcpy(lte->pending_info.username, str,
+                                     OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+
+     } else if (strcmp(property, LTE_PASSWORD) == 0) {
+
+             if (g_str_equal(str, lte->info.password))
+                     return dbus_message_new_method_return(msg);
+
+             g_strlcpy(lte->pending_info.password, str,
+                                     OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+
+     } else
+             return __ofono_error_invalid_args(msg);

src/lte.c static DBusMessage *lte_set_property(DBusConnection *conn, · line 324

Quoted 5 lines

src/lte.c +5 -0

+
+     lte->pending = dbus_message_ref(msg);
+     lte->driver->set_default_attach_info(lte, &lte->pending_info,
+                                     lte_set_default_attach_info_cb, lte);
+     return dbus_message_ref(msg);;

Your reference counting is completely wrong here. You might want to run valgrind and do some testing...

Quoted 8 lines

src/lte.c +0 -0

  }
 
  static const GDBusMethodTable lte_methods[] = {

@@ -374,6 +494,16 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
      return lte->driver_data;
  }
 

src/lte.c void *ofono_lte_get_data(const struct ofono_lte *lte) · line 506

Quoted 10 lines

src/lte.c +10 -0

+void ofono_lte_set_reg_info(struct ofono_modem *modem)
+{
+     struct ofono_lte *lte = __ofono_atom_find(OFONO_ATOM_TYPE_LTE, modem);
+
+     if(!lte || !lte->driver || !lte->driver->set_reg_info)
+             return;
+
+     lte->driver->set_reg_info(lte, &lte->info);
+}
+

Given that you have somewhat sane defaults for everything, I'm not convinced now that this is even needed...

Regards,
-Denis

Quoted 5 lines

Ugh. I'm not really liking this at all. The property name and the new value are already available inside the dbus_message object (e.g. lte->pending). There's nothing wrong with parsing that message again or simply store pointers to the data inside the dbus-message.

ah no! this is the famous "it's initialized to NULL when the structure is created", do you remember it?

The pointer to the dbus message would go out of scope once the dbus message is answered, which happens before the signal is emitted.

Quoted 13 lines

Quoted 12 lines

src/lte.c +6 -2

+             if (!*propstr)
+                     /* Clear entry on empty string. */
+                     g_key_file_remove_key(lte->settings,
+                             SETTINGS_GROUP, key, NULL);
               else
-                     g_key_file_set_string(lte->settings, SETTINGS_GROUP,
-                                             DEFAULT_APN_KEY, lte->info.apn);
+                     g_key_file_set_string(lte->settings,
+                             SETTINGS_GROUP, key, propstr);
 
               storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
       }

Quoted 2 lines

I can see this applying for APN and maybe Username/Password. But not the other settings...?

I don't get this sorry. which part is applicable only to apn/user/pwd? the propstr is never null for protocol and auth_method, due to the close-set enumeration.

---
  src/modem.c | 2 ++
  1 file changed, 2 insertions(+)

src/modem.c +0 -0

index 9e254482..74dbe7ad 100644

src/modem.c +0 -0

  		/* Modem is always online, proceed to online state. */
  		if (modem_is_always_online(modem) == TRUE)
  			set_online(modem, TRUE);

Quoted 9 lines · Giacinto Cifelli

---
  src/modem.c | 2 ++
  1 file changed, 2 insertions(+)

src/modem.c +0 -0

index 9e254482..74dbe7ad 100644

src/modem.c static void sim_state_watch(enum ofono_sim_state new_state, void *user) · line 733

Quoted 5 lines

src/modem.c +2 -0

@@ -729,6 +729,8 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
      case OFONO_SIM_STATE_READY:
              modem_change_state(modem, MODEM_STATE_OFFLINE);
 
+             ofono_lte_set_reg_info(modem);
+

modem_change_state(...OFFLINE) results in the post_sim() implementation being called. The drivers that implement the lte atom all call ofono_lte_create() there.

Your .._set_reg_info function checks for the existence of the atom and does some work. As such, why not just merge the contents of ofono_lte_set_reg_info() into the at_lte_probe function instead... from there, it's more obvious what's going on.

/Jonas

Quoted 3 lines

src/modem.c +0 -0

              /* Modem is always online, proceed to online state. */
              if (modem_is_always_online(modem) == TRUE)
                      set_online(modem, TRUE);

Hi,

Quoted 21 lines · Jonas Bonn

Quoted 9 lines · Giacinto Cifelli

---
   src/modem.c | 2 ++
   1 file changed, 2 insertions(+)

src/modem.c +0 -0

index 9e254482..74dbe7ad 100644

src/modem.c static void sim_state_watch(enum ofono_sim_state new_state, void *user) · line 733

Quoted 5 lines

src/modem.c +2 -0

@@ -729,6 +729,8 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
       case OFONO_SIM_STATE_READY:
               modem_change_state(modem, MODEM_STATE_OFFLINE);
 
+             ofono_lte_set_reg_info(modem);
+

modem_change_state(...OFFLINE) results in the post_sim() implementation being called. The drivers that implement the lte atom all call ofono_lte_create() there.

Quoted 4 lines

Your .._set_reg_info function checks for the existence of the atom and does some work. As such, why not just merge the contents of ofono_lte_set_reg_info() into the at_lte_probe function instead... from there, it's more obvious what's going on.

The point is that the function must be called before set_online (following), and not after post_sim, even if they happen to be together.

This is how it works for gprs-context: before activating the context, apn and auth are set.

It isn't so obvious to me that the probe function does the work. It is in general the first one to be called, then there is a chance to set the properties one by one, and eventually they are transferred to the modem.

Quoted 24 lines · Giacinto Cifelli

Hi,

Quoted 21 lines · Jonas Bonn

Quoted 9 lines · Giacinto Cifelli

---
   src/modem.c | 2 ++
   1 file changed, 2 insertions(+)

src/modem.c +0 -0

index 9e254482..74dbe7ad 100644

src/modem.c static void sim_state_watch(enum ofono_sim_state new_state, void *user) · line 733

Quoted 5 lines

src/modem.c +2 -0

@@ -729,6 +729,8 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
       case OFONO_SIM_STATE_READY:
               modem_change_state(modem, MODEM_STATE_OFFLINE);
 
+             ofono_lte_set_reg_info(modem);
+

modem_change_state(...OFFLINE) results in the post_sim() implementation being called. The drivers that implement the lte atom all call ofono_lte_create() there.

Quoted 7 lines

Quoted 4 lines

Your .._set_reg_info function checks for the existence of the atom and does some work. As such, why not just merge the contents of ofono_lte_set_reg_info() into the at_lte_probe function instead... from there, it's more obvious what's going on.

The point is that the function must be called before set_online (following), and not after post_sim, even if they happen to be together.

set_online only happens in the following line if the modem is "always online", which is generally not the case. For other modems, set_online happens elsewhere.

Quoted 7 lines

This is how it works for gprs-context: before activating the context, apn and auth are set.

It isn't so obvious to me that the probe function does the work. It is in general the first one to be called, then there is a chance to set the properties one by one, and eventually they are transferred to the modem.

Between "enable" and "online", you have a window to change the settings.

For modems that are "always online", you pretty much have to go "enabled/online", perhaps failing to do so(?), change the settings, "disable" and "reenable" the modem. Ugly, yes, but that's the way things look right now.

Twiddling the LTE settings just results in context reconfiguration (CGDCONT)... this, presumably, does not have any effect on any already established context... correct me if I'm wrong. As such, the changes don't really take effect until you've gone offline/online again, anyway.

/Jonas

drivers/atmodem/lte.c +0 -1

---
  AUTHORS | 3 +++
  1 file changed, 3 insertions(+)

AUTHORS +0 -0

index 2d360e6e..8f0f5893 100644

*
* 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

AUTHORS +12 -6

@@ -45,48 +46,67 @@ struct lte_driver_data {
  	GAtChat *chat;
  };
 
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
-							gpointer user_data)
+static void at_set_reg_info(const struct ofono_lte *lte,
+			const struct ofono_lte_default_attach_info *info)
  {
-	struct cb_data *cbd = user_data;
-	ofono_lte_cb_t cb = cbd->cb;
-	struct ofono_error error;
+	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+	char buf_cgdcont[32 + OFONO_GPRS_MAX_APN_LENGTH  +1];
+	char buf_cgauth[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+					OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
+	guint buflen = sizeof(buf_cgauth);
+	enum ofono_gprs_auth_method auth_method;
 
-	DBG("ok %d", ok);
+	if (strlen(info->apn) > 0)
+		snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+				"AT+CGDCONT=0,\"IP\",\"%s\"", info->apn);
+	else

AUTHORS +4 -0

+				buflen, "1,\"%s\",\"%s\"",
+				info->username, info->password);
+		break;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:

AUTHORS +0 -11

  }
 
  static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
  			const struct ofono_lte_default_attach_info *info,
  			ofono_lte_cb_t cb, void *data)
  {
-	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
-	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
-	struct cb_data *cbd = cb_data_new(cb, data);
-
-	DBG("LTE config with APN: %s", info->apn);
-
-	if (strlen(info->apn) > 0)
-		snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
-				info->apn);
-	else
-		snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");

Hi,

I think this patch deserves a couple of lines of justification in the patch description. From the subject, it isn't obvious what this is about, and a quick glance at the diff only raises further questions. Try to explain in the patch description what this is about, what problem your modification is solving.

---
 drivers/atmodem/lte.c | 79 +++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 29 deletions(-)

drivers/atmodem/lte.c +1 -0

index efa4e5fe..1d097c68 100644
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
 
 

Why is copyright attributed to Gemalto when you and the other purported authors (referencing your AUTHORS patch... see below) all use gmail addresses?

---
 AUTHORS | 3 +++
 1 file changed, 3 insertions(+)

AUTHORS +3 -0

index 2d360e6e..8f0f5893 100644
@@ -138,3 +138,6 @@ Florent Beillonnet <[email protected]>
 Martin Hundebøll <[email protected]>
 Julien Tournier <[email protected]>
 Nandini Rebello <[email protected]>
+Giacinto Cifelli <[email protected]>
+Martin Baschin <[email protected]>
+Sebastian Arnd <[email protected]>

Three authors but only Giacinto's name appears in the patches?

*
* 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

AUTHORS +19 -8

@@ -45,48 +46,67 @@ struct lte_driver_data {
  GAtChat *chat;
 };
 
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+static void at_set_reg_info(const struct ofono_lte *lte,
+ const struct ofono_lte_default_attach_info *info)
 {
- struct cb_data *cbd = user_data;
- ofono_lte_cb_t cb = cbd->cb;
- struct ofono_error error;
+ struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+ char buf_cgdcont[32 + OFONO_GPRS_MAX_APN_LENGTH  +1];
+ char buf_cgauth[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
+ guint buflen = sizeof(buf_cgauth);
+ enum ofono_gprs_auth_method auth_method;
 
- DBG("ok %d", ok);
+ if (strlen(info->apn) > 0)
+ snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+ "AT+CGDCONT=0,\"IP\",\"%s\"", info->apn);
+ else
+ snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+ "AT+CGDCONT=0,\"IP\"");
 
- decode_at_error(&error, g_at_result_final_response(result));
- cb(&error, cbd->data);
+ if (g_at_chat_send(ldd->chat, buf_cgdcont, NULL, NULL, NULL, NULL) == 0)
+ return;
+
+ snprintf(buf_cgauth, buflen, "AT+CGAUTH=0,");
+ buflen -= strlen(buf_cgauth);
 
 

Using snprintf() without checking the return value is mostly pointless... if you know with certainty that your buffer will hold your string just use sprintf. The above could be better written as:

n = sprintf(...);
buflen -= n;

AUTHORS +13 -0

+
+ auth_method = info->auth_method;
+
+ /*
+ * change the authentication method if the  parameters are invalid
+ * for behavior compatibility
+ */
+ if(!*info->username || !*info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ switch(auth_method) {
+ case OFONO_GPRS_AUTH_METHOD_PAP:
+ snprintf(buf_cgauth+strlen(buf_cgauth),

You know strlen(buf_cgauth) already... it's 'n' above, the return value from snprintf. No need to traverse the string again to figure this out.

+ buflen, "1,\"%s\",\"%s\"",
+ info->username, info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_CHAP:
+ snprintf(buf_cgauth+strlen(buf_cgauth),
+ buflen, "2,\"%s\",\"%s\"",
+ info->username, info->password);
+ break;
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ snprintf(buf_cgauth+strlen(buf_cgauth), buflen, "0");
+ break;
+ }
+
+ g_at_chat_send(ldd->chat, buf_cgauth, NULL, NULL, NULL, NULL);

Can't fail?

}

static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info, ofono_lte_cb_t cb, void *data)
{
- struct lte_driver_data *ldd = ofono_lte_get_data(lte);
- char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
- struct cb_data *cbd = cb_data_new(cb, data);

AUTHORS +1 -15

-
- DBG("LTE config with APN: %s", info->apn);
-
- if (strlen(info->apn) > 0)
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
- info->apn);
- else
- snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
-
- /* We can't do much in case of failure so don't check response. */
- if (g_at_chat_send(ldd->chat, buf, NULL,
- at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
- return;
-
- CALLBACK_WITH_FAILURE(cb, data);
+ CALLBACK_WITH_SUCCESS(cb, data);
 }

Aside from abandoning all error handling, what does set_reg_info provide that could not have been handled by set_default_attach_info?

static gboolean lte_delayed_register(gpointer user_data) {
- struct ofono_lte *lte = user_data;
-
- ofono_lte_register(lte);
+ ofono_lte_register(user_data);

I prefer the original version... nonetheless, this change doesn't belong in this patch.

return FALSE;
}

AUTHORS +1 -0

@@ -129,6 +149,7 @@ static struct ofono_lte_driver driver = {
  .probe = at_lte_probe,
  .remove = at_lte_remove,
  .set_default_attach_info = at_lte_set_default_attach_info,
+ .set_reg_info = at_set_reg_info,
 };
 
 void at_lte_init(void)

/Jonas

drivers/atmodem/lte.c +17 -0

+
+	auth_method = info->auth_method;
+
+	/*
+	 * change the authentication method if the  parameters are invalid
+	 * for behavior compatibility
+	 */
+	if(!*info->username || !*info->password)
+		auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+	switch(auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		snprintf(buf_cgauth+strlen(buf_cgauth),
+				buflen, "1,\"%s\",\"%s\"",
+				info->username, info->password);
+		break;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:

  }

static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info, ofono_lte_cb_t cb, void *data)
{

drivers/atmodem/lte.c +0 -9

-	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
-	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
-	struct cb_data *cbd = cb_data_new(cb, data);
-
-	DBG("LTE config with APN: %s", info->apn);
-
-	if (strlen(info->apn) > 0)
-		snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
-				info->apn);

  }
  
  static gboolean lte_delayed_register(gpointer user_data)
  {

drivers/atmodem/lte.c +1 -3

-	struct ofono_lte *lte = user_data;
-
-	ofono_lte_register(lte);
+	ofono_lte_register(user_data);
 
  	return FALSE;
  }

Hi Giacinto,

drivers/atmodem/lte.c struct lte_driver_data { · line 55

Quoted 10 lines

drivers/atmodem/lte.c +6 -3

+static void at_set_reg_info(const struct ofono_lte *lte,
+                     const struct ofono_lte_default_attach_info *info)
  {
-     struct cb_data *cbd = user_data;
-     ofono_lte_cb_t cb = cbd->cb;
-     struct ofono_error error;
+     struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+     char buf_cgdcont[32 + OFONO_GPRS_MAX_APN_LENGTH  +1];
+     char buf_cgauth[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+                                     OFONO_GPRS_MAX_PASSWORD_LENGTH +1];

Please pay attention to doc/coding-style.txt item M3

drivers/atmodem/lte.c struct lte_driver_data { · line 64

Quoted 11 lines

drivers/atmodem/lte.c +8 -1

+     guint buflen = sizeof(buf_cgauth);
+     enum ofono_gprs_auth_method auth_method;
 
-     DBG("ok %d", ok);
+     if (strlen(info->apn) > 0)
+             snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+                             "AT+CGDCONT=0,\"IP\",\"%s\"", info->apn);
+     else
+             snprintf(buf_cgdcont, sizeof(buf_cgdcont),
+                             "AT+CGDCONT=0,\"IP\"");

You're not taking IPv4/v6/Dual into account? Why bother adding that property then?

drivers/atmodem/lte.c struct lte_driver_data { · line 67

Quoted 5 lines

drivers/atmodem/lte.c +2 -2

-     decode_at_error(&error, g_at_result_final_response(result));
-     cb(&error, cbd->data);
+     if (g_at_chat_send(ldd->chat, buf_cgdcont, NULL, NULL, NULL, NULL) == 0)
+             return;

Uhh, you can't just return here

drivers/atmodem/lte.c struct lte_driver_data { · line 70

Quoted 4 lines

drivers/atmodem/lte.c +3 -0

+
+     snprintf(buf_cgauth, buflen, "AT+CGAUTH=0,");
+     buflen -= strlen(buf_cgauth);

You have way too many unnecessary strlen calls. Refer to 'man snprintf' (particularly the return value) to understand how these can be avoided.

Quoted 25 lines

drivers/atmodem/lte.c +24 -0

+
+     auth_method = info->auth_method;
+
+     /*
+      * change the authentication method if the  parameters are invalid
+      * for behavior compatibility
+      */
+     if(!*info->username || !*info->password)
+             auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+     switch(auth_method) {
+     case OFONO_GPRS_AUTH_METHOD_PAP:
+             snprintf(buf_cgauth+strlen(buf_cgauth),
+                             buflen, "1,\"%s\",\"%s\"",
+                             info->username, info->password);
+             break;
+     case OFONO_GPRS_AUTH_METHOD_CHAP:
+             snprintf(buf_cgauth+strlen(buf_cgauth),
+                             buflen, "2,\"%s\",\"%s\"",
+                             info->username, info->password);
+             break;
+     case OFONO_GPRS_AUTH_METHOD_NONE:
+             snprintf(buf_cgauth+strlen(buf_cgauth), buflen, "0");
+             break;

drivers/atmodem/lte.c struct lte_driver_data { · line 97

Quoted 3 lines

drivers/atmodem/lte.c +3 -0

+     }
+
+     g_at_chat_send(ldd->chat, buf_cgauth, NULL, NULL, NULL, NULL);

Anyway. All this boils down to is a +CGDCONT and a +CGAUTH call whenever a property changes. And you even take into account username/password not being valid to override the auth method. So I really see no point for set_reg_info now?

Quoted 20 lines

  }

static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
const struct ofono_lte_default_attach_info *info, ofono_lte_cb_t cb, void *data)
{

drivers/atmodem/lte.c +0 -13

-     struct lte_driver_data *ldd = ofono_lte_get_data(lte);
-     char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
-     struct cb_data *cbd = cb_data_new(cb, data);
-
-     DBG("LTE config with APN: %s", info->apn);
-
-     if (strlen(info->apn) > 0)
-             snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
-                             info->apn);
-     else
-             snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
-
-     /* We can't do much in case of failure so don't check response. */

drivers/atmodem/lte.c struct lte_driver_data { · line 104

Quoted 6 lines

drivers/atmodem/lte.c +1 -5

-     if (g_at_chat_send(ldd->chat, buf, NULL,
-                     at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
-             return;
-
-     CALLBACK_WITH_FAILURE(cb, data);
+     CALLBACK_WITH_SUCCESS(cb, data);

So why do we even bother having a driver method that does literally nothing?

Quoted 19 lines

  }

  static gboolean lte_delayed_register(gpointer user_data)
  {

drivers/atmodem/lte.c +2 -3

-     struct ofono_lte *lte = user_data;
-
-     ofono_lte_register(lte);
+     ofono_lte_register(user_data);
 
      return FALSE;
  }

@@ -129,6 +149,7 @@ static struct ofono_lte_driver driver = {
      .probe                          = at_lte_probe,
      .remove                         = at_lte_remove,
      .set_default_attach_info        = at_lte_set_default_attach_info,
+     .set_reg_info                   = at_set_reg_info,
  };
 

drivers/atmodem/lte.c static struct ofono_lte_driver driver = { · line 155

Quoted 2 lines

drivers/atmodem/lte.c +0 -0

  void at_lte_init(void)

Regards,
-Denis

---
  AUTHORS | 3 +++
  1 file changed, 3 insertions(+)

Hi Giacinto,

Quoted 4 lines · Giacinto Cifelli

---
  AUTHORS | 3 +++
  1 file changed, 3 insertions(+)

Please don't send me patches to AUTHORS. The maintainers will take care of it. If I forget, send me a nudge to do so.

Quoted 10 lines

AUTHORS +2 -0

index 2d360e6e..8f0f5893 100644
@@ -138,3 +138,6 @@ Florent Beillonnet <[email protected]>
  Martin Hundebøll <[email protected]>
  Julien Tournier <[email protected]>
  Nandini Rebello <[email protected]>
+Giacinto Cifelli <[email protected]>
+Martin Baschin <[email protected]>

AUTHORS Florent Beillonnet <[email protected]> · line 143

Quoted 2 lines

AUTHORS +1 -0

+Sebastian Arnd <[email protected]>

If these guys have written some of the code and you're submitting a series of patches, then make sure their name is set as the Author of the particular commit they wrote. If that is not possible, then we might need to look into using various git tags for this. E.g. perhaps Co-authored-by tag.

Regards,
-Denis

added 4 properties for handling the type of context and the authentication method, exactly like in any gprs context handling. The properties are named after the equivalent gprs-context one, for compatibility and uniformity.

---
  doc/lte-api.txt | 34 ++++++++++++++++++++++++++++++++++
  1 file changed, 34 insertions(+)

doc/lte-api.txt +34 -0

index 8a2a97d9..e9cbba0a 100644
@@ -33,3 +33,37 @@ Properties	string DefaultAccessPointName [readwrite]
 
  			Setting this property to an empty string clears the
  			default APN from the modem.
+
+		string Protocol [readwrite]
+
+			Holds the protocol for this context.  Valid values
+			are: "ip", "ipv6" and "dual". Default value is "ip".
+
+		string AuthenticationMethod [readwrite]
+
+			Sets the Method used for the authentication
+			for the default APN.
+
+			Available values are "none", "pap" and "chap".
+			Default is "none".
+
+			If the AuthenticationMethod is set to 'none' it remove
+			the authentication for the DefaultAPN.
+			In case of AuthenticationMethod 'none',
+			if the Username and Password properties are not empty,
+			the values are preserved in the properties, but they
+			are not used or transmitted to the module.
+			Conversely, if Username or Password are empty, the
+			authentication method selected internally is 'none',
+			but the property AuthenticationMethod is left unchanged.
+
+		string Username [readwrite]
+
+			Holds the username to be used for authentication
+			purposes.
+
+		string Password [readwrite]
+
+			Holds the password to be used for authentication
+			purposes.
+

Hi Giacinto,

Reading through your patches, I'm missing an overarching explanation of why we need this. Do you really have an LTE network in range that requires authentication for the default APN?

What happens if
authentication fails... are you connected to an alternative APN automatically, with other service parameters? What indication is available to the user if authentication fails?

/Jonas

Quoted 50 lines

added 4 properties for handling the type of context and the authentication method, exactly like in any gprs context handling. The properties are named after the equivalent gprs-context one, for compatibility and uniformity.

---
  doc/lte-api.txt | 34 ++++++++++++++++++++++++++++++++++
  1 file changed, 34 insertions(+)

doc/lte-api.txt +34 -0

index 8a2a97d9..e9cbba0a 100644
@@ -33,3 +33,37 @@ Properties string DefaultAccessPointName [readwrite]
 
                      Setting this property to an empty string clears the
                      default APN from the modem.
+
+             string Protocol [readwrite]
+
+                     Holds the protocol for this context.  Valid values
+                     are: "ip", "ipv6" and "dual". Default value is "ip".
+
+             string AuthenticationMethod [readwrite]
+
+                     Sets the Method used for the authentication
+                     for the default APN.
+
+                     Available values are "none", "pap" and "chap".
+                     Default is "none".
+
+                     If the AuthenticationMethod is set to 'none' it remove
+                     the authentication for the DefaultAPN.
+                     In case of AuthenticationMethod 'none',
+                     if the Username and Password properties are not empty,
+                     the values are preserved in the properties, but they
+                     are not used or transmitted to the module.
+                     Conversely, if Username or Password are empty, the
+                     authentication method selected internally is 'none',
+                     but the property AuthenticationMethod is left unchanged.
+
+             string Username [readwrite]
+
+                     Holds the username to be used for authentication
+                     purposes.
+
+             string Password [readwrite]
+
+                     Holds the password to be used for authentication
+                     purposes.
+

Quoted 3 lines

Reading through your patches, I'm missing an overarching explanation of why we need this. Do you really have an LTE network in range that requires authentication for the default APN?

yes, quite a few of them actually.
And all private APN I have seen so far require authentication, even for the combined attach.

Shall I extend the explanation?

There wasn't much of an explanation why we need a default APN for LTE, either.

Hi Giacinto,

Quoted 4 lines

Quoted 3 lines

Reading through your patches, I'm missing an overarching explanation of why we need this. Do you really have an LTE network in range that requires authentication for the default APN?

Quoted 4 lines

yes, quite a few of them actually.
And all private APN I have seen so far require authentication, even for the combined attach.

Are these networks something a typical user would see? Or is this mainly for M2M type usecases?

Fundamentally this whole username / password for a default bearer activation is utterly insane. The network provider has your IMSI, it can lookup whether you're authorized for that APN or not, or suggest you an APN that will work... But I digress..

Quoted 3 lines

Shall I extend the explanation?

An extended explanation is always a good idea. Especially for something that seems to be pretty esoteric.

Quoted 2 lines

There wasn't much of an explanation why we need a default APN for LTE, either.

Sure there was. Not sure why you say that :) Maybe it wasn't recorded in the git commit but it was discussed on the mailing list, IRC, etc. But again, see above. The whole default attach APN needing to be provisioned by the client is utterly insane.

Regards,
-Denis