[PATCH] atmodem/lte: proto and authentication handling

[Denis Kenzior]

Hi Giacinto,

The ofono_lte_default_attach_info now handles also the protocol and the
authentication method, username and password.
---
drivers/atmodem/lte.c | 118 ++++++++++++++++++++++++++++++++++++------
1 file changed, 101 insertions(+), 17 deletions(-)
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index c4866623..ae7a815c 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
@@ -44,41 +45,124 @@ struct lte_driver_data {
GAtChat *chat;
};
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+struct lte_cbd {
+ gint ref_count; /* Ref count */
+ ofono_lte_cb_t cb;
+ void *data;
+ GAtChat *chat;

Why don't you just replace this with struct lte_driver_data * instead;

+ const struct ofono_lte_default_attach_info *info;

And honestly, I'd just put this into lte_driver_data as well.

Nope you can't do this. This is the third time I nak-ed this part

+ struct ofono_modem *modem;

You don't use this member, so leave it out.

+};
+
+static struct lte_cbd *lte_cb_data_new0(void *cb, void *data,
+ GAtChat *chat, const struct ofono_lte_default_attach_info *info)
+{
+ struct lte_cbd *cbd = g_new0(struct lte_cbd, 1);
+
+ cbd->ref_count = 1;
+ cbd->cb = cb;
+ cbd->data = data;
+ cbd->chat = chat;
+ cbd->info = info;
+
+ return cbd;
+}
+
+static inline struct lte_cbd *lte_cb_data_ref(struct lte_cbd *cbd)

Why don't we simply add ref/unref to cb_data. e.g.

cb_data_ref()
cb_data_unref()

+{
+ if (cbd == NULL)
+ return NULL;
+
+ g_atomic_int_inc(&cbd->ref_count);
+
+ return cbd;
+}
+
+static void lte_cb_data_unref(gpointer user_data)
+{
+ gboolean is_zero;
+ struct lte_cbd *cbd = user_data;
+
+ if (cbd == NULL)
+ return;
+
+ is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
+
+ if (is_zero == TRUE)
+ g_free(cbd);
+}
+
+static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
- struct cb_data *cbd = user_data;
+ struct lte_cbd *cbd = user_data;
ofono_lte_cb_t cb = cbd->cb;
struct ofono_error error;
- DBG("ok %d", ok);
-
decode_at_error(&error, g_at_result_final_response(result));
cb(&error, cbd->data);
}
+static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct lte_cbd *cbd = user_data;
+ ofono_lte_cb_t cb = cbd->cb;
+ void *data = cbd->data;
+ struct ofono_error error;
+ char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+ OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+ size_t buflen = sizeof(buf);
+ size_t len;
+ enum ofono_gprs_auth_method auth_method;
+
+ if (!ok) {
+ lte_cb_data_unref(cbd);

This causes cb_data_unref to be called twice with reference count at 1.
E.g. most likely a crash right after you return from this callback.

+ decode_at_error(&error, g_at_result_final_response(result));
+ cb(&error, data);
+ return;
+ }
+
+ auth_method = cbd->info->auth_method;
+
+ /* change the authentication method if the parameters are invalid */
+ if (!*cbd->info->username || !*cbd->info->password)
+ auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+ len = snprintf(buf, buflen, "AT+CGAUTH=0,%d",
+ at_util_gprs_auth_method_to_auth_prot(auth_method));
+ buflen -= len;
+
+ if (auth_method != OFONO_GPRS_AUTH_METHOD_NONE)
+ snprintf(buf + len, buflen, ",\"%s\",\"%s\"",
+ cbd->info->username, cbd->info->password);
+
+ cbd = lte_cb_data_ref(cbd);
+ if (g_at_chat_send(cbd->chat, buf, NULL,
+ at_lte_set_auth_cb, cbd, lte_cb_data_unref) > 0)
+ return;
+
+ lte_cb_data_unref(cbd);
+ CALLBACK_WITH_FAILURE(cb, data);
+}
+
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);
+ struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info);
+ char *buf = at_util_get_cgdcont_command(0, info->proto, info->apn);

Honestly I'd just do it like:
cbd = cb_data_new(cb, data);
cb->user = ldd;

memcpy(&ldd->pending_username, ...);
memcpy(&ldd->pending_password, ...);
ldd->pending_auth = ...;

- 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;
+ at_lte_set_default_attach_info_cb,
+ cbd, lte_cb_data_unref) > 0)

cb_data_unref

+ goto end;
+ lte_cb_data_unref(cbd);

g_free/cb_data_unref.

CALLBACK_WITH_FAILURE(cb, data);
+ g_free(buf);
}
static gboolean lte_delayed_register(gpointer user_data)

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior

+ const struct ofono_lte_default_attach_info *info;

And honestly, I'd just put this into lte_driver_data as well.
Nope you can't do this. This is the third time I nak-ed this part

I can't do what? I don't see what you are referring to.

There are two comments here. First is that this 'pending' info stuff
doesn't belong here.

Second is that you cannot hold a reference to an object owned by the
core. The driver cannot assume that it will be valid for the duration
of the asynchronous operation. E.g. the core might have created a
temporary object on the stack to pass to the driver and it will go
*poof* as soon as the driver function returns.

There is very strict separation between driver managed memory /
structures and ones owned by the core.

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Denis Kenzior
There is very strict separation between driver managed memory /
structures and ones owned by the core.

Thank you for the clarification, now I understand.
And yes, it is indeed the 3rd time you point it out.
I hope it is the last one, but it is easy to fall for this,
there should be some mechanism to either prevent it or guarantee that

There is: you scribble the data you need into the part of memory you
control. Easy.

the object is persistent.

We're not changing the entire driver<->core model just because you find
it inconvenient ;)

Regards,
-Denis