[PATCH 1/6] lte-api: protocol and authentication properties

[PATCH 1/6] lte-api: protocol and authentication properties

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

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(+)

diff --git a/doc/lte-api.txt b/doc/lte-api.txt
index 8a2a97d9..e9cbba0a 100644
--- a/doc/lte-api.txt
+++ b/doc/lte-api.txt
@@ -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.
+
-- 
2.17.1

[PATCH 2/6] lte.h: added proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

The ofono_lte_default_attach_info is extended with protocol,
authentication method, username and password.
The transmission of this info from the src to the atom happens
through the existing set_default_attach_info.
A signal is emitted when one of these properties changes

There is a new function in the atom, set_reg_info, that the atom can
choose to use as a hint to transmit the information to the modem.

The newly added global function ofono_lte_set_reg_info locates the lte
atom if present, otherwise returns, and triggers the atom function
set_reg_info.
This function is to be called in src/modem just before setting it
online.
---
 include/lte.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/lte.h b/include/lte.h
index 0f2501c0..3f04984a 100644
--- a/include/lte.h
+++ b/include/lte.h
@@ -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
@@ -32,6 +33,10 @@ struct ofono_lte;
 
 struct ofono_lte_default_attach_info {
 	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+	enum ofono_gprs_proto proto;
+	enum ofono_gprs_auth_method auth_method;
+	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
 };
 
 typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void *data);
@@ -43,6 +48,8 @@ struct ofono_lte_driver {
 	void (*set_default_attach_info)(const struct ofono_lte *lte,
 			const struct ofono_lte_default_attach_info *info,
 			ofono_lte_cb_t cb, void *data);
+	void (*set_reg_info)(const struct ofono_lte *lte,
+			const struct ofono_lte_default_attach_info *info);
 };
 
 int ofono_lte_driver_register(const struct ofono_lte_driver *d);
@@ -61,6 +68,8 @@ void ofono_lte_set_data(struct ofono_lte *lte, void *data);
 
 void *ofono_lte_get_data(const struct ofono_lte *lte);
 
+void ofono_lte_set_reg_info(struct ofono_modem *modem);
+
 struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte);
 
 #ifdef __cplusplus
-- 
2.17.1

[PATCH 3/6] src/lte: added proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 11060 bytes --]

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

diff --git a/src/lte.c b/src/lte.c
index 23fe8e1c..2412fcec 100644
--- a/src/lte.c
+++ b/src/lte.c
@@ -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;
+	} 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
+		 */
+		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);
+	lte->driver->set_default_attach_info(lte, &lte->pending_info,
+					lte_set_default_attach_info_cb, lte);
+	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);
+
+	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);
-- 
2.17.1

[PATCH 5/6] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 3858 bytes --]

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

diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..1d097c68 100644
--- a/drivers/atmodem/lte.c
+++ b/drivers/atmodem/lte.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017  Intel Corporation. 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
@@ -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);
+
+	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;
+	}
+
+	g_at_chat_send(ldd->chat, buf_cgauth, NULL, NULL, NULL, NULL);
 }
 
 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\"");
-
-	/* 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);
 }
 
 static gboolean lte_delayed_register(gpointer user_data)
 {
-	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,
 };
 
 void at_lte_init(void)
-- 
2.17.1

[PATCH 4/6] src/modem: notify lte driver before going online

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

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

diff --git a/src/modem.c b/src/modem.c
index 9e254482..74dbe7ad 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -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 is always online, proceed to online state. */
 		if (modem_is_always_online(modem) == TRUE)
 			set_online(modem, TRUE);
-- 
2.17.1

[PATCH 6/6] Gemalto contributors

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

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

diff --git a/AUTHORS b/AUTHORS
index 2d360e6e..8f0f5893 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -138,3 +138,6 @@ Florent Beillonnet <florent.beillonnet@gmail.com>
 Martin Hundebøll <martin@geanix.com>
 Julien Tournier <tournier.julien@gmail.com>
 Nandini Rebello <nandini.rebello@intel.com>
