[RFC PATCH v6 0/1] lte core: additional properties for the default context

[RFC PATCH v6 0/1] lte core: additional properties for the default context

[Giacinto Cifelli]

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

This is a partial submit, for comments.
Related to v5, I have updated the submit description,
added a comment for the storage (that makes clear that protocol and 
authentication method are always saved because of their defaults),
and changed the handling of the SetProperty key, taking it from the
incoming message, as suggested by Denis.


Giacinto Cifelli (1):
  lte: protocol and authentication for default ctx

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

-- 
2.17.1

[RFC PATCH v6 1/1] lte: protocol and authentication for default ctx

[Giacinto Cifelli]

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

Many LTE networks require user authentication, even for the default
context. In particular, most of the private APNs use this facility
to add some control on top of the MNO providing the service, so that
another user of the same network cannot access the private one.
As such, we add these parameters to the default context
settings that will attempt to use when registering to the network.

The additional parameters added by this patch are:  protocol, user, and
password.  These are sufficient to allow to connect to networks
available to the patch author where ofono previously failed to register
to the network at all.

Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
---
 src/lte.c | 241 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 185 insertions(+), 56 deletions(-)

diff --git a/src/lte.c b/src/lte.c
index 23fe8e1c..11a22730 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;
@@ -50,6 +55,7 @@ struct ofono_lte {
 	DBusMessage *pending;
 	struct ofono_lte_default_attach_info pending_info;
 	struct ofono_lte_default_attach_info info;
+	const char *key;
 };
 
 static GSList *g_drivers = NULL;
@@ -57,6 +63,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 +79,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 && is_valid_apn(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 && strlen(username) <= OFONO_GPRS_MAX_USERNAME_LENGTH)
+		strcpy(lte->info.username, username);
+
+	if (password && strlen(password) <= OFONO_GPRS_MAX_PASSWORD_LENGTH)
+		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 +143,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 *propstr = NULL;
+	char *key = g_strdup(lte->key);
 
 	DBG("%s error %d", path, error->type);
 
@@ -118,55 +174,68 @@ 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);
+		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;
+		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);
+		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);
+		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
+		 */
+		ofono_error("unexpected property change: ignored");
+		g_free(key);
+		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);
+		/*
+		 * the following code removes from storage empty APN, user, pwd
+		 * for proto and auth_method, given that they always
+		 * have defaults, it will not do anything.
+		 */
+		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);
+	g_free(key);
 }
 
 static DBusMessage *lte_set_property(DBusConnection *conn,
@@ -177,6 +246,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 +269,68 @@ 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);
 
-		return lte_set_default_apn(lte, conn, msg, str);
-	}
+	if ((strcmp(property, LTE_APN) == 0)) {
+
+		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;
 
-	return __ofono_error_invalid_args(msg);
+	} else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
+
+		if (!gprs_auth_method_from_string(str, &auth_method))
+			return __ofono_error_invalid_format(msg);
+
+		if (auth_method == lte->info.auth_method)
+			return dbus_message_new_method_return(msg);
+
+		lte->pending_info.auth_method = auth_method;
+
+	} 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->key = property;
+	lte->pending = dbus_message_ref(msg);
+	lte->driver->set_default_attach_info(lte, &lte->pending_info,
+					lte_set_default_attach_info_cb, lte);
+
+	return NULL;
 }
 
 static const GDBusMethodTable lte_methods[] = {
-- 
2.17.1

Re: [RFC PATCH v6 1/1] lte: protocol and authentication for default ctx

[Denis Kenzior]

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

Hi Giacinto,

On 10/13/2018 02:34 AM, Giacinto Cifelli wrote:
> Many LTE networks require user authentication, even for the default
> context. In particular, most of the private APNs use this facility
> to add some control on top of the MNO providing the service, so that
> another user of the same network cannot access the private one.
> As such, we add these parameters to the default context
> settings that will attempt to use when registering to the network.
> 
> The additional parameters added by this patch are:  protocol, user, and
> password.  These are sufficient to allow to connect to networks
> available to the patch author where ofono previously failed to register
> to the network at all.
> 
> Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
> ---
>   src/lte.c | 241 +++++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 185 insertions(+), 56 deletions(-)
> 

So I'm fine with the overall approach taken here, but the actual 
implementation still has issues:

>   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 *propstr = NULL;
> +	char *key = g_strdup(lte->key);

I'd do this differently:

>   
>   	DBG("%s error %d", path, error->type);
>   

char *key;
char *value;
const char *str;
DBusMessageIter iter;
DbusMessageIter var;

....

// No error checking needed since we already validated pending:

dbus_message_iter_init(lte->pending, &iter);
dbus_message_iter_get_basic(&iter, &str);
key = strdup(str);

dbus_message_iter_next(&iter);
dbus_message_iter_recurse(&iter, &var);
dbus_message_iter_get_basic(&var, &str);
value = strdup(str);

> @@ -118,55 +174,68 @@ 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);
> +		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;
> +		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);
> +		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);
> +		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
> +		 */
> +		ofono_error("unexpected property change: ignored");
> +		g_free(key);
> +		key = NULL;
> +	}

Get rid of this entire if/else block and simply do:

memcpy(&lte->info, &lte->pending_info, sizeof(lte->info));

> +
> +	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);
> +		/*
> +		 * the following code removes from storage empty APN, user, pwd
> +		 * for proto and auth_method, given that they always
> +		 * have defaults, it will not do anything.
> +		 */
> +		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);

g_free(value);

