Hi Giacinto,
On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
---
src/gprs.c | 13 ++-
src/lte.c | 372 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
src/main.c | 5 -
3 files changed, 341 insertions(+), 49 deletions(-)
So you seem to have 3 completely unrelated things going on here... At
the very minimum this should be 3 commits.
Also, you're adding LTE D-Bus implementation without updating or
proposing changes to doc/lte-api.txt.
diff --git a/src/gprs.c b/src/gprs.c
index 377eced..40f43e3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum
ofono_gprs_auth_method auth)
return "chap";
case OFONO_GPRS_AUTH_METHOD_PAP:
return "pap";
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ return "none";
+ default:
+ return NULL;
};
Okay, but this patch likely needs to also ensure that username /
password are not settable if method is NONE. And follow up with an
update of all things that depend on OFONO_GPRS_AUTH_METHOD usage. E.g.
drivers, provisioning plugins, etc.
return NULL;
@@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const char
*str,
} else if (g_str_equal(str, "pap")) {
*auth = OFONO_GPRS_AUTH_METHOD_PAP;
return TRUE;
+ } else if (g_str_equal(str, "none")) {
+ *auth = OFONO_GPRS_AUTH_METHOD_NONE;
+ return TRUE;
}
return FALSE;
@@ -1008,7 +1015,7 @@ static void pri_read_settings_callback(const struct
ofono_error *error,
value = pri_ctx->active;
- gprs->flags &= !GPRS_FLAG_ATTACHING;
+ gprs->flags &= ~GPRS_FLAG_ATTACHING;
Okay, but this is a separate fix and should be documented properly.
gprs->driver_attached = TRUE;
gprs_set_attached_property(gprs, TRUE);
@@ -1635,6 +1642,9 @@ static void release_active_contexts(struct ofono_gprs
*gprs)
if (gc->driver->detach_shutdown != NULL)
gc->driver->detach_shutdown(gc, ctx->context.cid);
+
+ /* Make sure the context is properly cleared */
+ release_context(ctx);
As above, seems to be an unrelated fix.
}
}
@@ -2234,6 +2244,7 @@ static DBusMessage *gprs_remove_context(DBusConnection *conn,
}
DBG("Unregistering context: %s", ctx->path);
+ release_context(ctx);
As above. You can't just lump these changes into something unrelated.
You need to submit these fixes separately and describe what each one is
fixing and why.
context_dbus_unregister(ctx);
gprs->contexts = g_slist_remove(gprs->contexts, ctx);
diff --git a/src/lte.c b/src/lte.c
index a6d26b3..21b6a19 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 "AccessPointName"
No. You can't do that. The D-Bus API is stable and cannot be changed.
This is why you propose D-Bus API changes first, so that they can be
reviewed separately for any impacts.
+#define LTE_PROTO "Protocol"
+#define LTE_USERNAME "Username"
+#define LTE_PASSWORD "Password"
+#define LTE_AUTH_METHOD "AuthenticationMethod"
struct ofono_lte {
const struct ofono_lte_driver *driver;
@@ -50,13 +55,82 @@ struct ofono_lte {
DBusMessage *pending;
struct ofono_lte_default_attach_info pending_info;
struct ofono_lte_default_attach_info info;
+ const char *prop_changed;
?? What memory location is this const char pointing to?
Why don't you just use an enum. Or even better, don't do this at all
and simply compare pending_info & info to generate the right signal.
};
static GSList *g_drivers = NULL;
+static const char *gprs_proto_to_string(enum ofono_gprs_proto proto)
+{
+ switch (proto) {
+ case OFONO_GPRS_PROTO_IP:
+ return "ip";
+ case OFONO_GPRS_PROTO_IPV6:
+ return "ipv6";
+ case OFONO_GPRS_PROTO_IPV4V6:
+ return "dual";
+ };
+
+ return NULL;
+}
This needs to be moved to common.c
+
+static gboolean gprs_proto_from_string(const char *str,
+ enum ofono_gprs_proto *proto)
+{
+ if (g_str_equal(str, "ip")) {
+ *proto = OFONO_GPRS_PROTO_IP;
+ return TRUE;
+ } else if (g_str_equal(str, "ipv6")) {
+ *proto = OFONO_GPRS_PROTO_IPV6;
+ return TRUE;
+ } else if (g_str_equal(str, "dual")) {
+ *proto = OFONO_GPRS_PROTO_IPV4V6;
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
+static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
+{
+ switch (auth) {
+ case OFONO_GPRS_AUTH_METHOD_CHAP:
+ return "chap";
+ case OFONO_GPRS_AUTH_METHOD_PAP:
+ return "pap";
+ case OFONO_GPRS_AUTH_METHOD_NONE:
+ return "none";
+ default:
+ return NULL;
+ };
+
+ return NULL;
+}
+
+static gboolean gprs_auth_method_from_string(const char *str,
+ enum ofono_gprs_auth_method *auth)
+{
+ if (g_str_equal(str, "chap")) {
+ *auth = OFONO_GPRS_AUTH_METHOD_CHAP;
+ return TRUE;
+ } else if (g_str_equal(str, "pap")) {
+ *auth = OFONO_GPRS_AUTH_METHOD_PAP;
+ return TRUE;
+ } else if (g_str_equal(str, "none")) {
+ *auth = OFONO_GPRS_AUTH_METHOD_NONE;
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
And all these as well
static void lte_load_settings(struct ofono_lte *lte)
{
+ char *proto_str;
char *apn;
+ char *auth_method_str;
+ char *username;
+ char *password;
if (lte->imsi == NULL)
return;
@@ -69,114 +143,276 @@ 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) {
+ proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+ LTE_PROTO, NULL);
+ apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+ LTE_APN, NULL);
And now you broke the default attach APN setting of every existing oFono
user. No, you cannot do that.
+ 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 (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 (apn)
strcpy(lte->info.apn, apn);
- g_free(apn);
- }
+
+ 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(proto_str);
+ g_free(apn);
+ 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;
reply = dbus_message_new_method_return(msg);
+
Don't change random code that doesn't need to be changed. You should be
keeping your changes minimal and on-point. Any changes unrelated to the
purpose of the patch need to be submitted separately.
if (reply == NULL)
return NULL;
dbus_message_iter_init_append(reply, &iter);
-
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_PROTO, DBUS_TYPE_STRING, &proto);
+ ofono_dbus_dict_append(&dict, LTE_APN, DBUS_TYPE_STRING, &apn);
+ ofono_dbus_dict_append(&dict, LTE_AUTH_METHOD, DBUS_TYPE_STRING,
+ &auth_method);
+ ofono_dbus_dict_append(&dict, LTE_USERNAME, DBUS_TYPE_STRING,
+ &username);
+ ofono_dbus_dict_append(&dict, LTE_PASSWORD, DBUS_TYPE_STRING,
+ &password);
dbus_message_iter_close_container(&iter, &dict);
-
return reply;
}
static void lte_set_default_attach_info_cb(const struct ofono_error *error,
- void *data)
+ void *data)
{
struct ofono_lte *lte = data;
const char *path = __ofono_atom_get_path(lte->atom);
DBusConnection *conn = ofono_dbus_get_connection();
DBusMessage *reply;
- const char *apn = lte->info.apn;
+ const char *propstr;
- DBG("%s error %d", path, error->type);
+ if (error != NULL) {
+ DBG("%s error %d", path, error->type);
Why? Error should never be NULL. If you're faking a call to this
function, why don't you just pass a proper error structure in instead of
messing with this? And even then I would think it would be much cleaner
not to do this in the first place...
- if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
- __ofono_dbus_pending_reply(<e->pending,
- __ofono_error_failed(lte->pending));
- return;
+ if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+ __ofono_dbus_pending_reply(<e->pending,
+ __ofono_error_failed(lte->pending));
+ return;
+ }
This change is non-sense.
}
- g_strlcpy(lte->info.apn, lte->pending_info.apn,
- OFONO_GPRS_MAX_APN_LENGTH + 1);
+ if (g_str_equal(lte->prop_changed, LTE_PROTO)) {
+ lte->info.proto = lte->pending_info.proto;
+ propstr = gprs_proto_to_string(lte->info.proto);
- 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);
- else
+ if (lte->settings)
g_key_file_set_string(lte->settings, SETTINGS_GROUP,
- DEFAULT_APN_KEY, lte->info.apn);
+ LTE_PROTO, propstr);
- storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
+ } else if (g_str_equal(lte->prop_changed, LTE_APN)) {
+ g_strlcpy(lte->info.apn, lte->pending_info.apn,
+ OFONO_GPRS_MAX_APN_LENGTH + 1);
+ propstr = lte->info.apn;
+
+ if (lte->settings) {
+
+ if (!*lte->info.apn)
+ /* Clear entry on empty APN. */
+ g_key_file_remove_key(lte->settings,
+ SETTINGS_GROUP, LTE_APN, NULL);
+ else
+ g_key_file_set_string(lte->settings,
+ SETTINGS_GROUP, LTE_APN, lte->info.apn);
+ }
+
+ } else if (g_str_equal(lte->prop_changed, LTE_AUTH_METHOD)) {
+ lte->info.auth_method = lte->pending_info.auth_method;
+ propstr = gprs_auth_method_to_string(lte->info.auth_method);
+
+ if (lte->settings)
+ g_key_file_set_string(lte->settings, SETTINGS_GROUP,
+ LTE_AUTH_METHOD, propstr);
+
+ } else if (g_str_equal(lte->prop_changed, LTE_USERNAME)) {
+ g_strlcpy(lte->info.username, lte->pending_info.username,
+ OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+ propstr = lte->info.username;
+
+ if (lte->settings) {
+
+ if (!*lte->info.username)
+ /* Clear entry on empty Username. */
+ g_key_file_remove_key(lte->settings,
+ SETTINGS_GROUP, LTE_USERNAME, NULL);
+ else
+ g_key_file_set_string(lte->settings,
+ SETTINGS_GROUP,
+ LTE_USERNAME, lte->info.username);
+ }
+
You have boiler-plate code for nearly all of these settings. Why don't
you actually use a function for this?
+ } else if (g_str_equal(lte->prop_changed, LTE_PASSWORD)) {
+ g_strlcpy(lte->info.password, lte->pending_info.password,
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+ propstr = lte->info.password;
+
+ if (lte->settings) {
+
+ if (strlen(lte->info.password) == 0)
+ /* Clear entry on empty Password. */
+ g_key_file_remove_key(lte->settings,
+ SETTINGS_GROUP, LTE_PASSWORD, NULL);
+ else
+ g_key_file_set_string(lte->settings,
+ SETTINGS_GROUP,
+ LTE_PASSWORD, lte->info.password);
+ }
+
+ } else {
+ return;
You have a dbus message pending, you can't do that...
}
+ if (lte->settings)
+ 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);
+ lte->prop_changed,
+ DBUS_TYPE_STRING, &propstr);
}
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
+static DBusMessage *lte_set_proto(struct ofono_lte *lte,
DBusConnection *conn, DBusMessage *msg,
- const char *apn)
+ 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);
+ void *data = lte;
- if (g_str_equal(apn, lte->info.apn))
+ if (proto == lte->info.proto)
return dbus_message_new_method_return(msg);
+ lte->pending = dbus_message_ref(msg);
+ lte->pending_info.proto = proto;
+ lte_set_default_attach_info_cb(NULL, data);
+ return dbus_message_ref(msg);;
I have no idea what is happening here. Whatever it is, it is wrong.
+}
+
+static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
+ DBusConnection *conn, DBusMessage *msg,
+ const char *apn)
+{
/* 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->info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
+ lte->driver->set_default_attach_info(lte, <e->info,
+ lte_set_default_attach_info_cb, lte);
+ return dbus_message_ref(msg);
+}
- g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
+static DBusMessage *lte_set_auth_method(struct ofono_lte *lte,
+ DBusConnection *conn, DBusMessage *msg,
+ enum ofono_gprs_auth_method auth_method)
+{
+ void *data = lte;
- lte->driver->set_default_attach_info(lte, <e->pending_info,
- lte_set_default_attach_info_cb, lte);
+ if (auth_method == lte->info.auth_method)
+ return dbus_message_new_method_return(msg);
- return NULL;
+ lte->pending = dbus_message_ref(msg);
+ lte->pending_info.auth_method = auth_method;
+ lte_set_default_attach_info_cb(NULL, data);
+ return dbus_message_ref(msg);;
+}
+
+static DBusMessage *lte_set_username(struct ofono_lte *lte,
+ DBusConnection *conn, DBusMessage *msg,
+ const char *username)
+{
+ void *data = lte;
+
+ if (g_str_equal(username, lte->info.username))
+ return dbus_message_new_method_return(msg);
+
+ lte->pending = dbus_message_ref(msg);
+ g_strlcpy(lte->pending_info.username, username,
+ OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+ lte_set_default_attach_info_cb(NULL, data);
+ return dbus_message_ref(msg);;
+}
+
+static DBusMessage *lte_set_password(struct ofono_lte *lte,
+ DBusConnection *conn, DBusMessage *msg,
+ const char *password)
+{
+ void *data = lte;
+
+ if (g_str_equal(password, lte->info.password))
+ return dbus_message_new_method_return(msg);
+
+ lte->pending = dbus_message_ref(msg);
+ g_strlcpy(lte->pending_info.password, password,
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+ lte_set_default_attach_info_cb(NULL, data);
+
You do realize you're never actually calling into the driver method,
right? So none of these changes actually go out to the modem. Have you
actually tested any of this?
+ return dbus_message_ref(msg);;
}
static DBusMessage *lte_set_property(DBusConnection *conn,
- DBusMessage *msg, void *data)
+ DBusMessage *msg, void *data)
{
struct ofono_lte *lte = data;
DBusMessageIter iter;
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,13 +428,58 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
dbus_message_iter_recurse(&iter, &var);
- if (!strcmp(property, DEFAULT_APN_KEY)) {
+ lte->prop_changed=property;
+
+ if (!strcmp(property, LTE_PROTO)) {
+
+ if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&var, &str);
+
+ if (gprs_proto_from_string(str, &proto))
+ return lte_set_proto(lte, conn, msg, proto);
The return from this callback is always supposed to be a method_return
or NULL if the method_return will be done asynchronously (in your case
always) You're somehow returning the method_call itself...
+ else
+ return __ofono_error_invalid_format(msg);
+
+ } else if (!strcmp(property, LTE_APN)) {
+
if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
return __ofono_error_invalid_args(msg);
dbus_message_iter_get_basic(&var, &str);
return lte_set_default_apn(lte, conn, msg, str);
+
+ } else if (!strcmp(property, LTE_AUTH_METHOD)) {
+
+ if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&var, &str);
+
+ if (gprs_auth_method_from_string(str, &auth_method))
+ return lte_set_auth_method(lte, conn, msg, auth_method);
+ else
+ return __ofono_error_invalid_format(msg);
+
+ } else if (!strcmp(property, LTE_USERNAME)) {
+
+ if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&var, &str);
+
+ return lte_set_username(lte, conn, msg, str);
+
+ } else if (!strcmp(property, LTE_PASSWORD)) {
+
+ if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&var, &str);
+
+ return lte_set_password(lte, conn, msg, str);
}
return __ofono_error_invalid_args(msg);
@@ -373,3 +654,8 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
{
return lte->driver_data;
}
+
+struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
+{
+ return __ofono_atom_get_modem(lte->atom);
+}
\ No newline at end of file
Fix this.
diff --git a/src/main.c b/src/main.c
index 2d359dd..d8a06ba 100644
--- a/src/main.c
+++ b/src/main.c
@@ -211,11 +211,6 @@ int main(int argc, char **argv)
struct ell_event_source *source;
#endif
-#ifdef NEED_THREADS
- if (g_thread_supported() == FALSE)
- g_thread_init(NULL);
-#endif
-
Why? This is completely unrelated and can be turned off via configure.
context = g_option_context_new(NULL);
g_option_context_add_main_entries(context, options, NULL);
Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