+Giacinto Cifelli <gciofono@gmail.com>
+Martin Baschin <martin.baschin@googlemail.com>
+Sebastian Arnd <sebastianarnd@gmail.com>
-- 
2.17.1

Re: [PATCH 1/6] lte-api: protocol and authentication properties

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]

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


On 10/10/18 08:54, Giacinto Cifelli wrote:
> 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(+)
>
> diff --git a/doc/lte-api.txt b/doc/lte-api.txt
> index 8a2a97d9..e9cbba0a 100644
> --- a/doc/lte-api.txt
> +++ b/doc/lte-api.txt
> @@ -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.
> +

Re: [PATCH 5/6] atmodem/lte: proto and authentication handling

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 5948 bytes --]

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.


On 10/10/18 08:54, Giacinto Cifelli wrote:
> ---
>   drivers/atmodem/lte.c | 79 +++++++++++++++++++++++++++----------------
>   1 file changed, 50 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> index efa4e5fe..1d097c68 100644
> --- a/drivers/atmodem/lte.c
> +++ b/drivers/atmodem/lte.c
> @@ -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?


On 10/10/18 08:54, Giacinto Cifelli wrote:
> ---
>   AUTHORS | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/AUTHORS b/AUTHORS
> index 2d360e6e..8f0f5893 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -138,3 +138,6 @@ Florent Beillonnet<florent.beillonnet@gmail.com>
>   Martin Hundebøll<martin@geanix.com>
>   Julien Tournier<tournier.julien@gmail.com>
>   Nandini Rebello<nandini.rebello@intel.com>
> +Giacinto Cifelli<gciofono@gmail.com>
> +Martin Baschin<martin.baschin@googlemail.com>
> +Sebastian Arnd<sebastianarnd@gmail.com>

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
> @@ -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;


> +
> +	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);
> -
> -	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;
>   }
> @@ -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

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 7745 bytes --]

Re: [PATCH 3/6] src/lte: added proto and authentication handling

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 12402 bytes --]

Hi,


On 10/10/18 08:54, Giacinto Cifelli wrote:
> ---
>   src/lte.c | 242 +++++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 186 insertions(+), 56 deletions(-)
>
> diff --git a/src/lte.c b/src/lte.c
> index 23fe8e1c..2412fcec 100644
> --- a/src/lte.c
> +++ b/src/lte.c
> @@ -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;
> +	} 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.

> +		 * 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);
> +	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.

> +	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);
> +
> +	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

Re: [PATCH 4/6] src/modem: notify lte driver before going online

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]



On 10/10/18 08:54, Giacinto Cifelli wrote:
> ---
>   src/modem.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/src/modem.c b/src/modem.c
> index 9e254482..74dbe7ad 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -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


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

Re: [PATCH 1/6] lte-api: protocol and authentication properties

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 3247 bytes --]

Hi Jonas,

On Thu, Oct 11, 2018 at 9:07 PM Jonas Bonn <jonas@southpole.se> wrote:
>
> 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?

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

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

In general LTE registration is denied and fallback to legacy
technology if available.

>
> /Jonas

Shall I extend the explanation?
There wasn't much of an explanation why we need a default APN for LTE, either.

Regards,
Giacinto

>
>
> On 10/10/18 08:54, Giacinto Cifelli wrote:
> > 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(+)
> >
> > diff --git a/doc/lte-api.txt b/doc/lte-api.txt
> > index 8a2a97d9..e9cbba0a 100644
> > --- a/doc/lte-api.txt
> > +++ b/doc/lte-api.txt
> > @@ -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.
> > +
>

Re: [PATCH 5/6] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 6990 bytes --]

On Thu, Oct 11, 2018 at 9:51 PM Jonas Bonn <jonas@southpole.se> wrote:
>
> 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.
>

