5:54 p.m.
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
5:54 p.m.
New subject: [PATCH 2/6] lte.h: added proto and authentication handling
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
5:54 p.m.
New subject: [PATCH 3/6] src/lte: added proto and authentication handling
---
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, <e->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,
+ <e->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(<e->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(<e->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, <e->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, <e->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, <e->info);
+}
+
struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
{
return __ofono_atom_get_modem(lte->atom);
--
2.17.1
7:07 a.m.
New subject: [PATCH 3/6] src/lte: added proto and authentication handling
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, <e->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,
+ <e->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(<e->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(<e->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, <e->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, <e->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, <e->info);
+}
+
struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
{
return __ofono_atom_get_modem(lte->atom);
/Jonas
1:57 p.m.
New subject: [PATCH 3/6] src/lte: added proto and authentication handling
Hi,
On Thu, Oct 11, 2018 at 10:07 PM Jonas Bonn <jonas(a)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, <e->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,
> +
<e->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(<e->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(<e->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, <e->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, <e->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, <e->info);
> +}
> +
> struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
> {
> return __ofono_atom_get_modem(lte->atom);
/Jonas
Giacinto
2:43 p.m.
New subject: [PATCH 3/6] src/lte: added proto and authentication handling
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, <e->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,
+ <e->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(<e->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(<e->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, <e->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, <e->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, <e->info);
+}
+
Given that you have somewhat sane defaults for everything, I'm not
convinced now that this is even needed...
Regards,
-Denis
5:36 p.m.
New subject: [PATCH 3/6] src/lte: added proto and authentication handling
Hi Denis,
On Fri, Oct 12, 2018 at 5:43 AM Denis Kenzior <denkenz(a)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, <e->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,
> +
<e->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(<e->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(<e->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, <e->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, <e->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, <e->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
4:47 a.m.
New subject: [PATCH 3/6] src/lte: added proto and authentication handling
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
5:54 p.m.
New subject: [PATCH 5/6] atmodem/lte: proto and authentication handling
---
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
6:51 a.m.
New subject: [PATCH 5/6] atmodem/lte: proto and authentication handling
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(a)gmail.com>
Martin Hundebøll<martin(a)geanix.com>
Julien Tournier<tournier.julien(a)gmail.com>
Nandini Rebello<nandini.rebello(a)intel.com>
+Giacinto Cifelli<gciofono(a)gmail.com>
+Martin Baschin<martin.baschin(a)googlemail.com>
+Sebastian Arnd<sebastianarnd(a)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
1:54 p.m.
New subject: [PATCH 5/6] atmodem/lte: proto and authentication handling
On Thu, Oct 11, 2018 at 9:51 PM Jonas Bonn <jonas(a)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(a)gmail.com>
Martin Hundebøll <martin(a)geanix.com>
Julien Tournier <tournier.julien(a)gmail.com>
Nandini Rebello <nandini.rebello(a)intel.com>
+Giacinto Cifelli <gciofono(a)gmail.com>
+Martin Baschin <martin.baschin(a)googlemail.com>
+Sebastian Arnd <sebastianarnd(a)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
2:56 p.m.
New subject: [PATCH 5/6] atmodem/lte: proto and authentication handling
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
3:56 p.m.
New subject: [PATCH 5/6] atmodem/lte: proto and authentication handling
Hi Denis,
On Fri, Oct 12, 2018 at 5:56 AM Denis Kenzior <denkenz(a)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
5:54 p.m.
New subject: [PATCH 4/6] src/modem: notify lte driver before going online
---
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
7:19 a.m.
New subject: [PATCH 4/6] src/modem: notify lte driver before going online
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);
2:04 p.m.
New subject: [PATCH 4/6] src/modem: notify lte driver before going online
Hi,
On Thu, Oct 11, 2018 at 10:19 PM Jonas Bonn <jonas(a)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
5:17 p.m.
New subject: [PATCH 4/6] src/modem: notify lte driver before going online
On 12/10/18 05:04, Giacinto Cifelli wrote:
Hi,
On Thu, Oct 11, 2018 at 10:19 PM Jonas Bonn <jonas(a)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
5:19 p.m.
New subject: [PATCH 4/6] src/modem: notify lte driver before going online
On Fri, Oct 12, 2018 at 8:17 AM Jonas Bonn <jonas(a)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(a)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
5:54 p.m.
New subject: [PATCH 6/6] Gemalto contributors
---
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(a)gmail.com>
Martin Hundebøll <martin(a)geanix.com>
Julien Tournier <tournier.julien(a)gmail.com>
Nandini Rebello <nandini.rebello(a)intel.com>
+Giacinto Cifelli <gciofono(a)gmail.com>
+Martin Baschin <martin.baschin(a)googlemail.com>
+Sebastian Arnd <sebastianarnd(a)gmail.com>
--
2.17.1
3:10 p.m.
New subject: [PATCH 6/6] Gemalto contributors
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(a)gmail.com>
Martin Hundebøll <martin(a)geanix.com>
Julien Tournier <tournier.julien(a)gmail.com>
Nandini Rebello <nandini.rebello(a)intel.com>
+Giacinto Cifelli <gciofono(a)gmail.com>
+Martin Baschin <martin.baschin(a)googlemail.com>
+Sebastian Arnd <sebastianarnd(a)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
3:15 p.m.
New subject: [PATCH 6/6] Gemalto contributors
Hi Denis,
On Fri, Oct 12, 2018 at 6:10 AM Denis Kenzior <denkenz(a)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(a)gmail.com>
> Martin Hundebøll <martin(a)geanix.com>
> Julien Tournier <tournier.julien(a)gmail.com>
> Nandini Rebello <nandini.rebello(a)intel.com>
> +Giacinto Cifelli <gciofono(a)gmail.com>
> +Martin Baschin <martin.baschin(a)googlemail.com>
> +Sebastian Arnd <sebastianarnd(a)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
6:07 a.m.
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.
+
1:41 p.m.
Hi Jonas,
On Thu, Oct 11, 2018 at 9:07 PM Jonas Bonn <jonas(a)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.
> +
3:06 p.m.
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
3:13 p.m.
Hi Denis,
On Fri, Oct 12, 2018 at 6:06 AM Denis Kenzior <denkenz(a)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
1437
days inactive
1439
days old
24 comments
3 participants
participants (3)
-
Denis Kenzior -
Giacinto Cifelli -
Jonas Bonn