> -}
> -
> -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);
> +	g_free(key);
>   }
>   
>   static DBusMessage *lte_set_property(DBusConnection *conn,
> @@ -177,6 +246,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);
>   

So you may want to do:

memcpy(&lte->pending_info, &lte->info, sizeof(lte->info));

>   	if (!dbus_message_iter_init(msg, &iter))
>   		return __ofono_error_invalid_args(msg);
> @@ -192,16 +269,68 @@ 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);
>   
> -		return lte_set_default_apn(lte, conn, msg, str);
> -	}
> +	if ((strcmp(property, LTE_APN) == 0)) {
> +
> +		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;
>   
> -	return __ofono_error_invalid_args(msg);
> +	} else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
> +
> +		if (!gprs_auth_method_from_string(str, &auth_method))
> +			return __ofono_error_invalid_format(msg);
> +
> +		if (auth_method == lte->info.auth_method)
> +			return dbus_message_new_method_return(msg);
> +
> +		lte->pending_info.auth_method = auth_method;
> +
> +	} 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->key = property;

And this isn't needed.  Or alternatively store both the key and value 
being set to in ofono_lte.  But I much prefer just parsing the message 
again.

> +	lte->pending = dbus_message_ref(msg);
> +	lte->driver->set_default_attach_info(lte, &lte->pending_info,
> +					lte_set_default_attach_info_cb, lte);
> +
> +	return NULL;
>   }
>   
>   static const GDBusMethodTable lte_methods[] = {
> 

Regards,
-Denis

Re: [RFC PATCH v6 1/1] lte: protocol and authentication for default ctx

[Giacinto Cifelli]

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

Hi Denis,

On Tue, Oct 16, 2018 at 9:17 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/13/2018 02:34 AM, Giacinto Cifelli wrote:
> > Many LTE networks require user authentication, even for the default
> > context. In particular, most of the private APNs use this facility
> > to add some control on top of the MNO providing the service, so that
> > another user of the same network cannot access the private one.
> > As such, we add these parameters to the default context
> > settings that will attempt to use when registering to the network.
> >
> > The additional parameters added by this patch are:  protocol, user, and
> > password.  These are sufficient to allow to connect to networks
> > available to the patch author where ofono previously failed to register
> > to the network at all.
> >
> > Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
> > ---
> >   src/lte.c | 241 +++++++++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 185 insertions(+), 56 deletions(-)
> >
>
> So I'm fine with the overall approach taken here, but the actual
> implementation still has issues:
>
> >   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 *propstr = NULL;
> > +     char *key = g_strdup(lte->key);
>
> I'd do this differently:
>
> >
> >       DBG("%s error %d", path, error->type);
> >
>
> char *key;
> char *value;
> const char *str;
> DBusMessageIter iter;
> DbusMessageIter var;
>
> ....
>
> // No error checking needed since we already validated pending:
>
> dbus_message_iter_init(lte->pending, &iter);
> dbus_message_iter_get_basic(&iter, &str);
> key = strdup(str);
>
> dbus_message_iter_next(&iter);
> dbus_message_iter_recurse(&iter, &var);
> dbus_message_iter_get_basic(&var, &str);
> value = strdup(str);
>
> > @@ -118,55 +174,68 @@ 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);
> > +             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;
> > +             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);
> > +             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);
> > +             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
> > +              */
> > +             ofono_error("unexpected property change: ignored");
> > +             g_free(key);
> > +             key = NULL;
> > +     }
>
> Get rid of this entire if/else block and simply do:
>
> memcpy(&lte->info, &lte->pending_info, sizeof(lte->info));

this I like particularly.

>
> > +
> > +     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);
> > +             /*
> > +              * the following code removes from storage empty APN, user, pwd
> > +              * for proto and auth_method, given that they always
> > +              * have defaults, it will not do anything.
> > +              */
> > +             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);
>
> g_free(value);
>
> > -}
> > -
> > -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);
> > +     g_free(key);
> >   }
> >
> >   static DBusMessage *lte_set_property(DBusConnection *conn,
> > @@ -177,6 +246,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);
> >
>
> So you may want to do:
>
> memcpy(&lte->pending_info, &lte->info, sizeof(lte->info));
>
> >       if (!dbus_message_iter_init(msg, &iter))
> >               return __ofono_error_invalid_args(msg);
> > @@ -192,16 +269,68 @@ 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);
> >
> > -             return lte_set_default_apn(lte, conn, msg, str);
> > -     }
> > +     if ((strcmp(property, LTE_APN) == 0)) {
> > +
> > +             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;
> >
> > -     return __ofono_error_invalid_args(msg);
> > +     } else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
> > +
> > +             if (!gprs_auth_method_from_string(str, &auth_method))
> > +                     return __ofono_error_invalid_format(msg);
> > +
> > +             if (auth_method == lte->info.auth_method)
> > +                     return dbus_message_new_method_return(msg);
> > +
> > +             lte->pending_info.auth_method = auth_method;
> > +
> > +     } 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->key = property;
>
> And this isn't needed.  Or alternatively store both the key and value
> being set to in ofono_lte.  But I much prefer just parsing the message
> again.
>
> > +     lte->pending = dbus_message_ref(msg);
> > +     lte->driver->set_default_attach_info(lte, &lte->pending_info,
> > +                                     lte_set_default_attach_info_cb, lte);
> > +
> > +     return NULL;
> >   }
> >
> >   static const GDBusMethodTable lte_methods[] = {
> >
>
> Regards,
> -Denis

I have changed my code according to your suggestions and retested
(with the Gemalto atom).

Regards,
Giacinto