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
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
