Hi Denis,
On Fri, Oct 12, 2018 at 5:43 AM Denis Kenzior <[email protected]> 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
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