Ok, I will duplicate the explanation in patch 2/6.

>
> On 10/10/18 08:54, Giacinto Cifelli wrote:
>
> ---
>  drivers/atmodem/lte.c | 79 +++++++++++++++++++++++++++----------------
>  1 file changed, 50 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> index efa4e5fe..1d097c68 100644
> --- a/drivers/atmodem/lte.c
> +++ b/drivers/atmodem/lte.c
> @@ -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?

We decided not to use our Gemalto emails for this. Also because it is
difficult to send formatted patches from there.

>
>
> On 10/10/18 08:54, Giacinto Cifelli wrote:
>
> ---
>  AUTHORS | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/AUTHORS b/AUTHORS
> index 2d360e6e..8f0f5893 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -138,3 +138,6 @@ Florent Beillonnet <florent.beillonnet@gmail.com>
>  Martin Hundebøll <martin@geanix.com>
>  Julien Tournier <tournier.julien@gmail.com>
>  Nandini Rebello <nandini.rebello@intel.com>
> +Giacinto Cifelli <gciofono@gmail.com>
> +Martin Baschin <martin.baschin@googlemail.com>
> +Sebastian Arnd <sebastianarnd@gmail.com>
>
>
> Three authors but only Giacinto's name appears in the patches?

Yes, I have reworked the code of both Martin and Sebastian, but
doesn't seem fair not to mention them.

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

ok

>
> +
> + 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.

ok

>
> + 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?

I will check this

>
>  }
>
>  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\"");
> -
> - /* 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?

this is the result of a long discussion with Denis.
Since the properties are set one by one, setting ip, apn, auth_method,
username, password would cause the call of AT+CGDCONT and AT+CGAUTH
several times, with incomplete
and possibly modified parameters (for the authentication, you need to
have all 3 valid to have it executed properly).

An atom can still chose to apply the parameters on the go in
set_default_attach_info, but set_reg_info is executed just before
going online, with all parameters already set.
It has also the advantage of working more like gprs-context, when the
parameters are set before activating the context and not all the time.

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

ok

>
>
>   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,
>  };
>
>  void at_lte_init(void)
>
>
> /Jonas

Giacinto

Re: [PATCH 3/6] src/lte: added proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 15817 bytes --]

Hi,

On Thu, Oct 11, 2018 at 10:07 PM Jonas Bonn <jonas@southpole.se> wrote:
>
> Hi,
>
>
> On 10/10/18 08:54, Giacinto Cifelli wrote:
> > ---
> >   src/lte.c | 242 +++++++++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 186 insertions(+), 56 deletions(-)
> >
> > diff --git a/src/lte.c b/src/lte.c
> > index 23fe8e1c..2412fcec 100644
> > --- a/src/lte.c
> > +++ b/src/lte.c
> > @@ -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;
> > +     } 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.

an error log is an excellent idea, thanks.

>
> > +              * 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);
> > +     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.

that's disturbing. I will look into it.

>
> > +     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);
> > +
> > +     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

Giacinto

Re: [PATCH 4/6] src/modem: notify lte driver before going online

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

Hi,

On Thu, Oct 11, 2018 at 10:19 PM Jonas Bonn <jonas@southpole.se> wrote:
>
>
>
> On 10/10/18 08:54, Giacinto Cifelli wrote:
> > ---
> >   src/modem.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/src/modem.c b/src/modem.c
> > index 9e254482..74dbe7ad 100644
> > --- a/src/modem.c
> > +++ b/src/modem.c
> > @@ -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.

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.

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

Giacinto

Re: [PATCH 3/6] src/lte: added proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 9994 bytes --]

Hi Giacinto,

> @@ -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);

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

> -	}
> +
> +	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);
> +

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

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

<snip>

>   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;
> +	} 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
> +		 */
> +		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.

> +
> +	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);
>   	}

I can see this applying for APN and maybe Username/Password.  But not 
the other 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);

Umm, method?  How well tested is this submission?

