Hi Denis,
On Tue, Oct 16, 2018 at 9:17 PM Denis Kenzior <[email protected]> wrote:
>
> Hi Giacinto,
>
> On 10/13/2018 02:34 AM, Giacinto Cifelli wrote:
> > Many LTE networks require user authentication, even for the default
> > context. In particular, most of the private APNs use this facility
> > to add some control on top of the MNO providing the service, so that
> > another user of the same network cannot access the private one.
> > As such, we add these parameters to the default context
> > settings that will attempt to use when registering to the network.
> >
> > The additional parameters added by this patch are: protocol, user, and
> > password. These are sufficient to allow to connect to networks
> > available to the patch author where ofono previously failed to register
> > to the network at all.
> >
> > Co-authored-by: Martin Baschin <[email protected]>
> > ---
> > src/lte.c | 241 +++++++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 185 insertions(+), 56 deletions(-)
> >
>
> So I'm fine with the overall approach taken here, but the actual
> implementation still has issues:
>
> > static void lte_set_default_attach_info_cb(const struct ofono_error
> > *error,
> > - void *data)
> > + void *data)
> > {
> > struct ofono_lte *lte = data;
> > const char *path = __ofono_atom_get_path(lte->atom);
> > DBusConnection *conn = ofono_dbus_get_connection();
> > DBusMessage *reply;
> > - const char *apn = lte->info.apn;
> > + const char *propstr = NULL;
> > + char *key = g_strdup(lte->key);
>
> I'd do this differently:
>
> >
> > DBG("%s error %d", path, error->type);
> >
>
> char *key;
> char *value;
> const char *str;
> DBusMessageIter iter;
> DbusMessageIter var;
>
> ....
>
> // No error checking needed since we already validated pending:
>
> dbus_message_iter_init(lte->pending, &iter);
> dbus_message_iter_get_basic(&iter, &str);
> key = strdup(str);
>
> dbus_message_iter_next(&iter);
> dbus_message_iter_recurse(&iter, &var);
> dbus_message_iter_get_basic(&var, &str);
> value = strdup(str);
>
> > @@ -118,55 +174,68 @@ static void lte_set_default_attach_info_cb(const
> > struct ofono_error *error,
> > return;
> > }
> >
> > - g_strlcpy(lte->info.apn, lte->pending_info.apn,
> > - OFONO_GPRS_MAX_APN_LENGTH + 1);
> > + if (!g_str_equal(lte->pending_info.apn, lte->info.apn)) {
> > + g_strlcpy(lte->info.apn, lte->pending_info.apn,
> > + OFONO_GPRS_MAX_APN_LENGTH + 1);
> > + propstr = lte->info.apn;
> > + } else if (lte->pending_info.proto != lte->info.proto) {
> > + lte->info.proto = lte->pending_info.proto;
> > + key = LTE_PROTO;
> > + propstr = gprs_proto_to_string(lte->info.proto);
> > + } else if (lte->pending_info.auth_method != lte->info.auth_method) {
> > + lte->info.auth_method = lte->pending_info.auth_method;
> > + propstr = gprs_auth_method_to_string(lte->info.auth_method);
> > +
> > + } else if (!g_str_equal(lte->pending_info.username,
> > + lte->info.username)) {
> > + g_strlcpy(lte->info.username, lte->pending_info.username,
> > + OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
> > + propstr = lte->info.username;
> > + } else if (!g_str_equal(lte->pending_info.password,
> > + lte->info.password)) {
> > + g_strlcpy(lte->info.password, lte->pending_info.password,
> > + OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
> > + propstr = lte->info.password;
> > + } else {
> > + /*
> > + * this should never happen, because no property change is
> > + * checked before.
> > + * Neverthelss, in this case it will answer the D-Bus message
> > + * but emit no signal
> > + */
> > + ofono_error("unexpected property change: ignored");
> > + g_free(key);
> > + key = NULL;
> > + }
>
> Get rid of this entire if/else block and simply do:
>
> memcpy(<e->info, <e->pending_info, sizeof(lte->info));
this I like particularly.
>
> > +
> > + 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);
> > + /*
> > + * the following code removes from storage empty APN, user,
> > pwd
> > + * for proto and auth_method, given that they always
> > + * have defaults, it will not do anything.
> > + */
> > + if (!*propstr)
> > + /* Clear entry on empty string. */
> > + g_key_file_remove_key(lte->settings,
> > + SETTINGS_GROUP, key, NULL);
> > else
> > - g_key_file_set_string(lte->settings, SETTINGS_GROUP,
> > - DEFAULT_APN_KEY,
> > lte->info.apn);
> > + g_key_file_set_string(lte->settings,
> > + SETTINGS_GROUP, key, propstr);
> >
> > storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> > }
> >
> > - reply = dbus_message_new_method_return(lte->pending);
> > - __ofono_dbus_pending_reply(<e->pending, reply);
> > -
> > ofono_dbus_signal_property_changed(conn, path,
> > OFONO_CONNECTION_CONTEXT_INTERFACE,
> > - DEFAULT_APN_KEY,
> > - DBUS_TYPE_STRING, &apn);
>
> g_free(value);
>
> > -}
> > -
> > -static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> > - DBusConnection *conn, DBusMessage *msg,
> > - const char *apn)
> > -{
> > - if (lte->driver->set_default_attach_info == NULL)
> > - return __ofono_error_not_implemented(msg);
> > -
> > - if (lte->pending)
> > - return __ofono_error_busy(msg);
> > -
> > - if (g_str_equal(apn, lte->info.apn))
> > - return dbus_message_new_method_return(msg);
> > -
> > - /* We do care about empty value: it can be used for reset. */
> > - if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
> > - return __ofono_error_invalid_format(msg);
> > -
> > - lte->pending = dbus_message_ref(msg);
> > -
> > - g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
> > -
> > - lte->driver->set_default_attach_info(lte, <e->pending_info,
> > - lte_set_default_attach_info_cb, lte);
> > -
> > - return NULL;
> > + key,
> > + DBUS_TYPE_STRING, &propstr);
> > + g_free(key);
> > }
> >
> > static DBusMessage *lte_set_property(DBusConnection *conn,
> > @@ -177,6 +246,14 @@ static DBusMessage *lte_set_property(DBusConnection
> > *conn,
> > DBusMessageIter var;
> > const char *property;
> > const char *str;
> > + enum ofono_gprs_auth_method auth_method;
> > + enum ofono_gprs_proto proto;
> > +
> > + if (lte->driver->set_default_attach_info == NULL)
> > + return __ofono_error_not_implemented(msg);
> > +
> > + if (lte->pending)
> > + return __ofono_error_busy(msg);
> >
>
> So you may want to do:
>
> memcpy(<e->pending_info, <e->info, sizeof(lte->info));
>
> > if (!dbus_message_iter_init(msg, &iter))
> > return __ofono_error_invalid_args(msg);
> > @@ -192,16 +269,68 @@ static DBusMessage *lte_set_property(DBusConnection
> > *conn,
> >
> > dbus_message_iter_recurse(&iter, &var);
> >
> > - if (!strcmp(property, DEFAULT_APN_KEY)) {
> > - if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> > - return __ofono_error_invalid_args(msg);
> > + if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> > + return __ofono_error_invalid_args(msg);
> >
> > - dbus_message_iter_get_basic(&var, &str);
> > + dbus_message_iter_get_basic(&var, &str);
> >
> > - return lte_set_default_apn(lte, conn, msg, str);
> > - }
> > + if ((strcmp(property, LTE_APN) == 0)) {
> > +
> > + if (g_str_equal(str, lte->info.apn))
> > + return dbus_message_new_method_return(msg);
> > +
> > + /* We do care about empty value: it can be used for reset. */
> > + if (is_valid_apn(str) == FALSE && str[0] != '\0')
> > + return __ofono_error_invalid_format(msg);
> > +
> > + g_strlcpy(lte->pending_info.apn, str,
> > + OFONO_GPRS_MAX_APN_LENGTH + 1);
> > +
> > + } else if ((strcmp(property, LTE_PROTO) == 0)) {
> > +
> > + if (!gprs_proto_from_string(str, &proto))
> > + return __ofono_error_invalid_format(msg);
> > +
> > + if (proto == lte->info.proto)
> > + return dbus_message_new_method_return(msg);
> > +
> > + lte->pending_info.proto = proto;
> >
> > - return __ofono_error_invalid_args(msg);
> > + } else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
> > +
> > + if (!gprs_auth_method_from_string(str, &auth_method))
> > + return __ofono_error_invalid_format(msg);
> > +
> > + if (auth_method == lte->info.auth_method)
> > + return dbus_message_new_method_return(msg);
> > +
> > + lte->pending_info.auth_method = auth_method;
> > +
> > + } else if (strcmp(property, LTE_USERNAME) == 0) {
> > +
> > + if (g_str_equal(str, lte->info.username))
> > + return dbus_message_new_method_return(msg);
> > +
> > + g_strlcpy(lte->pending_info.username, str,
> > + OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
> > +
> > + } else if (strcmp(property, LTE_PASSWORD) == 0) {
> > +
> > + if (g_str_equal(str, lte->info.password))
> > + return dbus_message_new_method_return(msg);
> > +
> > + g_strlcpy(lte->pending_info.password, str,
> > + OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
> > +
> > + } else
> > + return __ofono_error_invalid_args(msg);
> > +
> > + lte->key = property;
>
> And this isn't needed. Or alternatively store both the key and value
> being set to in ofono_lte. But I much prefer just parsing the message
> again.
>
> > + lte->pending = dbus_message_ref(msg);
> > + lte->driver->set_default_attach_info(lte, <e->pending_info,
> > + lte_set_default_attach_info_cb, lte);
> > +
> > + return NULL;
> > }
> >
> > static const GDBusMethodTable lte_methods[] = {
> >
>
> Regards,
> -Denis
I have changed my code according to your suggestions and retested
(with the Gemalto atom).
Regards,
Giacinto
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