>   
> -	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);
> +	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...

>   }
>   
>   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);
> +
> +	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

Re: [PATCH 5/6] atmodem/lte: proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3994 bytes --]

Hi Giacinto,

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

> +	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?

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

> +
> +	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.

> +
> +	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;
> +	}
> +
> +	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?

>   }
>   
>   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\"");
> -
> -	/* 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);

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

>   }
>   
>   static gboolean lte_delayed_register(gpointer user_data)
>   {
> -	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,
>   };
>   
>   void at_lte_init(void)
> 

Regards,
-Denis

Re: [PATCH 1/6] lte-api: protocol and authentication properties

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

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?
> 
> 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..

> 
> Shall I extend the explanation?

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

> 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

Re: [PATCH 6/6] Gemalto contributors

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

Hi Giacinto,

On 10/10/2018 01:54 AM, Giacinto Cifelli wrote:
> ---
>   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.

> diff --git a/AUTHORS b/AUTHORS
> index 2d360e6e..8f0f5893 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -138,3 +138,6 @@ Florent Beillonnet <florent.beillonnet@gmail.com>
>   Martin Hundebøll <martin@geanix.com>
>   Julien Tournier <tournier.julien@gmail.com>
>   Nandini Rebello <nandini.rebello@intel.com>
> +Giacinto Cifelli <gciofono@gmail.com>
> +Martin Baschin <martin.baschin@googlemail.com>
> +Sebastian Arnd <sebastianarnd@gmail.com>
> 

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

Re: [PATCH 1/6] lte-api: protocol and authentication properties

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

Hi Denis,

On Fri, Oct 12, 2018 at 6:06 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> 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?
> >
> > 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?

My focus is mainly M2M use cases, but also public networks may require
authentication.

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

There is also another problem: the default and data context APN is
often the same, and if the auth is not set for both in the same way,
the context activation fails.
But I digress too.

>
> >
> > Shall I extend the explanation?
>
> An extended explanation is always a good idea.  Especially for something
> that seems to be pretty esoteric.
>
> > 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.

I prefer not to comment on these practices. For instance, is there an
archive of the chat and mailing list that I or anyone could access?

>
> Regards,
> -Denis

Regards,
Giacinto

Re: [PATCH 6/6] Gemalto contributors

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

Hi Denis,

On Fri, Oct 12, 2018 at 6:10 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/10/2018 01:54 AM, Giacinto Cifelli wrote:
> > ---
> >   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.
>
> > diff --git a/AUTHORS b/AUTHORS
> > index 2d360e6e..8f0f5893 100644
> > --- a/AUTHORS
> > +++ b/AUTHORS
> > @@ -138,3 +138,6 @@ Florent Beillonnet <florent.beillonnet@gmail.com>
> >   Martin Hundebøll <martin@geanix.com>
> >   Julien Tournier <tournier.julien@gmail.com>
> >   Nandini Rebello <nandini.rebello@intel.com>
> > +Giacinto Cifelli <gciofono@gmail.com>
> > +Martin Baschin <martin.baschin@googlemail.com>
> > +Sebastian Arnd <sebastianarnd@gmail.com>
> >
>
> 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.

this co-authored-by seems quite interesting. I will use it.


>
> Regards,
> -Denis

Regards,
Giacinto

Re: [PATCH 5/6] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 5621 bytes --]

Hi Denis,

On Fri, Oct 12, 2018 at 5:56 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> > +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

ok

>
> > +     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?
>

you are right. I did add them in a switch for vendors to be submitted
once this overall change is accepted, but not here.
I'll submit taking it into account.

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

I'll look into it.

>
> > +
> > +     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.

yes, also Jonas mentioned that. I will look into it.

>
> > +
> > +     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;
> > +     }
> > +
> > +     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?

ok, I desist and run the two commands on the go, as many times as the
properties are changed.
At least there won't be discussion on where to call set_reg_info and -
mainly - it will solve the immediate callback issue.

>
> >   }
> >
> >   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\"");
> > -
> > -     /* 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);
>
> So why do we even bother having a driver method that does literally nothing?

that could be an alternate solution: checking the method for null in
src/lte.c, but I will go for the immediate application of the
parameters.

>
> >   }
> >
> >   static gboolean lte_delayed_register(gpointer user_data)
> >   {
> > -     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,
> >   };
> >
> >   void at_lte_init(void)
> >
>
> Regards,
> -Denis

Regards,
Giacinto

Re: [PATCH 4/6] src/modem: notify lte driver before going online

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]



On 12/10/18 05:04, Giacinto Cifelli wrote:
> Hi,
>
> On Thu, Oct 11, 2018 at 10:19 PM Jonas Bonn <jonas@southpole.se> wrote:
>>
>>
>> On 10/10/18 08:54, Giacinto Cifelli wrote:
>>> ---
>>>    src/modem.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/modem.c b/src/modem.c
>>> index 9e254482..74dbe7ad 100644
>>> --- a/src/modem.c
>>> +++ b/src/modem.c
>>> @@ -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.
> 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.

> 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

Re: [PATCH 4/6] src/modem: notify lte driver before going online

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]

On Fri, Oct 12, 2018 at 8:17 AM Jonas Bonn <jonas@southpole.se> wrote:
>
>
>
> On 12/10/18 05:04, Giacinto Cifelli wrote:
> > Hi,
> >
> > On Thu, Oct 11, 2018 at 10:19 PM Jonas Bonn <jonas@southpole.se> wrote:
> >>
> >>
> >> On 10/10/18 08:54, Giacinto Cifelli wrote:
> >>> ---
> >>>    src/modem.c | 2 ++
> >>>    1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/src/modem.c b/src/modem.c
> >>> index 9e254482..74dbe7ad 100644
> >>> --- a/src/modem.c
> >>> +++ b/src/modem.c
> >>> @@ -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.
> > 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.
>
> > 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

I have decided to go for the direct application. I am changing the code now.
This part is therefore no longer applicable.

Giacinto

Re: [PATCH 3/6] src/lte: added proto and authentication handling

[Giacinto Cifelli]

[-- Attachment #1: Type: text/plain, Size: 13601 bytes --]

Hi Denis,

On Fri, Oct 12, 2018 at 5:43 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> > @@ -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);
>
> So we may want to be more paranoid here, similar to how load_context in
> gprs.c works.

it was not done, so I have left as-is, same logic below.
But I will add the checks.

>
> > -     }
> > +
> > +     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);
> > +
>
> Again, might want to be more paranoid here and ensure username /
> password don't overflow buffers.
>
> > +     g_free(apn);
> > +     g_free(proto_str);
> > +     g_free(auth_method_str);
> > +     g_free(username);
> > +     g_free(password);
> >   }
> >
>
> <snip>
>
> >   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;
> > +     } 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
> > +              */
> > +             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.
>

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.

> > +
> > +     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);
> >       }
>
> 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.

>
> >
> > -     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);
>
> Umm, method?  How well tested is this submission?

right, I have fixed this.

>
> >
> > -     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);
> > +     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...

fixed.

>
> >   }
> >
> >   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);
> > +
> > +     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...
>

ok, as said elsewhere, I remove this, and apply the settings to the
modem when they are set via dbus, possibly as often as 5 times.

> Regards,
> -Denis

Regards,
Giacinto

Re: [PATCH 3/6] src/lte: added proto and authentication handling

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

Hi Giacinto,

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

No?

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

So build the signal prior to answering the method return...  You can 
still send the signal afterwards.

>>> +             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);
>>>        }
>>
>> 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.

The removal part should probably only apply to APNs and Username / 
Password.  Fair enough that others would never be empty, but you might 
still want to make an explicit check or at least a comment stating this 
for clarity.

Regards,
-Denis